Merge pull request #637 from smowton/smowton/fix/log-injection-sanitizers

Fix sanitization by strings.Replace[All] in go/unsafe-quoting and go/log-injection

change-notes/2021-12-14-strings-replace-sanitizers.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Fixed sanitization by calls to `strings.Replace` and `strings.ReplaceAll` in queries `go/log-injection` and `go/unsafe-quoting`.

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

@@ -39,4 +39,23 @@ module LogInjection {
   class LoggerSink extends Sink {
     LoggerSink() { this = any(LoggerCall log).getAMessageComponent() }
   }
+
+  /**
+   * A call to `strings.Replace` or `strings.ReplaceAll`, considered as a sanitizer
+   * for log injection.
+   */
+  class ReplaceSanitizer extends Sanitizer {
+    ReplaceSanitizer() {
+      exists(string name, DataFlow::CallNode call |
+        this = call and
+        call.getTarget().hasQualifiedName("strings", name) and
+        call.getArgument(1).getStringValue().matches("%" + ["\r", "\n"] + "%")
+      |
+        name = "Replace" and
+        call.getArgument(3).getNumericValue() < 0
+        or
+        name = "ReplaceAll"
+      )
+    }
+  }
 }

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

@@ -92,7 +92,7 @@ module StringBreak {
       exists(string name, DataFlow::CallNode call |
         this = call and
         call.getTarget().hasQualifiedName("strings", name) and
-        call.getArgument(2).getStringValue().matches("%" + quote + "%")
+        call.getArgument(1).getStringValue().matches("%" + quote + "%")
       |
         name = "Replace" and
         call.getArgument(3).getNumericValue() < 0

ql/test/query-tests/Security/CWE-089/StringBreak.expected

@@ -1,8 +1,20 @@
 edges
 | StringBreak.go:10:2:10:40 | ... := ...[0] : slice type | StringBreak.go:14:47:14:57 | versionJSON |
+| StringBreakMismatched.go:12:2:12:40 | ... := ...[0] : slice type | StringBreakMismatched.go:13:29:13:47 | type conversion : slice type |
+| StringBreakMismatched.go:13:29:13:47 | type conversion : slice type | StringBreakMismatched.go:17:26:17:32 | escaped |
+| StringBreakMismatched.go:24:2:24:40 | ... := ...[0] : slice type | StringBreakMismatched.go:25:29:25:47 | type conversion : slice type |
+| StringBreakMismatched.go:25:29:25:47 | type conversion : slice type | StringBreakMismatched.go:29:27:29:33 | escaped |
 nodes
 | StringBreak.go:10:2:10:40 | ... := ...[0] : slice type | semmle.label | ... := ...[0] : slice type |
 | StringBreak.go:14:47:14:57 | versionJSON | semmle.label | versionJSON |
+| StringBreakMismatched.go:12:2:12:40 | ... := ...[0] : slice type | semmle.label | ... := ...[0] : slice type |
+| StringBreakMismatched.go:13:29:13:47 | type conversion : slice type | semmle.label | type conversion : slice type |
+| StringBreakMismatched.go:17:26:17:32 | escaped | semmle.label | escaped |
+| StringBreakMismatched.go:24:2:24:40 | ... := ...[0] : slice type | semmle.label | ... := ...[0] : slice type |
+| StringBreakMismatched.go:25:29:25:47 | type conversion : slice type | semmle.label | type conversion : slice type |
+| StringBreakMismatched.go:29:27:29:33 | escaped | semmle.label | escaped |
 subpaths
 #select
 | StringBreak.go:14:47:14:57 | versionJSON | StringBreak.go:10:2:10:40 | ... := ...[0] : slice type | StringBreak.go:14:47:14:57 | versionJSON | If this $@ contains a single quote, it could break out of the enclosing quotes. | StringBreak.go:10:2:10:40 | ... := ...[0] | JSON value |
+| StringBreakMismatched.go:17:26:17:32 | escaped | StringBreakMismatched.go:12:2:12:40 | ... := ...[0] : slice type | StringBreakMismatched.go:17:26:17:32 | escaped | If this $@ contains a single quote, it could break out of the enclosing quotes. | StringBreakMismatched.go:12:2:12:40 | ... := ...[0] | JSON value |
+| StringBreakMismatched.go:29:27:29:33 | escaped | StringBreakMismatched.go:24:2:24:40 | ... := ...[0] : slice type | StringBreakMismatched.go:29:27:29:33 | escaped | If this $@ contains a double quote, it could break out of the enclosing quotes. | StringBreakMismatched.go:24:2:24:40 | ... := ...[0] | JSON value |

ql/test/query-tests/Security/CWE-089/StringBreakGood.go

@@ -3,8 +3,10 @@ package main
 import (
 	"encoding/json"
 	sq "github.com/Masterminds/squirrel"
+	"strings"
 )
 
+// Good because there is no concatenation with quotes:
 func saveGood(id string, version interface{}) {
 	versionJSON, _ := json.Marshal(version)
 	sq.StatementBuilder.
@@ -13,3 +15,25 @@ func saveGood(id string, version interface{}) {
 		Values(id, sq.Expr("md5(?)", versionJSON)).
 		Exec()
 }
+
+// Good because quote characters are removed before concatenation:
+func saveGood2(id string, version interface{}) {
+	versionJSON, _ := json.Marshal(version)
+	escaped := strings.Replace(string(versionJSON), "\"", "", -1)
+	sq.StatementBuilder.
+		Insert("resources").
+		Columns("resource_id", "version_md5").
+		Values(id, sq.Expr("\""+escaped+"\"")).
+		Exec()
+}
+
+// Good because quote characters are removed before concatenation:
+func saveGood3(id string, version interface{}) {
+	versionJSON, _ := json.Marshal(version)
+	escaped := strings.ReplaceAll(string(versionJSON), "'", "")
+	sq.StatementBuilder.
+		Insert("resources").
+		Columns("resource_id", "version_md5").
+		Values(id, sq.Expr("'"+escaped+"'")).
+		Exec()
+}

ql/test/query-tests/Security/CWE-089/StringBreakMismatched.go

@@ -0,0 +1,31 @@
+package main
+
+import (
+	"encoding/json"
+	sq "github.com/Masterminds/squirrel"
+	"strings"
+)
+
+// Bad because quote characters are removed before concatenation,
+// but then enclosed in a different enclosing quote:
+func mismatch1(id string, version interface{}) {
+	versionJSON, _ := json.Marshal(version)
+	escaped := strings.Replace(string(versionJSON), "\"", "", -1)
+	sq.StatementBuilder.
+		Insert("resources").
+		Columns("resource_id", "version_md5").
+		Values(id, sq.Expr("'"+escaped+"'")).
+		Exec()
+}
+
+// Bad because quote characters are removed before concatenation,
+// but then enclosed in a different enclosing quote:
+func mismatch2(id string, version interface{}) {
+	versionJSON, _ := json.Marshal(version)
+	escaped := strings.Replace(string(versionJSON), "'", "", -1)
+	sq.StatementBuilder.
+		Insert("resources").
+		Columns("resource_id", "version_md5").
+		Values(id, sq.Expr("\""+escaped+"\"")).
+		Exec()
+}

ql/test/query-tests/Security/CWE-117/LogInjection.go

@@ -14,6 +14,7 @@ import (
 	"fmt"
 	"log"
 	"net/http"
+	"strings"
 
 	"github.com/astaxie/beego"
 	"github.com/astaxie/beego/logs"
@@ -360,3 +361,19 @@ func handler(req *http.Request, ctx *goproxy.ProxyCtx) {
 		sLogger.With(username)  // $ hasTaintFlow="username"
 	}
 }
+
+// GOOD: The user-provided value is escaped before being written to the log.
+func handlerGood(req *http.Request) {
+	username := req.URL.Query()["username"][0]
+	escapedUsername := strings.Replace(username, "\n", "", -1)
+	escapedUsername = strings.Replace(escapedUsername, "\r", "", -1)
+	log.Printf("user %s logged in.\n", escapedUsername)
+}
+
+// GOOD: The user-provided value is escaped before being written to the log.
+func handlerGood2(req *http.Request) {
+	username := req.URL.Query()["username"][0]
+	escapedUsername := strings.ReplaceAll(username, "\n", "")
+	escapedUsername = strings.ReplaceAll(escapedUsername, "\r", "")
+	log.Printf("user %s logged in.\n", escapedUsername)
+}