diff --git a/change-notes/2021-12-14-strings-replace-sanitizers.md b/change-notes/2021-12-14-strings-replace-sanitizers.md new file mode 100644 index 00000000000..69ca827fd39 --- /dev/null +++ b/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`. diff --git a/ql/lib/semmle/go/security/LogInjectionCustomizations.qll b/ql/lib/semmle/go/security/LogInjectionCustomizations.qll index a724aac254d..091d8a65b8e 100644 --- a/ql/lib/semmle/go/security/LogInjectionCustomizations.qll +++ b/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" + ) + } + } } diff --git a/ql/lib/semmle/go/security/StringBreakCustomizations.qll b/ql/lib/semmle/go/security/StringBreakCustomizations.qll index 0a6a3db8544..489ffb2d426 100644 --- a/ql/lib/semmle/go/security/StringBreakCustomizations.qll +++ b/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 diff --git a/ql/test/query-tests/Security/CWE-089/StringBreak.expected b/ql/test/query-tests/Security/CWE-089/StringBreak.expected index 4155b8d358e..cf3f362ead9 100644 --- a/ql/test/query-tests/Security/CWE-089/StringBreak.expected +++ b/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 | diff --git a/ql/test/query-tests/Security/CWE-089/StringBreakGood.go b/ql/test/query-tests/Security/CWE-089/StringBreakGood.go index dd7125629ba..7c4edbfaaa0 100644 --- a/ql/test/query-tests/Security/CWE-089/StringBreakGood.go +++ b/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() +} diff --git a/ql/test/query-tests/Security/CWE-089/StringBreakMismatched.go b/ql/test/query-tests/Security/CWE-089/StringBreakMismatched.go new file mode 100644 index 00000000000..ba8ee72d0fa --- /dev/null +++ b/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() +} diff --git a/ql/test/query-tests/Security/CWE-117/LogInjection.go b/ql/test/query-tests/Security/CWE-117/LogInjection.go index 22a30fe8d95..7beb055ad9a 100644 --- a/ql/test/query-tests/Security/CWE-117/LogInjection.go +++ b/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) +}