Skip to content

Go: allow read and store steps from named types #16504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 16, 2024
Merged

Go: allow read and store steps from named types #16504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1af3374
Add tests for data flow through ranged for loops
owen-mc May 15, 2024
21ff705
Fix bug with read/store steps and named types
owen-mc May 15, 2024
6ffa821
Add change note
owen-mc May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions go/ql/lib/change-notes/2024-05-16-read-store-steps-named-types.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* A bug has been fixed which meant flow was not followed through some ranged for loops. This may lead to more alerts being found.
118 changes: 61 additions & 57 deletions go/ql/lib/semmle/go/dataflow/internal/ContainerFlow.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,38 +14,40 @@ private import semmle.go.dataflow.ExternalFlow
* varargs.
*/
predicate containerStoreStep(Node node1, Node node2, Content c) {
c instanceof ArrayContent and
(
exists(Type t | t = node2.getType().getUnderlyingType() |
c instanceof ArrayContent and
(
node2.getType() instanceof ArrayType or
node2.getType() instanceof SliceType
) and
(
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), _, node1))
or
node1 = node2.(ImplicitVarargsSlice).getCallNode().getAnImplicitVarargsArgument()
or
// To model data flow from array elements of the base of a `SliceNode` to
// the `SliceNode` itself, we consider there to be a read step with array
// content from the base to the corresponding `SliceElementNode` and then
// a store step with array content from the `SliceelementNode` to the
// `SliceNode` itself.
node2 = node1.(SliceElementNode).getSliceNode()
(
t instanceof ArrayType or
t instanceof SliceType
) and
(
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), _, node1))
or
node1 = node2.(ImplicitVarargsSlice).getCallNode().getAnImplicitVarargsArgument()
or
// To model data flow from array elements of the base of a `SliceNode` to
// the `SliceNode` itself, we consider there to be a read step with array
// content from the base to the corresponding `SliceElementNode` and then
// a store step with array content from the `SliceelementNode` to the
// `SliceNode` itself.
node2 = node1.(SliceElementNode).getSliceNode()
)
)
or
c instanceof CollectionContent and
exists(SendStmt send |
send.getChannel() = node2.(ExprNode).asExpr() and send.getValue() = node1.(ExprNode).asExpr()
)
or
c instanceof MapKeyContent and
t instanceof MapType and
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), node1, _))
or
c instanceof MapValueContent and
t instanceof MapType and
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), _, node1))
)
or
c instanceof CollectionContent and
exists(SendStmt send |
send.getChannel() = node2.(ExprNode).asExpr() and send.getValue() = node1.(ExprNode).asExpr()
)
or
c instanceof MapKeyContent and
node2.getType() instanceof MapType and
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), node1, _))
or
c instanceof MapValueContent and
node2.getType() instanceof MapType and
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), _, node1))
}

