User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release) Build ID: 20140728123914 Steps to reproduce: 1. Set style-src to 'self' using content security policy headers 2. Try to load a stylesheet from some other source using @import I just submitted a test case to web-platform-tests for this (fails in Firefox, passes in Chrome) https://github.com/w3c/web-platform-tests/pull/1146 Actual results: The stylesheet fails to load, but no CSP violation report is sent Expected results: A report noting the style-src violation should be sent

The root cause for not sending reports when using @import is that we do not send reports for preloads [1] and that we incorrectly classify the loading of the imported CSS as a preload. In case of a CSP violation for a preload we do block the load but do not send a report so that we do not send two reports for the same violation. I provided a stacktrace underneath, particularly relevant is [2]. In the testcase we do use 'mDocument' as the requestingContext for CheckLoadAllowed() and later for ShouldLoad)_. Later in [1], we query: > nsCOMPtr<nsIDOMHTMLDocument> doc = do_QueryInterface(aRequestContext); which succeeds and hence we incorrectly classify the load as a preload. I don't think there is an easy fix to that because there is no other context available, but maybe David has an idea how we could resolve that problem! [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsCSPContext.cpp#138 [2] http://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#2122 #01: nsCSPContext::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, short*) (nsCSPContext.cpp:188, in XUL) #02: CSPService::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) (nsCSPService.cpp:194, in XUL) #03: nsContentPolicy::CheckPolicy(tag_nsresult (nsIContentPolicy::*)(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*), unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) (nsContentPolicy.cpp:125, in XUL) #04: nsContentPolicy::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) (nsContentPolicy.cpp:188, in XUL) #05: NS_CheckContentLoadPolicy(unsigned int, nsIURI*, nsIPrincipal*, nsISupports*, nsACString_internal const&, nsISupports*, short*, nsIContentPolicy*, nsIScriptSecurityManager*) (nsContentPolicyUtils.h:217, in XUL) #06: mozilla::css::Loader::CheckLoadAllowed(nsIPrincipal*, nsIURI*, nsISupports*) (Loader.cpp:1059, in XUL) #07: mozilla::css::Loader::LoadChildSheet(mozilla::CSSStyleSheet*, nsIURI*, nsMediaList*, mozilla::css::ImportRule*) (Loader.cpp:2126, in XUL) #08: (anonymous namespace)::CSSParserImpl::ProcessImport(nsString const&, nsMediaList*, void (*)(mozilla::css::Rule*, void*), void*, unsigned int, unsigned int) (nsCSSParser.cpp:3272, in XUL)

bz knows the CSS loader better than I do, though I could have a look if he's busy.

Providing a half-baked testcase. I copied test_csp_report.html and adapted some parts so that it loads a CSS which uses @import for loading another CSS. Probably good enough for a first debug session.

Loader::LoadChildSheet walks up the parent sheet chain looking for the linking element. Why doesn't it find it? Is aParentSheet->GetOwningDocument() returning false because aOwningSheet was preloaded, perhaps (and a clone is what has the owning node)? But a bigger question here is why there's this weird assumption about "context is document means preload"? For CSS stuff there is no sane context because of the coalescing we do, so document really makes the most sense in some ways anyway.

