Bug 27239 – Do not do HTTP Refresh to javascript: or other dangerous URI schemes

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Evans]

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.

[Chris Evans]

Created attachment 32686 [details]
Fix that limits which schemes we will Refresh to

[Chris Evans]

Comment on attachment 32686 [details]
Fix that limits which schemes we will Refresh to

Sam / Adam, something like this?

[Darin Adler]

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.

[Adam Barth]

Poor gopher.  Always getting left out.

[Alexey Proskuryakov]

Do we need to add a layout test for META refreshes, as well?

[Eric Seidel (no email)]

Comment on attachment 32686 [details]
Fix that limits which schemes we will Refresh to

r-, awaiting Chris's replies to the above comments.

[Adam Barth]

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.

[Adam Barth]

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.

[Chris Evans]

@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.

[Chris Evans]

Created attachment 41636 [details]
A much better fix.

A substantially better fix. i.e. it actually covers Refresh to an http URL.

[Chris Evans]

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.

[Chris Evans]

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".

[Adam Barth]

Comment on attachment 41636 [details]
A much better fix.

Nice.  Yeah, I don't know how to test it.

[Adam Barth]

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?

[Adam Barth]

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

[Chris Evans]

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...

[Chris Evans]

Created attachment 41700 [details]
Simple fix plus test

[Chris Evans]

Now with more test....

[Adam Barth]

Comment on attachment 41700 [details]
Simple fix plus test

This test will make Julie sad, but I'm glad to have it.

[Chris Evans]

Can you land it Adam?

[Adam Barth]

(In reply to comment #20)
> Can you land it Adam?

Yes.  Probably tomorrow morning.  The commit-queue doesn't work for security bugs.

[Eric Seidel (no email)]

Comment on attachment 41700 [details]
Simple fix plus test

This isn't a Security bug.

[Chris Evans]

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.

[WebKit Commit Bot]

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

[Chris Evans]

Grr :) Obviously, it runs for me... how do I get at the output for the failure case? Failure reason, any simplified text diffs, ...

[Eric Seidel (no email)]

(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.

[Chris Evans]

Created attachment 41771 [details]
Simple fix plus test v2

[Chris Evans]

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?

[Adam Barth]

Comment on attachment 41771 [details]
Simple fix plus test v2

The commit queue will freak out if a non-reviewer sets the review+ flag.

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

Comment on attachment 41771 [details]
Simple fix plus test v2

Clearing flags on attachment: 41771

Committed r50018: <http://trac.webkit.org/changeset/50018>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.