Fix URL whitelist bypass and server-side validation issues#87
Open
pau101 wants to merge 4 commits intoPlompi:masterfrom
Open
Fix URL whitelist bypass and server-side validation issues#87pau101 wants to merge 4 commits intoPlompi:masterfrom
pau101 wants to merge 4 commits intoPlompi:masterfrom
Conversation
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
1 participant
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.
Currently the URL whitelist since it was introduced in 1.4.0 has a flaw in its validation logic. The code computes
baseURLbut checks the originalurlusingstartsWith()against whitelist entries.However, even if
baseURLwere used,startsWith()would only protect against userinfo bypasses such asyoutube.com@evil.com, not subdomain spoofing whereyoutube.com.evil.commatches a whitelistedyoutube.com.Additionally,
URI.toURL()accepts URLs such ashttps:foobar.com. In this case the URI has no authority and thefoobar.comportion is parsed as the path, resulting in anullhost. When played, this causes the client to displayhttps://nullin the whitelisting prompt.The
SetURLRecordmessage also relies only on client-side validation for disc lock status and duration bounds, which can be bypassed by a modified client.This PR resolves these issues by validating URLs, requiring both a scheme and host, and performing an exact whitelist match on
scheme://host. Server-side validation has also been added so locked discs cannot be modified and record duration is clamped to the allowed range (0-3600 seconds) per the client disc_url_screen.xml.Note: manually configured whitelist entries with trailing slashes (e.g.
https://example.com/) will no longer match compared to the original logic.