From 81ccc40a941a55aa071194fcca431c6e96d52af5 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 4 May 2022 11:48:50 +0100 Subject: [PATCH 1/5] Make `strings.Replacer.Replace` a sanitizer for log injection --- .../security/LogInjectionCustomizations.qll | 10 +++++++++ .../Security/CWE-117/LogInjection.go | 22 ++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/ql/lib/semmle/go/security/LogInjectionCustomizations.qll b/ql/lib/semmle/go/security/LogInjectionCustomizations.qll index af0d07f28..1742b81d6 100644 --- a/ql/lib/semmle/go/security/LogInjectionCustomizations.qll +++ b/ql/lib/semmle/go/security/LogInjectionCustomizations.qll @@ -60,6 +60,16 @@ module LogInjection { } } + /** + * A call to `strings.Replacer.Replace`, considered as a sanitizer for log + * injection. + */ + class ReplacerReplaceSanitizer extends Sanitizer { + ReplacerReplaceSanitizer() { + this.(DataFlow::MethodCallNode).getTarget().hasQualifiedName("strings", "Replacer", "Replace") + } + } + /** * An argument that is formatted using the `%q` directive, considered as a sanitizer * for log injection. diff --git a/ql/test/query-tests/Security/CWE-117/LogInjection.go b/ql/test/query-tests/Security/CWE-117/LogInjection.go index b6dc97247..6166c0554 100644 --- a/ql/test/query-tests/Security/CWE-117/LogInjection.go +++ b/ql/test/query-tests/Security/CWE-117/LogInjection.go @@ -378,8 +378,28 @@ func handlerGood2(req *http.Request) { log.Printf("user %s logged in.\n", escapedUsername) } +// GOOD: The user-provided value is escaped before being written to the log. +func handlerGood3(req *http.Request) { + username := req.URL.Query()["username"][0] + replacer := strings.NewReplacer("\n", "", "\r", "") + log.Printf("user %s logged in.\n", replacer.Replace(username)) + log.Printf("user %s logged in.\n", replacerLocal1(username)) + log.Printf("user %s logged in.\n", replacerGlobal1(username)) +} + +func replacerLocal1(s string) string { + replacer := strings.NewReplacer("\n", "", "\r", "") + return replacer.Replace(s) +} + +var globalReplacer = strings.NewReplacer("\n", "", "\r", "") + +func replacerGlobal1(s string) string { + return globalReplacer.Replace(s) +} + // GOOD: User-provided values formatted using a %q directive, which escapes newlines -func handlerGood3(req *http.Request, ctx *goproxy.ProxyCtx) { +func handlerGood4(req *http.Request, ctx *goproxy.ProxyCtx) { username := req.URL.Query()["username"][0] testFlag := req.URL.Query()["testFlag"][0] log.Printf("user %q logged in.\n", username) From 3ab9bd3f22a62f0ff97172cf0a2309672d6a39b4 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 4 May 2022 14:25:31 +0100 Subject: [PATCH 2/5] Make `strings.Replacer.WriteString` a sanitizer for log injection --- .../go/security/LogInjectionCustomizations.qll | 13 +++++++++++++ .../query-tests/Security/CWE-117/LogInjection.go | 16 ++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/ql/lib/semmle/go/security/LogInjectionCustomizations.qll b/ql/lib/semmle/go/security/LogInjectionCustomizations.qll index 1742b81d6..afc9a2e8a 100644 --- a/ql/lib/semmle/go/security/LogInjectionCustomizations.qll +++ b/ql/lib/semmle/go/security/LogInjectionCustomizations.qll @@ -70,6 +70,19 @@ module LogInjection { } } + /** + * A call to `strings.Replacer.WriteString`, considered as a sanitizer for log + * injection. + */ + class ReplacerWriteStringSanitizer extends Sanitizer { + ReplacerWriteStringSanitizer() { + exists(DataFlow::MethodCallNode call | + call.getTarget().hasQualifiedName("strings", "Replacer", "WriteString") and + this = call.getArgument(1) + ) + } + } + /** * An argument that is formatted using the `%q` directive, considered as a sanitizer * for log injection. diff --git a/ql/test/query-tests/Security/CWE-117/LogInjection.go b/ql/test/query-tests/Security/CWE-117/LogInjection.go index 6166c0554..4f02cc9ae 100644 --- a/ql/test/query-tests/Security/CWE-117/LogInjection.go +++ b/ql/test/query-tests/Security/CWE-117/LogInjection.go @@ -11,6 +11,7 @@ package main //go:generate depstubber -vendor go.uber.org/zap Logger,SugaredLogger NewProduction import ( + "bytes" "fmt" "log" "net/http" @@ -384,7 +385,9 @@ func handlerGood3(req *http.Request) { replacer := strings.NewReplacer("\n", "", "\r", "") log.Printf("user %s logged in.\n", replacer.Replace(username)) log.Printf("user %s logged in.\n", replacerLocal1(username)) + log.Printf("user %s logged in.\n", replacerLocal2(username)) log.Printf("user %s logged in.\n", replacerGlobal1(username)) + log.Printf("user %s logged in.\n", replacerGlobal2(username)) } func replacerLocal1(s string) string { @@ -392,12 +395,25 @@ func replacerLocal1(s string) string { return replacer.Replace(s) } +func replacerLocal2(s string) string { + replacer := strings.NewReplacer("\n", "", "\r", "") + buf := new(bytes.Buffer) + replacer.WriteString(buf, s) + return buf.String() +} + var globalReplacer = strings.NewReplacer("\n", "", "\r", "") func replacerGlobal1(s string) string { return globalReplacer.Replace(s) } +func replacerGlobal2(s string) string { + buf := new(bytes.Buffer) + globalReplacer.WriteString(buf, s) + return buf.String() +} + // GOOD: User-provided values formatted using a %q directive, which escapes newlines func handlerGood4(req *http.Request, ctx *goproxy.ProxyCtx) { username := req.URL.Query()["username"][0] From 2a0fdd1c67f139d2c56032a542e712585945619a Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 4 May 2022 15:30:54 +0100 Subject: [PATCH 3/5] Add extra replace sanitizers to StringBreak --- .../go/security/StringBreakCustomizations.qll | 63 +++++++++++++++++++ .../Security/CWE-089/StringBreakGood.go | 29 ++++++++- 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/ql/lib/semmle/go/security/StringBreakCustomizations.qll b/ql/lib/semmle/go/security/StringBreakCustomizations.qll index 489ffb2d4..47bae2a01 100644 --- a/ql/lib/semmle/go/security/StringBreakCustomizations.qll +++ b/ql/lib/semmle/go/security/StringBreakCustomizations.qll @@ -103,4 +103,67 @@ module StringBreak { override Quote getQuote() { result = quote } } + + class StringsNewReplacerCall extends DataFlow::CallNode { + StringsNewReplacerCall() { this.getTarget().hasQualifiedName("strings", "NewReplacer") } + + DataFlow::Node getAReplacedArgument() { + exists(int m, int n | m = 2 * n and n = m / 2 and result = getArgument(m)) + } + } + + class StringsNewReplacerConfiguration extends DataFlow2::Configuration { + StringsNewReplacerConfiguration() { this = "StringsNewReplacerConfiguration" } + + override predicate isSource(DataFlow::Node source) { source instanceof StringsNewReplacerCall } + + override predicate isSink(DataFlow::Node sink) { + exists(DataFlow::MethodCallNode call | + sink = call.getReceiver() and + call.getTarget().hasQualifiedName("strings", "Replacer", ["Replace", "WriteString"]) + ) + } + } + + /** + * A call to `strings.Replacer.Replace`, considered as a sanitizer for unsafe + * quoting. + */ + class ReplacerReplaceSanitizer extends DataFlow::MethodCallNode, Sanitizer { + Quote quote; + + ReplacerReplaceSanitizer() { + exists(StringsNewReplacerConfiguration config, DataFlow::Node source, DataFlow::Node sink | + config.hasFlow(source, sink) and + this.getTarget().hasQualifiedName("strings", "Replacer", "Replace") and + sink = this.getReceiver() and + quote = source.(StringsNewReplacerCall).getAReplacedArgument().getStringValue() + ) + } + + override Quote getQuote() { result = quote } + } + + /** + * A call to `strings.Replacer.WriteString`, considered as a sanitizer for + * unsafe quoting. + */ + class ReplacerWriteStringSanitizer extends Sanitizer { + Quote quote; + + ReplacerWriteStringSanitizer() { + exists( + StringsNewReplacerConfiguration config, DataFlow::Node source, DataFlow::Node sink, + DataFlow::MethodCallNode call + | + config.hasFlow(source, sink) and + call.getTarget().hasQualifiedName("strings", "Replacer", "WriteString") and + sink = call.getReceiver() and + this = call.getArgument(1) and + quote = source.(StringsNewReplacerCall).getAReplacedArgument().getStringValue() + ) + } + + override Quote getQuote() { result = quote } + } } diff --git a/ql/test/query-tests/Security/CWE-089/StringBreakGood.go b/ql/test/query-tests/Security/CWE-089/StringBreakGood.go index 7c4edbfaa..4265db165 100644 --- a/ql/test/query-tests/Security/CWE-089/StringBreakGood.go +++ b/ql/test/query-tests/Security/CWE-089/StringBreakGood.go @@ -1,9 +1,11 @@ package main import ( + "bytes" "encoding/json" - sq "github.com/Masterminds/squirrel" "strings" + + sq "github.com/Masterminds/squirrel" ) // Good because there is no concatenation with quotes: @@ -37,3 +39,28 @@ func saveGood3(id string, version interface{}) { Values(id, sq.Expr("'"+escaped+"'")). Exec() } + +var globalReplacer = strings.NewReplacer("\"", "", "'", "") + +// Good because quote characters are removed before concatenation: +func saveGood4(id string, version interface{}) { + versionJSON, _ := json.Marshal(version) + escaped := globalReplacer.Replace(string(versionJSON)) + sq.StatementBuilder. + Insert("resources"). + Columns("resource_id", "version_md5"). + Values(id, sq.Expr("'"+escaped+"'")). + Exec() +} + +// Good because quote characters are removed before concatenation: +func saveGood5(id string, version interface{}) { + versionJSON, _ := json.Marshal(version) + buf := new(bytes.Buffer) + globalReplacer.WriteString(buf, string(versionJSON)) + sq.StatementBuilder. + Insert("resources"). + Columns("resource_id", "version_md5"). + Values(id, sq.Expr("'"+buf.String()+"'")). + Exec() +} From 926136d3c102ee4ab8aff9ec6e4b63397e3c95df Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 4 May 2022 15:36:25 +0100 Subject: [PATCH 4/5] Add change note --- .../2022-05-04-add-extra-string-replace-sanitizers.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ql/src/change-notes/2022-05-04-add-extra-string-replace-sanitizers.md diff --git a/ql/src/change-notes/2022-05-04-add-extra-string-replace-sanitizers.md b/ql/src/change-notes/2022-05-04-add-extra-string-replace-sanitizers.md new file mode 100644 index 000000000..6ae41312e --- /dev/null +++ b/ql/src/change-notes/2022-05-04-add-extra-string-replace-sanitizers.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The functions `strings.Replacer.Replace` and `strings.Replacer.WriteString` have been added as sanitizers for the queries "Log entries created from user input" and "Potentially unsafe quoting". From af3d6b0ca65b0ec914fec94a4f46b71c6e74ac67 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 4 May 2022 22:08:39 +0100 Subject: [PATCH 5/5] Address review comments --- .../go/security/LogInjectionCustomizations.qll | 7 ++++++- .../go/security/StringBreakCustomizations.qll | 14 ++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/ql/lib/semmle/go/security/LogInjectionCustomizations.qll b/ql/lib/semmle/go/security/LogInjectionCustomizations.qll index afc9a2e8a..52ca1a715 100644 --- a/ql/lib/semmle/go/security/LogInjectionCustomizations.qll +++ b/ql/lib/semmle/go/security/LogInjectionCustomizations.qll @@ -66,7 +66,12 @@ module LogInjection { */ class ReplacerReplaceSanitizer extends Sanitizer { ReplacerReplaceSanitizer() { - this.(DataFlow::MethodCallNode).getTarget().hasQualifiedName("strings", "Replacer", "Replace") + exists(DataFlow::MethodCallNode call | + call.(DataFlow::MethodCallNode) + .getTarget() + .hasQualifiedName("strings", "Replacer", "Replace") and + this = call.getResult() + ) } } diff --git a/ql/lib/semmle/go/security/StringBreakCustomizations.qll b/ql/lib/semmle/go/security/StringBreakCustomizations.qll index 47bae2a01..ca16c6f37 100644 --- a/ql/lib/semmle/go/security/StringBreakCustomizations.qll +++ b/ql/lib/semmle/go/security/StringBreakCustomizations.qll @@ -107,9 +107,7 @@ module StringBreak { class StringsNewReplacerCall extends DataFlow::CallNode { StringsNewReplacerCall() { this.getTarget().hasQualifiedName("strings", "NewReplacer") } - DataFlow::Node getAReplacedArgument() { - exists(int m, int n | m = 2 * n and n = m / 2 and result = getArgument(m)) - } + DataFlow::Node getAReplacedArgument() { exists(int n | n % 2 = 0 and result = getArgument(n)) } } class StringsNewReplacerConfiguration extends DataFlow2::Configuration { @@ -133,10 +131,14 @@ module StringBreak { Quote quote; ReplacerReplaceSanitizer() { - exists(StringsNewReplacerConfiguration config, DataFlow::Node source, DataFlow::Node sink | + exists( + StringsNewReplacerConfiguration config, DataFlow::Node source, DataFlow::Node sink, + DataFlow::MethodCallNode call + | config.hasFlow(source, sink) and - this.getTarget().hasQualifiedName("strings", "Replacer", "Replace") and - sink = this.getReceiver() and + call.getTarget().hasQualifiedName("strings", "Replacer", "Replace") and + sink = call.getReceiver() and + this = call.getResult() and quote = source.(StringsNewReplacerCall).getAReplacedArgument().getStringValue() ) }