Skip to content

Commit f90fc7b

Browse files
committed
Also reject ASCII control chars in relative redirects
Tab/newline/CR are stripped by the WHATWG URL parser before parsing, so /<TAB>/evil.test resolved to https://evil.test/ despite the backslash check. Reject any ASCII control character. Adds regression cases.
1 parent 67b9c00 commit f90fc7b

1 file changed

Lines changed: 12 additions & 4 deletions

File tree

src/webserver/oidc.rs

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1192,10 +1192,12 @@ impl AudienceVerifier {
11921192
/// independent of the client. Browsers implementing the same standard (Chromium,
11931193
/// Firefox, Safari) resolve a `Location: /\evil.test` the same way.
11941194
pub(crate) fn is_safe_relative_redirect(uri: &str) -> bool {
1195-
// Reject any embedded backslash: the WHATWG URL parser used by SQLPage's
1196-
// `url` crate (and by browsers) treats `\` as `/`, so it can smuggle in an
1197-
// authority component.
1198-
if uri.contains('\\') {
1195+
// Reject backslashes and ASCII control characters. The WHATWG URL parser
1196+
// used by SQLPage's `url` crate (and by browsers) treats `\` as `/`, and it
1197+
// removes ASCII tab/newline/CR from anywhere in the input before parsing.
1198+
// Either can smuggle in an authority component: `/\evil.test` and
1199+
// `/\t/evil.test` both resolve to `https://evil.test/`.
1200+
if uri.contains('\\') || uri.contains(|c: char| c.is_ascii_control()) {
11991201
return false;
12001202
}
12011203
let mut chars = uri.chars();
@@ -1242,6 +1244,12 @@ mod tests {
12421244
"\\evil.test",
12431245
"\\\\evil.test",
12441246
"/foo\\bar",
1247+
// ASCII tab/newline/CR are stripped by the WHATWG URL parser, so
1248+
// these resolve to an authority (`//evil.test`) after stripping.
1249+
"/\t/evil.test",
1250+
"/\n/evil.test",
1251+
"/\r/evil.test",
1252+
"/\u{0000}/evil.test",
12451253
"https://evil.test",
12461254
"evil.test",
12471255
"",

0 commit comments

Comments
 (0)