Skip to content

Commit ff3d795

Browse files
owen-mcowen-mc
authored
Merge pull request #20556 from owen-mc/go/test/safeurlflow
Go: Add tests for SafeUrlFlow, and fix a latent bug
2 parents f96a42c + 8983ac9 commit ff3d795

File tree

9 files changed

+285
-34
lines changed

9 files changed

+285
-34
lines changed

‎go/ql/lib/change-notes/2025-09-30-fewer-safe-urls.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* `go/unvalidated-url-redirection` and `go/request-forgery` have a shared notion of a safe URL, which is known to not be malicious. Some URLs which were incorrectly considered safe are now correctly considered unsafe. This may lead to more alerts for those two queries.

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

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
import go
88
import UrlConcatenation
9-
import SafeUrlFlowCustomizations
9+
private import SafeUrlFlowCustomizations
1010
import semmle.go.dataflow.barrierguardutil.RedirectCheckBarrierGuard
1111
import semmle.go.dataflow.barrierguardutil.RegexpCheck
1212
import semmle.go.dataflow.barrierguardutil.UrlCheck
@@ -121,21 +121,6 @@ module OpenUrlRedirect {
121121
/** A sink for an open redirect, considered as a sink for safe URL flow. */
122122
private class SafeUrlSink extends SafeUrlFlow::Sink instanceof OpenUrlRedirect::Sink { }
123123

124-
/**
125-
* A read of a field considered unsafe to redirect to, considered as a sanitizer for a safe
126-
* URL.
127-
*/
128-
private class UnsafeFieldReadSanitizer extends SafeUrlFlow::SanitizerEdge {
129-
UnsafeFieldReadSanitizer() {
130-
exists(DataFlow::FieldReadNode frn, string name |
131-
name = ["User", "RawQuery", "Fragment"] and
132-
frn.getField().hasQualifiedName("net/url", "URL")
133-
|
134-
this = frn.getBase()
135-
)
136-
}
137-
}
138-
139124
/**
140125
* Reinstate the usual field propagation rules for fields, which the OpenURLRedirect
141126
* query usually excludes, for fields of `Params` other than `Params.Fixed`.

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

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
import go
66
import UrlConcatenation
7-
import SafeUrlFlowCustomizations
7+
private import SafeUrlFlowCustomizations
88
import semmle.go.dataflow.barrierguardutil.RedirectCheckBarrierGuard
99
import semmle.go.dataflow.barrierguardutil.RegexpCheck
1010
import semmle.go.dataflow.barrierguardutil.UrlCheck
@@ -118,18 +118,3 @@ module RequestForgery {
118118

119119
/** A sink for request forgery, considered as a sink for safe URL flow. */
120120
private class SafeUrlSink extends SafeUrlFlow::Sink instanceof RequestForgery::Sink { }
121-
122-
/**
123-
* A read of a field considered unsafe for request forgery, considered as a sanitizer for a safe
124-
* URL.
125-
*/
126-
private class UnsafeFieldReadSanitizer extends SafeUrlFlow::SanitizerEdge {
127-
UnsafeFieldReadSanitizer() {
128-
exists(DataFlow::FieldReadNode frn, string name |
129-
(name = "RawQuery" or name = "Fragment" or name = "User") and
130-
frn.getField().hasQualifiedName("net/url", "URL")
131-
|
132-
this = frn.getBase()
133-
)
134-
}
135-
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,10 @@ module SafeUrlFlow {
3030

3131
predicate isBarrierOut(DataFlow::Node node) {
3232
// block propagation of this safe value when its host is overwritten
33-
exists(Write w, Field f | f.hasQualifiedName("net/url", "URL", "Host") |
34-
w.writesField(node.getASuccessor(), f, _)
33+
exists(Write w, DataFlow::Node b, Field f |
34+
f.hasQualifiedName("net/url", "URL", "Host") and
35+
b = node.getASuccessor() and
36+
w.writesField(b, f, _)
3537
)
3638
or
3739
node instanceof SanitizerEdge

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,4 +40,19 @@ module SafeUrlFlow {
4040
private class StringSlicingEdge extends SanitizerEdge {
4141
StringSlicingEdge() { this = any(DataFlow::SliceNode sn) }
4242
}
43+
44+
/**
45+
* A read of a field considered unsafe to redirect to, considered as a sanitizer for a safe
46+
* URL.
47+
*/
48+
private class UnsafeFieldReadSanitizer extends SanitizerEdge {
49+
UnsafeFieldReadSanitizer() {
50+
exists(DataFlow::FieldReadNode frn, string name |
51+
name = ["Fragment", "RawQuery", "User"] and
52+
frn.getField().hasQualifiedName("net/url", "URL", name)
53+
|
54+
this = frn.getBase()
55+
)
56+
}
57+
}
4358
}

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

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
#select
2+
| SafeUrlFlow.go:11:24:11:50 | ...+... | SafeUrlFlow.go:10:14:10:21 | selection of Host | SafeUrlFlow.go:11:24:11:50 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:10:14:10:21 | selection of Host | here |
3+
| SafeUrlFlow.go:14:29:14:44 | call to String | SafeUrlFlow.go:13:13:13:19 | selection of URL | SafeUrlFlow.go:14:29:14:44 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:13:13:13:19 | selection of URL | here |
4+
| SafeUrlFlow.go:18:11:18:28 | call to String | SafeUrlFlow.go:10:14:10:21 | selection of Host | SafeUrlFlow.go:18:11:18:28 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:10:14:10:21 | selection of Host | here |
5+
| SafeUrlFlow.go:45:24:45:61 | ...+... | SafeUrlFlow.go:37:13:37:19 | selection of URL | SafeUrlFlow.go:45:24:45:61 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:37:13:37:19 | selection of URL | here |
6+
| SafeUrlFlow.go:46:29:46:55 | ...+... | SafeUrlFlow.go:37:13:37:19 | selection of URL | SafeUrlFlow.go:46:29:46:55 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:37:13:37:19 | selection of URL | here |
7+
| SafeUrlFlow.go:47:11:47:42 | ...+... | SafeUrlFlow.go:37:13:37:19 | selection of URL | SafeUrlFlow.go:47:11:47:42 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:37:13:37:19 | selection of URL | here |
8+
| SafeUrlFlow.go:57:11:57:26 | call to String | SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:57:11:57:26 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:54:13:54:19 | selection of URL | here |
9+
| SafeUrlFlow.go:58:12:58:27 | call to String | SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:58:12:58:27 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:54:13:54:19 | selection of URL | here |
10+
| SafeUrlFlow.go:59:16:59:31 | call to String | SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:59:16:59:31 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:54:13:54:19 | selection of URL | here |
11+
| SafeUrlFlow.go:60:12:60:27 | call to String | SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:60:12:60:27 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:54:13:54:19 | selection of URL | here |
12+
| SafeUrlFlow.go:64:13:64:28 | call to String | SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:64:13:64:28 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:54:13:54:19 | selection of URL | here |
13+
| SafeUrlFlow.go:65:14:65:29 | call to String | SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:65:14:65:29 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:54:13:54:19 | selection of URL | here |
14+
| SafeUrlFlow.go:66:18:66:33 | call to String | SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:66:18:66:33 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:54:13:54:19 | selection of URL | here |
15+
| SafeUrlFlow.go:67:14:67:29 | call to String | SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:67:14:67:29 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:54:13:54:19 | selection of URL | here |
16+
| 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 |
17+
| 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 |
18+
| 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: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 |
20+
| SafeUrlFlow.go:109:11:109:23 | reconstructed | SafeUrlFlow.go:100:13:100:19 | selection of URL | SafeUrlFlow.go:109:11:109:23 | reconstructed | A safe URL flows here from $@. | SafeUrlFlow.go:100:13:100:19 | selection of URL | here |
21+
| SafeUrlFlow.go:112:24:112:50 | ...+... | SafeUrlFlow.go:100:13:100:19 | selection of URL | SafeUrlFlow.go:112:24:112:50 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:100:13:100:19 | selection of URL | here |
22+
| SafeUrlFlow.go:113:29:113:58 | ...+... | SafeUrlFlow.go:100:13:100:19 | selection of URL | SafeUrlFlow.go:113:29:113:58 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:100:13:100:19 | selection of URL | here |
23+
| SafeUrlFlow.go:114:12:114:42 | ...+... | SafeUrlFlow.go:100:13:100:19 | selection of URL | SafeUrlFlow.go:114:12:114:42 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:100:13:100:19 | selection of URL | here |
24+
| SafeUrlFlow.go:115:12:115:25 | safeOpaquePart | SafeUrlFlow.go:100:13:100:19 | selection of URL | SafeUrlFlow.go:115:12:115:25 | safeOpaquePart | A safe URL flows here from $@. | SafeUrlFlow.go:100:13:100:19 | selection of URL | here |
25+
edges
26+
| SafeUrlFlow.go:10:14:10:21 | selection of Host | SafeUrlFlow.go:11:24:11:50 | ...+... | provenance | Sink:MaD:1 |
27+
| SafeUrlFlow.go:10:14:10:21 | selection of Host | SafeUrlFlow.go:17:19:17:26 | safeHost | provenance | |
28+
| SafeUrlFlow.go:13:13:13:19 | selection of URL | SafeUrlFlow.go:14:29:14:35 | safeURL | provenance | Src:MaD:2 |
29+
| SafeUrlFlow.go:14:29:14:35 | safeURL | SafeUrlFlow.go:14:29:14:44 | call to String | provenance | MaD:3 |
30+
| SafeUrlFlow.go:17:19:17:26 | safeHost | SafeUrlFlow.go:18:11:18:19 | targetURL | provenance | Config |
31+
| SafeUrlFlow.go:18:11:18:19 | targetURL | SafeUrlFlow.go:18:11:18:28 | call to String | provenance | MaD:3 |
32+
| SafeUrlFlow.go:37:13:37:19 | selection of URL | SafeUrlFlow.go:45:24:45:61 | ...+... | provenance | Src:MaD:2 Sink:MaD:1 |
33+
| SafeUrlFlow.go:37:13:37:19 | selection of URL | SafeUrlFlow.go:46:29:46:55 | ...+... | provenance | Src:MaD:2 |
34+
| SafeUrlFlow.go:37:13:37:19 | selection of URL | SafeUrlFlow.go:47:11:47:42 | ...+... | provenance | Src:MaD:2 |
35+
| SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:57:11:57:17 | safeURL | provenance | Src:MaD:2 |
36+
| SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:58:12:58:18 | safeURL | provenance | Src:MaD:2 |
37+
| SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:59:16:59:22 | safeURL | provenance | Src:MaD:2 |
38+
| SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:60:12:60:18 | safeURL | provenance | Src:MaD:2 |
39+
| SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:64:13:64:19 | safeURL | provenance | Src:MaD:2 |
40+
| SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:65:14:65:20 | safeURL | provenance | Src:MaD:2 |
41+
| SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:66:18:66:24 | safeURL | provenance | Src:MaD:2 |
42+
| SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:67:14:67:20 | safeURL | provenance | Src:MaD:2 |
43+
| SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:70:39:70:45 | safeURL | provenance | Src:MaD:2 |
44+
| SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:74:70:74:76 | safeURL | provenance | Src:MaD:2 |
45+
| SafeUrlFlow.go:54:13:54:19 | selection of URL | SafeUrlFlow.go:78:40:78:46 | safeURL | provenance | Src:MaD:2 |
46+
| SafeUrlFlow.go:57:11:57:17 | safeURL | SafeUrlFlow.go:57:11:57:26 | call to String | provenance | MaD:3 |
47+
| SafeUrlFlow.go:58:12:58:18 | safeURL | SafeUrlFlow.go:58:12:58:27 | call to String | provenance | MaD:3 |
48+
| SafeUrlFlow.go:59:16:59:22 | safeURL | SafeUrlFlow.go:59:16:59:31 | call to String | provenance | MaD:3 |
49+
| SafeUrlFlow.go:60:12:60:18 | safeURL | SafeUrlFlow.go:60:12:60:27 | call to String | provenance | MaD:3 |
50+
| SafeUrlFlow.go:64:13:64:19 | safeURL | SafeUrlFlow.go:64:13:64:28 | call to String | provenance | MaD:3 |
51+
| SafeUrlFlow.go:65:14:65:20 | safeURL | SafeUrlFlow.go:65:14:65:29 | call to String | provenance | MaD:3 |
52+
| SafeUrlFlow.go:66:18:66:24 | safeURL | SafeUrlFlow.go:66:18:66:33 | call to String | provenance | MaD:3 |
53+
| SafeUrlFlow.go:67:14:67:20 | safeURL | SafeUrlFlow.go:67:14:67:29 | call to String | provenance | MaD:3 |
54+
| SafeUrlFlow.go:70:39:70:45 | safeURL | SafeUrlFlow.go:70:39:70:54 | call to String | provenance | MaD:3 |
55+
| SafeUrlFlow.go:74:70:74:76 | safeURL | SafeUrlFlow.go:74:70:74:85 | call to String | provenance | MaD:3 |
56+
| SafeUrlFlow.go:78:40:78:46 | safeURL | SafeUrlFlow.go:78:40:78:55 | call to String | provenance | MaD:3 |
57+
| SafeUrlFlow.go:84:14:84:21 | selection of Host | SafeUrlFlow.go:87:19:87:26 | safeHost | provenance | |
58+
| SafeUrlFlow.go:87:19:87:26 | safeHost | SafeUrlFlow.go:89:24:89:32 | targetURL | provenance | Config |
59+
| SafeUrlFlow.go:89:24:89:32 | targetURL | SafeUrlFlow.go:89:24:89:41 | call to String | provenance | MaD:3 Sink:MaD:1 |
60+
| SafeUrlFlow.go:100:13:100:19 | selection of URL | SafeUrlFlow.go:109:11:109:23 | reconstructed | provenance | Src:MaD:2 |
61+
| SafeUrlFlow.go:100:13:100:19 | selection of URL | SafeUrlFlow.go:112:24:112:50 | ...+... | provenance | Src:MaD:2 Sink:MaD:1 |
62+
| SafeUrlFlow.go:100:13:100:19 | selection of URL | SafeUrlFlow.go:113:29:113:58 | ...+... | provenance | Src:MaD:2 |
63+
| SafeUrlFlow.go:100:13:100:19 | selection of URL | SafeUrlFlow.go:114:12:114:42 | ...+... | provenance | Src:MaD:2 |
64+
| SafeUrlFlow.go:100:13:100:19 | selection of URL | SafeUrlFlow.go:115:12:115:25 | safeOpaquePart | provenance | Src:MaD:2 |
65+
models
66+
| 1 | Sink: net/http; ; false; Redirect; ; ; Argument[2]; url-redirection[0]; manual |
67+
| 2 | Source: net/http; Request; true; URL; ; ; ; remote; manual |
68+
| 3 | Summary: fmt; Stringer; true; String; ; ; Argument[receiver]; ReturnValue; taint; manual |
69+
nodes
70+
| SafeUrlFlow.go:10:14:10:21 | selection of Host | semmle.label | selection of Host |
71+
| SafeUrlFlow.go:11:24:11:50 | ...+... | semmle.label | ...+... |
72+
| SafeUrlFlow.go:13:13:13:19 | selection of URL | semmle.label | selection of URL |
73+
| SafeUrlFlow.go:14:29:14:35 | safeURL | semmle.label | safeURL |
74+
| SafeUrlFlow.go:14:29:14:44 | call to String | semmle.label | call to String |
75+
| SafeUrlFlow.go:17:19:17:26 | safeHost | semmle.label | safeHost |
76+
| SafeUrlFlow.go:18:11:18:19 | targetURL | semmle.label | targetURL |
77+
| SafeUrlFlow.go:18:11:18:28 | call to String | semmle.label | call to String |
78+
| SafeUrlFlow.go:37:13:37:19 | selection of URL | semmle.label | selection of URL |
79+
| SafeUrlFlow.go:45:24:45:61 | ...+... | semmle.label | ...+... |
80+
| SafeUrlFlow.go:46:29:46:55 | ...+... | semmle.label | ...+... |
81+
| SafeUrlFlow.go:47:11:47:42 | ...+... | semmle.label | ...+... |
82+
| SafeUrlFlow.go:54:13:54:19 | selection of URL | semmle.label | selection of URL |
83+
| SafeUrlFlow.go:57:11:57:17 | safeURL | semmle.label | safeURL |
84+
| SafeUrlFlow.go:57:11:57:26 | call to String | semmle.label | call to String |
85+
| SafeUrlFlow.go:58:12:58:18 | safeURL | semmle.label | safeURL |
86+
| SafeUrlFlow.go:58:12:58:27 | call to String | semmle.label | call to String |
87+
| SafeUrlFlow.go:59:16:59:22 | safeURL | semmle.label | safeURL |
88+
| SafeUrlFlow.go:59:16:59:31 | call to String | semmle.label | call to String |
89+
| SafeUrlFlow.go:60:12:60:18 | safeURL | semmle.label | safeURL |
90+
| SafeUrlFlow.go:60:12:60:27 | call to String | semmle.label | call to String |
91+
| SafeUrlFlow.go:64:13:64:19 | safeURL | semmle.label | safeURL |
92+
| SafeUrlFlow.go:64:13:64:28 | call to String | semmle.label | call to String |
93+
| SafeUrlFlow.go:65:14:65:20 | safeURL | semmle.label | safeURL |
94+
| SafeUrlFlow.go:65:14:65:29 | call to String | semmle.label | call to String |
95+
| SafeUrlFlow.go:66:18:66:24 | safeURL | semmle.label | safeURL |
96+
| SafeUrlFlow.go:66:18:66:33 | call to String | semmle.label | call to String |
97+
| SafeUrlFlow.go:67:14:67:20 | safeURL | semmle.label | safeURL |
98+
| SafeUrlFlow.go:67:14:67:29 | call to String | semmle.label | call to String |
99+
| SafeUrlFlow.go:70:39:70:45 | safeURL | semmle.label | safeURL |
100+
| SafeUrlFlow.go:70:39:70:54 | call to String | semmle.label | call to String |
101+
| SafeUrlFlow.go:74:70:74:76 | safeURL | semmle.label | safeURL |
102+
| SafeUrlFlow.go:74:70:74:85 | call to String | semmle.label | call to String |
103+
| SafeUrlFlow.go:78:40:78:46 | safeURL | semmle.label | safeURL |
104+
| SafeUrlFlow.go:78:40:78:55 | call to String | semmle.label | call to String |
105+
| SafeUrlFlow.go:84:14:84:21 | selection of Host | semmle.label | selection of Host |
106+
| SafeUrlFlow.go:87:19:87:26 | safeHost | semmle.label | safeHost |
107+
| SafeUrlFlow.go:89:24:89:32 | targetURL | semmle.label | targetURL |
108+
| SafeUrlFlow.go:89:24:89:41 | call to String | semmle.label | call to String |
109+
| SafeUrlFlow.go:100:13:100:19 | selection of URL | semmle.label | selection of URL |
110+
| SafeUrlFlow.go:109:11:109:23 | reconstructed | semmle.label | reconstructed |
111+
| SafeUrlFlow.go:112:24:112:50 | ...+... | semmle.label | ...+... |
112+
| SafeUrlFlow.go:113:29:113:58 | ...+... | semmle.label | ...+... |
113+
| SafeUrlFlow.go:114:12:114:42 | ...+... | semmle.label | ...+... |
114+
| SafeUrlFlow.go:115:12:115:25 | safeOpaquePart | semmle.label | safeOpaquePart |
115+
subpaths

0 commit comments

Comments
 (0)