Merge pull request #18402 from egregius313/egregius313/go/mad/database/database-sql

egregius313 · web-flow · commit 651052ba7aa5 · 2025-01-07T07:31:06.000-05:00
Go: Add `database` source models for the `database/sql` and `database/sql/driver` packages
diff --git a/go/ql/lib/change-notes/2025-01-03-database-sql-and-database-sql-driver-source-models.md b/go/ql/lib/change-notes/2025-01-03-database-sql-and-database-sql-driver-source-models.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* `database` local source models have been added for the `database/sql` and `database/sql/driver` packages.
diff --git a/go/ql/lib/ext/database.sql.driver.model.yml b/go/ql/lib/ext/database.sql.driver.model.yml
@@ -1,4 +1,12 @@
 extensions:
+  - addsTo:
+      pack: codeql/go-all
+      extensible: sourceModel
+    data:
+      - ["database/sql/driver", "Queryer", True, "Query", "", "", "ReturnValue[0]", "database", "manual"]
+      - ["database/sql/driver", "QueryerContext", True, "QueryContext", "", "", "ReturnValue[0]", "database", "manual"]
+      - ["database/sql/driver", "Stmt", True, "Query", "", "", "ReturnValue[0]", "database", "manual"]
+      - ["database/sql/driver", "StmtQueryContext", True, "QueryContext", "", "", "ReturnValue[0]", "database", "manual"]
   - addsTo:
       pack: codeql/go-all
       extensible: sinkModel
@@ -15,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]", "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
@@ -1,4 +1,22 @@
 extensions:
+  - addsTo:
+      pack: codeql/go-all
+      extensible: sourceModel
+    data:
+      - ["database/sql", "Conn", True, "QueryContext", "", "", "ReturnValue[0]", "database", "manual"]
+      - ["database/sql", "Conn", True, "QueryRowContext", "", "", "ReturnValue", "database", "manual"]
+      - ["database/sql", "DB", True, "Query", "", "", "ReturnValue[0]", "database", "manual"]
+      - ["database/sql", "DB", True, "QueryContext", "", "", "ReturnValue[0]", "database", "manual"]
+      - ["database/sql", "DB", True, "QueryRow", "", "", "ReturnValue", "database", "manual"]
+      - ["database/sql", "DB", True, "QueryRowContext", "", "", "ReturnValue", "database", "manual"]
+      - ["database/sql", "Stmt", True, "Query", "", "", "ReturnValue[0]", "database", "manual"]
+      - ["database/sql", "Stmt", True, "QueryContext", "", "", "ReturnValue[0]", "database", "manual"]
+      - ["database/sql", "Stmt", True, "QueryRow", "", "", "ReturnValue", "database", "manual"]
+      - ["database/sql", "Stmt", True, "QueryRowContext", "", "", "ReturnValue", "database", "manual"]
+      - ["database/sql", "Tx", True, "Query", "", "", "ReturnValue[0]", "database", "manual"]
+      - ["database/sql", "Tx", True, "QueryContext", "", "", "ReturnValue[0]", "database", "manual"]
+      - ["database/sql", "Tx", True, "QueryRow", "", "", "ReturnValue", "database", "manual"]
+      - ["database/sql", "Tx", True, "QueryRowContext", "", "", "ReturnValue", "database", "manual"]
   - addsTo:
       pack: codeql/go-all
       extensible: sinkModel
@@ -35,6 +53,8 @@ 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
@@ -66,24 +66,4 @@ 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
-    }
-  }
 }
