Skip to content

Commit 6e4dbe8

Browse files
owen-mcowen-mc
committed
Fix SafeUrlFlow so test passes
1 parent 620ae33 commit 6e4dbe8

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:70:39:70:54 | call to String | SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:70:39:70:54 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:54:13:54:19 | selection of URL | here |
1717
| SafeUrlFlow.go:74:70:74:85 | call to String | SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:74:70:74:85 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:54:13:54:19 | selection of URL | here |
1818
| SafeUrlFlow.go:78:40:78:55 | call to String | SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:78:40:78:55 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:54:13:54:19 | selection of URL | here |
19-
| SafeUrlFlow.go:92:11:92:28 | call to String | SafeUrlFlow.go:84:14:84:21 | selection of Host | SafeUrlFlow.go:92:11:92:28 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:84:14:84:21 | selection of Host | here |
19+
| SafeUrlFlow.go:89:24:89:41 | call to String | SafeUrlFlow.go:84:14:84:21 | selection of Host | SafeUrlFlow.go:89:24:89:41 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:84:14:84:21 | selection of Host | here |
2020
| SafeUrlFlow.go:105:11:105:23 | reconstructed | SafeUrlFlow.go:96:13:96:19 | selection of URL | SafeUrlFlow.go:105:11:105:23 | reconstructed | A safe URL flows here from $@. | SafeUrlFlow.go:96:13:96:19 | selection of URL | here |
2121
| SafeUrlFlow.go:108:24:108:50 | ...+... | SafeUrlFlow.go:96:13:96:19 | selection of URL | SafeUrlFlow.go:108:24:108:50 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:96:13:96:19 | selection of URL | here |
2222
| SafeUrlFlow.go:109:29:109:58 | ...+... | SafeUrlFlow.go:96:13:96:19 | selection of URL | SafeUrlFlow.go:109:29:109:58 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:96:13:96:19 | selection of URL | here |
@@ -27,9 +27,8 @@ edges
2727
| SafeUrlFlow.go:10:14:10:21 | selection of Host | SafeUrlFlow.go:17:19:17:26 | safeHost | provenance | |
2828
| SafeUrlFlow.go:13:13:13:19 | selection of URL | SafeUrlFlow.go:14:29:14:35 | safeURL | provenance | Src:MaD:2 |
2929
| SafeUrlFlow.go:14:29:14:35 | safeURL | 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:26 | safeHost | SafeUrlFlow.go:17:2:17:10 | targetURL | provenance | Config |
32-
| SafeUrlFlow.go:17:19:17:26 | safeHost | 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:26 | safeHost | 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:45:24:45:61 | ...+... | provenance | Src:MaD:2 Sink:MaD:1 |
3534
| SafeUrlFlow.go:37:13:37:19 | selection of URL | SafeUrlFlow.go:46:29:46:55 | ...+... | provenance | Src:MaD:2 |
@@ -57,10 +56,11 @@ edges
5756
| SafeUrlFlow.go:74:70:74:76 | safeURL | SafeUrlFlow.go:74:70:74:85 | call to String | provenance | MaD:3 |
5857
| SafeUrlFlow.go:78:40:78:46 | safeURL | SafeUrlFlow.go:78:40:78:55 | call to String | provenance | MaD:3 |
5958
| SafeUrlFlow.go:84:14:84:21 | selection of Host | SafeUrlFlow.go:87:19:87:26 | safeHost | provenance | |
60-
| SafeUrlFlow.go:87:19:87:26 | safeHost | SafeUrlFlow.go:91:2:91:10 | targetURL | provenance | Config |
61-
| SafeUrlFlow.go:87:19:87:26 | safeHost | SafeUrlFlow.go:92:11:92:19 | targetURL | provenance | Config |
62-
| SafeUrlFlow.go:91:2:91:10 | targetURL | SafeUrlFlow.go:92:11:92:19 | targetURL | provenance | |
63-
| SafeUrlFlow.go:92:11:92:19 | targetURL | SafeUrlFlow.go:92:11:92:28 | call to String | provenance | MaD:3 |
59+
| SafeUrlFlow.go:87:2:87:10 | implicit dereference [postupdate] | SafeUrlFlow.go:87:2:87:10 | targetURL [postupdate] | provenance | |
60+
| SafeUrlFlow.go:87:2:87:10 | targetURL [postupdate] | SafeUrlFlow.go:89:24:89:32 | targetURL | provenance | |
61+
| SafeUrlFlow.go:87:19:87:26 | safeHost | SafeUrlFlow.go:87:2:87:10 | implicit dereference [postupdate] | provenance | Config |
62+
| SafeUrlFlow.go:87:19:87:26 | safeHost | SafeUrlFlow.go:87:2:87:10 | targetURL [postupdate] | provenance | Config |
63+
| SafeUrlFlow.go:89:24:89:32 | targetURL | SafeUrlFlow.go:89:24:89:41 | call to String | provenance | MaD:3 Sink:MaD:1 |
6464
| SafeUrlFlow.go:96:13:96:19 | selection of URL | SafeUrlFlow.go:105:11:105:23 | reconstructed | provenance | Src:MaD:2 |
6565
| SafeUrlFlow.go:96:13:96:19 | selection of URL | SafeUrlFlow.go:108:24:108:50 | ...+... | provenance | Src:MaD:2 Sink:MaD:1 |
6666
| SafeUrlFlow.go:96:13:96:19 | selection of URL | SafeUrlFlow.go:109:29:109:58 | ...+... | 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 | safeURL | semmle.label | safeURL |
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:26 | safeHost | semmle.label | safeHost |
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:78:40:78:46 | safeURL | semmle.label | safeURL |
109109
| SafeUrlFlow.go:78:40:78:55 | call to String | semmle.label | call to String |
110110
| SafeUrlFlow.go:84:14:84:21 | selection of Host | semmle.label | selection of Host |
111+
| SafeUrlFlow.go:87:2:87:10 | implicit dereference [postupdate] | semmle.label | implicit dereference [postupdate] |
112+
| SafeUrlFlow.go:87:2:87:10 | targetURL [postupdate] | semmle.label | targetURL [postupdate] |
111113
| SafeUrlFlow.go:87:19:87:26 | safeHost | semmle.label | safeHost |
112-
| SafeUrlFlow.go:91:2:91:10 | targetURL | semmle.label | targetURL |
113-
| SafeUrlFlow.go:92:11:92:19 | targetURL | semmle.label | targetURL |
114-
| SafeUrlFlow.go:92:11:92:28 | call to String | semmle.label | call to String |
114+
| SafeUrlFlow.go:89:24:89:32 | targetURL | semmle.label | targetURL |
115+
| SafeUrlFlow.go:89:24:89:41 | call to String | semmle.label | call to String |
115116
| SafeUrlFlow.go:96:13:96:19 | selection of URL | semmle.label | selection of URL |
116117
| SafeUrlFlow.go:105:11:105:23 | reconstructed | semmle.label | reconstructed |
117118
| SafeUrlFlow.go:108:24:108:50 | ...+... | semmle.label | ...+... |
118119
| SafeUrlFlow.go:109:29:109:58 | ...+... | semmle.label | ...+... |
119120
| SafeUrlFlow.go:110:12:110:42 | ...+... | semmle.label | ...+... |
120121
| SafeUrlFlow.go:111:12:111:25 | safeOpaquePart | semmle.label | safeOpaquePart |
121122
subpaths
122-
testFailures
123-
| SafeUrlFlow.go:89:62:89:71 | comment | Missing result: Alert |
124-
| SafeUrlFlow.go:92:11:92:28 | call to String | Unexpected result: Alert |

0 commit comments

Comments
 (0)