Bug 181460

<rdar://problem/36379776>

Created attachment 330885 [details] patch

Comment on attachment 330885 [details] patch Attachment 330885 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6018037 New failing tests: webgl/1.0.2/conformance/uniforms/uniform-default-values.html

Created attachment 330890 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Comment on attachment 330885 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=330885&action=review > Source/WebCore/css/parser/CSSParser.cpp:81 > + , hasDocumentSecurityOrigin(document.baseURL() == baseURL ? true : document.securityOrigin().canRequest(baseURL)) I would use || here instead of ? :

Created attachment 330906 [details] patch

Comment on attachment 330906 [details] patch Clearing flags on attachment: 330906 Committed r226721: <https://trac.webkit.org/changeset/226721>

All reviewed patches have been landed. Closing bug.

Comment on attachment 330906 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=330906&action=review > Source/WebCore/css/parser/CSSParser.cpp:81 > + , hasDocumentSecurityOrigin(document.baseURL() == baseURL || document.securityOrigin().canRequest(baseURL)) Can this tweak be made in canRequest to make it fast instead? But also, I'm actually not sure if identical base URL is good enough for not having a security boundary, between origins in this case.

Comment on attachment 330906 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=330906&action=review >> Source/WebCore/css/parser/CSSParser.cpp:81 >> + , hasDocumentSecurityOrigin(document.baseURL() == baseURL || document.securityOrigin().canRequest(baseURL)) > > Can this tweak be made in canRequest to make it fast instead? > > But also, I'm actually not sure if identical base URL is good enough for not having a security boundary, between origins in this case. As for the first question, maybe there is some other way to make a fast path, but we can’t make this exact optimization because canRequest does not have access to the document’s baseURL. For the second question, I suggest we work together to turn your uncertainty into a test case.

Yeah, SecurityOrigin::canRequest is inefficient (heap allocating a temporary SecurityOrigin for example) and could be optimized. However that will require bit more refactoring than should be done in this patch. The logic here is about figuring out if the stylesheet content is readable via getMatchedCSSRules. The rules from the document itself are supposed to be accessible.

(In reply to Antti Koivisto from comment #11) > Yeah, SecurityOrigin::canRequest is inefficient (heap allocating a temporary > SecurityOrigin for example) and could be optimized. However that will > require bit more refactoring than should be done in this patch. > > The logic here is about figuring out if the stylesheet content is readable > via getMatchedCSSRules. The rules from the document itself are supposed to > be accessible. Are you sure this won't allow two null-origin documents to be treated as if sharing the same origin?

> Are you sure this won't allow two null-origin documents to be treated as if > sharing the same origin? CSSParser parses rules either from the document or from stylesheets loaded by the document. I don't know how that would happen.

(In reply to Antti Koivisto from comment #13) > > Are you sure this won't allow two null-origin documents to be treated as if > > sharing the same origin? > > CSSParser parses rules either from the document or from stylesheets loaded > by the document. I don't know how that would happen. How about data URLs?

> How about data URLs? Can you be more specific about the scenario?

The new security check is different form the old one. Base URL can be anything, as the page author can specify any base with the <base> element. It's not the same URL that the security context is created from. The sheetBaseURL argument seems like a misnomer. Chris and I looked at one caller, and it was actually passing final stylesheet URL there (the one after redirects). So that's controllable by the attacker, who can start with their own server, and redirect to the victim. It does seem likely that documents with unique origins also get extra powers with this change. But I can't follow CSS code enough to tell what the observable effect is. Given that we started with a security check, it probably is a security regression of some sort.

Comment on attachment 330906 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=330906&action=review >>> Source/WebCore/css/parser/CSSParser.cpp:81 >>> + , hasDocumentSecurityOrigin(document.baseURL() == baseURL || document.securityOrigin().canRequest(baseURL)) >> >> Can this tweak be made in canRequest to make it fast instead? >> >> But also, I'm actually not sure if identical base URL is good enough for not having a security boundary, between origins in this case. > > As for the first question, maybe there is some other way to make a fast path, but we can’t make this exact optimization because canRequest does not have access to the document’s baseURL. > > For the second question, I suggest we work together to turn your uncertainty into a test case. I would worry a lot less if this would be using document.url() rather than document.baseURL(). AFAIK, the document baseURL can be overridden by a <base> element and the document's securityOrigin comes from its URL, not its baseURL.

I think that document.url() would make the problem less obvious, but it needs to be explicitly demonstrated that unique origins are not a problem here. Even so, spreading slightly alternative security checks across the code base is a long term liability.

Re-opened since this is blocked by bug 181583

Created attachment 331184 [details] patch

Comment on attachment 331184 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=331184&action=review > Source/WebCore/ChangeLog:13 > + Don't do the expensive security origin test if the supplied sheet base URL is null. This > + is true for rules coming from the same document. I think that this is still changing the behavior when the document can't make requests to its own base URL (which is the case at least whenever there is a <base> element with a different origin URL, or for unique origins).

> I think that this is still changing the behavior when the document can't > make requests to its own base URL (which is the case at least whenever there > is a <base> element with a different origin URL, or for unique origins). The purpose of this bit is to block access to stylesheet via getMatchedCSSRules legacy API in cases where it could not be accessed otherwise via DOM. As you can see from CSSStyleSheet::canAccessRules normal access is granted when the sheet base URL is empty. (There is also a cors related test that is missing here, not sure if it is relevant).

> The purpose of this bit is to block access to stylesheet via getMatchedCSSRules legacy API in cases where it could not be accessed otherwise via DOM. As you can see from CSSStyleSheet::canAccessRules normal access is granted when the sheet base URL is empty. This may mean that the patch is OK, I didn't analyze CSS code deep enough to agree or disagree. But I think that it's missing a couple tests. We have three different behaviors here: 1. Original behavior before r226721. 2. Behavior with r226721. 3. Behavior with this patch. It seems wrong to not have any tests differentiating between these.

Improving test coverage would be nice but bit outside the scope of this bug.

I agree canRequest should be optimized. We should not allocate a SecurityOrigin object, at least in the usual cases of http/data URLs. If we were doing so, the proposed additional check might no longer be needed. If we land that patch, can we either add a FIXME in CSSParserContext::CSSParserContext or even better quickly come with an optimization of canRequest and revert this change at that time?

Comment on attachment 331184 [details] patch Can we write a test with a data frame containing inline CSS? That would also allow us to check what other browsers are doing there.

Gecko never implemented getMatchedCSSRules in the first place and Chrome is trying to remove it (or already removed?).

Comment on attachment 331184 [details] patch Clearing flags on attachment: 331184 Committed r227147: <https://trac.webkit.org/changeset/227147>

All reviewed patches have been landed. Closing bug.

Comment on attachment 331184 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=331184&action=review > Source/WebCore/css/parser/CSSParser.cpp:83 > + Extra blank line here.