Bug 15313 – Same-origin check wrong when document.domain set

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Adam Barth]

There are two issues here:

1) Two pages that have set their document.domain to the same value should only be considered the same origin if their protocols and port numbers match.  This is particularly important isolation between HTTP and HTTPS pages.

2) Once a page sets its document.domain, it should no longer be able to access pages that have not set their document.domain to the same value.  Otherwise, another page could inject script into that page and access the original origin.

[Adam Barth]

Created attachment 16438 [details]
Patch to fix same-origin check

This patch fixes both issues.

[Adam Barth]

Created attachment 16439 [details]
Improved patch

There was a mistake in my previous patch.  The host check should only occur if both pages have not set their document.domain.  Also moved that check first for performance (as it is the common case).

[Mark Rowe (bdash)]

This would certainly need an HTTP layout test to exercise this code.  There are some good tests in LayoutTests/http/tests/security that you can use for a reference as to how these should work.  You should also be sure to include a ChangeLog entry in your patch, and set the review flag to ? when attaching it so that it will be included in the review queue.

[Adam Barth]

Created attachment 16440 [details]
Test for first issue

Looks like you already have tests for the first issue:

LayoutTests/http/tests/security/cross-frame-access-protocol-explicit-domain.html
LayoutTests/http/tests/security/cross-frame-access-port-explicit-domain.html

But these test are wrong because they believe the access should be permitted.  Here is the attack:
1) Suppose there is an HTTPS site (www.example.com) that sets document.domain = "example.com".
2) A network attacker redirects the browser to http://www.example.com/ and
  a) injects script to set document.domain = "example.com", and
  b) opens a window to https://www.example.com/
3) Now the network attacker can inject script into the HTTPS page, stealing cookies and issuing banking transactions.

Firefox does not permit this access, see nsScriptSecurityManager::CheckSameOriginPrincipalInternal.

I've attached fixes for the tests.  (Tests for the second issue coming shortly.)

[Mark Rowe (bdash)]

Can you please include the changes as part of your patch, along with the updated test results.

[Adam Barth]

Created attachment 16441 [details]
Test for second issue

There is a test for the second issue.

It's hard for me to create updated test results because I neither have a Mac nor a Windows machine.  I can create one patch with all of these changes, but that will take some time as I'll need to pull the entire tree.

[Mark Rowe (bdash)]

Ok, someone else can easily generate the updated results once your complete patch is available.  Thanks for the patch.

[Maciej Stachowiak]

I believe Firefox does ignore the port, but not the protocol, when both documents set document.domain. We added ignoring the port deliberately to match them. 

I don't understand the exploit scenario. How do you "inject script" unless you already have access to example.com, in which case an XSS exploit has already occurred?

I also don't see how #2 from the original description applies. document.domain can only be set to either the true domain or a suffix of the true domain. For example, www.foo.com can set document.domain to foo.com, but not to bar.com. How is it exploitable to still let it access documents where the true domain (from the URL) is www.foo.com?

[Adam Barth]

Created attachment 16442 [details]
Changes all rolled into one patch

I've attached all the changes in one patch.

> I believe Firefox does ignore the port, but not the protocol, when both
> documents set document.domain.

This is possible.  I can run that test on Firefox.  It is essential, however, to check the protocol in order to avoid the HTTPS attack described.

> I don't understand the exploit scenario. How do you "inject script" unless
> you already have access to example.com

HTTPS is designed to be secure against an active network attacker.  An active network attacker can trivially inject script into an HTTP page as it travels across the network.  The issue is that he can then escalate to injecting script into an HTTPS page.  (Imagine a wireless HotSpot in a coffee shop or an airport.)

> I also don't see how #2 from the original description applies.

I've added an attack scenario in the ChangeLog.  The same-origin policy treats separate subdomains as mutually distrusting but allows a controlled form of communication via document.domain.  One common use of this is in mashups, where www.mashup.com and gadget.mashup.com communicate via iframes with document.domain set to "mashup.com" (but, of course, www.mashup.com doesn't fully trust gadget.mashup.com).  For detailed discussion, see <http://www.collinjackson.com/research/papers/fp801-jackson.pdf>.

[Maciej Stachowiak]

Can you provide attack scenarios in the form of test cases, not just a ChangeLog entry? Also, I am almost certain that your statement about how Firefox treats the port when document.domain is set are wrong. Given this, I'd really like test scenarios that can be replicated so we can make sure the behavior is right.

[Sam Weinig]

Comment on attachment 16442 [details]
Changes all rolled into one patch

Doing a port equality check for the domain relaxed case is inconsistent with FireFox and is known to break enterprise (SAP) apps.  Please also include tests for the second situation described in changelog.

[Adam Barth]

Created attachment 16466 [details]
Matches FF2 and IE6 with tests

Thanks go to Collin Jackson for running these test.  Here are how some other browsers behave:

Firefox 2:
Protocol mismatch, document.domain set: Denied.
Port mismatch, document.domain set: Allowed.
Only one page has set document.domain: Denied.

Internet Explorer 6:
Protocol mismatch, document.domain set: Denied.
Port mismatch, document.domain set: Allowed.
Only one page has set document.domain: Denied.

Internet Explorer 7:
Protocol mismatch, document.domain set: Denied.
Port mismatch, document.domain set: Denied.
Only one page has set document.domain: Denied.

Opera 9:
Protocol mismatch, document.domain set: Denied.
Port mismatch, document.domain set: Denied.
Only one page has set document.domain: Allowed.

I've updated the patch to match the behavior of Firefox 2 and IE6.  The scenarios where only one page has set document.domain are covered by two new tests:

http/tests/security/cross-frame-access-child-explicit-domain.html
http/tests/security/cross-frame-access-parent-explicit-domain.html

