Skip to content

Commit f0e5329

Browse files
owen-mcCopilot
owen-mc
and
Copilot
committed
Require known hosts in relative URL checks
Do not accept arbitrary comparison targets merely because empty netloc values are possible. Reuse the collection proof for allowlists containing empty strings and retain conservative handling of mutable collections. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent e224317 commit f0e5329

4 files changed

Lines changed: 144 additions & 23 deletions

File tree

‎python/ql/lib/change-notes/2026-10-22-url-redirect-validation.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
---
22
category: minorAnalysis
33
---
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`.
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. Host allowlists must resolve locally to known values, and collection allowlists are trusted only when immutable or used directly as literals. Hostname attributes are trusted only when all possible receivers originate from `urlparse` or `urlsplit` calls on constant URLs.

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

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -144,10 +144,11 @@ private module Urllib {
144144

145145
/**
146146
* Holds if every local source of `e` is an immutable collection literal, or a mutable
147-
* collection literal used directly in the membership test, whose elements or keys all exclude
148-
* `""`.
147+
* collection literal used directly in the membership test, with known host elements or keys.
148+
* Empty strings are accepted only when `allowEmpty` is true.
149149
*/
150-
private predicate membershipExcludesEmptyString(Expr e) {
150+
private predicate membershipChecksKnownHosts(Expr e, boolean allowEmpty) {
151+
allowEmpty in [true, false] and
151152
forex(DataFlow::LocalSourceNode source | source = DataFlow::exprNode(e).getALocalSource() |
152153
exists(Expr collection | collection = source.asExpr() |
153154
(collection instanceof List or collection instanceof Tuple or collection instanceof Set) and
@@ -160,12 +161,18 @@ private module Urllib {
160161
]
161162
|
162163
excludesEmptyString(elt)
164+
or
165+
allowEmpty = true and isEmptyString(elt)
163166
)
164167
or
165168
collection instanceof Dict and
166169
collection = e and
167170
not collection.(Dict).getAnItem() instanceof DictUnpacking and
168-
forall(Expr key | key = collection.(Dict).getAKey() | excludesEmptyString(key))
171+
forall(Expr key | key = collection.(Dict).getAKey() |
172+
excludesEmptyString(key)
173+
or
174+
allowEmpty = true and isEmptyString(key)
175+
)
169176
)
170177
)
171178
}
@@ -179,7 +186,7 @@ private module Urllib {
179186

180187
/** Holds if comparing against `e` using `op` cannot accept `""`. */
181188
private predicate comparisonExcludesEmptyString(Cmpop op, Expr e) {
182-
op.getSymbol() in ["in", "not in"] and membershipExcludesEmptyString(e)
189+
op.getSymbol() in ["in", "not in"] and membershipChecksKnownHosts(e, false)
183190
or
184191
op.getSymbol() in ["==", "is", "!=", "is not"] and excludesEmptyString(e)
185192
}
@@ -264,15 +271,19 @@ private module Urllib {
264271
)
265272
}
266273

267-
/** A `netloc` check that allows `""` (e.g. `not parsed.netloc` or `parsed.netloc in ["", host]`). */
274+
/** A `netloc` check allowing `""` and otherwise only known hosts. */
268275
private predicate possiblyEmptyNetlocCheck(
269276
DataFlow::GuardNode g, ControlFlowNode node, boolean branch
270277
) {
271278
parsedUrlAttributeFalsiness(g, node, branch, "netloc")
272279
or
273280
exists(Cmpop op, Expr other |
274281
parsedUrlAttributeComparison(g, node, branch, "netloc", op, other) and
275-
not comparisonExcludesEmptyString(op, other)
282+
(
283+
op.getSymbol() in ["==", "is", "!=", "is not"] and isEmptyString(other)
284+
or
285+
op.getSymbol() in ["in", "not in"] and membershipChecksKnownHosts(other, true)
286+
)
276287
)
277288
}
278289

‎python/ql/test/query-tests/Security/CWE-601-UrlRedirect/UrlRedirect.expected‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,10 @@
6161
| test.py:891:25:891:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:891:25:891:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
6262
| test.py:914:25:914:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:914:25:914:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
6363
| test.py:924:25:924:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:924:25:924:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
64+
| test.py:953:25:953:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:953:25:953:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
65+
| test.py:967:25:967:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:967:25:967:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
66+
| test.py:981:25:981:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:981:25:981:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
67+
| test.py:997:25:997:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:997:25:997:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
6468
edges
6569
| test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:1:26:1:32 | ControlFlowNode for request | provenance | |
6670
| test.py:1:26:1:32 | ControlFlowNode for request | test.py:7:14:7:20 | ControlFlowNode for request | provenance | |
@@ -125,6 +129,10 @@ edges
125129
| test.py:1:26:1:32 | ControlFlowNode for request | test.py:883:17:883:23 | ControlFlowNode for request | provenance | |
126130
| test.py:1:26:1:32 | ControlFlowNode for request | test.py:905:17:905:23 | ControlFlowNode for request | provenance | |
127131
| test.py:1:26:1:32 | ControlFlowNode for request | test.py:920:17:920:23 | ControlFlowNode for request | provenance | |
132+
| test.py:1:26:1:32 | ControlFlowNode for request | test.py:945:17:945:23 | ControlFlowNode for request | provenance | |
133+
| test.py:1:26:1:32 | ControlFlowNode for request | test.py:959:17:959:23 | ControlFlowNode for request | provenance | |
134+
| test.py:1:26:1:32 | ControlFlowNode for request | test.py:973:17:973:23 | ControlFlowNode for request | provenance | |
135+
| test.py:1:26:1:32 | ControlFlowNode for request | test.py:987:17:987:23 | ControlFlowNode for request | provenance | |
128136
| test.py:7:5:7:10 | ControlFlowNode for target | test.py:8:21:8:26 | ControlFlowNode for target | provenance | |
129137
| test.py:7:14:7:20 | ControlFlowNode for request | test.py:7:14:7:25 | ControlFlowNode for Attribute | provenance | AdditionalTaintStep |
130138
| test.py:7:14:7:25 | ControlFlowNode for Attribute | test.py:7:14:7:43 | ControlFlowNode for Attribute() | provenance | dict.get |
@@ -438,6 +446,26 @@ edges
438446
| test.py:920:17:920:28 | ControlFlowNode for Attribute | test.py:920:17:920:46 | ControlFlowNode for Attribute() | provenance | dict.get |
439447
| test.py:920:17:920:46 | ControlFlowNode for Attribute() | test.py:920:17:920:65 | ControlFlowNode for Attribute() | provenance | Config |
440448
| test.py:920:17:920:65 | ControlFlowNode for Attribute() | test.py:920:5:920:13 | ControlFlowNode for untrusted | provenance | |
449+
| test.py:945:5:945:13 | ControlFlowNode for untrusted | test.py:953:25:953:33 | ControlFlowNode for untrusted | provenance | |
450+
| test.py:945:17:945:23 | ControlFlowNode for request | test.py:945:17:945:28 | ControlFlowNode for Attribute | provenance | AdditionalTaintStep |
451+
| test.py:945:17:945:28 | ControlFlowNode for Attribute | test.py:945:17:945:46 | ControlFlowNode for Attribute() | provenance | dict.get |
452+
| test.py:945:17:945:46 | ControlFlowNode for Attribute() | test.py:945:17:945:65 | ControlFlowNode for Attribute() | provenance | Config |
453+
| test.py:945:17:945:65 | ControlFlowNode for Attribute() | test.py:945:5:945:13 | ControlFlowNode for untrusted | provenance | |
454+
| test.py:959:5:959:13 | ControlFlowNode for untrusted | test.py:967:25:967:33 | ControlFlowNode for untrusted | provenance | |
455+
| test.py:959:17:959:23 | ControlFlowNode for request | test.py:959:17:959:28 | ControlFlowNode for Attribute | provenance | AdditionalTaintStep |
456+
| test.py:959:17:959:28 | ControlFlowNode for Attribute | test.py:959:17:959:46 | ControlFlowNode for Attribute() | provenance | dict.get |
457+
| test.py:959:17:959:46 | ControlFlowNode for Attribute() | test.py:959:17:959:65 | ControlFlowNode for Attribute() | provenance | Config |
458+
| test.py:959:17:959:65 | ControlFlowNode for Attribute() | test.py:959:5:959:13 | ControlFlowNode for untrusted | provenance | |
459+
| test.py:973:5:973:13 | ControlFlowNode for untrusted | test.py:981:25:981:33 | ControlFlowNode for untrusted | provenance | |
460+
| test.py:973:17:973:23 | ControlFlowNode for request | test.py:973:17:973:28 | ControlFlowNode for Attribute | provenance | AdditionalTaintStep |
461+
| test.py:973:17:973:28 | ControlFlowNode for Attribute | test.py:973:17:973:46 | ControlFlowNode for Attribute() | provenance | dict.get |
462+
| test.py:973:17:973:46 | ControlFlowNode for Attribute() | test.py:973:17:973:65 | ControlFlowNode for Attribute() | provenance | Config |
463+
| test.py:973:17:973:65 | ControlFlowNode for Attribute() | test.py:973:5:973:13 | ControlFlowNode for untrusted | provenance | |
464+
| test.py:987:5:987:13 | ControlFlowNode for untrusted | test.py:997:25:997:33 | ControlFlowNode for untrusted | provenance | |
465+
| test.py:987:17:987:23 | ControlFlowNode for request | test.py:987:17:987:28 | ControlFlowNode for Attribute | provenance | AdditionalTaintStep |
466+
| test.py:987:17:987:28 | ControlFlowNode for Attribute | test.py:987:17:987:46 | ControlFlowNode for Attribute() | provenance | dict.get |
467+
| test.py:987:17:987:46 | ControlFlowNode for Attribute() | test.py:987:17:987:65 | ControlFlowNode for Attribute() | provenance | Config |
468+
| test.py:987:17:987:65 | ControlFlowNode for Attribute() | test.py:987:5:987:13 | ControlFlowNode for untrusted | provenance | |
441469
nodes
442470
| test.py:1:26:1:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember |
443471
| test.py:1:26:1:32 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
@@ -816,4 +844,28 @@ nodes
816844
| test.py:920:17:920:46 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
817845
| test.py:920:17:920:65 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
818846
| test.py:924:25:924:33 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted |
847+
| test.py:945:5:945:13 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted |
848+
| test.py:945:17:945:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
849+
| test.py:945:17:945:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
850+
| test.py:945:17:945:46 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
851+
| test.py:945:17:945:65 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
852+
| test.py:953:25:953:33 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted |
853+
| test.py:959:5:959:13 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted |
854+
| test.py:959:17:959:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
855+
| test.py:959:17:959:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
856+
| test.py:959:17:959:46 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
857+
| test.py:959:17:959:65 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
858+
| test.py:967:25:967:33 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted |
859+
| test.py:973:5:973:13 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted |
860+
| test.py:973:17:973:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
861+
| test.py:973:17:973:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
862+
| test.py:973:17:973:46 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
863+
| test.py:973:17:973:65 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
864+
| test.py:981:25:981:33 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted |
865+
| test.py:987:5:987:13 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted |
866+
| test.py:987:17:987:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
867+
| test.py:987:17:987:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
868+
| test.py:987:17:987:46 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
869+
| test.py:987:17:987:65 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
870+
| test.py:997:25:997:33 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted |
819871
subpaths

‎python/ql/test/query-tests/Security/CWE-601-UrlRedirect/test.py‎

Lines changed: 73 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ def checked_relative_url_early_return():
216216
not untrusted.isprintable()
217217
or untrusted.startswith("///")
218218
or parsed.scheme != ""
219-
or parsed.netloc not in ["", request.host]
219+
or parsed.netloc not in ["", "example.com"]
220220
):
221221
return redirect("/", code=302) # OK
222222
return redirect(untrusted, code=302) # OK
@@ -230,7 +230,7 @@ def checked_split_url():
230230
untrusted.isprintable()
231231
and not untrusted.startswith("///")
232232
and not parsed.scheme
233-
and parsed.netloc in ["", request.host]
233+
and parsed.netloc in ["", "example.com"]
234234
):
235235
return redirect(untrusted, code=302) # OK
236236
return redirect("/", code=302)
@@ -264,27 +264,27 @@ def named_empty_scheme():
264264
return redirect("/", code=302)
265265

266266

267-
@app.route('/checked_named_host_collections')
268-
def checked_named_host_collections():
267+
@app.route('/checked_host_collections')
268+
def checked_host_collections():
269269
untrusted = request.args.get('target', '').replace("\\", "/").strip()
270270
kind = request.args.get('kind')
271-
if kind == "list":
272-
allowed_hosts = ["", request.host]
273-
elif kind == "tuple":
274-
allowed_hosts = ("", request.host)
275-
elif kind == "set":
276-
allowed_hosts = {"", request.host}
277-
else:
278-
allowed_hosts = {"": True, request.host: True}
279-
hosts = allowed_hosts
280271
parsed = urlsplit(untrusted)
281272
if (
282273
untrusted.isprintable()
283274
and not untrusted.startswith("///")
284275
and not parsed.scheme
285-
and parsed.netloc in hosts
286276
):
287-
return redirect(untrusted, code=302) # OK
277+
if kind == "list" and parsed.netloc in ["", "example.com"]:
278+
return redirect(untrusted, code=302) # OK
279+
elif kind == "tuple":
280+
allowed_hosts = ("", "example.com")
281+
hosts = allowed_hosts
282+
if parsed.netloc in hosts:
283+
return redirect(untrusted, code=302) # OK
284+
elif kind == "set" and parsed.netloc in {"", "example.com"}:
285+
return redirect(untrusted, code=302) # OK
286+
elif kind == "dict" and parsed.netloc in {"": True, "example.com": True}:
287+
return redirect(untrusted, code=302) # OK
288288
return redirect("/", code=302)
289289

290290

@@ -938,3 +938,61 @@ def aliased_parser():
938938
):
939939
return redirect(untrusted, code=302) # OK
940940
return redirect("/", code=302)
941+
942+
943+
@app.route('/guarded_request_derived_hostname')
944+
def guarded_request_derived_hostname():
945+
untrusted = request.args.get('target', '').replace("\\", "/").strip()
946+
parts = urlsplit(untrusted)
947+
if (
948+
untrusted.isprintable()
949+
and not untrusted.startswith("///")
950+
and not parts.scheme
951+
and parts.netloc == parts.hostname
952+
):
953+
return redirect(untrusted, code=302) # $ Alert
954+
return redirect("/", code=302)
955+
956+
957+
@app.route('/guarded_request_derived_host_list')
958+
def guarded_request_derived_host_list():
959+
untrusted = request.args.get('target', '').replace("\\", "/").strip()
960+
parts = urlsplit(untrusted)
961+
if (
962+
untrusted.isprintable()
963+
and not untrusted.startswith("///")
964+
and not parts.scheme
965+
and parts.netloc in ["", parts.hostname]
966+
):
967+
return redirect(untrusted, code=302) # $ Alert
968+
return redirect("/", code=302)
969+
970+
971+
@app.route('/guarded_request_host')
972+
def guarded_request_host():
973+
untrusted = request.args.get('target', '').replace("\\", "/").strip()
974+
parts = urlsplit(untrusted)
975+
if (
976+
untrusted.isprintable()
977+
and not untrusted.startswith("///")
978+
and not parts.scheme
979+
and parts.netloc in ["", request.host]
980+
):
981+
return redirect(untrusted, code=302) # $ SPURIOUS: Alert
982+
return redirect("/", code=302)
983+
984+
985+
@app.route('/guarded_mutated_host_list')
986+
def guarded_mutated_host_list():
987+
untrusted = request.args.get('target', '').replace("\\", "/").strip()
988+
allowed_hosts = [""]
989+
allowed_hosts.append(request.args.get('host', ''))
990+
parts = urlsplit(untrusted)
991+
if (
992+
untrusted.isprintable()
993+
and not untrusted.startswith("///")
994+
and not parts.scheme
995+
and parts.netloc in allowed_hosts
996+
):
997+
return redirect(untrusted, code=302) # $ Alert
998+
return redirect("/", code=302)

0 commit comments

Comments
 (0)