Merge pull request #18235 from owen-mc/go/varargs-out-param

Go: Improve data flow out of variadic parameter

go/ql/lib/change-notes/2024-12-06-improve-flow-out-of-variadic-parameter.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Data flow out of variadic parameters now works in more situations. Summary models defined using models-as-data work. Source models defined using models-as-data do not work yet.

go/ql/lib/semmle/go/dataflow/internal/ContainerFlow.qll

@@ -66,6 +66,11 @@ predicate containerReadStep(Node node1, Node node2, Content c) {
     (
       node2.(Read).readsElement(node1, _)
       or
+      exists(ImplicitVarargsSlice ivs |
+        node1.(PostUpdateNode).getPreUpdateNode() = ivs and
+        node2.(PostUpdateNode).getPreUpdateNode() = ivs.getCallNode().getAnImplicitVarargsArgument()
+      )
+      or
       node2.(RangeElementNode).getBase() = node1
       or
       // To model data flow from array elements of the base of a `SliceNode` to

go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll

@@ -845,6 +845,9 @@ module Public {
         or
         preupd = getAWrittenNode()
         or
+        preupd instanceof ImplicitVarargsSlice and
+        mutableType(preupd.(ImplicitVarargsSlice).getType().(SliceType).getElementType())
+        or
         preupd = any(ArgumentNode arg).getACorrespondingSyntacticArgument() and
         mutableType(preupd.getType())
       ) and

go/ql/test/library-tests/semmle/go/dataflow/ExternalValueFlow/completetest.ql

@@ -9,9 +9,9 @@ import semmle.go.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
 import TestUtilities.InlineFlowTest
 
 module Config implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node src) { sourceNode(src, "qltest") }
+  predicate isSource(DataFlow::Node source) { sourceNode(source, "qltest") }
 
-  predicate isSink(DataFlow::Node src) { sinkNode(src, "qltest") }
+  predicate isSink(DataFlow::Node sink) { sinkNode(sink, "qltest") }
 }
 
 import ValueFlowTest<Config>

go/ql/test/library-tests/semmle/go/dataflow/VarArgs/main.go

@@ -4,7 +4,7 @@ func source() string {
 	return "untrusted data"
 }
 
