Bug 27239 - Do not do HTTP Refresh to javascript: or other dangerous URI schemes
Summary: Do not do HTTP Refresh to javascript: or other dangerous URI schemes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-13 15:32 PDT by Chris Evans
Modified: 2009-10-23 19:07 PDT (History)
6 users (show)

See Also:


Attachments
Fix that limits which schemes we will Refresh to (3.07 KB, patch)
2009-07-13 15:50 PDT, Chris Evans
eric: review-
Details | Formatted Diff | Diff
A much better fix. (1.05 KB, patch)
2009-10-21 21:26 PDT, Chris Evans
abarth: review-
Details | Formatted Diff | Diff
Simple fix plus test (3.74 KB, patch)
2009-10-22 17:18 PDT, Chris Evans
abarth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Simple fix plus test v2 (3.74 KB, patch)
2009-10-23 18:47 PDT, Chris Evans
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Evans 2009-07-13 15:32:51 PDT
Patch to follow.
It brings WebKit in line with IE, Firefox and Opera.

See:
http://code.google.com/p/browsersec/wiki/Part2#Redirection_restrictions

The proposed whitelist is fairly restrictive. We could feasibly add "data" if you wanted, since the data: protocol causes a change of document.domain.
Comment 1 Chris Evans 2009-07-13 15:50:31 PDT
Created attachment 32686 [details]
Fix that limits which schemes we will Refresh to
Comment 2 Chris Evans 2009-07-13 15:51:39 PDT
Comment on attachment 32686 [details]
Fix that limits which schemes we will Refresh to

Sam / Adam, something like this?
Comment 3 Darin Adler 2009-07-14 11:56:56 PDT
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.
Comment 4 Adam Barth 2009-07-14 12:02:15 PDT
Poor gopher.  Always getting left out.
Comment 5 Alexey Proskuryakov 2009-07-14 16:47:04 PDT
Do we need to add a layout test for META refreshes, as well?
Comment 6 Eric Seidel (no email) 2009-08-07 12:40:41 PDT
Comment on attachment 32686 [details]
Fix that limits which schemes we will Refresh to

r-, awaiting Chris's replies to the above comments.
Comment 7 Adam Barth 2009-08-10 08:57:53 PDT
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 8 Adam Barth 2009-08-10 09:12:57 PDT
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.
Comment 9 Chris Evans 2009-08-10 12:14:45 PDT
@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.
Comment 10 Chris Evans 2009-10-21 21:26:25 PDT
Created attachment 41636 [details]
A much better fix.

A substantially better fix. i.e. it actually covers Refresh to an http URL.
Comment 11 Chris Evans 2009-10-21 21:27:52 PDT
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.
Comment 12 Chris Evans 2009-10-21 21:33:18 PDT
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 13 Adam Barth 2009-10-21 22:36:45 PDT
Comment on attachment 41636 [details]
A much better fix.

Nice.  Yeah, I don't know how to test it.
Comment 14 Adam Barth 2009-10-21 23:01:55 PDT
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?
Comment 15 Adam Barth 2009-10-21 23:10:55 PDT
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
Comment 16 Chris Evans 2009-10-21 23:16:31 PDT
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...
Comment 17 Chris Evans 2009-10-22 17:18:28 PDT
Created attachment 41700 [details]
Simple fix plus test
Comment 18 Chris Evans 2009-10-22 17:18:47 PDT
Now with more test....
Comment 19 Adam Barth 2009-10-22 17:48:10 PDT
Comment on attachment 41700 [details]
Simple fix plus test

This test will make Julie sad, but I'm glad to have it.
Comment 20 Chris Evans 2009-10-22 19:30:34 PDT
Can you land it Adam?
Comment 21 Adam Barth 2009-10-22 23:56:08 PDT
(In reply to comment #20)
> Can you land it Adam?

Yes.  Probably tomorrow morning.  The commit-queue doesn't work for security bugs.
Comment 22 Eric Seidel (no email) 2009-10-23 14:01:31 PDT
Comment on attachment 41700 [details]
Simple fix plus test

This isn't a Security bug.
Comment 23 Chris Evans 2009-10-23 14:05:10 PDT
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 24 WebKit Commit Bot 2009-10-23 14:09:55 PDT
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
Comment 25 Chris Evans 2009-10-23 14:27:27 PDT
Grr :) Obviously, it runs for me... how do I get at the output for the failure case? Failure reason, any simplified text diffs, ...
Comment 26 Eric Seidel (no email) 2009-10-23 14:39:51 PDT
(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.
Comment 27 Chris Evans 2009-10-23 18:47:12 PDT
Created attachment 41771 [details]
Simple fix plus test v2
Comment 28 Chris Evans 2009-10-23 18:48:29 PDT
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 29 Adam Barth 2009-10-23 18:56:21 PDT
Comment on attachment 41771 [details]
Simple fix plus test v2

The commit queue will freak out if a non-reviewer sets the review+ flag.
Comment 30 Eric Seidel (no email) 2009-10-23 19:02:24 PDT
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 31 WebKit Commit Bot 2009-10-23 19:06:55 PDT
Comment on attachment 41771 [details]
Simple fix plus test v2

Clearing flags on attachment: 41771

Committed r50018: <http://trac.webkit.org/changeset/50018>
Comment 32 WebKit Commit Bot 2009-10-23 19:07:00 PDT
All reviewed patches have been landed.  Closing bug.