| Summary: | Do not do HTTP Refresh to javascript: or other dangerous URI schemes | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Evans <scarybeasts> | ||||||||||
| Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, collinj, commit-queue, eric, sam, scarybeasts | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | OS X 10.5 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Chris Evans
2009-07-13 15:32:51 PDT
Created attachment 32686 [details]
Fix that limits which schemes we will Refresh to
Comment on attachment 32686 [details]
Fix that limits which schemes we will Refresh to
Sam / Adam, something like this?
Comment on attachment 32686 [details]
Fix that limits which schemes we will Refresh to
If we think there will be other places where "HTTP family plus FTP" is the set we'll care about, then we may want a new function to do this.
A protocolIs function already exists that works on a string without creating and parsing a KURL, so there's no need to make a KURL in the new code. But we would need a version of protocolInHTTPFamily that worked on a plain string.
The change log lists the file changed, but not the function name. It should list the function name.
Poor gopher. Always getting left out. Do we need to add a layout test for META refreshes, as well? Comment on attachment 32686 [details]
Fix that limits which schemes we will Refresh to
r-, awaiting Chris's replies to the above comments.
Chris, are you planning to update the patch to address Darin's comments? If you're working on higher priority issues, we can find someone else to look at this bug. Comment from a user on the Chromium issue tracker: [[ Comment 6 by sbjesse, Today (1 minute ago) @abarth thx for the pointer. but i think the most direct fix is on the view-source scheme, where no redirection/refresh is expected, not restricting refresh-target schemes (although either a hardcoded or a tunable list will be quite cool) ]] Should we restrict this change to view source frames only? We should test interoperability here. @adam: at this time, I am pretty buried auditing some of the new features for the next version of Chrome. If you wanted this addressed sooner, I'd gladly accept the offer of help. Re: comment #8, I agree with sbjesse. This is not the proper fix for the view-source: Refresh interaction; even with this restriction in place, someone could simply Refresh: 0;http://www.evil.com/ which still hijacks window.location (which could then execute script, as per the original complaint that it should not be possible for a view-source: URL to render active content). This fix still has individual merit, however. Looks like Firefox just made this change as a defense-in-depth measure for web apps, such that careless construction of Refresh: headers cannot lead to XSS. Created attachment 41636 [details]
A much better fix.
A substantially better fix. i.e. it actually covers Refresh to an http URL.
Ok, can we land this? I fiddled with a layout test but I'm not sure it can be done, so I request an exemption. Best I can tell, you can't navigate to or iframe a view-source: URL in a layout test. One more comment - the bug title isn't really accurate any more. It's now more like "Don't execute javascript in view-source mode". Comment on attachment 41636 [details]
A much better fix.
Nice. Yeah, I don't know how to test it.
Comment on attachment 41636 [details]
A much better fix.
Actually, I just made a big deal on webkit-dev about how we couldn't change FrameLoader any more without tests. Can you make a manual-test?
Wait, isn't there some attribute you can put on frames to put them in view-source mode? http://trac.webkit.org/browser/trunk/LayoutTests/fast/frames/viewsource-unfinished-tags.html view-source:https://cevans-app.appspot.com/refresh Success: you see HTML source. Fail: a redirect to www.google.com This tests both the HTTP header and the meta tag method. n.b. on my Linux dev channel Chrome, the above URL does nothing (blank page) if the page is not in the cache. If you see that, hitting refresh will get you a success or fail condition as outlined above. But let me first investigate that weird viewsource iframe attribute (my grep was for view-source so I missed the hyphenless version... Created attachment 41700 [details]
Simple fix plus test
Now with more test.... Comment on attachment 41700 [details]
Simple fix plus test
This test will make Julie sad, but I'm glad to have it.
Can you land it Adam? (In reply to comment #20) > Can you land it Adam? Yes. Probably tomorrow morning. The commit-queue doesn't work for security bugs. Comment on attachment 41700 [details]
Simple fix plus test
This isn't a Security bug.
I agree it has no particular security consequence. Regrettably, it was incorrectly reported in a media story as a noteworthy vulnerability. A few lower-tier security professionals have also misunderstood this. I'll pull the fix into Chromium when it is landed, just to keep everyone placated. Comment on attachment 41700 [details]
Simple fix plus test
Rejecting patch 41700 from commit-queue.
Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11510 test cases.
http/tests/security/view-source-no-refresh.html -> failed
Exiting early after 1 failures. 8791 tests run.
248.69s total testing time
8790 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output
Grr :) Obviously, it runs for me... how do I get at the output for the failure case? Failure reason, any simplified text diffs, ... (In reply to comment #25) > Grr :) Obviously, it runs for me... how do I get at the output for the failure > case? Failure reason, any simplified text diffs, ... --- /tmp/layout-test-results/http/tests/security/view-source-no-refresh-expected.txt 2009-10-23 14:09:49.000000000 -0700 +++ /tmp/layout-test-results/http/tests/security/view-source-no-refresh-actual.txt 2009-10-23 14:09:49.000000000 -0700 @@ -1 +1,3 @@ Success - did not redirect to Javascript + + bugzilla-tool isn't smart enough to upload the failure diffs yet. Not sure where it would upload them to. Created attachment 41771 [details]
Simple fix plus test v2
Weird, looks like a simple whitespace issue. My local "run_webkit_tests" does not care but the commit queue obviously does, so can you see if the latest patch (identical apart from the addition of two blank lines) passes? Comment on attachment 41771 [details]
Simple fix plus test v2
The commit queue will freak out if a non-reviewer sets the review+ flag.
I've been told that run_webkit_tests (chromium's python script) may produce different expected results from run-webkit-tests (webkit's script). That may be causing the line spacing. Comment on attachment 41771 [details] Simple fix plus test v2 Clearing flags on attachment: 41771 Committed r50018: <http://trac.webkit.org/changeset/50018> All reviewed patches have been landed. Closing bug. |