diff --git a/go/ql/lib/semmle/go/security/SafeUrlFlow.qll b/go/ql/lib/semmle/go/security/SafeUrlFlow.qll index d88c2c852ae..7144614c305 100644 --- a/go/ql/lib/semmle/go/security/SafeUrlFlow.qll +++ b/go/ql/lib/semmle/go/security/SafeUrlFlow.qll @@ -22,18 +22,21 @@ module SafeUrlFlow { predicate isSink(DataFlow::Node sink) { sink instanceof Sink } predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - // propagate to a URL when its host is assigned to - exists(Write w, Field f, SsaWithFields v | f.hasQualifiedName("net/url", "URL", "Host") | - w.writesFieldPreUpdate(v.getAUse(), f, node1) and - node2 = v.getAUse() + // propagate taint to the post-update node of a URL when its host is + // assigned to + exists(Write w, Field f | f.hasQualifiedName("net/url", "URL", "Host") | + w.writesField(node2, f, node1) ) } predicate isBarrierOut(DataFlow::Node node) { // block propagation of this safe value when its host is overwritten - exists(Write w, DataFlow::Node base, Field f | f.hasQualifiedName("net/url", "URL", "Host") | - w.writesField(base, f, _) and - [base, base.(DataFlow::PostUpdateNode).getPreUpdateNode()] = node.getASuccessor() + exists(Write w, Field f | f.hasQualifiedName("net/url", "URL", "Host") | + // We sanitize the pre-update node to block flow from previous value. + // This fits in with the additional flow step above propagating taint + // from the value written to the Host field to the post-update node of + // the URL. + w.writesFieldPreUpdate(node, f, _) ) or node instanceof SanitizerEdge diff --git a/go/ql/test/library-tests/semmle/go/security/SafeUrlFlow/SafeUrlFlow.expected b/go/ql/test/library-tests/semmle/go/security/SafeUrlFlow/SafeUrlFlow.expected index 7182e118ff1..c2f82841d83 100644 --- a/go/ql/test/library-tests/semmle/go/security/SafeUrlFlow/SafeUrlFlow.expected +++ b/go/ql/test/library-tests/semmle/go/security/SafeUrlFlow/SafeUrlFlow.expected @@ -16,7 +16,7 @@ | 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 | | 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 | | 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 | -| 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 | +| 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 | | 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 | | 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 | | 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 | SafeUrlFlow.go:10:14:10:21 | selection of Host | SafeUrlFlow.go:17:19:17:26 | safeHost | provenance | | | SafeUrlFlow.go:13:13:13:19 | selection of URL | SafeUrlFlow.go:14:29:14:35 | safeURL | provenance | Src:MaD:2 | | SafeUrlFlow.go:14:29:14:35 | safeURL | SafeUrlFlow.go:14:29:14:44 | call to String | provenance | MaD:3 | -| SafeUrlFlow.go:17:2:17:10 | targetURL | SafeUrlFlow.go:18:11:18:19 | targetURL | provenance | | -| SafeUrlFlow.go:17:19:17:26 | safeHost | SafeUrlFlow.go:17:2:17:10 | targetURL | provenance | Config | -| SafeUrlFlow.go:17:19:17:26 | safeHost | SafeUrlFlow.go:18:11:18:19 | targetURL | provenance | Config | +| SafeUrlFlow.go:17:2:17:10 | targetURL [postupdate] | SafeUrlFlow.go:18:11:18:19 | targetURL | provenance | | +| SafeUrlFlow.go:17:19:17:26 | safeHost | SafeUrlFlow.go:17:2:17:10 | targetURL [postupdate] | provenance | Config | | SafeUrlFlow.go:18:11:18:19 | targetURL | SafeUrlFlow.go:18:11:18:28 | call to String | provenance | MaD:3 | | SafeUrlFlow.go:37:13:37:19 | selection of URL | SafeUrlFlow.go:45:24:45:61 | ...+... | provenance | Src:MaD:2 Sink:MaD:1 | | SafeUrlFlow.go:37:13:37:19 | selection of URL | SafeUrlFlow.go:46:29:46:55 | ...+... | provenance | Src:MaD:2 | @@ -57,10 +56,11 @@ edges | SafeUrlFlow.go:74:70:74:76 | safeURL | SafeUrlFlow.go:74:70:74:85 | call to String | provenance | MaD:3 | | SafeUrlFlow.go:78:40:78:46 | safeURL | SafeUrlFlow.go:78:40:78:55 | call to String | provenance | MaD:3 | | SafeUrlFlow.go:84:14:84:21 | selection of Host | SafeUrlFlow.go:87:19:87:26 | safeHost | provenance | | -| SafeUrlFlow.go:87:19:87:26 | safeHost | SafeUrlFlow.go:91:2:91:10 | targetURL | provenance | Config | -| SafeUrlFlow.go:87:19:87:26 | safeHost | SafeUrlFlow.go:92:11:92:19 | targetURL | provenance | Config | -| SafeUrlFlow.go:91:2:91:10 | targetURL | SafeUrlFlow.go:92:11:92:19 | targetURL | provenance | | -| SafeUrlFlow.go:92:11:92:19 | targetURL | SafeUrlFlow.go:92:11:92:28 | call to String | provenance | MaD:3 | +| SafeUrlFlow.go:87:2:87:10 | implicit dereference [postupdate] | SafeUrlFlow.go:87:2:87:10 | targetURL [postupdate] | provenance | | +| SafeUrlFlow.go:87:2:87:10 | targetURL [postupdate] | SafeUrlFlow.go:89:24:89:32 | targetURL | provenance | | +| SafeUrlFlow.go:87:19:87:26 | safeHost | SafeUrlFlow.go:87:2:87:10 | implicit dereference [postupdate] | provenance | Config | +| SafeUrlFlow.go:87:19:87:26 | safeHost | SafeUrlFlow.go:87:2:87:10 | targetURL [postupdate] | provenance | Config | +| SafeUrlFlow.go:89:24:89:32 | targetURL | SafeUrlFlow.go:89:24:89:41 | call to String | provenance | MaD:3 Sink:MaD:1 | | SafeUrlFlow.go:96:13:96:19 | selection of URL | SafeUrlFlow.go:105:11:105:23 | reconstructed | provenance | Src:MaD:2 | | SafeUrlFlow.go:96:13:96:19 | selection of URL | SafeUrlFlow.go:108:24:108:50 | ...+... | provenance | Src:MaD:2 Sink:MaD:1 | | SafeUrlFlow.go:96:13:96:19 | selection of URL | SafeUrlFlow.go:109:29:109:58 | ...+... | provenance | Src:MaD:2 | @@ -76,7 +76,7 @@ nodes | SafeUrlFlow.go:13:13:13:19 | selection of URL | semmle.label | selection of URL | | SafeUrlFlow.go:14:29:14:35 | safeURL | semmle.label | safeURL | | SafeUrlFlow.go:14:29:14:44 | call to String | semmle.label | call to String | -| SafeUrlFlow.go:17:2:17:10 | targetURL | semmle.label | targetURL | +| SafeUrlFlow.go:17:2:17:10 | targetURL [postupdate] | semmle.label | targetURL [postupdate] | | SafeUrlFlow.go:17:19:17:26 | safeHost | semmle.label | safeHost | | SafeUrlFlow.go:18:11:18:19 | targetURL | semmle.label | targetURL | | SafeUrlFlow.go:18:11:18:28 | call to String | semmle.label | call to String | @@ -108,10 +108,11 @@ nodes | SafeUrlFlow.go:78:40:78:46 | safeURL | semmle.label | safeURL | | SafeUrlFlow.go:78:40:78:55 | call to String | semmle.label | call to String | | SafeUrlFlow.go:84:14:84:21 | selection of Host | semmle.label | selection of Host | +| SafeUrlFlow.go:87:2:87:10 | implicit dereference [postupdate] | semmle.label | implicit dereference [postupdate] | +| SafeUrlFlow.go:87:2:87:10 | targetURL [postupdate] | semmle.label | targetURL [postupdate] | | SafeUrlFlow.go:87:19:87:26 | safeHost | semmle.label | safeHost | -| SafeUrlFlow.go:91:2:91:10 | targetURL | semmle.label | targetURL | -| SafeUrlFlow.go:92:11:92:19 | targetURL | semmle.label | targetURL | -| SafeUrlFlow.go:92:11:92:28 | call to String | semmle.label | call to String | +| SafeUrlFlow.go:89:24:89:32 | targetURL | semmle.label | targetURL | +| SafeUrlFlow.go:89:24:89:41 | call to String | semmle.label | call to String | | SafeUrlFlow.go:96:13:96:19 | selection of URL | semmle.label | selection of URL | | SafeUrlFlow.go:105:11:105:23 | reconstructed | semmle.label | reconstructed | | SafeUrlFlow.go:108:24:108:50 | ...+... | semmle.label | ...+... | @@ -119,6 +120,3 @@ nodes | SafeUrlFlow.go:110:12:110:42 | ...+... | semmle.label | ...+... | | SafeUrlFlow.go:111:12:111:25 | safeOpaquePart | semmle.label | safeOpaquePart | subpaths -testFailures -| SafeUrlFlow.go:89:62:89:71 | comment | Missing result: Alert | -| SafeUrlFlow.go:92:11:92:28 | call to String | Unexpected result: Alert |