Skip to content

Commit f8fa903

Browse files
owen-mcowen-mc
committed
Fix URL redirection validation sanitizers
1 parent 1a8dbb2 commit f8fa903

7 files changed

Lines changed: 1662 additions & 69 deletions

File tree

‎python/ql/lib/change-notes/2026-10-22-url-redirect-validation.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+
* URL redirection sanitizers based on `urllib.parse.urlparse` and `urllib.parse.urlsplit` now require checks of the scheme, leading slashes, and browser-ignored characters when the network location may be empty. Membership allowlists are trusted only when immutable or used directly as literals, and hostname attributes are treated as non-empty only when all possible receivers originate from `urlparse` or `urlsplit`.

‎python/ql/lib/semmle/python/frameworks/Stdlib/Urllib.qll‎

Lines changed: 254 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -81,47 +81,278 @@ private module Urllib {
8181
*/
8282
module Parse {
8383
/**
84-
* A call to `urllib.parse.urlparse`.
84+
* A call to `urllib.parse.urlparse` or `urllib.parse.urlsplit`.
8585
*/
8686
private DataFlow::CallCfgNode getUrlParseCall() {
87-
result = API::moduleImport("urllib").getMember("parse").getMember("urlparse").getACall()
87+
result =
88+
API::moduleImport("urllib")
89+
.getMember("parse")
90+
.getMember(["urlparse", "urlsplit"])
91+
.getACall()
92+
}
93+
94+
/** A URL parser call whose URL argument resolves entirely to string literals. */
95+
private DataFlow::CallCfgNode getConstantUrlParseCall() {
96+
result = getUrlParseCall() and
97+
forex(DataFlow::LocalSourceNode source |
98+
source = [result.getArg(0), result.getArgByName(["url", "urlstring"])].getALocalSource()
99+
|
100+
source.asExpr() instanceof StringLiteral
101+
)
102+
}
103+
104+
/** Holds if `e` is a parsed URL hostname, which is either non-empty or `None`. */
105+
private predicate isParsedUrlHostname(Expr e) {
106+
exists(DataFlow::AttrRead read |
107+
read = DataFlow::exprNode(e) and
108+
read.getAttributeName() = "hostname" and
109+
forex(DataFlow::LocalSourceNode source | source = read.getObject().getALocalSource() |
110+
source = getConstantUrlParseCall()
111+
)
112+
)
113+
}
114+
115+
/** Holds if `e` itself is known not to be `""`. */
116+
private predicate sourceExcludesEmptyString(Expr e) {
117+
e.(StringLiteral).getText() != ""
118+
or
119+
isParsedUrlHostname(e)
120+
}
121+
122+
/** Holds if every local source of `e` excludes `""`. */
123+
private predicate excludesEmptyString(Expr e) {
124+
forex(DataFlow::LocalSourceNode source | source = DataFlow::exprNode(e).getALocalSource() |
125+
sourceExcludesEmptyString(source.asExpr())
126+
)
127+
}
128+
129+
/** Holds if every local source of `e` is a collection literal, rather than a string. */
130+
private predicate membershipChecksCollection(Expr e) {
131+
forex(DataFlow::LocalSourceNode source | source = DataFlow::exprNode(e).getALocalSource() |
132+
source.asCfgNode() instanceof IterableNode
133+
or
134+
source.asCfgNode() instanceof DictNode
135+
)
88136
}
89137

90138
/**
91-
* A read of the `netloc` attribute of a parsed URL as returned by `urllib.parse.urlparse`,
92-
* which is being checked in a way that is relevant for URL redirection vulnerabilities.
139+
* Holds if every local source of `e` is an immutable collection literal, or a mutable
140+
* collection literal used directly in the membership test, whose elements or keys all exclude
141+
* `""`.
93142
*/
94-
private predicate netlocCheck(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) {
95-
exists(DataFlow::CallCfgNode urlParseCall, DataFlow::AttrRead netlocRead |
143+
private predicate membershipExcludesEmptyString(Expr e) {
144+
forex(DataFlow::LocalSourceNode source | source = DataFlow::exprNode(e).getALocalSource() |
145+
exists(Expr collection | collection = source.asExpr() |
146+
(collection instanceof List or collection instanceof Tuple or collection instanceof Set) and
147+
(collection instanceof Tuple or collection = e) and
148+
forall(Expr elt |
149+
elt =
150+
[
151+
collection.(List).getAnElt(), collection.(Tuple).getAnElt(),
152+
collection.(Set).getAnElt()
153+
]
154+
|
155+
excludesEmptyString(elt)
156+
)
157+
or
158+
collection instanceof Dict and
159+
collection = e and
160+
not collection.(Dict).getAnItem() instanceof DictUnpacking and
161+
forall(Expr key | key = collection.(Dict).getAKey() | excludesEmptyString(key))
162+
)
163+
)
164+
}
165+
166+
/** Holds if every local source of `e` is `""`. */
167+
private predicate isEmptyString(Expr e) {
168+
forex(DataFlow::LocalSourceNode source | source = DataFlow::exprNode(e).getALocalSource() |
169+
source.asExpr().(StringLiteral).getText() = ""
170+
)
171+
}
172+
173+
/** Holds if comparing against `e` using `op` cannot accept `""`. */
174+
private predicate comparisonExcludesEmptyString(Cmpop op, Expr e) {
175+
op.getSymbol() in ["in", "not in"] and membershipExcludesEmptyString(e)
176+
or
177+
op.getSymbol() in ["==", "is", "!=", "is not"] and excludesEmptyString(e)
178+
}
179+
180+
/** Holds if `read` reads `attribute` from the result of `urlParseCall`. */
181+
private predicate parsedUrlAttributeRead(
182+
DataFlow::CallCfgNode urlParseCall, DataFlow::AttrRead read, string attribute
183+
) {
184+
read.getAttributeName() = attribute and
185+
forex(DataFlow::LocalSourceNode source | source = read.getObject().getALocalSource() |
186+
source = urlParseCall
187+
)
188+
}
189+
190+
/**
191+
* Holds if `g` is a falsiness check `not parsed.<attribute>` on a parsed URL `node`.
192+
*/
193+
private predicate parsedUrlAttributeFalsiness(
194+
DataFlow::GuardNode g, ControlFlowNode node, boolean branch, string attribute
195+
) {
196+
exists(DataFlow::CallCfgNode urlParseCall, DataFlow::AttrRead attributeRead |
96197
urlParseCall = getUrlParseCall() and
97-
netlocRead = urlParseCall.getAnAttributeRead("netloc") and
98-
node = urlParseCall.getArg(0).asCfgNode()
99-
|
100-
// either a simple check of the netloc attribute
101-
g = netlocRead.asCfgNode() and
198+
parsedUrlAttributeRead(urlParseCall, attributeRead, attribute) and
199+
g = attributeRead.asCfgNode() and
200+
node = [urlParseCall.getArg(0), urlParseCall.getArgByName(["url", "urlstring"])].asCfgNode() and
102201
branch = false
202+
)
203+
}
204+
205+
/**
206+
* Holds if `g` compares `attribute` (`"netloc"` or `"scheme"`) of a parsed URL `node`
207+
* against `other` using `op`, and the equality or collection membership holds on `branch`.
208+
*/
209+
private predicate parsedUrlAttributeComparison(
210+
DataFlow::GuardNode g, ControlFlowNode node, boolean branch, string attribute, Cmpop op,
211+
Expr other
212+
) {
213+
exists(
214+
DataFlow::CallCfgNode urlParseCall, DataFlow::AttrRead attributeRead, CompareNode cmp,
215+
ControlFlowNode otherNode
216+
|
217+
urlParseCall = getUrlParseCall() and
218+
parsedUrlAttributeRead(urlParseCall, attributeRead, attribute) and
219+
node = [urlParseCall.getArg(0), urlParseCall.getArgByName(["url", "urlstring"])].asCfgNode() and
220+
cmp = g and
221+
op = unique(Cmpop opp | opp = cmp.getNode().getAnOp()) and
222+
other = otherNode.getNode()
223+
|
224+
op.getSymbol() in ["==", "is"] and
225+
branch = true and
226+
(
227+
cmp.operands(attributeRead.asCfgNode(), op, otherNode) or
228+
cmp.operands(otherNode, op, attributeRead.asCfgNode())
229+
)
103230
or
104-
// or a comparison (we don't care against what)
105-
exists(Compare cmp, string op |
106-
cmp = g.getNode() and
107-
op = unique(Cmpop opp | opp = cmp.getAnOp()).getSymbol() and
108-
cmp.getASubExpression() = netlocRead.asExpr()
109-
|
110-
op in ["==", "is", "in"] and branch = true
111-
or
112-
op in ["!=", "is not", "not in"] and branch = false
231+
op.getSymbol() in ["!=", "is not"] and
232+
branch = false and
233+
(
234+
cmp.operands(attributeRead.asCfgNode(), op, otherNode) or
235+
cmp.operands(otherNode, op, attributeRead.asCfgNode())
113236
)
237+
or
238+
op.getSymbol() = "in" and
239+
membershipChecksCollection(other) and
240+
branch = true and
241+
cmp.operands(attributeRead.asCfgNode(), op, otherNode)
242+
or
243+
op.getSymbol() = "not in" and
244+
membershipChecksCollection(other) and
245+
branch = false and
246+
cmp.operands(attributeRead.asCfgNode(), op, otherNode)
247+
)
248+
}
249+
250+
/** A `netloc` check that does not allow `""` (e.g. `parsed.netloc == request.host`). */
251+
private predicate nonEmptyNetlocCheck(
252+
DataFlow::GuardNode g, ControlFlowNode node, boolean branch
253+
) {
254+
exists(Cmpop op, Expr other |
255+
parsedUrlAttributeComparison(g, node, branch, "netloc", op, other) and
256+
comparisonExcludesEmptyString(op, other)
257+
)
258+
}
259+
260+
/** A `netloc` check that allows `""` (e.g. `not parsed.netloc` or `parsed.netloc in ["", host]`). */
261+
private predicate possiblyEmptyNetlocCheck(
262+
DataFlow::GuardNode g, ControlFlowNode node, boolean branch
263+
) {
264+
parsedUrlAttributeFalsiness(g, node, branch, "netloc")
265+
or
266+
exists(Cmpop op, Expr other |
267+
parsedUrlAttributeComparison(g, node, branch, "netloc", op, other) and
268+
not comparisonExcludesEmptyString(op, other)
269+
)
270+
}
271+
272+
/** A `scheme` check that ensures `scheme` is `""`. */
273+
private predicate emptySchemeCheck(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) {
274+
parsedUrlAttributeFalsiness(g, node, branch, "scheme")
275+
or
276+
exists(Cmpop op, Expr other |
277+
parsedUrlAttributeComparison(g, node, branch, "scheme", op, other) and
278+
op.getSymbol() in ["==", "is", "!=", "is not"] and
279+
isEmptyString(other)
280+
)
281+
}
282+
283+
/** Holds if every possible callee is `methodName` bound to `object`. */
284+
private predicate callsOnlyMethod(
285+
DataFlow::MethodCallNode call, DataFlow::Node object, string methodName
286+
) {
287+
forex(DataFlow::LocalSourceNode source | source = call.getFunction().getALocalSource() |
288+
source.(DataFlow::AttrRead).getAttributeName() = methodName and
289+
source.(DataFlow::AttrRead).getObject() = object
290+
)
291+
}
292+
293+
private predicate leadingSlashesCheck(
294+
DataFlow::GuardNode g, ControlFlowNode node, boolean branch
295+
) {
296+
exists(DataFlow::MethodCallNode call, DataFlow::Node url, Expr prefix |
297+
callsOnlyMethod(call, url, "startswith") and
298+
(
299+
prefix = call.getArg(0).asExpr() or
300+
prefix = call.getArg(0).asExpr().(Tuple).getAnElt()
301+
) and
302+
prefix.(StringLiteral).getText() in ["/", "//", "///"] and
303+
not exists(call.getArg(1)) and
304+
not exists(call.getStarArg()) and
305+
g = call.asCfgNode() and
306+
node = url.asCfgNode() and
307+
branch = false
308+
)
309+
}
310+
311+
/** Holds if every local source of `node` is a no-argument `strip` or `lstrip` call. */
312+
private predicate isStripped(DataFlow::Node node) {
313+
forex(DataFlow::LocalSourceNode source | source = node.getALocalSource() |
314+
callsOnlyMethod(source, _, ["strip", "lstrip"]) and
315+
not exists(source.asExpr().(Call).getAPositionalArg()) and
316+
not exists(source.asExpr().(Call).getANamedArg())
317+
)
318+
}
319+
320+
/** An `isprintable` check on a stripped URL. */
321+
private predicate normalizedPrintableCheck(
322+
DataFlow::GuardNode g, ControlFlowNode node, boolean branch
323+
) {
324+
exists(DataFlow::MethodCallNode call, DataFlow::Node url |
325+
callsOnlyMethod(call, url, "isprintable") and
326+
not exists(call.asExpr().(Call).getAPositionalArg()) and
327+
not exists(call.asExpr().(Call).getANamedArg()) and
328+
isStripped(url) and
329+
g = call.asCfgNode() and
330+
node = url.asCfgNode() and
331+
branch = true
114332
)
115333
}
116334

117335
/**
118-
* A check of `urllib.parse.urlparse().netloc`, considered as a sanitizer-guard for URL redirection.
336+
* A check of `urllib.parse.urlparse` or `urllib.parse.urlsplit` considered as a
337+
* sanitizer-guard for URL redirection.
338+
*
339+
* Either `netloc` is restricted to a non-empty host, or `netloc` may be empty
340+
* and `scheme`, leading slashes (`///`), and browser-ignored characters are also checked.
119341
*/
120342
private class NetlocCheck extends UrlRedirect::Sanitizer {
121-
NetlocCheck() { this = DataFlow::BarrierGuard<netlocCheck/3>::getABarrierNode() }
343+
NetlocCheck() {
344+
this = DataFlow::BarrierGuard<nonEmptyNetlocCheck/3>::getABarrierNode()
345+
or
346+
this = DataFlow::BarrierGuard<possiblyEmptyNetlocCheck/3>::getABarrierNode() and
347+
this = DataFlow::BarrierGuard<emptySchemeCheck/3>::getABarrierNode() and
348+
// Browsers interpret three or more leading slashes as an authority, unlike `urlparse`.
349+
this = DataFlow::BarrierGuard<leadingSlashesCheck/3>::getABarrierNode() and
350+
// Browsers ignore leading whitespace and control characters within URLs.
351+
this = DataFlow::BarrierGuard<normalizedPrintableCheck/3>::getABarrierNode()
352+
}
122353

123354
override predicate sanitizes(UrlRedirect::FlowState state) {
124-
// `urlparse` does not handle backslashes
355+
// Neither parser handles backslashes as browsers do.
125356
state instanceof UrlRedirect::NoBackslashes
126357
}
127358
}

‎python/ql/src/Security/CWE-601/UrlRedirect.qhelp‎

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ redirects on the server; then choose from that list based on the user input prov
1818
</p>
1919
<p>
2020
If this is not possible, then the user input should be validated in some other way,
21-
for example, by verifying that the target URL does not include an explicit host name.
21+
using a maintained framework helper that accounts for how browsers interpret URLs.
2222
</p>
2323
</recommendation>
2424

@@ -38,40 +38,41 @@ and check that the user input is in that list:
3838
<sample src="examples/redirect_good.py"/>
3939

4040
<p>
41-
Often this is not possible, so an alternative is to check that the target URL does not
42-
specify an explicit host name. For example, you can use the <code>urlparse</code> function
43-
from the Python standard library to parse the URL and check that the <code>netloc</code>
44-
attribute is empty.
41+
If an allowlist is not practical, use a maintained framework helper to validate the
42+
redirect target. For Django applications, <code>url_has_allowed_host_and_scheme</code>
43+
checks whether a URL is safe to redirect to:
4544
</p>
4645

46+
<sample src="examples/redirect_good3.py"/>
47+
4748
<p>
48-
Note, however, that some cases are not handled as we desire out-of-the-box by <code>urlparse</code>, so we need to adjust two things, as shown in the example below:
49+
Note that <code>url_has_allowed_host_and_scheme</code> handles backslashes correctly, so no
50+
additional processing is required.
51+
</p>
52+
53+
<p>
54+
Do not rely on <code>urlparse</code> or <code>urlsplit</code> alone to validate redirect
55+
targets. These functions do not interpret all URLs in the same way as browsers:
4956
</p>
5057

5158
<ul>
5259
<li>
5360
Many browsers accept backslash characters (<code>\</code>) as equivalent
54-
to forward slash characters (<code>/</code>) in URLs, but the <code>urlparse</code> function
55-
does not.
61+
to forward slash characters (<code>/</code>) in URLs, but these parsers do not.
5662
</li>
5763
<li>
5864
Mistyped URLs such as <code>https:/example.com</code> or <code>https:///example.com</code> are parsed as having an empty <code>netloc</code> attribute, while browsers will still redirect to the correct site.
5965
</li>
66+
<li>
67+
URLs such as <code>///example.com</code> and <code>////example.com</code> are parsed as
68+
having both an empty <code>netloc</code> and an empty <code>scheme</code>, but browsers
69+
interpret them as URLs with a host name.
70+
</li>
6071
</ul>
6172

62-
63-
<sample src="examples/redirect_good2.py"/>
64-
6573
<p>
66-
For Django application, you can use the function <code>url_has_allowed_host_and_scheme</code>
67-
to check that a URL is safe to redirect to, as shown in the following example:
68-
</p>
69-
70-
<sample src="examples/redirect_good3.py"/>
71-
72-
<p>
73-
Note that <code>url_has_allowed_host_and_scheme</code> handles backslashes correctly, so no
74-
additional processing is required.
74+
Hand-written validation must also account for whitespace and control characters that
75+
browsers may ignore. Validate the same normalized value that is used in the redirect.
7576
</p>
7677

7778
</example>

‎python/ql/src/Security/CWE-601/examples/redirect_good2.py‎

Lines changed: 0 additions & 14 deletions
This file was deleted.

‎python/ql/src/change-notes/2026-10-22-url-redirect-validation.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+
* The `py/url-redirection` query now reports redirects guarded only by incomplete `urllib.parse` checks, including checks that accept URLs beginning with three or more slashes. The query help no longer recommends an empty network location and scheme as sufficient validation.

0 commit comments

Comments
 (0)