Merge pull request #18284 from egregius313/egregius313/go/dataflow/so…

…urces/commandargs/os-args Go: Model `os.Args` as a `commandargs` source
github · Dec 13, 2024 · 9948f6e · 9948f6e
2 parents 2cbb072 + 7852c86
commit 9948f6e
Show file tree

Hide file tree

Showing 7 changed files with 48 additions and 5 deletions.
diff --git a/go/ql/lib/change-notes/2024-12-13-os-args-model.md b/go/ql/lib/change-notes/2024-12-13-os-args-model.md
@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Added a `commandargs` local source model for the `os.Args` variable.
+
diff --git a/go/ql/lib/ext/os.model.yml b/go/ql/lib/ext/os.model.yml
@@ -46,6 +46,7 @@ extensions:
       pack: codeql/go-all
       extensible: sourceModel
     data:
+      - ["os", "", False, "Args", "", "", "", "commandargs", "manual"]
       - ["os", "", False, "Environ", "", "", "ReturnValue", "environment", "manual"] # TODO: when sources can have access paths, use .ArrayElement
       - ["os", "", False, "ExpandEnv", "", "", "ReturnValue", "environment", "manual"]
       - ["os", "", False, "Getenv", "", "", "ReturnValue", "environment", "manual"]

diff --git a/go/ql/test/experimental/CWE-74/DsnInjectionLocal.expected b/go/ql/test/experimental/CWE-74/DsnInjectionLocal.expected
@@ -2,8 +2,8 @@
 | Dsn.go:29:29:29:33 | dbDSN | Dsn.go:26:11:26:17 | selection of Args | Dsn.go:29:29:29:33 | dbDSN | This query depends on a $@. | Dsn.go:26:11:26:17 | selection of Args | user-provided value |
 | Dsn.go:68:29:68:33 | dbDSN | Dsn.go:63:19:63:25 | selection of Args | Dsn.go:68:29:68:33 | dbDSN | This query depends on a $@. | Dsn.go:63:19:63:25 | selection of Args | user-provided value |
 edges
-| Dsn.go:26:11:26:17 | selection of Args | Dsn.go:28:102:28:109 | index expression | provenance |  |
-| Dsn.go:28:11:28:110 | []type{args} [array] | Dsn.go:28:11:28:110 | call to Sprintf | provenance | MaD:1 |
+| Dsn.go:26:11:26:17 | selection of Args | Dsn.go:28:102:28:109 | index expression | provenance | Src:MaD:1  |
+| Dsn.go:28:11:28:110 | []type{args} [array] | Dsn.go:28:11:28:110 | call to Sprintf | provenance | MaD:2 |
 | Dsn.go:28:11:28:110 | call to Sprintf | Dsn.go:29:29:29:33 | dbDSN | provenance |  |
 | Dsn.go:28:102:28:109 | index expression | Dsn.go:28:11:28:110 | []type{args} [array] | provenance |  |
 | Dsn.go:28:102:28:109 | index expression | Dsn.go:28:11:28:110 | call to Sprintf | provenance | FunctionModel |
@@ -12,17 +12,18 @@ edges
 | Dsn.go:63:9:63:11 | cfg [pointer] | Dsn.go:63:9:63:11 | implicit dereference | provenance |  |
 | Dsn.go:63:9:63:11 | implicit dereference | Dsn.go:62:2:62:4 | definition of cfg [pointer] | provenance |  |
 | Dsn.go:63:9:63:11 | implicit dereference | Dsn.go:67:102:67:108 | selection of dsn | provenance |  |
-| Dsn.go:63:19:63:25 | selection of Args | Dsn.go:63:19:63:29 | slice expression | provenance |  |
+| Dsn.go:63:19:63:25 | selection of Args | Dsn.go:63:19:63:29 | slice expression | provenance | Src:MaD:1  |
 | Dsn.go:63:19:63:29 | slice expression | Dsn.go:63:9:63:11 | implicit dereference | provenance | FunctionModel |
-| Dsn.go:67:11:67:109 | []type{args} [array] | Dsn.go:67:11:67:109 | call to Sprintf | provenance | MaD:1 |
+| Dsn.go:67:11:67:109 | []type{args} [array] | Dsn.go:67:11:67:109 | call to Sprintf | provenance | MaD:2 |
 | Dsn.go:67:11:67:109 | call to Sprintf | Dsn.go:68:29:68:33 | dbDSN | provenance |  |
 | Dsn.go:67:102:67:104 | cfg [pointer] | Dsn.go:67:102:67:104 | implicit dereference | provenance |  |
 | Dsn.go:67:102:67:104 | implicit dereference | Dsn.go:63:9:63:11 | implicit dereference | provenance |  |
 | Dsn.go:67:102:67:104 | implicit dereference | Dsn.go:67:102:67:108 | selection of dsn | provenance |  |
 | Dsn.go:67:102:67:108 | selection of dsn | Dsn.go:67:11:67:109 | []type{args} [array] | provenance |  |
 | Dsn.go:67:102:67:108 | selection of dsn | Dsn.go:67:11:67:109 | call to Sprintf | provenance | FunctionModel |
 models
-| 1 | Summary: fmt; ; false; Sprintf; ; ; Argument[1].ArrayElement; ReturnValue; taint; manual |
+| 1 | Source: os; ; false; Args; ; ; ; commandargs; manual |
+| 2 | Summary: fmt; ; false; Sprintf; ; ; Argument[1].ArrayElement; ReturnValue; taint; manual |
 nodes
 | Dsn.go:26:11:26:17 | selection of Args | semmle.label | selection of Args |
 | Dsn.go:28:11:28:110 | []type{args} [array] | semmle.label | []type{args} [array] |

diff --git a/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/commandargs/test.expected b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/commandargs/test.expected
@@ -0,0 +1,2 @@
+testFailures
+invalidModelRow
diff --git a/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/commandargs/test.ext.yml b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/commandargs/test.ext.yml
@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/threat-models
+      extensible: threatModelConfiguration
+    data:
+      - ["commandargs", true, 0]
diff --git a/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/commandargs/test.ql b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/commandargs/test.ql
@@ -0,0 +1,19 @@
+import go
+import ModelValidation
+import TestUtilities.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/commandargs/test_os.go b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/commandargs/test_os.go
@@ -0,0 +1,9 @@
+package test
+
+import "os"
+
+func loopThroughCommandArgs() {
+	for _, arg := range os.Args { // $ source
+		_ = arg
+	}
+}