Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Disallow javascript: navigation in marvin by default #6

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

uint0
Copy link
Contributor

@uint0 uint0 commented Jul 4, 2024

Currently it is possible to ask marvin to visit a javascript: url (e.g. javascript:console.log(1)). This poses a potential challenge bypass for certain configurations of challenges. It is safer to just disallow this behavior by default.

This behavior was originally intended to be banned by a request interceptor, however it turns out the request interceptor doesn't trigger for javascript: urls 😠. Instead this introduces a precheck validator that blocks the visit request prior to the browser attempting to visiting the page.

This change also fixes up some local dev niceties (its a bit noisy but i cbsd making another pr).


Example bug reproduction:

Client

$ curl localhost:8000/visit -X POST -i -H 'x-ssrf-protection:1' -H content-type:application/json -d '{"url":"javascript:(function(){console.log(\"kururu\")})()"}'
...

Old State

Server

{"name":"Browser","hostname":"...","pid":35689,"level":30,"url":"javascript:(function(){console.log(\"kururu\")})()","msg":"received navigation intent","time":"2024-07-04T09:11:45.437Z","v":0}
{"name":"VisitTask","hostname":"...","pid":35689,"level":30,"msg":"Job 21 succeeded","time":"2024-07-04T09:11:45.439Z","v":0}
{"name":"Browser","hostname":"...","pid":35689,"level":30,"type":"log","trace":"unknown (0:20)\nunknown (0:35)","console":"kururu","msg":"console","time":"2024-07-04T09:11:45.469Z","v":0}

New State

{"name":"Browser","hostname":"DESKTOP-393G3RI","pid":36435,"level":30,"url":"javascript:(function(){console.log(\"kururu\")})()","msg":"received navigation intent","time":"2024-07-04T09:13:13.272Z","v":0}
{"name":"Browser","hostname":"DESKTOP-393G3RI","pid":36435,"level":30,"msg":"navigation intent precheck failed with: Invalid protocol. refusing navigation","time":"2024-07-04T09:13:13.273Z","v":0}
{"name":"VisitTask","hostname":"DESKTOP-393G3RI","pid":36435,"level":30,"msg":"Job 22 succeeded","time":"2024-07-04T09:13:13.274Z","v":0}

@uint0
Copy link
Contributor Author

uint0 commented Jul 4, 2024

No rush to merge, but jfyi I don't have merge perms so someone else will need to merge it.

@BlueAlder BlueAlder merged commit 690c526 into DownUnderCTF:master Jul 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants