From ca1e9d7c7521989924d730e20225202f64d290bb Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Tue, 7 Jan 2025 21:08:43 -0500 Subject: [PATCH 1/4] Revert "`database/sql` summary models for `Row` types" This reverts commit 80ad349a481317248da1989f9bbfda7822fdaf39. --- go/ql/lib/ext/database.sql.driver.model.yml | 1 - go/ql/lib/ext/database.sql.model.yml | 2 -- .../go/frameworks/stdlib/DatabaseSql.qll | 20 +++++++++++++++++++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/go/ql/lib/ext/database.sql.driver.model.yml b/go/ql/lib/ext/database.sql.driver.model.yml index 0f33a6e14b8c..f4e46eaa3434 100644 --- a/go/ql/lib/ext/database.sql.driver.model.yml +++ b/go/ql/lib/ext/database.sql.driver.model.yml @@ -23,6 +23,5 @@ extensions: data: - ["database/sql/driver", "Conn", True, "Prepare", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"] - ["database/sql/driver", "ConnPrepareContext", True, "PrepareContext", "", "", "Argument[1]", "ReturnValue[0]", "taint", "manual"] - - ["database/sql/driver", "Rows", True, "Next", "", "", "Argument[receiver]", "Argument[0]", "taint", "manual"] - ["database/sql/driver", "ValueConverter", True, "ConvertValue", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"] - ["database/sql/driver", "Valuer", True, "Value", "", "", "Argument[receiver]", "ReturnValue[0]", "taint", "manual"] diff --git a/go/ql/lib/ext/database.sql.model.yml b/go/ql/lib/ext/database.sql.model.yml index 34042a397744..8d67dd921423 100644 --- a/go/ql/lib/ext/database.sql.model.yml +++ b/go/ql/lib/ext/database.sql.model.yml @@ -53,8 +53,6 @@ extensions: - ["database/sql", "Conn", True, "PrepareContext", "", "", "Argument[1]", "ReturnValue[0]", "taint", "manual"] - ["database/sql", "DB", True, "Prepare", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"] - ["database/sql", "DB", True, "PrepareContext", "", "", "Argument[1]", "ReturnValue[0]", "taint", "manual"] - - ["database/sql", "Row", True, "Scan", "", "", "Argument[receiver]", "Argument[0].ArrayElement", "taint", "manual"] - - ["database/sql", "Rows", True, "Scan", "", "", "Argument[receiver]", "Argument[0].ArrayElement", "taint", "manual"] - ["database/sql", "Scanner", True, "Scan", "", "", "Argument[0]", "Argument[receiver]", "taint", "manual"] - ["database/sql", "Tx", True, "Prepare", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"] - ["database/sql", "Tx", True, "PrepareContext", "", "", "Argument[1]", "ReturnValue[0]", "taint", "manual"] diff --git a/go/ql/lib/semmle/go/frameworks/stdlib/DatabaseSql.qll b/go/ql/lib/semmle/go/frameworks/stdlib/DatabaseSql.qll index 14534d2fa453..f41326887961 100644 --- a/go/ql/lib/semmle/go/frameworks/stdlib/DatabaseSql.qll +++ b/go/ql/lib/semmle/go/frameworks/stdlib/DatabaseSql.qll @@ -66,4 +66,24 @@ module DatabaseSql { result = this.getReceiver().getAPredecessor*().(DataFlow::MethodCallNode).getAnArgument() } } + + // These are expressed using TaintTracking::FunctionModel because varargs functions don't work with Models-as-Data sumamries yet. + private class SqlMethodModels extends TaintTracking::FunctionModel, Method { + FunctionInput inp; + FunctionOutput outp; + + SqlMethodModels() { + // signature: func (*Row) Scan(dest ...interface{}) error + this.hasQualifiedName("database/sql", "Row", "Scan") and + (inp.isReceiver() and outp.isParameter(_)) + or + // signature: func (*Rows) Scan(dest ...interface{}) error + this.hasQualifiedName("database/sql", "Rows", "Scan") and + (inp.isReceiver() and outp.isParameter(_)) + } + + override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { + input = inp and output = outp + } + } } From 6a862f2f80e0a05862657bdb6040b0199bbcc93e Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Tue, 7 Jan 2025 21:10:48 -0500 Subject: [PATCH 2/4] Add Rows::Next back --- go/ql/lib/ext/database.sql.driver.model.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/go/ql/lib/ext/database.sql.driver.model.yml b/go/ql/lib/ext/database.sql.driver.model.yml index f4e46eaa3434..ddaea25b7936 100644 --- a/go/ql/lib/ext/database.sql.driver.model.yml +++ b/go/ql/lib/ext/database.sql.driver.model.yml @@ -23,5 +23,6 @@ extensions: data: - ["database/sql/driver", "Conn", True, "Prepare", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"] - ["database/sql/driver", "ConnPrepareContext", True, "PrepareContext", "", "", "Argument[1]", "ReturnValue[0]", "taint", "manual"] + - ["database/sql/driver", "Rows", True, "Next", "", "", "Argument[receiver]", "ReturnValue[0]", "taint", "manual"] - ["database/sql/driver", "ValueConverter", True, "ConvertValue", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"] - ["database/sql/driver", "Valuer", True, "Value", "", "", "Argument[receiver]", "ReturnValue[0]", "taint", "manual"] From c086945a9e1ebd1c7ab4cb909efb648ca26c7bf6 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Tue, 7 Jan 2025 21:14:25 -0500 Subject: [PATCH 3/4] Fix typo --- go/ql/lib/ext/database.sql.driver.model.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/ql/lib/ext/database.sql.driver.model.yml b/go/ql/lib/ext/database.sql.driver.model.yml index ddaea25b7936..0f33a6e14b8c 100644 --- a/go/ql/lib/ext/database.sql.driver.model.yml +++ b/go/ql/lib/ext/database.sql.driver.model.yml @@ -23,6 +23,6 @@ extensions: data: - ["database/sql/driver", "Conn", True, "Prepare", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"] - ["database/sql/driver", "ConnPrepareContext", True, "PrepareContext", "", "", "Argument[1]", "ReturnValue[0]", "taint", "manual"] - - ["database/sql/driver", "Rows", True, "Next", "", "", "Argument[receiver]", "ReturnValue[0]", "taint", "manual"] + - ["database/sql/driver", "Rows", True, "Next", "", "", "Argument[receiver]", "Argument[0]", "taint", "manual"] - ["database/sql/driver", "ValueConverter", True, "ConvertValue", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"] - ["database/sql/driver", "Valuer", True, "Value", "", "", "Argument[receiver]", "ReturnValue[0]", "taint", "manual"] From bc68e4456a7a5698fe6b133318278520e5b83bb2 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 8 Jan 2025 10:22:00 -0500 Subject: [PATCH 4/4] Fix test results --- .../query-tests/Security/CWE-078/StoredCommand.expected | 7 +------ .../test/query-tests/Security/CWE-079/StoredXss.expected | 9 ++------- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/go/ql/test/query-tests/Security/CWE-078/StoredCommand.expected b/go/ql/test/query-tests/Security/CWE-078/StoredCommand.expected index 223c9fd7d394..59369e97597d 100644 --- a/go/ql/test/query-tests/Security/CWE-078/StoredCommand.expected +++ b/go/ql/test/query-tests/Security/CWE-078/StoredCommand.expected @@ -2,21 +2,16 @@ | StoredCommand.go:14:22:14:28 | cmdName | StoredCommand.go:11:2:11:27 | ... := ...[0] | StoredCommand.go:14:22:14:28 | cmdName | This command depends on a $@. | StoredCommand.go:11:2:11:27 | ... := ...[0] | stored value | edges | StoredCommand.go:11:2:11:27 | ... := ...[0] | StoredCommand.go:13:2:13:5 | rows | provenance | Src:MaD:2 | -| StoredCommand.go:13:2:13:5 | rows | StoredCommand.go:13:2:13:20 | []type{args} | provenance | MaD:3 | -| StoredCommand.go:13:2:13:5 | rows | StoredCommand.go:13:2:13:20 | []type{args} [array] | provenance | MaD:3 | -| StoredCommand.go:13:2:13:20 | []type{args} | StoredCommand.go:13:12:13:19 | &... | provenance | | -| StoredCommand.go:13:2:13:20 | []type{args} | StoredCommand.go:14:22:14:28 | cmdName | provenance | Sink:MaD:1 | +| StoredCommand.go:13:2:13:5 | rows | StoredCommand.go:13:12:13:19 | &... | provenance | FunctionModel | | StoredCommand.go:13:2:13:20 | []type{args} [array] | StoredCommand.go:13:12:13:19 | &... | provenance | | | StoredCommand.go:13:12:13:19 | &... | StoredCommand.go:13:2:13:20 | []type{args} [array] | provenance | | | StoredCommand.go:13:12:13:19 | &... | StoredCommand.go:14:22:14:28 | cmdName | provenance | Sink:MaD:1 | models | 1 | Sink: os/exec; ; false; Command; ; ; Argument[0]; command-injection; manual | | 2 | Source: database/sql; DB; true; Query; ; ; ReturnValue[0]; database; manual | -| 3 | Summary: database/sql; Rows; true; Scan; ; ; Argument[receiver]; Argument[0].ArrayElement; taint; manual | nodes | StoredCommand.go:11:2:11:27 | ... := ...[0] | semmle.label | ... := ...[0] | | StoredCommand.go:13:2:13:5 | rows | semmle.label | rows | -| StoredCommand.go:13:2:13:20 | []type{args} | semmle.label | []type{args} | | StoredCommand.go:13:2:13:20 | []type{args} [array] | semmle.label | []type{args} [array] | | StoredCommand.go:13:12:13:19 | &... | semmle.label | &... | | StoredCommand.go:14:22:14:28 | cmdName | semmle.label | cmdName | diff --git a/go/ql/test/query-tests/Security/CWE-079/StoredXss.expected b/go/ql/test/query-tests/Security/CWE-079/StoredXss.expected index d9bb0c2a2e2b..5a6051e8dd62 100644 --- a/go/ql/test/query-tests/Security/CWE-079/StoredXss.expected +++ b/go/ql/test/query-tests/Security/CWE-079/StoredXss.expected @@ -5,11 +5,8 @@ edges | StoredXss.go:13:21:13:31 | call to Name | StoredXss.go:13:21:13:36 | ...+... | provenance | | | stored.go:18:3:18:28 | ... := ...[0] | stored.go:25:14:25:17 | rows | provenance | Src:MaD:1 | -| stored.go:25:14:25:17 | rows | stored.go:25:14:25:34 | []type{args} | provenance | MaD:2 | -| stored.go:25:14:25:17 | rows | stored.go:25:14:25:34 | []type{args} [array] | provenance | MaD:2 | -| stored.go:25:14:25:34 | []type{args} | stored.go:25:24:25:26 | &... | provenance | | -| stored.go:25:14:25:34 | []type{args} | stored.go:25:29:25:33 | &... | provenance | | -| stored.go:25:14:25:34 | []type{args} | stored.go:30:22:30:25 | name | provenance | | +| stored.go:25:14:25:17 | rows | stored.go:25:24:25:26 | &... | provenance | FunctionModel | +| stored.go:25:14:25:17 | rows | stored.go:25:29:25:33 | &... | provenance | FunctionModel | | stored.go:25:14:25:34 | []type{args} [array] | stored.go:25:24:25:26 | &... | provenance | | | stored.go:25:14:25:34 | []type{args} [array] | stored.go:25:29:25:33 | &... | provenance | | | stored.go:25:24:25:26 | &... | stored.go:25:14:25:34 | []type{args} [array] | provenance | | @@ -18,13 +15,11 @@ edges | stored.go:59:30:59:33 | definition of path | stored.go:61:22:61:25 | path | provenance | | models | 1 | Source: database/sql; DB; true; Query; ; ; ReturnValue[0]; database; manual | -| 2 | Summary: database/sql; Rows; true; Scan; ; ; Argument[receiver]; Argument[0].ArrayElement; taint; manual | nodes | StoredXss.go:13:21:13:31 | call to Name | semmle.label | call to Name | | StoredXss.go:13:21:13:36 | ...+... | semmle.label | ...+... | | stored.go:18:3:18:28 | ... := ...[0] | semmle.label | ... := ...[0] | | stored.go:25:14:25:17 | rows | semmle.label | rows | -| stored.go:25:14:25:34 | []type{args} | semmle.label | []type{args} | | stored.go:25:14:25:34 | []type{args} [array] | semmle.label | []type{args} [array] | | stored.go:25:24:25:26 | &... | semmle.label | &... | | stored.go:25:29:25:33 | &... | semmle.label | &... |