upgrade-inecure-reuqests set within a Meta-CSP does not work properly for preloads

RESOLVED FIXED in Firefox 45

Status

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: franziskus, Assigned: ckerschb)

Tracking

unspecified
mozilla46
Dependency tree / graph

Firefox Tracking Flags

(firefox44 unaffected, firefox45 fixed, firefox46 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Posted image Screenshot.png
Upgrade insecure should upgrade insecure http requests to secure https ones. While this seems to work, the UI doesn't update.

To reproduce:
Go to https://googlechrome.github.io/samples/csp-upgrade-insecure-requests/

Expected result:
green lock showing everything is ok

Actual result:
mixed content warning is displayed even though the request has been performed via https. also see attached screenshot
I see the following webconsole output[1].

This logging message "Loading mixed (insecure) display content" gets added to the console by this code in nsMixedContentBlocker::ShouldLoad():
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#871

If the upgrade-insecure-request directive was set in CSP and we attempt to load an HTTP subresource,  nsMixedContentBlocker::ShoudlLoad() would return early here: http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#656.  And hence we should never reach the code at line 871 that logs this message.  This implies that nsMixedContentBlocker doesn't know about the upgrade-insecure-request directive yet.  But the console message only shows up once.  Where in other examples [2] of mixed display content, the loading message has the red bubble on the right hand side with the number 2 or 3 (indicating a duplicate message, which we have seen occur because of preloads)

Inspecting the headers, there is no mention of CSP, because it is in a meta tag.  I wonder if the behavior would differ if the CSP was in a header?  Christoph might know more about what is going on here.

[1] https://googlechrome.github.io/samples/csp-upgrade-insecure-requests
GET https://googlechrome.github.io/samples/csp-upgrade-insecure-requests/ [HTTP/1.1 200 OK 188ms]
GET https://googlechrome.github.io/samples/styles/main.css [HTTP/1.1 200 OK 30ms]
Loading mixed (insecure) display content "http://googlechrome.github.io/samples/images/touch/chrome-touch-icon-192x192.png" on a secure page[Learn More] csp-upgrade-insecure-requests
Content Security Policy: Upgrading insecure request 'http://googlechrome.github.io/samples/images/touch/chrome-touch-icon-192x192.png' to use 'https' <unknown>
GET https://www.google-analytics.com/analytics.js [HTTP/2.0 200 OK 94ms]
GET https://googlechrome.github.io/samples/images/touch/chrome-touch-icon-192x192.png [HTTP/1.1 200 OK 103ms]
GET https://www.google-analytics.com/r/collect

[2] https://people.mozilla.com/~tvyas/mixeddisplay.html
GET 
https://people.mozilla.org/~tvyas/mixeddisplay.html [HTTP/1.1 200 OK 148ms]
Loading mixed (insecure) display content "http://people.mozilla.org/~tvyas/redminus1.jpg" on a secure page[Learn More] mixeddisplay.html
GET 
http://people.mozilla.org/~tvyas/redminus1.jpg
Flags: needinfo?(mozilla)
According to the spec [1]:
> Authors are strongly encouraged to place meta elements as early in
> the document as possible, because policies in meta elements are
> not applied to content which preceeds them.

What happens on https://googlechrome.github.io/samples/csp-upgrade-insecure-requests/ is that the icon load preceeds the meta-csp element and hence the icon load is not upgraded from http to https. So it's absolutely correct that we display the mixed content warning for that page.

[1] http://www.w3.org/TR/CSP/#delivery-html-meta-element
Flags: needinfo?(mozilla)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> According to the spec [1]:
> > Authors are strongly encouraged to place meta elements as early in
> > the document as possible, because policies in meta elements are
> > not applied to content which preceeds them.
> 
> What happens on
> https://googlechrome.github.io/samples/csp-upgrade-insecure-requests/ is
> that the icon load preceeds the meta-csp element and hence the icon load is
> not upgraded from http to https. So it's absolutely correct that we display
> the mixed content warning for that page.
> 
> [1] http://www.w3.org/TR/CSP/#delivery-html-meta-element

The icon load is actually not in the <head> and is after the meta csp tag.
 <img src="http://googlechrome.github.io/samples/images/touch/chrome-touch-icon-192x192.png">

Since the meta csp policies are supposed to be enforced on preloads, I'm not sure what's going wrong here.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Summary: UI doesn't update on upgrade insecure request → upgrade-inecure-reuqests set within a Meta-CSP does not work properly for preloads
Thanks Tanvi for following up and thanks Franziskus for filing. In fact Meta CSP works slightly different than CSP delivered through a header especially for upgrade insecure requests. We have a special flag for upgrade-insecure-preloads which we have to check within mixedContentBlocker, imgLoader (for insecure redirects) and also the CORSListener.

I'll put a patch up in a minute.
Jonas, when a CSP is delivered through a meta tag we have to account for speculative loads and hence added mUpgradeInsecurePreloads. While we correctly update preloads within nsHttpChannel::Connect() we haven't added that special logic to mixed content blocker, CORS and imgLoader.

MixedContentBlocker turned out to display the wrong UI, which is not that great. I am not sure about the imgLoader problem for insecure redirects, but the CORS problem is more problematic in my opinion, because CORS would have really blocked such a preload.
Attachment #8699782 - Flags: review?(jonas)
Comment on attachment 8699782 [details] [diff] [review]
bug_1233098_upgrade_insecure_preloads.patch

     nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo();
-    bool upgradeInsecureRequests = loadInfo ? loadInfo->GetUpgradeInsecureRequests()
-                                            : false;
-    if (!upgradeInsecureRequests) {
-      mHadInsecureRedirect = true;
+    if (loadInfo) {
+      bool isPreload =
+        nsContentUtils::IsPreloadType(loadInfo->InternalContentPolicyType());
+      bool upgradeInsecureRequests =
+        loadInfo->GetUpgradeInsecureRequests() ||
+        (isPreload && loadInfo->GetUpgradeInsecurePreloads());

This part should probably be outside the if(loadInfo) part, so we fail closed when we don't have loadInfo like we used to:
+      if (!upgradeInsecureRequests) {
+        mHadInsecureRedirect = true;
+      }

     }
I thought about this again and in fact it doesn't make sense that the loadinfo provides a separate attribute for preloads. In this patch I refactored those bits which makes upgrade insecure requests more robust. The only downside is, that when serializing mUpgradeInsecureRequests and mUpgradeInsecurePreloads that the former might be polluted by the latter. In the end it doesn't make a difference because upgradeInsecureReuqests() always returns the correct result back.

Overall, this approach seems way more robust to me. Jonas, agreed?
Attachment #8699782 - Attachment is obsolete: true
Attachment #8699782 - Flags: review?(jonas)
Attachment #8699830 - Flags: review?(jonas)
Thought about this patch yet again and I think there is no need to store 2 flags within the loadInfo, because either the channel is a preload or not, hence it's sufficient to only store one flag within the loadInfo that indicates whether the channel needs to be upgraded from http to https before it hits the network. We can distinguish that in the loadInfo constructor and set that flag.

The only part that needs two flags is the document, which needs to be able to distinguish between speculative preloads and actual loads.

Refactoring those bits within the loadInfo shrinks the size of the loadInfo, abstracts the decision of whether to upgrade the channel within the loadInfo and makes upgrade insecure requests more robust over all. We should have done so in the first place.
Attachment #8699830 - Attachment is obsolete: true
Attachment #8699830 - Flags: review?(jonas)
Attachment #8700040 - Flags: review?(jonas)
Comment on attachment 8700040 [details] [diff] [review]
bug_1233098_upgrade_insecure_preloads.patch

Review of attachment 8700040 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsContentPolicy.cpp
@@ +116,5 @@
>      nsContentPolicyType externalType =
>          nsContentUtils::InternalContentPolicyTypeToExternal(contentType);
>  
> +    nsContentPolicyType externalTypeOrMCBInternal =
> +        nsContentUtils::InternalContentPolicyTypeToExternalOrMCBInternal(contentType);

This seems really messy, and not something that is generic enough to belong on nsContentUtils.

Why not just convert to an external type, and then use the internal type or the external type as needed?

I.e. why can't we send the raw internal type to the CSP and MCB listeners?

::: netwerk/base/LoadInfo.cpp
@@ +87,5 @@
>  
> +    // if the document forces all requests to be upgraded from http to https, then
> +    // we should do that for all requests. If it only forces preloads to be upgraded
> +    // then we should enforce upgrade insecure requests only for preloads.
> +    mUpgradeInsecureRequests = aLoadingContext->OwnerDoc()->GetUpgradeInsecureRequests(false);

mUpgradeInsecureRequests =
  aLoadingContext->OwnerDoc()->GetUpgradeInsecureRequests(false) ||
(nsContentUtils::IsPreloadType(mInternalContentPolicyType) &&
 aLoadingContext->OwnerDoc()->GetUpgradeInsecureRequests(true))
Attachment #8700040 - Flags: review?(jonas) → review-
(In reply to Jonas Sicking (:sicking) from comment #9)
-------------------------------------------------------------
> I.e. why can't we send the raw internal type to the CSP and MCB listeners?

Initially we only needed to send internal types for workers to MCB, by now we need so many internal types that it makes sense to send the internal types to CSP and MCB and convert withing those listeners. I filed 1239397 to get that fixed.

> ::: netwerk/base/LoadInfo.cpp
> mUpgradeInsecureRequests =
>   aLoadingContext->OwnerDoc()->GetUpgradeInsecureRequests(false) ||
> (nsContentUtils::IsPreloadType(mInternalContentPolicyType) &&
>  aLoadingContext->OwnerDoc()->GetUpgradeInsecureRequests(true))

I incorporated that change but otherwise the patch is completely untouched.
Attachment #8700040 - Attachment is obsolete: true
Attachment #8707583 - Flags: review?(jonas)
Meta CSP landed in 45, setting according tracking flags to make sure this patch gets upliftet.
https://hg.mozilla.org/mozilla-central/rev/4a9aa8e07595
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment on attachment 8708563 [details] [diff] [review]
bug_1233098_upgrade_preloads_rebased_for_aurora.patch

> Providing this patch which is rebased for AURORA:

Approval Request Comment

Meta-CSP landed within Bug 663570 and introcuded that regression for upgrade-insecure-requests. Even though the backend works correctly and upgrades preloads from http to https, the UI didn't get updated correctly.

[Feature/regressing bug #]:
Bug 663570

[User impact if declined]:
Users will see incorrect UI for mixed content blocking when a site uses meta-csp's upgrade-insecure-requests.

[Describe test coverage new/current, TreeHerder]:
1) Manual testing using
   https://googlechrome.github.io/samples/csp-upgrade-insecure-requests/

2) Automatic testing using dom/security/test/test_meta_* family.

[Risks and why]:
low to medium. Affected are only pages that:
a) deliever a CSP using the meta tag, and in addition
b) use the directive upgrade-insecure-reuqests

[String/UUID change made/needed]:
yes, for nsILoadInfo.idl
Attachment #8708563 - Flags: approval-mozilla-aurora?
Comment on attachment 8708563 [details] [diff] [review]
bug_1233098_upgrade_preloads_rebased_for_aurora.patch

Improve a new feature, taking it.
Attachment #8708563 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.