From 63a3054aeba6af09c16ffdd70d284addb2c3f078 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 9 Oct 2024 11:56:41 -0400 Subject: [PATCH 1/6] `os.Args` variable read --- go/ql/lib/semmle/go/frameworks/stdlib/Os.qll | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/go/ql/lib/semmle/go/frameworks/stdlib/Os.qll b/go/ql/lib/semmle/go/frameworks/stdlib/Os.qll index 72ea4cc6c573..7218d9d3cb4b 100644 --- a/go/ql/lib/semmle/go/frameworks/stdlib/Os.qll +++ b/go/ql/lib/semmle/go/frameworks/stdlib/Os.qll @@ -43,4 +43,10 @@ module Os { input = inp and output = outp } } + + private class ArgsSource extends SourceNode { + ArgsSource() { exists(Variable v | v.hasQualifiedName("os", "Args") | this = v.getARead()) } + + override string getThreatModel() { result = "commandargs" } + } } From 3f9af5bfe4a8aaaa0bf5af1e6aa0b609146255cd Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Fri, 13 Dec 2024 12:39:59 -0500 Subject: [PATCH 2/6] Tests --- .../local/commandargs/test.expected | 3 +++ .../local/commandargs/test.ext.yml | 6 ++++++ .../flowsources/local/commandargs/test.ql | 19 +++++++++++++++++++ .../flowsources/local/commandargs/test_os.go | 9 +++++++++ 4 files changed, 37 insertions(+) create mode 100644 go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/commandargs/test.expected create mode 100644 go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/commandargs/test.ext.yml create mode 100644 go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/commandargs/test.ql create mode 100644 go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/commandargs/test_os.go 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 new file mode 100644 index 000000000000..db33d6d2504a --- /dev/null +++ b/go/ql/test/library-tests/semmle/go/dataflow/flowsources/local/commandargs/test.expected @@ -0,0 +1,3 @@ +testFailures +invalidModelRow +failures 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 new file mode 100644 index 000000000000..c720e53fd7b8 --- /dev/null +++ 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] \ No newline at end of file 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 new file mode 100644 index 000000000000..eb7ba46508e7 --- /dev/null +++ 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 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 new file mode 100644 index 000000000000..f84347749abd --- /dev/null +++ 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 + } +} From f8cfa394929c29f8eebfa33f6ecb68024cd60f0b Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Fri, 13 Dec 2024 12:40:16 -0500 Subject: [PATCH 3/6] Change note --- go/ql/lib/change-notes/2024-12-13-os-args-model.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 go/ql/lib/change-notes/2024-12-13-os-args-model.md 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 new file mode 100644 index 000000000000..20a16d222e44 --- /dev/null +++ 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. + From f844105722a11db22a59e6f52e9c6a3ec54c6331 Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Fri, 13 Dec 2024 14:53:58 -0500 Subject: [PATCH 4/6] Fix test result --- .../go/dataflow/flowsources/local/commandargs/test.expected | 1 - 1 file changed, 1 deletion(-) 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 index db33d6d2504a..55e9aed2e93c 100644 --- 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 @@ -1,3 +1,2 @@ testFailures invalidModelRow -failures From 88256e269ad70f5587d49e7b75f1b66cdd524f26 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Fri, 13 Dec 2024 14:59:32 -0500 Subject: [PATCH 5/6] Convert model from QL to MaD --- go/ql/lib/ext/os.model.yml | 1 + go/ql/lib/semmle/go/frameworks/stdlib/Os.qll | 6 ------ 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/go/ql/lib/ext/os.model.yml b/go/ql/lib/ext/os.model.yml index 2c1c64db93ac..b4f074146b79 100644 --- 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/lib/semmle/go/frameworks/stdlib/Os.qll b/go/ql/lib/semmle/go/frameworks/stdlib/Os.qll index 7218d9d3cb4b..72ea4cc6c573 100644 --- a/go/ql/lib/semmle/go/frameworks/stdlib/Os.qll +++ b/go/ql/lib/semmle/go/frameworks/stdlib/Os.qll @@ -43,10 +43,4 @@ module Os { input = inp and output = outp } } - - private class ArgsSource extends SourceNode { - ArgsSource() { exists(Variable v | v.hasQualifiedName("os", "Args") | this = v.getARead()) } - - override string getThreatModel() { result = "commandargs" } - } } From 7852c8666c53716374e142288f16b8b0c4a9ed77 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Fri, 13 Dec 2024 15:22:17 -0500 Subject: [PATCH 6/6] Update provenance in test results --- .../experimental/CWE-74/DsnInjectionLocal.expected | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/go/ql/test/experimental/CWE-74/DsnInjectionLocal.expected b/go/ql/test/experimental/CWE-74/DsnInjectionLocal.expected index ff83f06ebb28..634d637c5881 100644 --- 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,9 +12,9 @@ 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 | | @@ -22,7 +22,8 @@ edges | 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] |