Skip to content

Commit d26dc68

Browse files
owen-mcowen-mc
authored
Merge pull request #14798 from owen-mc/go/improve-value-flow-through-slice-exprs
Go: model value flow with array content through slice expressions
2 parents 0668b71 + 64bf6cc commit d26dc68

File tree

8 files changed

+138
-3
lines changed

8 files changed

+138
-3
lines changed

‎go/ql/lib/change-notes/2023-11-15-bug-fix-value-flow-in-slice-expression.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: fix
3+
---
4+
* A bug has been fixed that meant that value flow through a slice expression was not tracked correctly. Taint flow was tracked correctly.

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,13 @@ predicate containerStoreStep(Node node1, Node node2, Content c) {
2424
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), _, node1))
2525
or
2626
node1 = node2.(ImplicitVarargsSlice).getCallNode().getAnImplicitVarargsArgument()
27+
or
28+
// To model data flow from array elements of the base of a `SliceNode` to
29+
// the `SliceNode` itself, we consider there to be a read step with array
30+
// content from the base to the corresponding `SliceElementNode` and then
31+
// a store step with array content from the `SliceelementNode` to the
32+
// `SliceNode` itself.
33+
node2 = node1.(SliceElementNode).getSliceNode()
2734
)
2835
)
2936
or
@@ -57,6 +64,13 @@ predicate containerReadStep(Node node1, Node node2, Content c) {
5764
)
5865
or
5966
node2.(RangeElementNode).getBase() = node1
67+
or
68+
// To model data flow from array elements of the base of a `SliceNode` to
69+
// the `SliceNode` itself, we consider there to be a read step with array
70+
// content from the base to the corresponding `SliceElementNode` and then
71+
// a store step with array content from the `SliceelementNode` to the
72+
// `SliceNode` itself.
73+
node2.(SliceElementNode).getSliceNode().getBase() = node1
6074
)
6175
or
6276
c instanceof CollectionContent and

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ private newtype TNode =
1111
MkSsaNode(SsaDefinition ssa) or
1212
MkGlobalFunctionNode(Function f) or
1313
MkImplicitVarargsSlice(CallExpr c) { c.hasImplicitVarargs() } or
14+
MkSliceElementNode(SliceExpr se) or
1415
MkFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn)
1516

1617
/** Nodes intended for only use inside the data-flow libraries. */
@@ -998,6 +999,37 @@ module Public {
998999
Node getMax() { result = DataFlow::instructionNode(insn.getMax()) }
9991000
}
10001001

1002+
/**
1003+
* A data-flow node which exists solely to model the value flow from array
1004+
* elements of the base of a `SliceNode` to array elements of the `SliceNode`
1005+
* itself.
1006+
*/
1007+
class SliceElementNode extends Node, MkSliceElementNode {
1008+
IR::SliceInstruction si;
1009+
1010+
SliceElementNode() { this = MkSliceElementNode(si.getExpr()) }
1011+
1012+
override ControlFlow::Root getRoot() { result = this.getSliceNode().getRoot() }
1013+
1014+
override Type getType() {
1015+
result = si.getResultType().(ArrayType).getElementType() or
1016+
result = si.getResultType().(SliceType).getElementType()
1017+
}
1018+
1019+
override string getNodeKind() { result = "slice element node" }
1020+
1021+
override string toString() { result = "slice element node" }
1022+
1023+
override predicate hasLocationInfo(
1024+
string filepath, int startline, int startcolumn, int endline, int endcolumn
1025+
) {
1026+
si.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
1027+
}
1028+
1029+
/** Gets the `SliceNode` which this node relates to. */
1030+
SliceNode getSliceNode() { result = DataFlow::instructionNode(si) }
1031+
}
1032+
10011033
/**
10021034
* A data-flow node corresponding to an expression with a binary operator.
10031035
*/

‎go/ql/test/library-tests/semmle/go/dataflow/ArrayConversion/main.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ func source() string {
44
return "untrusted data"
55
}
66