/**
Expand All @@ -55,35 +57,37 @@ predicate containerStoreStep(Node node1, Node node2, Content c) {
* as well as array iteration through enhanced `for` statements.
*/
predicate containerReadStep(Node node1, Node node2, Content c) {
c instanceof ArrayContent and
(
node1.getType() instanceof ArrayType or
node1.getType() instanceof SliceType
) and
(
node2.(Read).readsElement(node1, _)
exists(Type t | t = node1.getType().getUnderlyingType() |
c instanceof ArrayContent and
(
t instanceof ArrayType or
t instanceof SliceType
) and
(
node2.(Read).readsElement(node1, _)
or
node2.(RangeElementNode).getBase() = node1
or
// To model data flow from array elements of the base of a `SliceNode` to
// the `SliceNode` itself, we consider there to be a read step with array
// content from the base to the corresponding `SliceElementNode` and then
// a store step with array content from the `SliceelementNode` to the
// `SliceNode` itself.
node2.(SliceElementNode).getSliceNode().getBase() = node1
)
or
node2.(RangeElementNode).getBase() = node1
c instanceof CollectionContent and
exists(UnaryOperationNode recv | recv = node2 |
node1 = recv.getOperand() and
recv.getOperator() = "<-"
)
or
// To model data flow from array elements of the base of a `SliceNode` to
// the `SliceNode` itself, we consider there to be a read step with array
// content from the base to the corresponding `SliceElementNode` and then
// a store step with array content from the `SliceelementNode` to the
// `SliceNode` itself.
node2.(SliceElementNode).getSliceNode().getBase() = node1
)
or
c instanceof CollectionContent and
exists(UnaryOperationNode recv | recv = node2 |
node1 = recv.getOperand() and
recv.getOperator() = "<-"
c instanceof MapKeyContent and
t instanceof MapType and
node2.(RangeIndexNode).getBase() = node1
or
c instanceof MapValueContent and
t instanceof MapType and
(node2.(Read).readsElement(node1, _) or node2.(RangeElementNode).getBase() = node1)
)
or
c instanceof MapKeyContent and
node1.getType() instanceof MapType and
node2.(RangeIndexNode).getBase() = node1
or
c instanceof MapValueContent and
node1.getType() instanceof MapType and
(node2.(Read).readsElement(node1, _) or node2.(RangeElementNode).getBase() = node1)
}
56 changes: 32 additions & 24 deletions go/ql/test/library-tests/semmle/go/dataflow/ExternalTaintFlow/sinks.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,27 +10,35 @@ invalidModelRow
| test.go:79:10:79:15 | taint6 | qltest |
| test.go:82:10:82:15 | taint7 | qltest |
| test.go:85:10:85:18 | index expression | qltest |
| test.go:89:10:89:15 | taint9 | qltest |
| test.go:92:10:92:33 | call to GetElement | qltest |
| test.go:93:10:93:18 | <-... | qltest |
| test.go:97:10:97:16 | taint11 | qltest |
| test.go:100:10:100:32 | call to GetMapKey | qltest |
| test.go:102:11:102:11 | k | qltest |
| test.go:105:11:105:11 | k | qltest |
| test.go:110:10:110:16 | taint13 | qltest |
| test.go:113:10:113:20 | index expression | qltest |
| test.go:117:10:117:16 | taint15 | qltest |
| test.go:121:10:121:17 | index expression | qltest |
| test.go:127:10:127:18 | index expression | qltest |
| test.go:132:10:132:16 | taint16 | qltest |
| test.go:136:10:136:13 | selection of F | qltest |
| test.go:139:10:139:17 | call to Get | qltest |
| test.go:143:10:143:17 | call to Get | qltest |
| test.go:148:10:148:17 | call to Get | qltest |
| test.go:152:10:152:14 | selection of F | qltest |
| test.go:155:10:155:32 | call to GetThroughPointer | qltest |
| test.go:159:10:159:32 | call to GetThroughPointer | qltest |
| test.go:164:10:164:32 | call to GetThroughPointer | qltest |
| test.go:170:17:170:20 | arg1 | qltest |
| test.go:170:23:170:26 | arg2 | qltest |
| test.go:170:29:170:32 | arg3 | qltest |
| test.go:87:11:87:11 | x | qltest |
| test.go:90:11:90:11 | x | qltest |
| test.go:95:10:95:15 | taint9 | qltest |
| test.go:98:10:98:33 | call to GetElement | qltest |
| test.go:99:10:99:18 | <-... | qltest |
| test.go:101:11:101:11 | e | qltest |
| test.go:104:11:104:11 | e | qltest |
| test.go:109:10:109:16 | taint11 | qltest |
| test.go:112:10:112:32 | call to GetMapKey | qltest |
| test.go:114:11:114:11 | k | qltest |
| test.go:117:11:117:11 | k | qltest |
| test.go:120:11:120:11 | k | qltest |
| test.go:123:11:123:11 | k | qltest |
| test.go:128:10:128:16 | taint13 | qltest |
| test.go:131:10:131:20 | index expression | qltest |
| test.go:133:11:133:11 | v | qltest |
| test.go:136:11:136:11 | v | qltest |
| test.go:141:10:141:16 | taint15 | qltest |
| test.go:145:10:145:17 | index expression | qltest |
| test.go:151:10:151:18 | index expression | qltest |
| test.go:156:10:156:16 | taint16 | qltest |
| test.go:160:10:160:13 | selection of F | qltest |
| test.go:163:10:163:17 | call to Get | qltest |
| test.go:167:10:167:17 | call to Get | qltest |
| test.go:172:10:172:17 | call to Get | qltest |
| test.go:176:10:176:14 | selection of F | qltest |
| test.go:179:10:179:32 | call to GetThroughPointer | qltest |
| test.go:183:10:183:32 | call to GetThroughPointer | qltest |
| test.go:188:10:188:32 | call to GetThroughPointer | qltest |
| test.go:194:17:194:20 | arg1 | qltest |
| test.go:194:23:194:26 | arg2 | qltest |
| test.go:194:29:194:32 | arg3 | qltest |
24 changes: 12 additions & 12 deletions go/ql/test/library-tests/semmle/go/dataflow/ExternalTaintFlow/srcs.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@ invalidModelRow
| test.go:41:2:41:21 | ... = ...[1] | qltest-w-subtypes |
| test.go:42:2:42:22 | ... = ...[1] | qltest-w-subtypes |
| test.go:58:9:58:16 | call to Src1 | qltest |
| test.go:91:46:91:53 | call to Src1 | qltest |
| test.go:95:35:95:42 | call to Src1 | qltest |
| test.go:99:42:99:49 | call to Src1 | qltest |
| test.go:130:8:130:15 | call to Src1 | qltest |
| test.go:135:9:135:16 | call to Src1 | qltest |
| test.go:138:15:138:22 | call to Src1 | qltest |
| test.go:142:9:142:16 | call to Src1 | qltest |
| test.go:146:9:146:16 | call to Src1 | qltest |
| test.go:151:24:151:31 | call to Src1 | qltest |
| test.go:154:17:154:24 | call to Src1 | qltest |
| test.go:158:24:158:31 | call to Src1 | qltest |
| test.go:162:24:162:31 | call to Src1 | qltest |
| test.go:97:46:97:53 | call to Src1 | qltest |
| test.go:107:35:107:42 | call to Src1 | qltest |
| test.go:111:42:111:49 | call to Src1 | qltest |
| test.go:154:8:154:15 | call to Src1 | qltest |
| test.go:159:9:159:16 | call to Src1 | qltest |
| test.go:162:15:162:22 | call to Src1 | qltest |
| test.go:166:9:166:16 | call to Src1 | qltest |
| test.go:170:9:170:16 | call to Src1 | qltest |
| test.go:175:24:175:31 | call to Src1 | qltest |
| test.go:178:17:178:24 | call to Src1 | qltest |
| test.go:182:24:182:31 | call to Src1 | qltest |
| test.go:186:24:186:31 | call to Src1 | qltest |
28 changes: 28 additions & 0 deletions go/ql/test/library-tests/semmle/go/dataflow/ExternalTaintFlow/test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@ func simpleflow() {

taint8 := test.StepArgResArrayContent(src)
b.Sink1(taint8[0]) // $ hasTaintFlow="index expression"
for _, x := range taint8 {
b.Sink1(x) // $ hasTaintFlow="x"
}
for _, x := range arraytype(taint8) {
b.Sink1(x) // $ hasTaintFlow="x"
}

srcArray := []interface{}{nil, src}
taint9 := test.StepArgArrayContentRes(srcArray)
Expand All @@ -91,6 +97,12 @@ func simpleflow() {
taint10 := test.StepArgResCollectionContent(a.Src1()).(chan interface{})
b.Sink1(test.GetElement(taint10)) // $ hasTaintFlow="call to GetElement"
b.Sink1(<-taint10) // $ hasTaintFlow="<-..."
for e := range taint10 {
b.Sink1(e) // $ MISSING: hasTaintFlow="e"
}
for e := range channeltype(taint10) {
b.Sink1(e) // $ MISSING: hasTaintFlow="e"
}

srcCollection := test.SetElement(a.Src1())
taint11 := test.StepArgCollectionContentRes(srcCollection)
Expand All @@ -104,13 +116,25 @@ func simpleflow() {
for k := range taint12 {
b.Sink1(k) // $ hasTaintFlow="k"
}
for k, _ := range mapstringstringtype(taint12) {
b.Sink1(k) // $ hasTaintFlow="k"
}
for k := range mapstringstringtype(taint12) {
b.Sink1(k) // $ hasTaintFlow="k"
}

srcMap13 := map[string]string{src.(string): ""}
taint13 := test.StepArgMapKeyContentRes(srcMap13)
b.Sink1(taint13) // $ hasTaintFlow="taint13"

taint14 := test.StepArgResMapValueContent(src).(map[string]string)
b.Sink1(taint14[""]) // $ hasTaintFlow="index expression"
for _, v := range taint14 {
b.Sink1(v) // $ hasTaintFlow="v"
}
for _, v := range mapstringstringtype(taint14) {
b.Sink1(v) // $ hasTaintFlow="v"
}

srcMap15 := map[string]string{"": src.(string)}
taint15 := test.StepArgMapValueContentRes(srcMap15)
Expand Down Expand Up @@ -169,3 +193,7 @@ func simpleflow() {
arg4 := src
b.SinkManyArgs(arg1, arg2, arg3, arg4) // $ hasTaintFlow="arg1" hasTaintFlow="arg2" hasTaintFlow="arg3"
}

type mapstringstringtype map[string]string
type arraytype []interface{}
type channeltype chan interface{}
68 changes: 38 additions & 30 deletions go/ql/test/library-tests/semmle/go/dataflow/ExternalValueFlow/sinks.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,33 +10,41 @@ invalidModelRow
| test.go:79:10:79:15 | taint6 | qltest |
| test.go:82:10:82:15 | taint7 | qltest |
| test.go:85:10:85:18 | index expression | qltest |
| test.go:89:10:89:15 | taint9 | qltest |
| test.go:92:10:92:33 | call to GetElement | qltest |
| test.go:93:10:93:18 | <-... | qltest |
| test.go:97:10:97:16 | taint11 | qltest |
| test.go:100:10:100:32 | call to GetMapKey | qltest |
| test.go:102:11:102:11 | k | qltest |
| test.go:105:11:105:11 | k | qltest |
| test.go:110:10:110:16 | taint13 | qltest |
| test.go:113:10:113:20 | index expression | qltest |
| test.go:117:10:117:16 | taint15 | qltest |
| test.go:121:10:121:17 | index expression | qltest |
| test.go:127:10:127:18 | index expression | qltest |
| test.go:132:10:132:16 | taint16 | qltest |
| test.go:136:10:136:13 | selection of F | qltest |
| test.go:139:10:139:17 | call to Get | qltest |
| test.go:143:10:143:17 | call to Get | qltest |
| test.go:148:10:148:17 | call to Get | qltest |
| test.go:152:10:152:14 | selection of F | qltest |
| test.go:155:10:155:32 | call to GetThroughPointer | qltest |
| test.go:159:10:159:32 | call to GetThroughPointer | qltest |
| test.go:164:10:164:32 | call to GetThroughPointer | qltest |
| test.go:170:17:170:20 | arg1 | qltest |
| test.go:170:23:170:26 | arg2 | qltest |
| test.go:170:29:170:32 | arg3 | qltest |
| test.go:173:10:173:26 | call to max | qltest |
| test.go:174:10:174:26 | call to max | qltest |
| test.go:175:10:175:26 | call to max | qltest |
| test.go:176:10:176:26 | call to min | qltest |
| test.go:177:10:177:26 | call to min | qltest |
| test.go:178:10:178:26 | call to min | qltest |
| test.go:87:11:87:11 | x | qltest |
| test.go:90:11:90:11 | x | qltest |
| test.go:95:10:95:15 | taint9 | qltest |
| test.go:98:10:98:33 | call to GetElement | qltest |
| test.go:99:10:99:18 | <-... | qltest |
| test.go:101:11:101:11 | e | qltest |
| test.go:104:11:104:11 | e | qltest |
| test.go:109:10:109:16 | taint11 | qltest |
| test.go:112:10:112:32 | call to GetMapKey | qltest |
| test.go:114:11:114:11 | k | qltest |
| test.go:117:11:117:11 | k | qltest |
| test.go:120:11:120:11 | k | qltest |
| test.go:123:11:123:11 | k | qltest |
| test.go:128:10:128:16 | taint13 | qltest |
| test.go:131:10:131:20 | index expression | qltest |
| test.go:133:11:133:11 | v | qltest |
| test.go:136:11:136:11 | v | qltest |
| test.go:141:10:141:16 | taint15 | qltest |
| test.go:145:10:145:17 | index expression | qltest |
| test.go:151:10:151:18 | index expression | qltest |
| test.go:156:10:156:16 | taint16 | qltest |
| test.go:160:10:160:13 | selection of F | qltest |
| test.go:163:10:163:17 | call to Get | qltest |
| test.go:167:10:167:17 | call to Get | qltest |
| test.go:172:10:172:17 | call to Get | qltest |
| test.go:176:10:176:14 | selection of F | qltest |
| test.go:179:10:179:32 | call to GetThroughPointer | qltest |
| test.go:183:10:183:32 | call to GetThroughPointer | qltest |
| test.go:188:10:188:32 | call to GetThroughPointer | qltest |
| test.go:194:17:194:20 | arg1 | qltest |
| test.go:194:23:194:26 | arg2 | qltest |
| test.go:194:29:194:32 | arg3 | qltest |
| test.go:197:10:197:26 | call to max | qltest |
| test.go:198:10:198:26 | call to max | qltest |
| test.go:199:10:199:26 | call to max | qltest |
| test.go:200:10:200:26 | call to min | qltest |
| test.go:201:10:201:26 | call to min | qltest |
| test.go:202:10:202:26 | call to min | qltest |
24 changes: 12 additions & 12 deletions go/ql/test/library-tests/semmle/go/dataflow/ExternalValueFlow/srcs.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@ invalidModelRow
| test.go:41:2:41:21 | ... = ...[1] | qltest-w-subtypes |
| test.go:42:2:42:22 | ... = ...[1] | qltest-w-subtypes |
| test.go:58:9:58:16 | call to Src1 | qltest |
| test.go:91:46:91:53 | call to Src1 | qltest |
| test.go:95:35:95:42 | call to Src1 | qltest |
| test.go:99:42:99:49 | call to Src1 | qltest |
| test.go:130:8:130:15 | call to Src1 | qltest |
| test.go:135:9:135:16 | call to Src1 | qltest |
| test.go:138:15:138:22 | call to Src1 | qltest |
| test.go:142:9:142:16 | call to Src1 | qltest |
| test.go:146:9:146:16 | call to Src1 | qltest |
| test.go:151:24:151:31 | call to Src1 | qltest |
| test.go:154:17:154:24 | call to Src1 | qltest |
| test.go:158:24:158:31 | call to Src1 | qltest |
| test.go:162:24:162:31 | call to Src1 | qltest |
| test.go:97:46:97:53 | call to Src1 | qltest |
| test.go:107:35:107:42 | call to Src1 | qltest |
| test.go:111:42:111:49 | call to Src1 | qltest |
| test.go:154:8:154:15 | call to Src1 | qltest |
| test.go:159:9:159:16 | call to Src1 | qltest |
| test.go:162:15:162:22 | call to Src1 | qltest |
| test.go:166:9:166:16 | call to Src1 | qltest |
| test.go:170:9:170:16 | call to Src1 | qltest |
| test.go:175:24:175:31 | call to Src1 | qltest |
| test.go:178:17:178:24 | call to Src1 | qltest |
| test.go:182:24:182:31 | call to Src1 | qltest |
| test.go:186:24:186:31 | call to Src1 | qltest |
Loading
Loading