diff --git a/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/sink.go b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/sink.go
@@ -0,0 +1,5 @@
+package test
+
+func sink(x ...any) {}
+
+func ignore(...any) {}
diff --git a/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/source.expected b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/source.expected
@@ -0,0 +1,2 @@
+testFailures
+invalidModelRow
diff --git a/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/source.ext.yml b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/source.ext.yml
@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/threat-models
+      extensible: threatModelConfiguration
+    data:
+      - ["database", true, 0]
diff --git a/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/source.ql b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/source.ql
@@ -0,0 +1,19 @@
+import go
+import ModelValidation
+import utils.test.InlineExpectationsTest
+
+module SourceTest implements TestSig {
+  string getARelevantTag() { result = "source" }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    exists(ActiveThreatModelSource s |
+      s.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
+        location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
+      element = s.toString() and
+      value = "" and
+      tag = "source"
+    )
+  }
+}
+
+import MakeTest<SourceTest>
diff --git a/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/test.expected b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/test.expected
@@ -0,0 +1,2 @@
+testFailures
+invalidModelRow
diff --git a/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/test.ext.yml b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/test.ext.yml
@@ -0,0 +1,7 @@
+extensions:
+
+  - addsTo:
+      pack: codeql/threat-models
+      extensible: threatModelConfiguration
+    data:
+      - ["database", true, 0]
diff --git a/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/test.ql b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/test.ql
@@ -0,0 +1,15 @@
+import go
+import semmle.go.dataflow.ExternalFlow
+import ModelValidation
+import experimental.frameworks.CleverGo
+import utils.test.InlineFlowTest
+
+module Config implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof ActiveThreatModelSource }
+
+  predicate isSink(DataFlow::Node sink) {
+    sink.asExpr() = any(CallExpr c | c.getTarget().getName() = "sink").getAnArgument()
+  }
+}
+
+import TaintFlowTest<Config>
diff --git a/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/test_database_sql.go b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/test_database_sql.go
@@ -0,0 +1,157 @@
+package test
+
+import (
+	"database/sql"
+)
+
+// test querying a Conn
+func testConnQuery(conn *sql.Conn) {
+	rows, err := conn.QueryContext(nil, "SELECT * FROM users") // $ source
+
+	if err != nil {
+		return
+	}
+
+	defer rows.Close()
+
+	for rows.Next() {
+		var id int
+		var name string
+		err = rows.Scan(&id, &name)
+		if err != nil {
+			return
+		}
+
+		sink(id, name) // $ hasTaintFlow="id" hasTaintFlow="name"
+	}
+
+	row := conn.QueryRowContext(nil, "SELECT * FROM users WHERE id = 1") // $ source
+
+	var id int
+	var name string
+
+	err = row.Scan(&id, &name)
+
+	sink(id, name) // $ hasTaintFlow="id" hasTaintFlow="name"
+}
+
+// test querying a DB
+func testDBQuery(db *sql.DB) {
+	example, err := db.Query("SELECT * FROM users") // $ source
+	ignore(example)
+
+	rows, err := db.QueryContext(nil, "SELECT * FROM users") // $ source
+
+	if err != nil {
+		return
+	}
+
+	defer rows.Close()
+
+	for rows.Next() {
+		var id int
+		var name string
+		err = rows.Scan(&id, &name)
+
+		if err != nil {
+			return
+		}
+
+		sink(id, name) // $ hasTaintFlow="id" hasTaintFlow="name"
+	}
+
+	row := db.QueryRowContext(nil, "SELECT * FROM users WHERE id = 1") // $ source
+
+	var id int
+	var name string
+
+	err = row.Scan(&id, &name)
+
+	sink(id, name) // $ hasTaintFlow="id" hasTaintFlow="name"
+
+	dog := db.QueryRow("SELECT * FROM dogs WHERE id = 1") // $ source
+	ignore(dog)
+}
+
+// test querying a Stmt
+func testStmtQuery(stmt *sql.Stmt) {
+	example, err := stmt.Query("SELECT * FROM users") // $ source
+	ignore(example)
+
+	rows, err := stmt.QueryContext(nil, "SELECT * FROM users") // $ source
+
+	if err != nil {
+		return
+	}
+
+	defer rows.Close()
+
+	for rows.Next() {
+		var id int
+		var name string
+		err = rows.Scan(&id, &name)
+
+		if err != nil {
+			return
+		}
+
+		sink(id, name) // $ hasTaintFlow="id" hasTaintFlow="name"
+	}
+
+	row := stmt.QueryRowContext(nil, "SELECT * FROM users WHERE id = 1") // $ source
+
+	var id int
+	var name string
+
+	err = row.Scan(&id, &name)
+
+	if err != nil {
+		return
+	}
+
+	sink(id, name) // $ hasTaintFlow="id" hasTaintFlow="name"
+
+	dog := stmt.QueryRow("SELECT * FROM dogs WHERE id = 1") // $ source
+	ignore(dog)
+}
+
+// test querying a Tx
+func testTxQuery(tx *sql.Tx) {
+	example, err := tx.Query("SELECT * FROM users") // $ source
+	ignore(example)
+
+	rows, err := tx.QueryContext(nil, "SELECT * FROM users") // $ source
+	if err != nil {
+		return
+	}
+
+	defer rows.Close()
+
+	for rows.Next() {
+		var id int
+		var name string
+		err = rows.Scan(&id, &name)
+
+		if err != nil {
+			return
+		}
+
+		sink(id, name) // $ hasTaintFlow="id" hasTaintFlow="name"
+	}
+
+	row := tx.QueryRowContext(nil, "SELECT * FROM users WHERE id = 1") // $ source
+
+	var id int
+	var name string
+
+	err = row.Scan(&id, &name)
+
+	if err != nil {
+		return
+	}
+
+	sink(id, name) // $ hasTaintFlow="id" hasTaintFlow="name"
+
+	dog := tx.QueryRow("SELECT * FROM dogs WHERE id = 1") // $ source
+	ignore(dog)
+}
diff --git a/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/test_database_sql_driver.go b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/database/test_database_sql_driver.go
@@ -0,0 +1,23 @@
+package test
+
+import "database/sql/driver"
+
+func testQueryer(q driver.Queryer) {
+	rows, err := q.Query("SELECT * FROM users", make([]driver.Value, 0)) // $ source
+	ignore(rows, err)
+}
+
+func testQueryerContext(q driver.QueryerContext) {
+	rows, err := q.QueryContext(nil, "SELECT * FROM users", make([]driver.NamedValue, 0)) // $ source
+	ignore(rows, err)
+}
+
+func testStmt(stmt driver.Stmt) {
+	rows, err := stmt.Query(make([]driver.Value, 0)) // $ source
+	ignore(rows, err)
+}
+
+func testStmtContext(stmt driver.StmtQueryContext) {
+	rows, err := stmt.QueryContext(nil, make([]driver.NamedValue, 0)) // $ source
+	ignore(rows, err)
+}
diff --git a/go/ql/test/query-tests/Security/CWE-078/StoredCommand.expected b/go/ql/test/query-tests/Security/CWE-078/StoredCommand.expected
@@ -1,16 +1,22 @@
 #select
 | 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 |  |
