Bug 16073 – xss possible because of a bug in Document::setDomain

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Feng Qian]

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.

[Mark Rowe (bdash)]

<rdar://problem/5609222>

[Feng Qian]

Created attachment 17421 [details]
A patch with a test

A test case according to the format. Feel free to change.

[Sam Weinig]

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.

[Feng Qian]

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

[Feng Qian]

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

[Adam Roben (:aroben)]

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.

[Feng Qian]

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.

[Feng Qian]

More test results:

Firefox does not allow http to access https even when both domain are the same and set to themselves. Sounds reasonable.

[Sam Weinig]

Fix landed in r28062.