Bug 16073 - xss possible because of a bug in Document::setDomain
Summary: xss possible because of a bug in Document::setDomain
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.4
: P2 Major
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2007-11-20 10:35 PST by Feng Qian
Modified: 2007-11-26 17:53 PST (History)
2 users (show)

See Also:


Attachments
A patch with a test (4.34 KB, patch)
2007-11-20 16:10 PST, Feng Qian
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Feng Qian 2007-11-20 10:35:03 PST
Document::setDomain updates securityOrigin to new domain even when new domain is not a suffix of the current domain. If frame A and B change their domains to an invalid third party domain, A and B are accessible to each other even when there are from different domain.

A layout test and fix is coming.
Comment 1 Mark Rowe (bdash) 2007-11-20 12:14:17 PST
<rdar://problem/5609222>
Comment 2 Feng Qian 2007-11-20 16:10:21 PST
Created attachment 17421 [details]
A patch with a test

A test case according to the format. Feel free to change.
Comment 3 Sam Weinig 2007-11-20 16:55:02 PST
Comment on attachment 17421 [details]
A patch with a test

Assuming you tested the test case before and after the change, r=me.

It's not that important, but Document::setDomain is also in need of a little love to get rid of those nasty nested ifs.  Early return for the win.
Comment 4 Feng Qian 2007-11-20 16:59:42 PST
(In reply to comment #3)
> (From update of attachment 17421 [details] [edit])
> Assuming you tested the test case before and after the change, r=me.

Yes.

> 
> It's not that important, but Document::setDomain is also in need of a little
> love to get rid of those nasty nested ifs.  Early return for the win.
> 

Can we get rid of Document::m_domain and use m_securityOrigin.host() instead? Looks like it is not much useful.


Comment 5 Feng Qian 2007-11-20 19:04:50 PST
Thanks, are you going to land the patch soon?

(In reply to comment #3)
> (From update of attachment 17421 [details] [edit])
> Assuming you tested the test case before and after the change, r=me.
> 
> It's not that important, but Document::setDomain is also in need of a little
> love to get rid of those nasty nested ifs.  Early return for the win.
> 
Comment 6 Adam Roben (:aroben) 2007-11-20 20:12:59 PST
Comment on attachment 17421 [details]
A patch with a test

Whoever lands this patch should split the ChangeLog entry and put it in WebCore/ChangeLog and LayoutTests/ChangeLog, instead of at the top level.
Comment 7 Feng Qian 2007-11-21 16:03:12 PST
The patch is over-restricted. It should allow set to the same domain. So it needs a fix before getting the length of domain names.

    // Both NS and IE specify that changing the domain is only allowed when
    // the new domain is a suffix of the old domain.

    // FIXME: We should add logging indicating why a domain was not allowed.

    // NOTE: If the new domain is the same as the old domain, still call
    // m_securityOrigin.setDomainForDOM. This will change the
    // security check behavior. For example, if a page with https:// scheme
    // assigns its current domain to document.domain, the page will
    // allow other http:// (and ports) pages in the same domain to
    // access this page. Firefox and Safari behaves like this.
    // Is this a good design?
    if (m_domain == newDomain) {
        m_securityOrigin.setDomainFromDOM(newDomain);
        return;
    }

    int oldLength = m_domain.length();
    int newLength = newDomain.length();

    // e.g. newDomain=kde.org (7) and m_domain=www.kde.org (11)

Layout test, http/tests/security/cross-frame-access-port-explicit-domain.html failed due to this.
I verified that Firefox and Safari both allow change document.document to the current domain, and treat it as set by DOM after that, so cross frame access is possible when protocol and port are different.

Comment 8 Feng Qian 2007-11-21 16:20:20 PST
More test results:

Firefox does not allow http to access https even when both domain are the same and set to themselves. Sounds reasonable.
Comment 9 Sam Weinig 2007-11-26 17:53:10 PST
Fix landed in r28062.