Add missing string replacement sanitizers to log-injection and string-break

ql/lib/semmle/go/security/LogInjectionCustomizations.qll

@@ -60,6 +60,34 @@ module LogInjection {
     }
   }
 
+  /**
+   * A call to `strings.Replacer.Replace`, considered as a sanitizer for log
+   * injection.
+   */
+  class ReplacerReplaceSanitizer extends Sanitizer {
+    ReplacerReplaceSanitizer() {
+      exists(DataFlow::MethodCallNode call |
+        call.(DataFlow::MethodCallNode)
+            .getTarget()
+            .hasQualifiedName("strings", "Replacer", "Replace") and
+        this = call.getResult()
+      )
+    }
+  }
+
+  /**
+   * 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.

ql/lib/semmle/go/security/StringBreakCustomizations.qll

@@ -103,4 +103,69 @@ module StringBreak {
 
     override Quote getQuote() { result = quote }
   }
+
+  class StringsNewReplacerCall extends DataFlow::CallNode {
+    StringsNewReplacerCall() { this.getTarget().hasQualifiedName("strings", "NewReplacer") }
+
+    DataFlow::Node getAReplacedArgument() { exists(int n | n % 2 = 0 and result = getArgument(n)) }
+  }
+
+  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,
+        DataFlow::MethodCallNode call
+      |
+        config.hasFlow(source, sink) and
+        call.getTarget().hasQualifiedName("strings", "Replacer", "Replace") and
+        sink = call.getReceiver() and
+        this = call.getResult() 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 }
+  }
 }

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".

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()
+}

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"
@@ -378,8 +379,43 @@ 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", 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 {
+	replacer := strings.NewReplacer("\n", "", "\r", "")
+	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 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)