Bug 110880 – Cross-origin XSLT requests with CORS should be allowed

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Raphael Kubo da Costa (:rakuco)]

These days, if one attempts to load an XSLT resource from a different origin (as described in <http://www.w3.org/wiki/CORS#xml-stylesheet_processing_instruction_.28XMLSS.29>), the request fails because the origins do not match.

Revisions r34719 and r38065 seem to implement the current behavior, but they reference bugs I do not have permission to view. r34719's commit message said it made WebKit match Firefox, IE and Opera at the time. These days, loading cross-origin XSLT resources works fine on IE and Firefox (and Opera simply ignored the request).

Before spending some time trying to "fix" the issue, I'd like to know if there's an issue to be fixed at all or if we're happy with the current behavior. FWIW, there's a Chromium bug which references this same issue, <https://code.google.com/p/chromium/issues/detail?id=101152>.

[Adam Barth]

I've added you to the bugs in question.  They're old enough that we can probably make them public.  The short answer is that cross-origin XSLT can lead to XSS vulnerabilities.

[Raphael Kubo da Costa (:rakuco)]

Thanks. Only now have I noticed that I didn't explicitly mention I had CORS in mind, sorry about that.

My actual question is what to do when http://bar.baz/something.xml sends a request to http://foo.bar/resource.xsl, which send back a proper Access-Control-Allow-Origin header; in this case, Gecko and IE load it, whereas we don't.

[Adam Barth]

Does the fetch happen with credentials?  Typically we prefer that people use CORS in "anonymous" mode so they don't accidentally share cookie-authenticated data.  For example, that's the default for the crossorigin attribute on <img>.

[Adam Barth]

The short answer is yes, though.  We can use CORS to let web sites open into allowing this behavior.  I'd have to look at the security issue again to remember exactly which resources need to supply CORS headers.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #3)
> Does the fetch happen with credentials?  Typically we prefer that people use CORS in "anonymous" mode so they don't accidentally share cookie-authenticated data.  For example, that's the default for the crossorigin attribute on <img>.

Given that there's no equivalent to the crossorigin attribute for <?xml-stylesheet>, I think both Gecko and IE perform the request without credentials, but I can check that.

[Raphael Kubo da Costa (:rakuco)]

Firefox always seems to send its requests in anonymous mode. IE10, on the other hand, does not really seem to apply CORS' rules in the request: if Protected Mode is disabled (or if only port numbers differ from what I could verify) cross-resource XSLT requests are performed and cookies are sent; the request fails and a warning is written on the console otherwise.

[Raphael Kubo da Costa (:rakuco)]

Actually, testing a bit more, Protected Mode does not alter IE10's behavior: in this case, whether it performs the cross-origin request or not depends on the "Access data sources across domains" setting. It does not take the CORS headers into account in any case, though.

[Adam Barth]

Doing anonymous requests is probably better so that folks can use Access-Control-Allow-Origin: *

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #8)
> Doing anonymous requests is probably better so that folks can use Access-Control-Allow-Origin: *

I'm currently having some trouble with redirects; namely, the ResourceRequest used when requesting a CachedXSLResource is not the one used when the network backend reports a redirect to the loader. This means that even if I, say, write some code in SubresourceLoader::willSendRequest to call updateRequestForAccessControl() the ResourceRequest that I eventually use in ProcessingInstruction::setXSLStyleSheet() has not been changed, and calling something like passesAccessControlCheck() may fail.

Is this really intentional and I'm missing something obvious? Is one really supposed to change both CachedResourceLoader and something like SubresourceLoader to treat both original requests and redirects (I had thought both went through a similar code path)?

[Raphael Kubo da Costa (:rakuco)]

Created attachment 191504 [details]
First attempt

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 191504 [details]
First attempt

There are a few things I don't like in this patch very much: for one, passing a ResourceResponse in setXSLStyleSheet feels weird. Also, performing almost the same origin checks in 3 different places makes me feel I'm doing something wrong.

[WebKit Review Bot]

Comment on attachment 191504 [details]
First attempt

Attachment 191504 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17015011

New failing tests:
http/tests/security/cross-origin-xsl-BLOCKED.html

[Build Bot]

Comment on attachment 191504 [details]
First attempt

Attachment 191504 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17044024

New failing tests:
http/tests/security/cross-origin-xsl-BLOCKED.html

[Raphael Kubo da Costa (:rakuco)]

Created attachment 191768 [details]
First attempt, fix test

[Raphael Kubo da Costa (:rakuco)]

Created attachment 191771 [details]
Remove some leftover

[Alexey Proskuryakov]

Comment on attachment 191771 [details]
Remove some leftover

View in context: https://bugs.webkit.org/attachment.cgi?id=191771&action=review

> Source/WebCore/loader/SubresourceLoader.cpp:147
> +        if (m_resource->type() == CachedResource::XSLStyleSheet && !m_documentLoader->document()->securityOrigin()->canRequest(newRequest.url()))
> +            updateRequestForAccessControl(newRequest, m_documentLoader->document()->securityOrigin(), DoNotAllowStoredCredentials);
>          m_resource->willSendRequest(newRequest, redirectResponse);

Is this the right place to make this check? It seems like the line below should get us inside CachedXSLStyleSheet, which would be better for XSLStyleSheet specific code than SubresourceLoader.

Additionally, this handling of redirects seems unique - I don't see any similar updateRequestForAccessControl calls elsewhere.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 191771 [details]
Remove some leftover

View in context: https://bugs.webkit.org/attachment.cgi?id=191771&action=review

>> Source/WebCore/loader/SubresourceLoader.cpp:147
>>          m_resource->willSendRequest(newRequest, redirectResponse);
> 
> Is this the right place to make this check? It seems like the line below should get us inside CachedXSLStyleSheet, which would be better for XSLStyleSheet specific code than SubresourceLoader.
> 
> Additionally, this handling of redirects seems unique - I don't see any similar updateRequestForAccessControl calls elsewhere.

I thought about putting it in CachedXSLStyleSheet too. This would probably mean adding some empty method to CachedResource that's only implemented by CachedXSLStyleSheet. Is that OK?

WRT this looking unique: other places, such as <img> + <canvas>, or XHR, provide means for users to specify the behavior their want. XSL was a special case before by simply discarding cross-origin requests. Were you thinking of something in specific?

[Alexey Proskuryakov]

> This would probably mean adding some empty method to CachedResource that's only implemented by CachedXSLStyleSheet.

Wouldn't CachedXSLStyleSheet::willSendRequest() work?

> other places, such as <img> + <canvas>, or XHR, provide means for users to specify the behavior their want

What is the place where other loaders do the equivalent of updateRequestForAccessControl() on redirect? Or do they not need to?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #18)
> > This would probably mean adding some empty method to CachedResource that's only implemented by CachedXSLStyleSheet.
> 
> Wouldn't CachedXSLStyleSheet::willSendRequest() work?

This would mean either passing m_documentLoader->securityOrigin() to willSendRequest() or calling loader()->documentLoader()->securityOrigin() there. I was a bit afraid of this chain not always being non-NULL, but can give it a try.

> > other places, such as <img> + <canvas>, or XHR, provide means for users to specify the behavior their want
> 
> What is the place where other loaders do the equivalent of updateRequestForAccessControl() on redirect? Or do they not need to?

From what I understood from the behavior of <img>+<canvas> and XHR, at creation time (respectively, by setting the crossorigin attribute in <img> and using XHR.withCredentials) one defines whether all requests should be made without any credentials (which means the original request and any occasional ones created by redirects) are sent without credentials or all of them are sent with credentials.

[Adam Barth]

Comment on attachment 191771 [details]
Remove some leftover

View in context: https://bugs.webkit.org/attachment.cgi?id=191771&action=review

This looks like a good start.  My main question is why XSLT needs special handling in subresource loader.

>>> Source/WebCore/loader/SubresourceLoader.cpp:147
>>> +        if (m_resource->type() == CachedResource::XSLStyleSheet && !m_documentLoader->document()->securityOrigin()->canRequest(newRequest.url()))
>>> +            updateRequestForAccessControl(newRequest, m_documentLoader->document()->securityOrigin(), DoNotAllowStoredCredentials);
>>>          m_resource->willSendRequest(newRequest, redirectResponse);
>> 
>> Is this the right place to make this check? It seems like the line below should get us inside CachedXSLStyleSheet, which would be better for XSLStyleSheet specific code than SubresourceLoader.
>> 
>> Additionally, this handling of redirects seems unique - I don't see any similar updateRequestForAccessControl calls elsewhere.
> 
> I thought about putting it in CachedXSLStyleSheet too. This would probably mean adding some empty method to CachedResource that's only implemented by CachedXSLStyleSheet. Is that OK?
> 
> WRT this looking unique: other places, such as <img> + <canvas>, or XHR, provide means for users to specify the behavior their want. XSL was a special case before by simply discarding cross-origin requests. Were you thinking of something in specific?

Why is this check needed for XSLStyleSheet but not for CORS-enabled image fetches?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:238
> +    KURL requestURL = request.resourceRequest().url();
> +    bool sameOriginRequest = m_document->securityOrigin()->canRequest(requestURL);
> +
> +    if (!sameOriginRequest)
> +        updateRequestForAccessControl(request.mutableResourceRequest(), m_document->securityOrigin(), DoNotAllowStoredCredentials);

So, we're only doing an anonymous fetch if the initial URL is cross-origin.  We need to be sure to test the case where a same-origin request gets redirected to a cross-origin URL

> LayoutTests/http/tests/security/cross-origin-xsl-BLOCKED-expected.txt:1
> -CONSOLE MESSAGE: line 2: Unsafe attempt to load URL http://localhost:8000/security/resources/forbidden-stylesheet.xsl from frame with URL http://127.0.0.1:8000/security/resources/cross-origin-xsl.xml. Domains, protocols and ports must match.
> +CONSOLE MESSAGE: Unsafe attempt to load URL http://localhost:8000/security/resources/forbidden-stylesheet.xsl from frame with URL http://127.0.0.1:8000/security/resources/cross-origin-xsl-BLOCKED.xml. Domains, protocols and ports must match.

Why are we losing the line number?

[Raphael Kubo da Costa (:rakuco)]

Sorry for the delay, I was flying across continents.

(In reply to comment #20)
> This looks like a good start.  My main question is why XSLT needs special handling in subresource loader.

As I see it XSL fetches behave differently than the rest because we're sort of adapting the behavior of something that does not really know about CORS, that is, in this case there's no "crossorigin" tag as there is for the <img> tag.

> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:238
> > +    KURL requestURL = request.resourceRequest().url();
> > +    bool sameOriginRequest = m_document->securityOrigin()->canRequest(requestURL);
> > +
> > +    if (!sameOriginRequest)
> > +        updateRequestForAccessControl(request.mutableResourceRequest(), m_document->securityOrigin(), DoNotAllowStoredCredentials);
>
> So, we're only doing an anonymous fetch if the initial URL is cross-origin.  We need to be sure to test the case where a same-origin request gets redirected to a cross-origin URL

This is what http/tests/security/cross-origin-xsl-redirect.html is testing, and why I had to add code to SubresourceLoader: when the original request is a same-origin one, updateRequestForAccessControl() is not called. The platform-specific networking code then processes the redirect, creates a ResourceRequest based on the existing one and calls SubresourceLoader.

The test I added there checks if this is happening and strips the request accordingly.

> >>> Source/WebCore/loader/SubresourceLoader.cpp:147
> >>> +        if (m_resource->type() == CachedResource::XSLStyleSheet && !m_documentLoader->document()->securityOrigin()->canRequest(newRequest.url()))
> >>> +            updateRequestForAccessControl(newRequest, m_documentLoader->document()->securityOrigin(), DoNotAllowStoredCredentials);
> >>>          m_resource->willSendRequest(newRequest, redirectResponse);
>
> [...]
>
> Why is this check needed for XSLStyleSheet but not for CORS-enabled image fetches?

The CORS policy for image fetches is defined by the "crossorigin" attribute in <img>. Specifically, if crossorigin is "anonymous" the original request is already stripped so the RequestResource created after the redirect does not allow cookies either. Analogously, if crossorigin is set to "use-credentials" none of the requests have the omit credentials flag set.

> > LayoutTests/http/tests/security/cross-origin-xsl-BLOCKED-expected.txt:1
> > -CONSOLE MESSAGE: line 2: Unsafe attempt to load URL http://localhost:8000/security/resources/forbidden-stylesheet.xsl from frame with URL http://127.0.0.1:8000/security/resources/cross-origin-xsl.xml. Domains, protocols and ports must match.
> > +CONSOLE MESSAGE: Unsafe attempt to load URL http://localhost:8000/security/resources/forbidden-stylesheet.xsl from frame with URL http://127.0.0.1:8000/security/resources/cross-origin-xsl-BLOCKED.xml. Domains, protocols and ports must match.
>
> Why are we losing the line number?

The line was shown because the document was still being parsed when the loading stopped and the message was logged. As the code in CachedResourceLoader::canRequest() has been removed, the message is added after parsing finishes.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 193186 [details]
Do not touch SubresourceLoader

[Raphael Kubo da Costa (:rakuco)]

Ping?

[Alexey Proskuryakov]

Comment on attachment 193186 [details]
Do not touch SubresourceLoader

View in context: https://bugs.webkit.org/attachment.cgi?id=193186&action=review

> Source/WebCore/dom/ProcessingInstruction.cpp:244
> +    if (!document()->securityOrigin()->canRequest(response.url())
> +        && !passesAccessControlCheck(response, DoNotAllowStoredCredentials, document()->securityOrigin(), errorDescription)) {

It feels strange to see an access check here, this is not loading code. I think that this may be too late.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:238
> +    if (!sameOriginRequest)
> +        updateRequestForAccessControl(request.mutableResourceRequest(), m_document->securityOrigin(), DoNotAllowStoredCredentials);

I think that this is insufficient to make the request anonymous. ResourceHandleClient should return false from shouldUseCredentialStorage() to prevent ResourceHandle from using credentials stored in Keychain.

[Andreas Kling]

Comment on attachment 193186 [details]
Do not touch SubresourceLoader

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

This bug still appears to be unfixed, at least in current iOS WebKit.

[Anne van Kesteren]

I don't think we should add support for this unless it's properly standardized. Only Gecko supports this at the moment after all.