Also, the patch updates the existing document.domain, protocol-mismatch test:

http/tests/security/cross-frame-access-protocol-explicit-domain.html

The port-mismatch case is already covered by a LayoutTest.

I'm not marking the older patch as obsolete because you may decide to
follow IE7s lead and be more secure.

[Sam Weinig]

<rdar://problem/5514516>

[Sam Weinig]

Adam, this patch is looking awesome, thanks for all your help.  The only comment I have left is that since this code is in question, it might be helpful to put a comment in the code (and in the changelog) as to how we differ from other browsers.  Again, great research and great patch!

[Adam Barth]

Created attachment 16481 [details]
Added comments

Thanks.  I added a comment explaining how we differ from other browsers and noted the differences in the ChangeLog.

[Sam Weinig]

Comment on attachment 16481 [details]
Added comments

r+ for the feature branch.

[Eric Seidel (no email)]

Wow.  Let me underscore Wenig's comment:  this is an awesome patch.  Nice comments, clean code.

I tried to land this tonight and when I ran run-webkit-tests it kept hanging on these test cases.  I think something is wrong with the tests (maybe missing a layoutTestController.notifyDone()?)

Anyway, we'll need to fix that before landing.  I assume you ran the tests locally?

[Adam Barth]

> I assume you ran the tests locally?

Unfortunately, I'm unable to run the tests locally because I only have a Linux box.  The tests are very similar to the existing cross-frame-access-* tests.  The issue could be something simple, but I don't have a good way to debug it here.

[Sam Weinig]

Comment on attachment 16481 [details]
Added comments

I believe there is a bug with this patch that is showing up in the layout test.  If you set document.domain it seems you can no longer access the contents of a frame without a domain, such as an iframe without a src.  I will continue to investigate this.

[Adam Barth]

If it would be helpful, I can track down a Windows machine and investigate this, but I don't want to duplicate effort if you're making progress.

[Adam Barth]

Created attachment 17002 [details]
Update for SecurityOrigin class

The new SecurityOrigin::allowAccessFrom function has the same bugs that Window::isSafeScript used to.  Attached is an updated patch.  I haven't investigated whether there is still an issue with empty frames.

Also, the name of the new function is very confusing.  "allowsAccessFrom" sounds like the parameter going to act on the object, but the function is written (and called) in the reverse sense, i.e. "allowAccessTo".  This only really matters for the "file" test (the others are symmetric):

if (m_protocol == "file")
    return true;

Getting this backwards would allow web sites to read your local file system (for example, /etc/passwd).

[Sam Weinig]

This updated patch unfortunately suffers from the same issue as the last one in that it breaks some tests.  I am going to try and fix the issues and get a new patch up for review shortly.  

You are correct that the name of the method allowsAccessFrom() is confusing.  After discussing it with Maciej, I think a better name would be canAccess() so that someOrigin.canAccess(otherOrigin).  Adam, does this seem clearer?

[Adam Barth]

> After discussing it with Maciej, I think a better name would be canAccess() so
> that someOrigin.canAccess(otherOrigin).  Adam, does this seem clearer?

Yeah, that sounds great.

[Sam Weinig]

Landed a change which changed the function name to canAccess in r27433.

[Adam Barth]

Created attachment 17765 [details]
Work in progress patch

I think I understand why the tests were breaking.  In the empty frame case when one frame sets its document.domain, the old code "worked" because it incorrectly allowed access.  In Firefox, the empty frame shares an nsIPrincipal with its parent.  Thus, when one sets its document.domain, the other does as well.  See test case at:

http://crypto.stanford.edu/~abarth/research/webkit/empty-frame/

When a WebKit frame inherits its SecurityOrigin from another frame, it makes a copy, instead of taking a reference.  To match the Firefox behavior, the attached patch changes SecurityOrigin to be RefCounted and shares some SecurityOrigins between the Documents.

Another detail is that Documents can no longer cache their domain property.  Instead, they must obtain them from their SecurityOrigin so that changes made by one Document are reflected in the DOM of the other Documents.

This patch is still a work in progress because I don't understand the role setDomainInternal plays.  It's called exactly once (in FrameLoader::checkCallImplicitClose), but the explanation refers to rdar bugs to which I don't seem to have access.

This patch is really a bit more complex than I'd like.  Is anyone with more experience interested in taking the ball from here?

[Adam Barth]

Turns out the comments refer to KHTML bugs, not rdar bugs:

http://bugs.kde.org/show_bug.cgi?id=22039
http://bugs.kde.org/show_bug.cgi?id=44162

The code appears to just be completely wrong, see test case:

http://crypto.stanford.edu/~abarth/research/webkit/frames/

I'll finish up the patch and hopefully have something for review in a bit.

[Adam Barth]

Created attachment 17824 [details]
Patch for this issue

Here is a patch for this issue.  Hopefully it fixes both the security issue and the broken tests.  There are two caveats:

1) I can't get the LayoutTests working on my Linux machine, so I haven't been able to run the patch through the regression suite.

2) I'm not sure how threading works in storage/Database land.  It's possible the Database object needs to make a copy of the SecurityOrigin instead of just taking a reference.

[Maciej Stachowiak]

Sam says this looks good but he wants to discuss the database aspect with Brady.

[Adam Barth]

Great.

The existing calls to setDomainInternal that the patch removes are actually a pretty serve security vulnerability, sometimes leading to arbitrary script execution in the parent's origin.  (Although the attacker does need to do a bit more work than shown in the demo above.)

I don't know what your patching plans are, but you might consider pushing at least that part out to your customers at some point.

[Sam Weinig]

Comment on attachment 17824 [details]
Patch for this issue

I think this looks great.  I am going to land it ASAP.  Thanks for all the hard work.

[Sam Weinig]

Landed in r28912.  Thanks again.