Revert "Convert database/sql sql-injection sinks to MaD"

This reverts commit 501bb3e.
github · Aug 24, 2024 · fa07f16 · fa07f16
1 parent b3326ba
commit fa07f16
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 34 deletions.
diff --git 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
@@ -26,14 +26,32 @@ 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
       result = this.getReceiver().getAPredecessor*().(DataFlow::MethodCallNode).getAnArgument()
     }
   }
 
+  /** 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
@@ -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 |