-func sink(string) {
+func sink(any) {
 }
 
 type A struct {
@@ -19,6 +19,10 @@ func functionWithVarArgsParameter(s ...string) string {
 	return s[1]
 }
 
+func functionWithVarArgsOutParameter(in string, out ...*string) {
+	*out[0] = in
+}
+
 func functionWithSliceOfStructsParameter(s []A) string {
 	return s[1].f
 }
@@ -38,6 +42,12 @@ func main() {
 	sink(functionWithVarArgsParameter(sSlice...)) // $ hasValueFlow="call to functionWithVarArgsParameter"
 	sink(functionWithVarArgsParameter(s0, s1))    // $ hasValueFlow="call to functionWithVarArgsParameter"
 
+	var out1 *string
+	var out2 *string
+	functionWithVarArgsOutParameter(source(), out1, out2)
+	sink(out1) // $ MISSING: hasValueFlow="out1"
+	sink(out2) // $ MISSING: hasValueFlow="out2"
+
 	sliceOfStructs := []A{{f: source()}}
 	sink(sliceOfStructs[0].f) // $ hasValueFlow="selection of f"
 

go/ql/test/library-tests/semmle/go/dataflow/VarArgsWithExternalFlow/Flows.expected

@@ -0,0 +1,2 @@
+testFailures
+invalidModelRow

go/ql/test/library-tests/semmle/go/dataflow/VarArgsWithExternalFlow/Flows.ext.yml

@@ -0,0 +1,21 @@
+extensions:
+  - addsTo:
+      pack: codeql/go-all
+      extensible: summaryModel
+    data:
+      - ["github.com/nonexistent/test", "", False, "FunctionWithParameter", "", "", "Argument[0]", "ReturnValue", "value", "manual"]
+      - ["github.com/nonexistent/test", "", False, "FunctionWithSliceParameter", "", "", "Argument[0].ArrayElement", "ReturnValue", "value", "manual"]
+      - ["github.com/nonexistent/test", "", False, "FunctionWithVarArgsParameter", "", "", "Argument[0].ArrayElement", "ReturnValue", "value", "manual"]
+      - ["github.com/nonexistent/test", "", False, "FunctionWithVarArgsOutParameter", "", "", "Argument[0]", "Argument[1].ArrayElement", "value", "manual"]
+      - ["github.com/nonexistent/test", "", False, "FunctionWithSliceOfStructsParameter", "", "", "Argument[0].ArrayElement.Field[github.com/nonexistent/test.A.Field]", "ReturnValue", "value", "manual"]
+      - ["github.com/nonexistent/test", "", False, "FunctionWithVarArgsOfStructsParameter", "", "", "Argument[0].ArrayElement.Field[github.com/nonexistent/test.A.Field]", "ReturnValue", "value", "manual"]
+  - addsTo:
+      pack: codeql/go-all
+      extensible: sourceModel
+    data:
+      - ["github.com/nonexistent/test", "", False, "VariadicSource", "", "", "Argument[0]", "qltest", "manual"]
+  - addsTo:
+      pack: codeql/go-all
+      extensible: sinkModel
+    data:
+      - ["github.com/nonexistent/test", "", False, "VariadicSink", "", "", "Argument[0]", "qltest", "manual"]

go/ql/test/library-tests/semmle/go/dataflow/VarArgsWithExternalFlow/Flows.ql

@@ -0,0 +1,22 @@
+import go
+import semmle.go.dataflow.ExternalFlow
+import ModelValidation
+import TestUtilities.InlineFlowTest
+
+module Config implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    sourceNode(source, "qltest")
+    or
+    exists(Function fn | fn.hasQualifiedName(_, ["source", "taint"]) |
+      source = fn.getACall().getResult()
+    )
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    sinkNode(sink, "qltest")
+    or
+    exists(Function fn | fn.hasQualifiedName(_, "sink") | sink = fn.getACall().getAnArgument())
+  }
+}
+
+import FlowTest<Config, Config>

go/ql/test/library-tests/semmle/go/dataflow/VarArgsWithExternalFlow/go.mod

@@ -0,0 +1,5 @@
+module semmle.go.Packages
+
+go 1.23
+
+require github.com/nonexistent/test v0.0.0-20200203000000-0000000000000

go/ql/test/library-tests/semmle/go/dataflow/VarArgsWithExternalFlow/main.go

@@ -0,0 +1,51 @@
+package main
+
+import (
+	"github.com/nonexistent/test"
+)
+
+func source() string {
+	return "untrusted data"
+}
+
+func sink(any) {
+}
+
+func main() {
+	s := source()
+	sink(test.FunctionWithParameter(s)) // $ hasValueFlow="call to FunctionWithParameter"
+
+	stringSlice := []string{source()}
+	sink(stringSlice[0]) // $ hasValueFlow="index expression"
+
+	s0 := ""
+	s1 := source()
+	sSlice := []string{s0, s1}
+	sink(test.FunctionWithParameter(sSlice[1]))        // $ hasValueFlow="call to FunctionWithParameter"
+	sink(test.FunctionWithSliceParameter(sSlice))      // $ hasValueFlow="call to FunctionWithSliceParameter"
+	sink(test.FunctionWithVarArgsParameter(sSlice...)) // $ hasValueFlow="call to FunctionWithVarArgsParameter"
+	sink(test.FunctionWithVarArgsParameter(s0, s1))    // $ hasValueFlow="call to FunctionWithVarArgsParameter"
+
+	var out1 *string
+	var out2 *string
+	test.FunctionWithVarArgsOutParameter(source(), out1, out2)
+	sink(out1) // $ hasValueFlow="out1"
+	sink(out2) // $ hasValueFlow="out2"
+
+	sliceOfStructs := []test.A{{Field: source()}}
+	sink(sliceOfStructs[0].Field) // $ hasValueFlow="selection of Field"
+
+	a0 := test.A{Field: ""}
+	a1 := test.A{Field: source()}
+	aSlice := []test.A{a0, a1}
+	sink(test.FunctionWithSliceOfStructsParameter(aSlice))      // $ hasValueFlow="call to FunctionWithSliceOfStructsParameter"
+	sink(test.FunctionWithVarArgsOfStructsParameter(aSlice...)) // $ hasValueFlow="call to FunctionWithVarArgsOfStructsParameter"
+	sink(test.FunctionWithVarArgsOfStructsParameter(a0, a1))    // $ hasValueFlow="call to FunctionWithVarArgsOfStructsParameter"
+
+	var variadicSource string
+	test.VariadicSource(&variadicSource)
+	sink(variadicSource) // $ MISSING: hasTaintFlow="variadicSource"
+
+	test.VariadicSink(source()) // $ hasTaintFlow="[]type{args}"
+
+}

go/ql/test/library-tests/semmle/go/dataflow/VarArgsWithExternalFlow/vendor/modules.txt

@@ -0,0 +1,3 @@
+# github.com/nonexistent/test v0.0.0-20200203000000-0000000000000
+## explicit
+github.com/nonexistent/test

go/ql/test/library-tests/semmle/go/dataflow/VarArgsWithFunctionModels/Flows.ql

@@ -19,6 +19,9 @@ class SummaryModelTest extends DataFlow::FunctionModel {
     this.hasQualifiedName("github.com/nonexistent/test", "FunctionWithVarArgsParameter") and
     (inp.isParameter(_) and outp.isResult())
     or
+    this.hasQualifiedName("github.com/nonexistent/test", "FunctionWithVarArgsOutParameter") and
+    (inp.isParameter(0) and outp.isParameter(any(int i | i >= 1)))
+    or
     this.hasQualifiedName("github.com/nonexistent/test", "FunctionWithSliceOfStructsParameter") and
     (inp.isParameter(0) and outp.isResult())
     or

go/ql/test/library-tests/semmle/go/dataflow/VarArgsWithFunctionModels/go.mod

@@ -1,5 +1,5 @@
 module semmle.go.Packages
 
-go 1.17
+go 1.23
 
 require github.com/nonexistent/test v0.0.0-20200203000000-0000000000000

go/ql/test/library-tests/semmle/go/dataflow/VarArgsWithFunctionModels/main.go

@@ -8,7 +8,7 @@ func source() string {
 	return "untrusted data"
 }
 
-func sink(string) {
+func sink(any) {
 }
 
 func main() {
@@ -21,10 +21,17 @@ func main() {
 	s0 := ""
 	s1 := source()
 	sSlice := []string{s0, s1}
-	sink(test.FunctionWithParameter(sSlice[1]))        // $ hasValueFlow="call to FunctionWithParameter"
-	sink(test.FunctionWithSliceParameter(sSlice))      // $ hasTaintFlow="call to FunctionWithSliceParameter" MISSING: hasValueFlow="call to FunctionWithSliceParameter"
-	sink(test.FunctionWithVarArgsParameter(sSlice...)) // $ hasTaintFlow="call to FunctionWithVarArgsParameter" MISSING: hasValueFlow="call to FunctionWithVarArgsParameter"
-	sink(test.FunctionWithVarArgsParameter(s0, s1))    // $ MISSING: hasValueFlow="call to FunctionWithVarArgsParameter"
+	sink(test.FunctionWithParameter(sSlice[1]))           // $ hasValueFlow="call to FunctionWithParameter"
+	sink(test.FunctionWithSliceParameter(sSlice))         // $ hasTaintFlow="call to FunctionWithSliceParameter" MISSING: hasValueFlow="call to FunctionWithSliceParameter"
+	sink(test.FunctionWithVarArgsParameter(sSlice...))    // $ hasTaintFlow="call to FunctionWithVarArgsParameter" MISSING: hasValueFlow="call to FunctionWithVarArgsParameter"
+	randomFunctionWithMoreThanOneParameter(1, 2, 3, 4, 5) // This is needed to make the next line pass, because we need to have seen a call to a function with at least 2 parameters for ParameterInput to exist with index 1.
+	sink(test.FunctionWithVarArgsParameter(s0, s1))       // $ hasValueFlow="call to FunctionWithVarArgsParameter"
+
+	var out1 *string
+	var out2 *string
+	test.FunctionWithVarArgsOutParameter(source(), out1, out2)
+	sink(out1) // $ hasValueFlow="out1"
+	sink(out2) // $ hasValueFlow="out2"
 
 	sliceOfStructs := []test.A{{Field: source()}}
 	sink(sliceOfStructs[0].Field) // $ hasValueFlow="selection of Field"
@@ -37,3 +44,6 @@ func main() {
 	sink(test.FunctionWithVarArgsOfStructsParameter(aSlice...)) // $ MISSING: hasValueFlow="call to FunctionWithVarArgsOfStructsParameter"
 	sink(test.FunctionWithVarArgsOfStructsParameter(a0, a1))    // $ MISSING: hasValueFlow="call to FunctionWithVarArgsOfStructsParameter"
 }
+
+func randomFunctionWithMoreThanOneParameter(i1, i2, i3, i4, i5 int) {
+}

go/ql/test/library-tests/semmle/go/frameworks/BeegoOrm/StoredXss.expected

@@ -24,7 +24,11 @@ edges
 | test.go:148:16:148:23 | &... | test.go:149:13:149:39 | type conversion | provenance |  |
 | test.go:152:15:152:24 | &... | test.go:153:13:153:47 | type conversion | provenance |  |
 | test.go:156:18:156:30 | &... | test.go:157:13:157:38 | type conversion | provenance |  |
+| test.go:160:2:160:23 | []type{args} [array] | test.go:160:14:160:22 | &... | provenance |  |
+| test.go:160:14:160:22 | &... | test.go:160:2:160:23 | []type{args} [array] | provenance |  |
 | test.go:160:14:160:22 | &... | test.go:161:13:161:28 | type conversion | provenance |  |
+| test.go:164:2:164:25 | []type{args} [array] | test.go:164:15:164:24 | &... | provenance |  |
+| test.go:164:15:164:24 | &... | test.go:164:2:164:25 | []type{args} [array] | provenance |  |
 | test.go:164:15:164:24 | &... | test.go:165:13:165:32 | type conversion | provenance |  |
 nodes
 | test.go:80:13:80:16 | &... | semmle.label | &... |
@@ -76,8 +80,10 @@ nodes
 | test.go:153:13:153:47 | type conversion | semmle.label | type conversion |
 | test.go:156:18:156:30 | &... | semmle.label | &... |
 | test.go:157:13:157:38 | type conversion | semmle.label | type conversion |
+| test.go:160:2:160:23 | []type{args} [array] | semmle.label | []type{args} [array] |
 | test.go:160:14:160:22 | &... | semmle.label | &... |
 | test.go:161:13:161:28 | type conversion | semmle.label | type conversion |
+| test.go:164:2:164:25 | []type{args} [array] | semmle.label | []type{args} [array] |
 | test.go:164:15:164:24 | &... | semmle.label | &... |
 | test.go:165:13:165:32 | type conversion | semmle.label | type conversion |
 subpaths

go/ql/test/query-tests/Security/CWE-078/StoredCommand.expected

@@ -3,12 +3,15 @@
 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: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 |
 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} [array] | semmle.label | []type{args} [array] |
 | StoredCommand.go:13:12:13:19 | &... | semmle.label | &... |
 | StoredCommand.go:14:22:14:28 | cmdName | semmle.label | cmdName |
 subpaths