From fa07f16bcc75fdcd7193323a2329d5714fe017e9 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Sat, 24 Aug 2024 17:42:55 +0100 Subject: [PATCH] Revert "Convert database/sql sql-injection sinks to MaD" This reverts commit 501bb3eb56ace85259119ccf8cb4aeeaddbf1634. --- go/ql/lib/ext/database.sql.model.yml | 28 ------------------- .../go/frameworks/stdlib/DatabaseSql.qll | 20 ++++++++++++- .../frameworks/XNetHtml/SqlInjection.expected | 9 +++--- 3 files changed, 23 insertions(+), 34 deletions(-) diff --git a/go/ql/lib/ext/database.sql.model.yml b/go/ql/lib/ext/database.sql.model.yml index b8fb85bb893..e1083f6e49a 100644 --- a/go/ql/lib/ext/database.sql.model.yml +++ b/go/ql/lib/ext/database.sql.model.yml @@ -1,32 +1,4 @@ extensions: - - addsTo: - pack: codeql/go-all - extensible: sinkModel - data: - - ["database/sql", "Conn", False, "Exec", "", "", "Argument[0]", "sql-injection", "manual"] - - ["database/sql", "Conn", False, "ExecContext", "", "", "Argument[1]", "sql-injection", "manual"] - - ["database/sql", "Conn", False, "Prepare", "", "", "Argument[0]", "sql-injection", "manual"] - - ["database/sql", "Conn", False, "PrepareContext", "", "", "Argument[1]", "sql-injection", "manual"] - - ["database/sql", "Conn", False, "Query", "", "", "Argument[0]", "sql-injection", "manual"] - - ["database/sql", "Conn", False, "QueryContext", "", "", "Argument[1]", "sql-injection", "manual"] - - ["database/sql", "Conn", False, "QueryRow", "", "", "Argument[0]", "sql-injection", "manual"] - - ["database/sql", "Conn", False, "QueryRowContext", "", "", "Argument[1]", "sql-injection", "manual"] - - ["database/sql", "DB", False, "Exec", "", "", "Argument[0]", "sql-injection", "manual"] - - ["database/sql", "DB", False, "ExecContext", "", "", "Argument[1]", "sql-injection", "manual"] - - ["database/sql", "DB", False, "Prepare", "", "", "Argument[0]", "sql-injection", "manual"] - - ["database/sql", "DB", False, "PrepareContext", "", "", "Argument[1]", "sql-injection", "manual"] - - ["database/sql", "DB", False, "Query", "", "", "Argument[0]", "sql-injection", "manual"] - - ["database/sql", "DB", False, "QueryContext", "", "", "Argument[1]", "sql-injection", "manual"] - - ["database/sql", "DB", False, "QueryRow", "", "", "Argument[0]", "sql-injection", "manual"] - - ["database/sql", "DB", False, "QueryRowContext", "", "", "Argument[1]", "sql-injection", "manual"] - - ["database/sql", "Tx", False, "Exec", "", "", "Argument[0]", "sql-injection", "manual"] - - ["database/sql", "Tx", False, "ExecContext", "", "", "Argument[1]", "sql-injection", "manual"] - - ["database/sql", "Tx", False, "Prepare", "", "", "Argument[0]", "sql-injection", "manual"] - - ["database/sql", "Tx", False, "PrepareContext", "", "", "Argument[1]", "sql-injection", "manual"] - - ["database/sql", "Tx", False, "Query", "", "", "Argument[0]", "sql-injection", "manual"] - - ["database/sql", "Tx", False, "QueryContext", "", "", "Argument[1]", "sql-injection", "manual"] - - ["database/sql", "Tx", False, "QueryRow", "", "", "Argument[0]", "sql-injection", "manual"] - - ["database/sql", "Tx", False, "QueryRowContext", "", "", "Argument[1]", "sql-injection", "manual"] - addsTo: pack: codeql/go-all extensible: summaryModel diff --git a/go/ql/lib/semmle/go/frameworks/stdlib/DatabaseSql.qll b/go/ql/lib/semmle/go/frameworks/stdlib/DatabaseSql.qll index 5f53ea2de8c..845225af5bd 100644 --- a/go/ql/lib/semmle/go/frameworks/stdlib/DatabaseSql.qll +++ b/go/ql/lib/semmle/go/frameworks/stdlib/DatabaseSql.qll @@ -26,7 +26,7 @@ module DatabaseSql { override DataFlow::Node getAResult() { result = this.getResult(0) } override SQL::QueryString getAQueryString() { - result = this.getASyntacticArgument() + result = this.getAnArgument() or // attempt to resolve a `QueryString` for `Stmt`s using local data flow. t = "Stmt" and @@ -34,6 +34,24 @@ module DatabaseSql { } } + /** A query string used in an API function of the `database/sql` package. */ + private class QueryString extends SQL::QueryString::Range { + QueryString() { + exists(Method meth, string base, string t, string m, int n | + t = ["DB", "Tx", "Conn"] and + meth.hasQualifiedName("database/sql", t, m) and + this = meth.getACall().getArgument(n) + | + base = ["Exec", "Prepare", "Query", "QueryRow"] and + ( + m = base and n = 0 + or + m = base + "Context" and n = 1 + ) + ) + } + } + /** A query in the standard `database/sql/driver` package. */ private class DriverQuery extends SQL::Query::Range, DataFlow::MethodCallNode { DriverQuery() { diff --git a/go/ql/test/library-tests/semmle/go/frameworks/XNetHtml/SqlInjection.expected b/go/ql/test/library-tests/semmle/go/frameworks/XNetHtml/SqlInjection.expected index b7a66a6a903..446695b259b 100644 --- a/go/ql/test/library-tests/semmle/go/frameworks/XNetHtml/SqlInjection.expected +++ b/go/ql/test/library-tests/semmle/go/frameworks/XNetHtml/SqlInjection.expected @@ -1,12 +1,11 @@ #select | test.go:57:11:57:41 | call to EscapeString | test.go:56:2:56:42 | ... := ...[0] | test.go:57:11:57:41 | call to EscapeString | This query depends on a $@. | test.go:56:2:56:42 | ... := ...[0] | user-provided value | edges -| test.go:56:2:56:42 | ... := ...[0] | test.go:57:29:57:40 | selection of Value | provenance | Src:MaD:3 | -| test.go:57:29:57:40 | selection of Value | test.go:57:11:57:41 | call to EscapeString | provenance | MaD:2 Sink:MaD:1 | +| test.go:56:2:56:42 | ... := ...[0] | test.go:57:29:57:40 | selection of Value | provenance | Src:MaD:2 | +| test.go:57:29:57:40 | selection of Value | test.go:57:11:57:41 | call to EscapeString | provenance | MaD:1 | models -| 1 | Sink: database/sql; DB; false; Query; ; ; Argument[0]; sql-injection; manual | -| 2 | Summary: golang.org/x/net/html; ; false; EscapeString; ; ; Argument[0]; ReturnValue; taint; manual | -| 3 | Source: net/http; Request; true; Cookie; ; ; ReturnValue[0]; remote; manual | +| 1 | Summary: golang.org/x/net/html; ; false; EscapeString; ; ; Argument[0]; ReturnValue; taint; manual | +| 2 | Source: net/http; Request; true; Cookie; ; ; ReturnValue[0]; remote; manual | nodes | test.go:56:2:56:42 | ... := ...[0] | semmle.label | ... := ...[0] | | test.go:57:11:57:41 | call to EscapeString | semmle.label | call to EscapeString |