(In reply to Boris Zbarsky [:bz] from comment #4) > Loader::LoadChildSheet walks up the parent sheet chain looking for the > linking element. Why doesn't it find it? Is > aParentSheet->GetOwningDocument() returning false because aOwningSheet was > preloaded, perhaps (and a clone is what has the owning node)? Not sure about the reason, but aParentSheet->GetOwningDocument() returns false, probably for the reason you mentioned and hence we use mDocument as the context. > But a bigger question here is why there's this weird assumption about > "context is document means preload"? I don't like that part either, but I think at the moment this is the best we can come up with. If there is a better way to identify whether we are calling shouldLoad() for a preload or not, please let us know and I am more than happy to fix this immediately. > For CSS stuff there is no sane context > because of the coalescing we do, so document really makes the most sense in > some ways anyway. That makes sense, maybe we can find a better solution to identify preloads in ShouldLoad().

> If there is a better way to identify whether we are calling shouldLoad() for a preload or > not Most simply, we could add new content policy types, right?

Since we are about to add some new internal content policy types, I am marking the work from ehsan [Bug 1174307] blocking this bug.

Hey Ehsan, as discussed yesterday, I added new internal content policy types for * image preloads * script preloads * style preloads so that CSP is able to distinguish between actual loads and preloads. Ulitmately that separation will be used for speculative loads when implementing Meta CSP [Bug 663570], but it makes sense to do that within this patch to also clean up the way we currently identify preloads within CSP. We have to propagate those flags for (a) content policy checks as well as (b) for creation of new channels since we need to store the contentPolicyType within the loadInfo. Further, we want to send the external content policyType for all contentPolicy checks but use the internal one for CSP checks. Are you fine with that approach or am I missing something? If you are ok with that approach, I would split the patches up and ask individual peers for the preload scenarios. William provided the following list for preloads, I suppose we only need the first three, but I am not 100% sure - here is the list. [yes] Script: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1603 [yes] Style: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#2024 [yes] Image: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#2009 [???] Picture: source: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#1986 [???] Manifest: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentSink.cpp#1036 [???] Preconnect: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#2051

Hi Christoph, Separate internal content policy types for preloads sounds good! How are you going to expose them only to CSP? Since CSP is called from NS_CheckContentLoadPolicy and that API takes the external content policy type, do you plan to separate CSP out of NS_CheckContentLoadPolicy and call it individually? Thanks!

(In reply to Tanvi Vyas [:tanvi] from comment #9) > Hi Christoph, > > Separate internal content policy types for preloads sounds good! How are > you going to expose them only to CSP? Since CSP is called from > NS_CheckContentLoadPolicy and that API takes the external content policy > type, do you plan to separate CSP out of NS_CheckContentLoadPolicy and call > it individually? Nope, CSP remains an nsIContentPolicy. Just passing the internalPolicyType to CSP instead of the external one and CSP does it's own translation - see changes within nsContentPolicy::CheckPolicy in the attached patch.

So, here is more robust test that also works in e10s.

Comment on attachment 8641337 [details] [diff] [review] bug_1048048_preload_types.patch Review of attachment 8641337 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsIContentPolicyBase.idl @@ +270,5 @@ > + /** > + * Indicates a stylesheet preload (e.g., STYLE elements). > + */ > + const nsContentPolicyType TYPE_INTERNAL_STYLESHEET_PRELOAD = 35; > + I just realized that I forgot to update the DBSchema: http://hg.mozilla.org/mozilla-central/rev/593ac5424b6e We have to make sure that happens here as well.

Comment on attachment 8641337 [details] [diff] [review] bug_1048048_preload_types.patch Review of attachment 8641337 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsContentPolicy.cpp @@ +136,5 @@ > + // * TYPE_INTERNAL_SCRIPT_PRELOAD > + // * TYPE_INTERNAL_IMAGE_PRELOAD > + // * TYPE_INTERNAL_STYLESHEET_PRELOAD > + bool isCSP = cspService == entries[i]; > + rv = (entries[i]->*policyMethod)(isCSP ? contentType : externalType, We should not pass the external type to the CSP policy! I think what we want to do is to add another helper such as nsContentUtils::InternalContentPolicyTypeToExternalOrPreload() or some such (that name is awful!) that woudl handle the above _PRELOAD values by passing them through and calling into InternalContentPolicyTypeToExternal for other cases. ::: dom/base/nsIContentPolicyBase.idl @@ +264,5 @@ > + > + /** > + * Indicates an image preload (e.g., IMG elements). > + */ > + const nsContentPolicyType TYPE_INTERNAL_IMAGE_PRELOAD = 34; We need to be able to differentiate between TYPE_IMAGE as opposed to this type, and TYPE_IMAGE as the mapped external type, so you need to add a TYPE_INTERNAL_IMAGE too. TYPE_INTERNAL_SCRIPT also exists for this very reason. @@ +269,5 @@ > + > + /** > + * Indicates a stylesheet preload (e.g., STYLE elements). > + */ > + const nsContentPolicyType TYPE_INTERNAL_STYLESHEET_PRELOAD = 35; Ditto. ::: dom/fetch/InternalRequest.cpp @@ +122,5 @@ > break; > case nsIContentPolicy::TYPE_INTERNAL_SHARED_WORKER: > context = RequestContext::Sharedworker; > break; > case nsIContentPolicy::TYPE_IMAGE: This would be TYPE_INTERNAL_IMAGE. @@ +128,3 @@ > context = RequestContext::Image; > break; > case nsIContentPolicy::TYPE_STYLESHEET: Ditto. ::: dom/fetch/InternalRequest.h @@ +42,5 @@ > * form | > * frame | TYPE_INTERNAL_FRAME > * hyperlink | > * iframe | TYPE_INTERNAL_IFRAME > + * image | TYPE_IMAGE, TYPE_INTERNAL_IMAGE_PRELOAD This would be TYPE_INTERNAL_IMAGE. @@ +57,3 @@ > * sharedworker | TYPE_INTERNAL_SHARED_WORKER > * subresource | Not supported by Gecko > + * style | TYPE_STYLESHEET, TYPE_INTERNAL_STYLESHEET_PRELOAD Ditto. ::: dom/security/nsCSPContext.cpp @@ +118,5 @@ > + aContentType == nsIContentPolicy::TYPE_INTERNAL_STYLESHEET_PRELOAD); > + > + // Since we know whether we are dealing with a preload, we have to convert > + // the internal policytype ot the external policy type before moving on. > + aContentType = nsContentUtils::InternalContentPolicyTypeToExternal(aContentType); This should become unnecessary. ::: dom/security/nsCSPService.cpp @@ -105,5 @@ > nsIPrincipal *aRequestPrincipal, > int16_t *aDecision) > { > - MOZ_ASSERT(aContentType == nsContentUtils::InternalContentPolicyTypeToExternal(aContentType), > - "We should only see external content policy types here."); You would need to change this assertion according to my comment on nsContentPolicy.cpp. @@ +249,5 @@ > int16_t *aDecision) > { > + // If this body is ever going to return something else than ACCEPT, > + // then we whave to convert aContentType to user the external rather > + // then the internal policy type!!!! Same here.

Thanks for the feedback Ehsan! (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #13) > ::: dom/base/nsContentPolicy.cpp > @@ +136,5 @@ > > + // * TYPE_INTERNAL_SCRIPT_PRELOAD > > + // * TYPE_INTERNAL_IMAGE_PRELOAD > > + // * TYPE_INTERNAL_STYLESHEET_PRELOAD > > + bool isCSP = cspService == entries[i]; > > + rv = (entries[i]->*policyMethod)(isCSP ? contentType : externalType, > > We should not pass the external type to the CSP policy! I suppose you mean the internal one, right? We *do* want to pass the external one - the same way we pass the external on to all other content polcies. > I think what we > want to do is to add another helper such as > nsContentUtils::InternalContentPolicyTypeToExternalOrPreload() or some such > (that name is awful!) that woudl handle the above _PRELOAD values by passing > them through and calling into InternalContentPolicyTypeToExternal for other > cases. I think that is not possible because if we pass the external contentPolicyType to CSP then there is a one to many relation that we can not resolve. Let's assume we pass TYPE_IMAGE to CSP then we don't know whether we are dealing with TYPE_INTERNAL_IMAGE or TYPE_INTERNAL_IMAGE_PRELOAD, or am I missing something?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14) > Thanks for the feedback Ehsan! > > (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, > needinfo? me!) from comment #13) > > ::: dom/base/nsContentPolicy.cpp > > @@ +136,5 @@ > > > + // * TYPE_INTERNAL_SCRIPT_PRELOAD > > > + // * TYPE_INTERNAL_IMAGE_PRELOAD > > > + // * TYPE_INTERNAL_STYLESHEET_PRELOAD > > > + bool isCSP = cspService == entries[i]; > > > + rv = (entries[i]->*policyMethod)(isCSP ? contentType : externalType, > > > > We should not pass the external type to the CSP policy! > > I suppose you mean the internal one, right? We *do* want to pass the > external one - the same way we pass the external on to all other content > polcies. Oops, yeah I mean the internal one. My bad! > > I think what we > > want to do is to add another helper such as > > nsContentUtils::InternalContentPolicyTypeToExternalOrPreload() or some such > > (that name is awful!) that woudl handle the above _PRELOAD values by passing > > them through and calling into InternalContentPolicyTypeToExternal for other > > cases. > > I think that is not possible because if we pass the external > contentPolicyType to CSP then there is a one to many relation that we can > not resolve. Let's assume we pass TYPE_IMAGE to CSP then we don't know > whether we are dealing with TYPE_INTERNAL_IMAGE or > TYPE_INTERNAL_IMAGE_PRELOAD, or am I missing something? The point is that for the CSP service, we should never pass TYPE_IMAGE. We should either pass TYPE_INTERNAL_IMAGE, or TYPE_INTERNAL_IMAGE_PRELOAD. The assertions I suggested you should add back will ensure that. (Similarly for stylesheets.) For all other content policy implementations, we should never pass TYPE_INTERNAL_IMAGE* and only pass TYPE_IMAGE to them. (Again, similarly for stylesheets.) In other words, the CSP service will get to see some of the internal types that we hide from other content policy implementations. Does that make sense?

Hi Ehsan, I split the patches up to allow easier reviewing for images, stylesheets and scripts. I am only going to flag you for this first one, but in fact it would be great if you could review the following four patches within this bug: bug_1048048_preload_types bug_1048048_preload_types_images bug_1048048_preload_types_stylesheets bug_1048048_preload_types_scripts

Comment on attachment 8644645 [details] [diff] [review] bug_1048048_preload_types.patch Review of attachment 8644645 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! ::: dom/base/nsContentUtils.h @@ +939,5 @@ > /** > + * Map internal content policy types to external ones or preload types: > + * * TYPE_INTERNAL_SCRIPT_PRELOAD > + * * TYPE_INTERNAL_IMAGE_PRELOAD > + * * TYPE_INTERNAL_STYLESHEET_PRELOAD Can you please add something along the lines of "this is probably not what you want to use, use InternalContentPolicyTypeToExternal instead"? ::: dom/base/nsIContentPolicyBase.idl @@ +284,5 @@ > + * Indicates an image (e.g., IMG elements). > + */ > + > + /** > + * Indicates an internal constant for images. Nit: normal images. @@ +300,5 @@ > + */ > + const nsContentPolicyType TYPE_INTERNAL_IMAGE_PRELOAD = 37; > + > + /** > + * Indicates an internal constant for stylesheets. Nit: normal stylesheets.

Could you get someone else to review the patches assigned to me, unless there's a specific reason you want my review?

Comment on attachment 8644647 [details] [diff] [review] bug_1048048_preload_types_images.patch Seth, any chance you can review those bits? The idea is to split the contentPolicyType for preloads and actual loads so that the LoadInfo on the channel can tell us whether we are dealing with a speculative load or an actual load. So IMO the most important thing you would have to make sure is that nsDocument::MaybePreLoadImage passes the right argument. All the other arguments are merely just updates from TYPE_IMAGE to TYPE_INTERNAL_IMAGE to be consistent throughout the codebase.

Comment on attachment 8644648 [details] [diff] [review] bug_1048048_preload_types_stylesheets.patch Cameron, a bit of background: We are going to use a different content policy type for preloads and actual loads so that CSP can distinguish between preloads and actual loads. We need to pass that information to content policies, but also store the new content policy type within the loadInfo of the channel, because within Bug 1182535 we are going to remove all security checks from the callsite and move them into asyncOpen(2). Hence we need to know about the type in the loadInfo as well.

Comment on attachment 8644649 [details] [diff] [review] bug_1048048_preload_types_scripts.patch Baku, any chance you can review this? Already chattet with Ehsan, he also suggested you as a reviewer. Please see Comment 23 and Comment 24 for details. Thanks!

Comment on attachment 8644647 [details] [diff] [review] bug_1048048_preload_types_images.patch Review of attachment 8644647 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Probably should update the commit message to say "r=seth" (or "r=ehsan,seth" if Ehsan did review this patch; I may have missed it).

Comment on attachment 8644649 [details] [diff] [review] bug_1048048_preload_types_scripts.patch Please let me know if you also want me to review this.

Comment on attachment 8644648 [details] [diff] [review] bug_1048048_preload_types_stylesheets.patch Review of attachment 8644648 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure what bits of this ehsan has already reviewed. The s/TYPE_STYLESHEET/TYPE_INTERNAL_STYLESHEET/ bits seem to accord with previous discussion on the bug, so rs=me on those bits and r=me on the rest. ::: layout/style/Loader.cpp @@ +1002,5 @@ > nsresult > Loader::CheckLoadAllowed(nsIPrincipal* aSourcePrincipal, > nsIURI* aTargetURI, > + nsISupports* aContext, > + bool aIsPreload) Please add a doc comment for the new arg. @@ +1023,5 @@ > > // Check with content policy > + nsContentPolicyType contentPolicyType = aIsPreload > + ? nsIContentPolicy::TYPE_INTERNAL_STYLESHEET_PRELOAD > + : nsIContentPolicy::TYPE_INTERNAL_STYLESHEET; Please rewrap to fit 80 columns, maybe like: nsContentPolicyType contentPolicyType = aIsPreload ? nsIContentPolicy::TYPE_INTERNAL_STYLESHEET_PRELOAD : nsIContentPolicy::TYPE_INTERNAL_STYLESHEET; @@ +1554,5 @@ > } > > + nsContentPolicyType contentPolicyType = aIsPreload > + ? nsIContentPolicy::TYPE_INTERNAL_STYLESHEET_PRELOAD > + : nsIContentPolicy::TYPE_INTERNAL_STYLESHEET; As above. ::: layout/style/Loader.h @@ +444,5 @@ > CSSStyleSheet* aParentSheet, > ImportRule* aParentRule); > > nsresult InternalLoadNonDocumentSheet(nsIURI* aURL, > + bool aIsPreload, The number of boolean arguments is getting a bit out of hand here. :( But Loader's API needs cleanup in general...

Watch out for conflicts with bug 1035091 (which does convert one of those boolean args into an enum) when rebasing.

Comment on attachment 8644651 [details] [diff] [review] bug_1048048_preload_types_csp.patch Review of attachment 8644651 [details] [diff] [review]: ----------------------------------------------------------------- r=dveditz

Just rebased all the patches, carrying over r+ for all of them.

Hey Dan, I think it's a good sign that the web-platform test for this issue is not failing anymore. What do you think? :-)

Comment on attachment 8663467 [details] [diff] [review] bug_1048048_preload_types_web_platform_tests.patch r=dveditz

https://hg.mozilla.org/integration/mozilla-inbound/rev/02c6d6aef163 https://hg.mozilla.org/integration/mozilla-inbound/rev/740ab1ecd079 https://hg.mozilla.org/integration/mozilla-inbound/rev/88c2333ff745 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a727c40eb68 https://hg.mozilla.org/integration/mozilla-inbound/rev/450d4a13c90e https://hg.mozilla.org/integration/mozilla-inbound/rev/4f91b10e8be0 https://hg.mozilla.org/integration/mozilla-inbound/rev/b5abe23a4ea5

I backed this out for Android bustage, but I believe it was some other patches that caused that bustage. I'll reland before I reopen, assuming the bustage is fixed by my other backouts. Sorry for the churn here. https://hg.mozilla.org/integration/mozilla-inbound/rev/2f837630f556

https://hg.mozilla.org/integration/mozilla-inbound/rev/7398104fda0b https://hg.mozilla.org/integration/mozilla-inbound/rev/0e81b19b0f22 https://hg.mozilla.org/integration/mozilla-inbound/rev/dadd2ff2f761 https://hg.mozilla.org/integration/mozilla-inbound/rev/7d75db26d40d https://hg.mozilla.org/integration/mozilla-inbound/rev/56058bf8ec8c https://hg.mozilla.org/integration/mozilla-inbound/rev/94c62c16128d https://hg.mozilla.org/integration/mozilla-inbound/rev/54777c071b5d

https://hg.mozilla.org/mozilla-central/rev/7398104fda0b https://hg.mozilla.org/mozilla-central/rev/0e81b19b0f22 https://hg.mozilla.org/mozilla-central/rev/dadd2ff2f761 https://hg.mozilla.org/mozilla-central/rev/7d75db26d40d https://hg.mozilla.org/mozilla-central/rev/56058bf8ec8c https://hg.mozilla.org/mozilla-central/rev/94c62c16128d https://hg.mozilla.org/mozilla-central/rev/54777c071b5d

Andrea, Boris, when writing those patches I was relying on the fact that IsPreload() [1] returns whether it's a script preload or not - apparently that's not always reliable, because PreloadURI() [2] sets the mElement to a nullptr. Unfortunately the patch only added a testcase for stylesheets so that problem remained undiscovered until now when I accidentally realized the problem. Any ideas how we can fix that? [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.h#93 [2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1683

> apparently that's not always reliable, because PreloadURI() [2] sets the mElement to a > nullptr. I'm not sure I follow. PreloadURI should be the only thing that sets mElement to nullptr, so IsPreload() is true if and only if the load came through PreloadURI, right? Which part of that is causing problems?

(In reply to Boris Zbarsky [:bz] from comment #44) > > apparently that's not always reliable, because PreloadURI() [2] sets the mElement to a > > nullptr. > > I'm not sure I follow. PreloadURI should be the only thing that sets > mElement to nullptr, so IsPreload() is true if and only if the load came > through PreloadURI, right? Which part of that is causing problems? Facepalm, I converted nsScriptLoader to use AsyncOpen2() but forgot to pass the *internal* policy type to content policies. All good now. [1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#247