Skip to content

Commit 7ac735f

Browse files
owen-mcowen-mc
committed
Fix SafeUrlFlow so test passes
1 parent 095881e commit 7ac735f

File tree

2 files changed

+23
-22
lines changed

2 files changed

+23
-22
lines changed

‎go/ql/lib/semmle/go/security/SafeUrlFlow.qll‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,18 +22,21 @@ module SafeUrlFlow {
2222
predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
2323

2424
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
25-
// propagate to a URL when its host is assigned to
26-
exists(Write w, Field f, SsaWithFields v | f.hasQualifiedName("net/url", "URL", "Host") |
27-
w.writesFieldPreUpdate(v.getAUse(), f, node1) and
28-
node2 = v.getAUse()
25+
// propagate taint to the post-update node of a URL when its host is
26+
// assigned to
27+
exists(Write w, Field f | f.hasQualifiedName("net/url", "URL", "Host") |
28+
w.writesField(node2, f, node1)
2929
)
3030
}
3131

3232
predicate isBarrierOut(DataFlow::Node node) {
3333
// block propagation of this safe value when its host is overwritten
34-
exists(Write w, DataFlow::Node base, Field f | f.hasQualifiedName("net/url", "URL", "Host") |
35-
w.writesField(base, f, _) and
36-
[base, base.(DataFlow::PostUpdateNode).getPreUpdateNode()] = node.getASuccessor()
34+
exists(Write w, Field f | f.hasQualifiedName("net/url", "URL", "Host") |
35+
// We sanitize the pre-update node to block flow from previous value.
36+
// This fits in with the additional flow step above propagating taint
37+
// from the value written to the Host field to the post-update node of
38+
// the URL.
39+
w.writesFieldPreUpdate(node, f, _)
3740
)
3841
or
3942
node instanceof SanitizerEdge

‎go/ql/test/library-tests/semmle/go/security/SafeUrlFlow/SafeUrlFlow.expected‎

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
| SafeUrlFlow.go:71:39:71:54 | call to String | SafeUrlFlow.go:55:13:55:19 | selection of URL | SafeUrlFlow.go:71:39:71:54 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:55:13:55:19 | selection of URL | here |
1717
| SafeUrlFlow.go:75:70:75:85 | call to String | SafeUrlFlow.go:55:13:55:19 | selection of URL | SafeUrlFlow.go:75:70:75:85 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:55:13:55:19 | selection of URL | here |
1818
| SafeUrlFlow.go:79:40:79:55 | call to String | SafeUrlFlow.go:55:13:55:19 | selection of URL | SafeUrlFlow.go:79:40:79:55 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:55:13:55:19 | selection of URL | here |
19-
| SafeUrlFlow.go:93:11:93:28 | call to String | SafeUrlFlow.go:85:10:85:17 | selection of Host | SafeUrlFlow.go:93:11:93:28 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:85:10:85:17 | selection of Host | here |
19+
| SafeUrlFlow.go:90:24:90:41 | call to String | SafeUrlFlow.go:85:10:85:17 | selection of Host | SafeUrlFlow.go:90:24:90:41 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:85:10:85:17 | selection of Host | here |
2020
| SafeUrlFlow.go:107:11:107:23 | reconstructed | SafeUrlFlow.go:97:13:97:19 | selection of URL | SafeUrlFlow.go:107:11:107:23 | reconstructed | A safe URL flows here from $@. | SafeUrlFlow.go:97:13:97:19 | selection of URL | here |
2121
| SafeUrlFlow.go:110:24:110:46 | ...+... | SafeUrlFlow.go:97:13:97:19 | selection of URL | SafeUrlFlow.go:110:24:110:46 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:97:13:97:19 | selection of URL | here |
2222
| SafeUrlFlow.go:111:29:111:54 | ...+... | SafeUrlFlow.go:97:13:97:19 | selection of URL | SafeUrlFlow.go:111:29:111:54 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:97:13:97:19 | selection of URL | here |
@@ -27,9 +27,8 @@ edges
2727
| SafeUrlFlow.go:10:10:10:17 | selection of Host | SafeUrlFlow.go:17:19:17:22 | host | provenance | |
2828
| SafeUrlFlow.go:13:13:13:19 | selection of URL | SafeUrlFlow.go:14:29:14:35 | baseURL | provenance | Src:MaD:2 |
2929
| SafeUrlFlow.go:14:29:14:35 | baseURL | SafeUrlFlow.go:14:29:14:44 | call to String | provenance | MaD:3 |
30-
| SafeUrlFlow.go:17:2:17:10 | targetURL | SafeUrlFlow.go:18:11:18:19 | targetURL | provenance | |
31-
| SafeUrlFlow.go:17:19:17:22 | host | SafeUrlFlow.go:17:2:17:10 | targetURL | provenance | Config |
32-
| SafeUrlFlow.go:17:19:17:22 | host | SafeUrlFlow.go:18:11:18:19 | targetURL | provenance | Config |
30+
| SafeUrlFlow.go:17:2:17:10 | targetURL [postupdate] | SafeUrlFlow.go:18:11:18:19 | targetURL | provenance | |
31+
| SafeUrlFlow.go:17:19:17:22 | host | SafeUrlFlow.go:17:2:17:10 | targetURL [postupdate] | provenance | Config |
3332
| SafeUrlFlow.go:18:11:18:19 | targetURL | SafeUrlFlow.go:18:11:18:28 | call to String | provenance | MaD:3 |
3433
| SafeUrlFlow.go:37:13:37:19 | selection of URL | SafeUrlFlow.go:47:24:47:57 | ...+... | provenance | Src:MaD:2 Sink:MaD:1 |
3534
| SafeUrlFlow.go:37:13:37:19 | selection of URL | SafeUrlFlow.go:48:29:48:51 | ...+... | provenance | Src:MaD:2 |
@@ -57,10 +56,11 @@ edges
5756
| SafeUrlFlow.go:75:70:75:76 | baseURL | SafeUrlFlow.go:75:70:75:85 | call to String | provenance | MaD:3 |
5857
| SafeUrlFlow.go:79:40:79:46 | baseURL | SafeUrlFlow.go:79:40:79:55 | call to String | provenance | MaD:3 |
5958
| SafeUrlFlow.go:85:10:85:17 | selection of Host | SafeUrlFlow.go:88:19:88:22 | host | provenance | |
60-
| SafeUrlFlow.go:88:19:88:22 | host | SafeUrlFlow.go:92:2:92:10 | targetURL | provenance | Config |
61-
| SafeUrlFlow.go:88:19:88:22 | host | SafeUrlFlow.go:93:11:93:19 | targetURL | provenance | Config |
62-
| SafeUrlFlow.go:92:2:92:10 | targetURL | SafeUrlFlow.go:93:11:93:19 | targetURL | provenance | |
63-
| SafeUrlFlow.go:93:11:93:19 | targetURL | SafeUrlFlow.go:93:11:93:28 | call to String | provenance | MaD:3 |
59+
| SafeUrlFlow.go:88:2:88:10 | implicit dereference [postupdate] | SafeUrlFlow.go:88:2:88:10 | targetURL [postupdate] | provenance | |
60+
| SafeUrlFlow.go:88:2:88:10 | targetURL [postupdate] | SafeUrlFlow.go:90:24:90:32 | targetURL | provenance | |
61+
| SafeUrlFlow.go:88:19:88:22 | host | SafeUrlFlow.go:88:2:88:10 | implicit dereference [postupdate] | provenance | Config |
62+
| SafeUrlFlow.go:88:19:88:22 | host | SafeUrlFlow.go:88:2:88:10 | targetURL [postupdate] | provenance | Config |
63+
| SafeUrlFlow.go:90:24:90:32 | targetURL | SafeUrlFlow.go:90:24:90:41 | call to String | provenance | MaD:3 Sink:MaD:1 |
6464
| SafeUrlFlow.go:97:13:97:19 | selection of URL | SafeUrlFlow.go:107:11:107:23 | reconstructed | provenance | Src:MaD:2 |
6565
| SafeUrlFlow.go:97:13:97:19 | selection of URL | SafeUrlFlow.go:110:24:110:46 | ...+... | provenance | Src:MaD:2 Sink:MaD:1 |
6666
| SafeUrlFlow.go:97:13:97:19 | selection of URL | SafeUrlFlow.go:111:29:111:54 | ...+... | provenance | Src:MaD:2 |
@@ -76,7 +76,7 @@ nodes
7676
| SafeUrlFlow.go:13:13:13:19 | selection of URL | semmle.label | selection of URL |
7777
| SafeUrlFlow.go:14:29:14:35 | baseURL | semmle.label | baseURL |
7878
| SafeUrlFlow.go:14:29:14:44 | call to String | semmle.label | call to String |
79-
| SafeUrlFlow.go:17:2:17:10 | targetURL | semmle.label | targetURL |
79+
| SafeUrlFlow.go:17:2:17:10 | targetURL [postupdate] | semmle.label | targetURL [postupdate] |
8080
| SafeUrlFlow.go:17:19:17:22 | host | semmle.label | host |
8181
| SafeUrlFlow.go:18:11:18:19 | targetURL | semmle.label | targetURL |
8282
| SafeUrlFlow.go:18:11:18:28 | call to String | semmle.label | call to String |
@@ -108,17 +108,15 @@ nodes
108108
| SafeUrlFlow.go:79:40:79:46 | baseURL | semmle.label | baseURL |
109109
| SafeUrlFlow.go:79:40:79:55 | call to String | semmle.label | call to String |
110110
| SafeUrlFlow.go:85:10:85:17 | selection of Host | semmle.label | selection of Host |
111+
| SafeUrlFlow.go:88:2:88:10 | implicit dereference [postupdate] | semmle.label | implicit dereference [postupdate] |
112+
| SafeUrlFlow.go:88:2:88:10 | targetURL [postupdate] | semmle.label | targetURL [postupdate] |
111113
| SafeUrlFlow.go:88:19:88:22 | host | semmle.label | host |
112-
| SafeUrlFlow.go:92:2:92:10 | targetURL | semmle.label | targetURL |
113-
| SafeUrlFlow.go:93:11:93:19 | targetURL | semmle.label | targetURL |
114-
| SafeUrlFlow.go:93:11:93:28 | call to String | semmle.label | call to String |
114+
| SafeUrlFlow.go:90:24:90:32 | targetURL | semmle.label | targetURL |
115+
| SafeUrlFlow.go:90:24:90:41 | call to String | semmle.label | call to String |
115116
| SafeUrlFlow.go:97:13:97:19 | selection of URL | semmle.label | selection of URL |
116117
| SafeUrlFlow.go:107:11:107:23 | reconstructed | semmle.label | reconstructed |
117118
| SafeUrlFlow.go:110:24:110:46 | ...+... | semmle.label | ...+... |
118119
| SafeUrlFlow.go:111:29:111:54 | ...+... | semmle.label | ...+... |
119120
| SafeUrlFlow.go:112:12:112:38 | ...+... | semmle.label | ...+... |
120121
| SafeUrlFlow.go:113:12:113:21 | opaquePart | semmle.label | opaquePart |
121122
subpaths
122-
testFailures
123-
| SafeUrlFlow.go:90:62:90:71 | comment | Missing result: Alert |
124-
| SafeUrlFlow.go:93:11:93:28 | call to String | Unexpected result: Alert |

0 commit comments

Comments
 (0)