-| StoredCommand.go:13:2:13:5 | rows | StoredCommand.go:13:12:13:19 | &... | provenance | FunctionModel |
+| 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: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
@@ -1,27 +1,34 @@
+#select
+| StoredXss.go:13:21:13:36 | ...+... | StoredXss.go:13:21:13:31 | call to Name | StoredXss.go:13:21:13:36 | ...+... | Stored cross-site scripting vulnerability due to $@. | StoredXss.go:13:21:13:31 | call to Name | stored value |
+| stored.go:30:22:30:25 | name | stored.go:18:3:18:28 | ... := ...[0] | stored.go:30:22:30:25 | name | Stored cross-site scripting vulnerability due to $@. | stored.go:18:3:18:28 | ... := ...[0] | stored value |
+| stored.go:61:22:61:25 | path | stored.go:59:30:59:33 | definition of path | stored.go:61:22:61:25 | path | Stored cross-site scripting vulnerability due to $@. | stored.go:59:30:59:33 | definition of path | stored value |
 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 |  |
-| 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: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: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 |  |
 | stored.go:25:29:25:33 | &... | stored.go:25:14:25:34 | []type{args} [array] | provenance |  |
 | stored.go:25:29:25:33 | &... | stored.go:30:22:30:25 | name | provenance |  |
 | 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 | &... |
 | stored.go:30:22:30:25 | name | semmle.label | name |
 | stored.go:59:30:59:33 | definition of path | semmle.label | definition of path |
 | stored.go:61:22:61:25 | path | semmle.label | path |
 subpaths
-#select
-| StoredXss.go:13:21:13:36 | ...+... | StoredXss.go:13:21:13:31 | call to Name | StoredXss.go:13:21:13:36 | ...+... | Stored cross-site scripting vulnerability due to $@. | StoredXss.go:13:21:13:31 | call to Name | stored value |
-| stored.go:30:22:30:25 | name | stored.go:18:3:18:28 | ... := ...[0] | stored.go:30:22:30:25 | name | Stored cross-site scripting vulnerability due to $@. | stored.go:18:3:18:28 | ... := ...[0] | stored value |
-| stored.go:61:22:61:25 | path | stored.go:59:30:59:33 | definition of path | stored.go:61:22:61:25 | path | Stored cross-site scripting vulnerability due to $@. | stored.go:59:30:59:33 | definition of path | stored value |

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +---
 +category: minorAnalysis
 +---
 +* `database` local source models have been added for the `database/sql` and `database/sql/driver` packages.