Redirect hardening #1460
Merged
Redirect hardening #1460
Conversation
This file contains 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
As far as I know, the `urllib.parse`-based filtering is safe, but there’s no legitimate way for the `GET` handler to receive a `referer` – the entire handler is only there in case someone manually goes to their browser’s address bar and loads the current address while in the middle of 2FA, and possibly shouldn’t exist at all.
Just applying normal good practice to a key place; as far as I know, nothing was exploitable.
instead of filtering just on the 2FA endpoint – for consistency.
Of course it was exploitable! This is “fixed” in WebOb 1.8.8, but it’s not really WebOb’s responsibility to turn `Location: //…` into an absolute URL where the value is treated as a path-absolute URL. In fact, since RFC 7231 (the latest HTTP/1.1 specification), `Location` doesn’t need to be absolute; WebOb shouldn’t touch it at all. Treating the value differently from clients is bound to cause confusion (e.g. when we move away from WebOb, would this have been a problem waiting to happen?). WebOb’s current fix is to alter the prefix to `/%2f`, which is *semantically different*. WebOb 1.8.8 also checks for the specific prefix `"//"`, but it still uses `urljoin`, which will happily cause exactly the same problem with `" //evil.example"` (substitute tab for space on older patches of Python, e.g. 3.10 before 3.10.12 (CVE-2023-24329)). That’s not an issue for what’s probably the most common way to have this open redirect vulnerability – what we did, echoing the request path – but, again, it’s not WebOb’s issue in general. Finally, we should probably just redirect to slash-normalized URLs globally before even getting to the Python layer, but… another time.
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.
Note
As a security fix, this is already deployed.