handlers: reject protocol-relative URLs in redirect validation#5281
Open
adilburaksen wants to merge 1 commit into
Open
handlers: reject protocol-relative URLs in redirect validation#5281adilburaksen wants to merge 1 commit into
adilburaksen wants to merge 1 commit into
Conversation
_SAFE_URL_PATTERN's relative-URL branch matched the empty string prefix of '//evil.com', so check_redirect_url accepted protocol-relative URLs as safe. Browsers interpret '//evil.com/path' as 'same-scheme://evil.com/path', enabling open redirect from any endpoint that passes the user-supplied 'next' / 'redirect' parameter through check_redirect_url. Add a negative lookahead (?!//) before the relative-URL branch so that any URL beginning with '//' is rejected unless it already matched the explicit-scheme branch (http://, https://, etc).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
check_redirect_urlinsrc/appengine/handlers/base_handler.pyuses_SAFE_URL_PATTERN(based on the Closure LibrarySafeUrlpattern) tovalidate redirect targets. The relative-URL branch of that pattern is:
[^:/?#]*can match zero characters, after which(?:[/?#]|$)matchesthe leading
/of//evil.com. This means the pattern accepts anyprotocol-relative URL such as
//attacker.example.com/stealas safe.Browsers interpret
//evil.com/pathas scheme-relative (same scheme asthe current page), so a
next=//evil.com/loginparameter causes apost-login redirect to the attacker's host.
Fix
Add a negative lookahead
(?!//)to the relative-URL branch:This rejects any URL that the relative branch would match but that begins
with
//, while continuing to accept ordinary relative paths (/login,relative/path) and absolute URLs with safe schemes.Verification