7-
func sink(string) {
7+
func sink(any) {
88
}
99

1010
func sliceToArray(p []string) [1]string {
@@ -15,11 +15,15 @@ func main() {
1515
// Test the new slice->array conversion permitted in Go 1.20
1616
var a [4]string
1717
a[0] = source()
18-
alias := sliceToArray(a[:])
19-
sink(alias[0]) // $ hasTaintFlow="index expression"
18+
alias := [2]string(a[:])
19+
sink(alias[0]) // $ hasValueFlow="index expression"
20+
sink(alias[1]) // $ SPURIOUS: hasValueFlow="index expression" // we don't distinguish different elements of arrays or slices
21+
sink(alias) // $ hasTaintFlow="alias"
2022

2123
// Compare with the standard dataflow support for arrays
2224
var b [4]string
2325
b[0] = source()
2426
sink(b[0]) // $ hasValueFlow="index expression"
27+
sink(b[1]) // $ SPURIOUS: hasValueFlow="index expression" // we don't distinguish different elements of arrays or slices
28+
sink(b) // $ hasTaintFlow="b"
2529
}

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

Whitespace-only changes.

‎go/ql/test/library-tests/semmle/go/dataflow/SliceExpressions/Flows.ql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
import go
2+
import TestUtilities.InlineFlowTest
3+
import DefaultFlowTest

‎go/ql/test/library-tests/semmle/go/dataflow/SliceExpressions/main.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
package main
2+
3+
func source() string {
4+
return "untrusted data"
5+
}
6+
7+
func sink(any) {
8+
}
9+
10+
func main() {
11+
}
12+
13+
// Value flow with array content through slice expressions
14+
15+
func arrayBase(base [4]string) {
16+
base[1] = source()
17+
slice := base[1:4]
18+
sink(slice[0]) // $ hasValueFlow="index expression"
19+
sink(slice[1]) // $ SPURIOUS: hasValueFlow="index expression" // we don't distinguish different elements of arrays or slices
20+
sink(slice) // $ hasTaintFlow="slice"
21+
}
22+
23+
func arrayPointerBase(base *[4]string) {
24+
base[1] = source()
25+
slice := base[1:4]
26+
sink(slice[0]) // $ hasValueFlow="index expression"
27+
sink(slice[1]) // $ SPURIOUS: hasValueFlow="index expression" // we don't distinguish different elements of arrays or slices
28+
sink(slice) // $ hasTaintFlow="slice"
29+
}
30+
31+
func sliceBase(base []string) {
32+
base[1] = source()
33+
slice := base[1:4]
34+
sink(slice[0]) // $ hasValueFlow="index expression"
35+
sink(slice[1]) // $ SPURIOUS: hasValueFlow="index expression" // we don't distinguish different elements of arrays or slices
36+
sink(slice) // $ hasTaintFlow="slice"
37+
}
38+
39+
func slicePointerBase(base *[]string) {
40+
(*base)[1] = source()
41+
slice := (*base)[1:4]
42+
sink(slice[0]) // $ hasValueFlow="index expression"
43+
sink(slice[1]) // $ SPURIOUS: hasValueFlow="index expression" // we don't distinguish different elements of arrays or slices
44+
sink(slice) // $ hasTaintFlow="slice"
45+
}

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,17 @@ edges
1010
| GitSubcommands.go:10:13:10:27 | call to Query | GitSubcommands.go:15:35:15:41 | tainted |
1111
| GitSubcommands.go:10:13:10:27 | call to Query | GitSubcommands.go:16:36:16:42 | tainted |
1212
| SanitizingDoubleDash.go:9:13:9:19 | selection of URL | SanitizingDoubleDash.go:9:13:9:27 | call to Query |
13+
| SanitizingDoubleDash.go:9:13:9:27 | call to Query | SanitizingDoubleDash.go:13:25:13:31 | tainted |
1314
| SanitizingDoubleDash.go:9:13:9:27 | call to Query | SanitizingDoubleDash.go:14:23:14:33 | slice expression |
1415
| SanitizingDoubleDash.go:9:13:9:27 | call to Query | SanitizingDoubleDash.go:39:31:39:37 | tainted |
1516
| SanitizingDoubleDash.go:9:13:9:27 | call to Query | SanitizingDoubleDash.go:53:21:53:28 | arrayLit |
1617
| SanitizingDoubleDash.go:9:13:9:27 | call to Query | SanitizingDoubleDash.go:68:31:68:37 | tainted |
1718
| SanitizingDoubleDash.go:9:13:9:27 | call to Query | SanitizingDoubleDash.go:80:23:80:29 | tainted |
19+
| SanitizingDoubleDash.go:13:15:13:32 | array literal [array] | SanitizingDoubleDash.go:14:23:14:30 | arrayLit [array] |
20+
| SanitizingDoubleDash.go:13:25:13:31 | tainted | SanitizingDoubleDash.go:13:15:13:32 | array literal [array] |
21+
| SanitizingDoubleDash.go:14:23:14:30 | arrayLit [array] | SanitizingDoubleDash.go:14:23:14:33 | slice element node |
22+
| SanitizingDoubleDash.go:14:23:14:33 | slice element node | SanitizingDoubleDash.go:14:23:14:33 | slice expression [array] |
23+
| SanitizingDoubleDash.go:14:23:14:33 | slice expression [array] | SanitizingDoubleDash.go:14:23:14:33 | slice expression |
1824
| SanitizingDoubleDash.go:39:14:39:44 | call to append | SanitizingDoubleDash.go:40:23:40:30 | arrayLit |
1925
| SanitizingDoubleDash.go:39:31:39:37 | tainted | SanitizingDoubleDash.go:39:14:39:44 | call to append |
2026
| SanitizingDoubleDash.go:53:14:53:35 | call to append | SanitizingDoubleDash.go:54:23:54:30 | arrayLit |
@@ -24,7 +30,9 @@ edges
2430
| SanitizingDoubleDash.go:69:14:69:35 | call to append | SanitizingDoubleDash.go:70:23:70:30 | arrayLit |
2531
| SanitizingDoubleDash.go:69:21:69:28 | arrayLit | SanitizingDoubleDash.go:69:14:69:35 | call to append |
2632
| SanitizingDoubleDash.go:92:13:92:19 | selection of URL | SanitizingDoubleDash.go:92:13:92:27 | call to Query |
33+
| SanitizingDoubleDash.go:92:13:92:27 | call to Query | SanitizingDoubleDash.go:95:25:95:31 | tainted |
2734
| SanitizingDoubleDash.go:92:13:92:27 | call to Query | SanitizingDoubleDash.go:96:24:96:34 | slice expression |
35+
| SanitizingDoubleDash.go:92:13:92:27 | call to Query | SanitizingDoubleDash.go:100:31:100:37 | tainted |
2836
| SanitizingDoubleDash.go:92:13:92:27 | call to Query | SanitizingDoubleDash.go:101:24:101:34 | slice expression |
2937
| SanitizingDoubleDash.go:92:13:92:27 | call to Query | SanitizingDoubleDash.go:105:30:105:36 | tainted |
3038
| SanitizingDoubleDash.go:92:13:92:27 | call to Query | SanitizingDoubleDash.go:106:24:106:31 | arrayLit |
@@ -36,6 +44,16 @@ edges
3644
| SanitizingDoubleDash.go:92:13:92:27 | call to Query | SanitizingDoubleDash.go:142:31:142:37 | tainted |
3745
| SanitizingDoubleDash.go:92:13:92:27 | call to Query | SanitizingDoubleDash.go:148:30:148:36 | tainted |
3846
| SanitizingDoubleDash.go:92:13:92:27 | call to Query | SanitizingDoubleDash.go:152:24:152:30 | tainted |
47+
| SanitizingDoubleDash.go:95:15:95:32 | array literal [array] | SanitizingDoubleDash.go:96:24:96:31 | arrayLit [array] |
48+
| SanitizingDoubleDash.go:95:25:95:31 | tainted | SanitizingDoubleDash.go:95:15:95:32 | array literal [array] |
49+
| SanitizingDoubleDash.go:96:24:96:31 | arrayLit [array] | SanitizingDoubleDash.go:96:24:96:34 | slice element node |
50+
| SanitizingDoubleDash.go:96:24:96:34 | slice element node | SanitizingDoubleDash.go:96:24:96:34 | slice expression [array] |
51+
| SanitizingDoubleDash.go:96:24:96:34 | slice expression [array] | SanitizingDoubleDash.go:96:24:96:34 | slice expression |
52+
| SanitizingDoubleDash.go:100:15:100:38 | array literal [array] | SanitizingDoubleDash.go:101:24:101:31 | arrayLit [array] |
53+
| SanitizingDoubleDash.go:100:31:100:37 | tainted | SanitizingDoubleDash.go:100:15:100:38 | array literal [array] |
54+
| SanitizingDoubleDash.go:101:24:101:31 | arrayLit [array] | SanitizingDoubleDash.go:101:24:101:34 | slice element node |
55+
| SanitizingDoubleDash.go:101:24:101:34 | slice element node | SanitizingDoubleDash.go:101:24:101:34 | slice expression [array] |
56+
| SanitizingDoubleDash.go:101:24:101:34 | slice expression [array] | SanitizingDoubleDash.go:101:24:101:34 | slice expression |
3957
| SanitizingDoubleDash.go:105:15:105:37 | slice literal [array] | SanitizingDoubleDash.go:106:24:106:31 | arrayLit |
4058
| SanitizingDoubleDash.go:105:30:105:36 | tainted | SanitizingDoubleDash.go:105:15:105:37 | slice literal [array] |
4159
| SanitizingDoubleDash.go:111:14:111:44 | call to append | SanitizingDoubleDash.go:112:24:112:31 | arrayLit |
@@ -68,7 +86,12 @@ nodes
6886
| GitSubcommands.go:16:36:16:42 | tainted | semmle.label | tainted |
6987
| SanitizingDoubleDash.go:9:13:9:19 | selection of URL | semmle.label | selection of URL |
7088
| SanitizingDoubleDash.go:9:13:9:27 | call to Query | semmle.label | call to Query |
89+
| SanitizingDoubleDash.go:13:15:13:32 | array literal [array] | semmle.label | array literal [array] |
90+
| SanitizingDoubleDash.go:13:25:13:31 | tainted | semmle.label | tainted |
91+
| SanitizingDoubleDash.go:14:23:14:30 | arrayLit [array] | semmle.label | arrayLit [array] |
92+
| SanitizingDoubleDash.go:14:23:14:33 | slice element node | semmle.label | slice element node |
7193
| SanitizingDoubleDash.go:14:23:14:33 | slice expression | semmle.label | slice expression |
94+
| SanitizingDoubleDash.go:14:23:14:33 | slice expression [array] | semmle.label | slice expression [array] |
7295
| SanitizingDoubleDash.go:39:14:39:44 | call to append | semmle.label | call to append |
7396
| SanitizingDoubleDash.go:39:31:39:37 | tainted | semmle.label | tainted |
7497
| SanitizingDoubleDash.go:40:23:40:30 | arrayLit | semmle.label | arrayLit |
@@ -83,8 +106,18 @@ nodes
83106
| SanitizingDoubleDash.go:80:23:80:29 | tainted | semmle.label | tainted |
84107
| SanitizingDoubleDash.go:92:13:92:19 | selection of URL | semmle.label | selection of URL |
85108
| SanitizingDoubleDash.go:92:13:92:27 | call to Query | semmle.label | call to Query |
109+
| SanitizingDoubleDash.go:95:15:95:32 | array literal [array] | semmle.label | array literal [array] |
110+
| SanitizingDoubleDash.go:95:25:95:31 | tainted | semmle.label | tainted |
111+
| SanitizingDoubleDash.go:96:24:96:31 | arrayLit [array] | semmle.label | arrayLit [array] |
112+
| SanitizingDoubleDash.go:96:24:96:34 | slice element node | semmle.label | slice element node |
86113
| SanitizingDoubleDash.go:96:24:96:34 | slice expression | semmle.label | slice expression |
114+
| SanitizingDoubleDash.go:96:24:96:34 | slice expression [array] | semmle.label | slice expression [array] |
115+
| SanitizingDoubleDash.go:100:15:100:38 | array literal [array] | semmle.label | array literal [array] |
116+
| SanitizingDoubleDash.go:100:31:100:37 | tainted | semmle.label | tainted |
117+
| SanitizingDoubleDash.go:101:24:101:31 | arrayLit [array] | semmle.label | arrayLit [array] |
118+
| SanitizingDoubleDash.go:101:24:101:34 | slice element node | semmle.label | slice element node |
87119
| SanitizingDoubleDash.go:101:24:101:34 | slice expression | semmle.label | slice expression |
120+
| SanitizingDoubleDash.go:101:24:101:34 | slice expression [array] | semmle.label | slice expression [array] |
88121
| SanitizingDoubleDash.go:105:15:105:37 | slice literal [array] | semmle.label | slice literal [array] |
89122
| SanitizingDoubleDash.go:105:30:105:36 | tainted | semmle.label | tainted |
90123
| SanitizingDoubleDash.go:106:24:106:31 | arrayLit | semmle.label | arrayLit |

0 commit comments

Comments
 (0)