Consider upgrading mixed display content

RESOLVED FIXED in Firefox 60

Status

()

P2
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: jkt, Assigned: jkt)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla60
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
As an experiment implement a pref to upgrade mixed content on secure pages. This would be instead of marking the padlock as mixed on secure pages aiding developers in the same way Upgrade Insecure Requests works.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (obsolete)

Comment 4

a year ago
mozreview-review
Comment on attachment 8948410 [details]
Bug 1435733 - Upgrade mixed display content pref.

https://reviewboard.mozilla.org/r/217874/#review223988

::: netwerk/base/nsNetUtil.cpp:2959
(Diff revision 2)
>        // If any of the documents up the chain to the root document makes use of
>        // the CSP directive 'upgrade-insecure-requests', then it's time to fulfill
>        // the promise to CSP and mixed content blocking to upgrade the channel
>        // from http to https.
> -      if (aLoadInfo->GetUpgradeInsecureRequests()) {
> +      if (aLoadInfo->GetUpgradeInsecureRequests() ||
> +          aLoadInfo->GetUpgradeDisplayInsecureRequests()) {

Alternatively, could we just check the ContentType for LoadInfo and check the value of the bool pref here?  Then you don't even need to create a upgradeDisplayInsecureRequests?

I suppose then you would have an issue with the nsCORSListenerProxy.

::: netwerk/base/nsNetUtil.cpp:2968
(Diff revision 2)
>          // append the additional 's' for security to the scheme :-)
>          scheme.AppendASCII("s");
>          NS_ConvertUTF8toUTF16 reportSpec(aURI->GetSpecOrDefault());
>          NS_ConvertUTF8toUTF16 reportScheme(scheme);
>  
> +        if (true) { //aLoadInfo->GetUpgradeInsecureRequests()) {

What is this for?
(Assignee)

Comment 5

a year ago
The rationale to adding: GetUpgradeDisplayInsecureRequests was that changing GetUpgradeInsecureRequests would impact telemetry. However I wanted to keep the implementation as close as possible to avoid potential implementation differences.

Alternatively we could flip this on it's head:
- Make the telemetry (in netwerk/base/nsNetUtil.cpp:2968) check that it's a normal UIR and not this browser upgrade
  - Consider helper GetUpgradeInsecureRequestsFromPage
- Make GetUpgradeInsecureRequests do the GetUpgradeDisplayInsecureRequests functionality and remove  GetUpgradeDisplayInsecureRequests

I would need to verify this is the only telemetry that would be impacted by this change, however I would prefer this as a solution is Christoph was happy with it.

My only concern with the approach above is it might work for display content however it couldn't be expanded to something that could impact framing to get reliable stats (where as this technique could be expanded to active content).

I think I should change GetUpgradeDisplayInsecureRequests to GetBrowserUpgradeInsecureRequests or similar so we could expand for fp active etc.

> if (true) {

You caught me debugging the telemetry :P Will fix.
Flags: needinfo?(ckerschb)
(In reply to Jonathan Kingston [:jkt] from comment #5)
> I would need to verify this is the only telemetry that would be impacted by
> this change, however I would prefer this as a solution is Christoph was
> happy with it.

I think having a separate flag here is the way to go. Please also rename to GetUpgradeInsecureDisplayRequests()
Flags: needinfo?(ckerschb)
(Assignee)

Comment 7

a year ago
browser/base/content/test/siteIdentity/browser_no_mcb_for_loopback.js is the only failing test I can see now related however I am seeing it in builds without this patch using the --verify flag.

I'm going to default the pref on for nightly so we can test this.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8948410 - Flags: review?(honzab.moz)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a year ago
So I modified in this patch dom/security/test/mixedcontentblocker/test_main.html to also check this new pref, this is the only testing I added but I think it is sufficient given that it tests so many combinations.

Toggle passed checks Toggle failed checks Toggle todo checks
Passed: 96
Failed: 0
Todo: 0


blockActive set to true, blockDisplay set to true, upgradeDisplay set to true.
test: iframe, msg: insecure iframe blocked logging message.
test: xhr, msg: insecure xhr blocked logging message.
test: image, msg: secure image loaded logging message.
test: media, msg: secure media loaded logging message.
test: imageSrcset, msg: secure image loaded logging message.
test: imageSrcsetFallback, msg: secure image loaded logging message.
test: imagePicture, msg: secure image loaded logging message.
test: imageJoinPicture, msg: secure image loaded logging message.
test: imageLeavePicture, msg: secure image loaded logging message.
test: object, msg: insecure object blocked logging message.
test: script, msg: insecure script blocked logging message.
test: stylesheet, msg: insecure stylesheet blocked logging message.

blockDisplay set to true, blockActive set to false, upgradeDisplay set to true
test: script, msg: insecure script loaded logging message.
test: xhr, msg: insecure xhr loaded logging message.
test: iframe, msg: insecure iframe loaded logging message.
test: object, msg: insecure object loaded logging message.
test: stylesheet, msg: insecure stylesheet loaded logging message.
test: image, msg: secure image loaded logging message.
test: media, msg: secure media loaded logging message.
test: imageSrcset, msg: secure image loaded logging message.
test: imageSrcsetFallback, msg: secure image loaded logging message.
test: imagePicture, msg: secure image loaded logging message.
test: imageJoinPicture, msg: secure image loaded logging message.
test: imageLeavePicture, msg: secure image loaded logging message.

blockDisplay set to false, blockActive set to true, upgradeDisplay set to true
test: iframe, msg: insecure iframe blocked logging message.
test: xhr, msg: insecure xhr blocked logging message.
test: image, msg: secure image loaded logging message.
test: media, msg: secure media loaded logging message.
test: imageSrcset, msg: insecure image blocked logging message.
test: imageSrcsetFallback, msg: insecure image blocked logging message.
test: imagePicture, msg: insecure image blocked logging message.
test: imageJoinPicture, msg: insecure image blocked logging message.
test: imageLeavePicture, msg: secure image loaded logging message.
test: object, msg: insecure object blocked logging message.
test: script, msg: insecure script blocked logging message.
test: stylesheet, msg: insecure stylesheet blocked logging message.

blockDisplay set to false, blockActive set to false, upgradeDisplay set to true
test: script, msg: insecure script loaded logging message.
test: xhr, msg: insecure xhr loaded logging message.
test: iframe, msg: insecure iframe loaded logging message.
test: image, msg: secure image loaded logging message.
test: object, msg: insecure object loaded logging message.
test: stylesheet, msg: insecure stylesheet loaded logging message.
test: media, msg: secure media loaded logging message.
test: imageSrcset, msg: secure image loaded logging message.
test: imageSrcsetFallback, msg: secure image loaded logging message.
test: imagePicture, msg: secure image loaded logging message.
test: imageJoinPicture, msg: secure image loaded logging message.
test: imageLeavePicture, msg: secure image loaded logging message.

blockDisplay set to true, blockActive set to true, upgradeDisplay set to false
test: iframe, msg: insecure iframe blocked logging message.
test: xhr, msg: insecure xhr blocked logging message.
test: image, msg: insecure image blocked logging message.
test: media, msg: insecure media blocked logging message.
test: imageSrcset, msg: insecure image blocked logging message.
test: imageSrcsetFallback, msg: insecure image blocked logging message.
test: imagePicture, msg: insecure image blocked logging message.
test: imageJoinPicture, msg: insecure image blocked logging message.
test: imageLeavePicture, msg: insecure image blocked logging message.
test: object, msg: insecure object blocked logging message.
test: script, msg: insecure script blocked logging message.
test: stylesheet, msg: insecure stylesheet blocked logging message.

blockDisplay set to true, blockActive set to false, upgradeDisplay set to false
test: script, msg: insecure script loaded logging message.
test: xhr, msg: insecure xhr loaded logging message.
test: image, msg: insecure image blocked logging message.
test: iframe, msg: insecure iframe loaded logging message.
test: media, msg: insecure media blocked logging message.
test: imageSrcset, msg: insecure image loaded logging message.
test: imageSrcsetFallback, msg: insecure image loaded logging message.
test: object, msg: insecure object loaded logging message.
test: stylesheet, msg: insecure stylesheet loaded logging message.
test: imagePicture, msg: insecure image loaded logging message.
test: imageLeavePicture, msg: insecure image blocked logging message.
test: imageJoinPicture, msg: insecure image loaded logging message.

blockDisplay set to false, blockActive set to true, upgradeDisplay set to false
test: iframe, msg: insecure iframe blocked logging message.
test: xhr, msg: insecure xhr blocked logging message.
test: image, msg: insecure image loaded logging message.
test: media, msg: insecure media loaded logging message.
test: imageSrcset, msg: insecure image blocked logging message.
test: imageSrcsetFallback, msg: insecure image blocked logging message.
test: imagePicture, msg: insecure image blocked logging message.
test: imageJoinPicture, msg: insecure image blocked logging message.
test: imageLeavePicture, msg: insecure image loaded logging message.
test: object, msg: insecure object blocked logging message.
test: script, msg: insecure script blocked logging message.
test: stylesheet, msg: insecure stylesheet blocked logging message.

blockDisplay set to false, blockActive set to false, upgradeDisplay set to false
test: script, msg: insecure script loaded logging message.
test: xhr, msg: insecure xhr loaded logging message.
test: iframe, msg: insecure iframe loaded logging message.
test: object, msg: insecure object loaded logging message.
test: stylesheet, msg: insecure stylesheet loaded logging message.
test: image, msg: insecure image loaded logging message.
test: media, msg: insecure media loaded logging message.
test: imageSrcset, msg: insecure image loaded logging message.
test: imageSrcsetFallback, msg: insecure image loaded logging message.
test: imagePicture, msg: insecure image loaded logging message.
test: imageJoinPicture, msg: insecure image loaded logging message.
test: imageLeavePicture, msg: insecure image loaded logging message.


Note the changed behaviour for "secure image loaded", Also note how srcset and friends behave like active content still.

:francois We likely will want some telemetry here too. Am I able just to add a new key to: Telemetry::Accumulate(Telemetry::HSTS_UPGRADE_SOURCE, ...);
Or would it be preferred to add a new metric?
Flags: needinfo?(tanvi)
Flags: needinfo?(francois)
Flags: needinfo?(ckerschb)
(In reply to Jonathan Kingston [:jkt] from comment #10)
> :francois We likely will want some telemetry here too. Am I able just to add
> a new key to: Telemetry::Accumulate(Telemetry::HSTS_UPGRADE_SOURCE, ...);
> Or would it be preferred to add a new metric?

You'd have to create a new HSTS_UPGRADE_SOURCE_2 probe because these details can't be modified after the fact.
Flags: needinfo?(francois)
Comment hidden (mozreview-request)

Comment 13

a year ago
mozreview-review
Comment on attachment 8948410 [details]
Bug 1435733 - Upgrade mixed display content pref.

https://reviewboard.mozilla.org/r/217874/#review227276

datareview+
Attachment #8948410 - Flags: review?(francois) → review+

Comment 14

a year ago
mozreview-review
Comment on attachment 8948410 [details]
Bug 1435733 - Upgrade mixed display content pref.

https://reviewboard.mozilla.org/r/217874/#review227350

r+ for the DOM part with comments applied.

::: dom/base/nsContentUtils.cpp:8833
(Diff revision 5)
>  }
>  
> +/* static */
> +bool
> +nsContentUtils::IsUpgradableDisplayType(nsContentPolicyType aType)
> +{

MOZ_ASSERT(NS_IsMainThread());
If you need it cross-threads, use DOMPrefs.

::: dom/security/nsMixedContentBlocker.cpp:748
(Diff revision 5)
>      return NS_OK;
>    }
>  
> +
> +  // Allow these mixed display if we are choosing to upgrade them
> +  // TODO consider adding GetBrowserUpgradeInsecureRequests(url) to the document

Follow up? File a bug and add the bug ID here.

::: dom/security/test/mixedcontentblocker/file_server.sjs:13
(Diff revision 5)
> +    Components.classes["@mozilla.org/file/directory_service;1"].
> +    getService(Components.interfaces.nsIProperties).
> +    get("CurWorkD", Components.interfaces.nsIFile);
> +  var dirs = path.split("/");
> +  for (var i = 0; i < dirs.length; i++) {
> +    testContentFile.append(dirs[i]);

Are you sure you cannot add a path here?

::: image/imgRequest.cpp:1353
(Diff revision 5)
>      // The csp directive upgrade-insecure-requests performs an internal redirect
>      // to upgrade all requests from http to https before any data is fetched from
>      // the network. Do not pollute mHadInsecureRedirect in case of such an internal
>      // redirect.
>      nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo();
> -    bool upgradeInsecureRequests = loadInfo ? loadInfo->GetUpgradeInsecureRequests()
> +    bool upgradeInsecureRequests = loadInfo ? loadInfo->GetUpgradeInsecureRequests() || loadInfo->GetBrowserUpgradeInsecureRequests()

Indentation.

bool upgradeInsecureRequests = loadInfo ?
  loadInfo->GetUpgradeInsecureRequests() || loadInfo->GetBrowserUpgradeInsecureRequests()

or anything that is <= 80 chars.

::: netwerk/base/LoadInfo.cpp:193
(Diff revision 5)
> +   if (nsContentUtils::IsUpgradableDisplayType(externalType)) {
> +     nsCOMPtr<nsIURI> uri;
> +     mLoadingPrincipal->GetURI(getter_AddRefs(uri));
> +     if (uri) {
> +        bool isHttpsScheme = false;
> +        uri->SchemeIs("https", &isHttpsScheme);

nsresult rv = uri->SchemeIs(...
if (NS_SUCCEEDED(rv) && isHttpsScheme) {

::: netwerk/base/nsILoadInfo.idl:510
(Diff revision 5)
>     *
>     * Warning: If the loadingDocument is null, then the
>     * upgradeInsecureRequests is false.
>     */
>    [infallible] readonly attribute boolean upgradeInsecureRequests;
>  

Here I want to see a good comment about what browserUpgradeInsecureRequests is.
Attachment #8948410 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 15

a year ago
mozreview-review-reply
Comment on attachment 8948410 [details]
Bug 1435733 - Upgrade mixed display content pref.

https://reviewboard.mozilla.org/r/217874/#review227350

> Follow up? File a bug and add the bug ID here.

Going to drop this comment for now. This will likely be needed if we are to look into doing FP active upgrading. It will have to behave even more closely to GetUIR.

> Are you sure you cannot add a path here?

Yup I got ISE's with (NS_ERROR_FILE_UNRECOGNIZED_PATH) error console messages.
Comment hidden (mozreview-request)

Comment 17

a year ago
mozreview-review
Comment on attachment 8948410 [details]
Bug 1435733 - Upgrade mixed display content pref.

https://reviewboard.mozilla.org/r/217874/#review227400

::: netwerk/base/LoadInfo.cpp:192
(Diff revision 6)
> +     nsContentUtils::InternalContentPolicyTypeToExternal(mInternalContentPolicyType);
> +   if (nsContentUtils::IsUpgradableDisplayType(externalType)) {
> +     nsCOMPtr<nsIURI> uri;
> +     mLoadingPrincipal->GetURI(getter_AddRefs(uri));
> +     if (uri) {
> +        bool isHttpsScheme = false;

no need to initialize?

::: netwerk/base/LoadInfo.cpp:193
(Diff revision 6)
> +   if (nsContentUtils::IsUpgradableDisplayType(externalType)) {
> +     nsCOMPtr<nsIURI> uri;
> +     mLoadingPrincipal->GetURI(getter_AddRefs(uri));
> +     if (uri) {
> +        bool isHttpsScheme = false;
> +        nsresult rv = uri->SchemeIs("https", &isHttpsScheme);

(would really use exposing "is securec context" somewhere on nsIPrincipal or so, the (same) string comparing on every sub-request is expensive..

but no need to that in this bug :)  pointing out just that this IsHttps() pattern is spreading quite a bit

::: netwerk/base/LoadInfo.cpp:196
(Diff revision 6)
> +     if (uri) {
> +        bool isHttpsScheme = false;
> +        nsresult rv = uri->SchemeIs("https", &isHttpsScheme);
> +        if (NS_SUCCEEDED(rv) && isHttpsScheme) {
> +          mBrowserUpgradeInsecureRequests = true;
> +        }

nit: ident

::: netwerk/base/LoadInfo.cpp:200
(Diff revision 6)
> +          mBrowserUpgradeInsecureRequests = true;
> +        }
> +     }
> +   }
> +
> +

no need for two blank lines

::: netwerk/base/nsILoadInfo.idl:515
(Diff revision 6)
>  
>    /**
> +   * Returns true if the the page is https and the content is upgradable from http
> +   * requires 'security.mixed_content.upgrade_display_content' pref to be true.
> +   * Currently this only upgrades display content but might be expanded to other loads.
> +   * This is very similat in implementation to upgradeInsecureRequests but browser set.

similat (typo)

::: security/manager/ssl/tests/mochitest/mixedcontent/test_bug455367.html:21
(Diff revision 6)
>  
>    function runTest()
>    {
> +   SpecialPowers.pushPrefEnv(
> +     {"set": [["security.mixed_content.upgrade_display_content", false]]},
> +     null);

nit: indent
Attachment #8948410 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 18

a year ago
Gijs suggested as this is a histogram telemetry I can just keep the same key and just add another key. This would be preferred as we likely have one or two combinations we would like to try for this histogram.

I changed the patch back as this seems better, alternatively would it be better to add keys into the v2 histogram description for the future ideas?
Flags: needinfo?(rweiss)
Flags: needinfo?(francois)
Comment hidden (mozreview-request)
(In reply to Jonathan Kingston [:jkt] from comment #10)
> So I modified in this patch
> dom/security/test/mixedcontentblocker/test_main.html to also check this new
> pref, this is the only testing I added but I think it is sufficient given
> that it tests so many combinations.

Yeah, that sounds reasonable to me.
Flags: needinfo?(ckerschb)
(Assignee)

Comment 21

a year ago
Request for data collection review form

1. What questions will you answer with this data?

How often we are upgrading mixed content when it is present on the page.

2. Why does Mozilla need to answer these questions?

We would like to establish if it is possible to upgrade mixed content safely and how often users are choosing to do so.

3. What alternative methods did you consider to answer these questions?

Additional telemetry to gather how much slower pages are when upgrading however this is much more difficult to measure.

4. Can current instrumentation answer these questions?

We have telemetry that tells us how often we are allowing mixed content but this change upgrades the mixed content. This is really to inform us of how often the pref is flipped and if the code is working correctly. I modified the existing telemetry at the suggestion from chutten.

5. List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories on the found on the Mozilla wiki.

Measurement Description 	           Data Collection Category 	Tracking Bug #
Capture number of Mixed content resources  “Interaction data”           Bug 1340021, Bug 1435733
		
6. How long will this data be collected? Choose one of the following:

I'll mentor this data but it should last forever like the existing telemetry does.

7. What populations will you measure?

This will measure all existing releases, however the pref for now won't be enabled by default in beta and stable.

8. If this data collection is default on, what is the opt-out mechanism for users?

The opt out mechanism is the default as built into firefox.

9. Please provide a general description of how you will analyze this data.

I plan to study http://telemetry.mozilla.org/ to map our change as this data goes live.

10. Where do you intend to share the results of your analysis?

Given this is likely to be informal for now as we will need other metrics to give a better analysis of the impact I think it's fine to not publish anything other than what will be observable on http://telemetry.mozilla.org/
Comment hidden (mozreview-request)

Comment 23

a year ago
mozreview-review
Comment on attachment 8948410 [details]
Bug 1435733 - Upgrade mixed display content pref.

https://reviewboard.mozilla.org/r/217874/#review227582

::: toolkit/components/telemetry/Histograms.json:2132
(Diff revision 8)
>      "record_in_processes": ["main", "content"],
> -    "alert_emails": ["seceng-telemetry@mozilla.com"],
> -    "bug_numbers": [1340021],
> +    "alert_emails": ["seceng-telemetry@mozilla.com", "jkt@mozilla.com"],
> +    "bug_numbers": [1340021, 1435733],
>      "releaseChannelCollection": "opt-out",
>      "expires_in_version": "never",
> -    "kind": "enumerated",
> +    "kind": "categorical",

My understanding is that yes, you could add a _value_ (when you said _key_, I thought you meant turning this histogram into a keyed histogram), but you can't change the kind of histogram.

So if you leave it enumerated and just add "5", it should work since you have some unused values available.
Attachment #8948410 - Flags: review+ → review-
Jonathan, can you paste the data review answers from comment 21 into a textfile, attach it to the bug and r? me on it?

Two small things to fix while you're at it:

- The data is Category 1 (information about features and APIs used by websites)
https://wiki.mozilla.org/Firefox/Data_Collection#Data_Collection_Categories

- Typo: "mentor" in question 6 should be "monitor"
Flags: needinfo?(francois)
:jkt, I just browsed through the code and maybe I missed it, but I think we should log to the console letting the end user/developer know that we are upgrading the load to https. You can check what we do for UIR and log a similar message.
Flags: needinfo?(jkt)
(Assignee)

Comment 26

a year ago
Created attachment 8952444 [details]
data-review1435733.txt
Flags: needinfo?(jkt)
Attachment #8952444 - Flags: review?(francois)
(Assignee)

Comment 27

a year ago
I actually think the categorical here is something we should use. We might end up with a few different attempts at changing browser behaviour here that might be short lived. Is there any issue with changing this over to "categorical"?
Flags: needinfo?(francois)
(In reply to Jonathan Kingston [:jkt] from comment #27)
> I actually think the categorical here is something we should use. We might
> end up with a few different attempts at changing browser behaviour here that
> might be short lived. Is there any issue with changing this over to
> "categorical"?

You'll have to check with a telemetry peer. I don't think you can change the type of an existing probe.
Flags: needinfo?(francois)
(Assignee)

Comment 29

a year ago
I renamed from HTTP_SCHEME_UPGRADE to HTTP_SCHEME_UPGRADE_TYPE too, so should be all good here so long as the datareview is fine.
Flags: needinfo?(francois)
(In reply to Jonathan Kingston [:jkt] from comment #29)
> I renamed from HTTP_SCHEME_UPGRADE to HTTP_SCHEME_UPGRADE_TYPE too, so
> should be all good here so long as the datareview is fine.

You're right. I missed that.
Flags: needinfo?(francois)

Comment 31

a year ago
mozreview-review
Comment on attachment 8948410 [details]
Bug 1435733 - Upgrade mixed display content pref.

https://reviewboard.mozilla.org/r/217874/#review227656

Histograms.json changes look fine.
Attachment #8948410 - Flags: review- → review+
Comment on attachment 8952444 [details]
data-review1435733.txt

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in Histograms.json.

2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes, Jonathan.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1.

5) Is the data collection request for default-on or default-off?

Default on, all channels.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, it's permanent.
Flags: needinfo?(rweiss)
Attachment #8952444 - Flags: review?(francois) → review+
(In reply to Jonathan Kingston [:jkt] from comment #10)
> So I modified in this patch
> dom/security/test/mixedcontentblocker/test_main.html to also check this new
> pref, this is the only testing I added but I think it is sufficient given
> that it tests so many combinations.
...
> 
> Note the changed behaviour for "secure image loaded", Also note how srcset
> and friends behave like active content still.
> 
Can we change that text to "secure image loaded after it was upgraded from http to https" or something like that?  If it is easy.  I don't want to make the test too complicated to achieve that string.

Add some flexibility to add more to the probe, if you haven't already.  I will take a look at the patch.

Comment 34

a year ago
mozreview-review
Comment on attachment 8948410 [details]
Bug 1435733 - Upgrade mixed display content pref.

https://reviewboard.mozilla.org/r/217874/#review227710

* How do security.mixed_content.block_display_content and security.mixed_content.upgrade_display_content interact with each other?

For example:
Assume someone is using Nightly and has previously set security.mixed_content.block_display_content to true.  Now we add security.mixed_content.upgrade_display_content and set it to true in Nightly.  What happens to that user?  Does the browser upgrade the content or block it?

If security.mixed_content.block_display_content is false, then I assume a true security.mixed_content.upgrade_display_content will upgrade.

* I agree with Chris that we should defnitley log to the console from the beginning (or as soon as we turn the pref on anywhere), else this will be very hard to debug.

* Overall code looks good.  Chris should take a look through it too.

Thanks!

::: dom/security/nsMixedContentBlocker.cpp:747
(Diff revision 8)
>      *aDecision = ACCEPT;
>      return NS_OK;
>    }
>  
> +
> +  // Allow these mixed display if we are choosing to upgrade them

Can you make this comment a bit longer, similar the comment right above about upgrade insecure requests?  Saying that we accept with the promise that the channel will get upgraded in ... etc.

::: dom/security/test/mixedcontentblocker/test_main.html:157
(Diff revision 8)
>          break;
>  
>        /* Images using the "imageset" policy, from <img srcset> and <picture>, do not get the mixed display exception */
>        case "imageSrcset":
> +        // Odly for these mixed active checks
> +        // When blockDisplay && blockActive && upgradeDisplay are true the request is blocked

?
Flags: needinfo?(tanvi)
(Assignee)

Comment 35

a year ago
> Add some flexibility to add more to the probe, if you haven't already

The probe is renamed so we could change the type to categorical which adds flexibility for up to 50 probes.

> How do security.mixed_content.block_display_content and security.mixed_content.upgrade_display_content interact with each other?

In Comment 10 it shows how the prefs interact. Upgrade take precedence over mixedDisplay. For some reason when mixedDisplay=false and mixedActive=true it takes precedence over upgradeDisplay. I didn't dig into why as I didn't touch that code but I don't think it will impact us much/at all.


> Can we change that text to "secure image loaded after it was upgraded from http to https" or something like that?  If it is easy.

Yup

> I don't want to make the test too complicated to achieve that string.

I don't understand this?

If this is referring to the srcset behaviour the code is following the mixed display code for only upgrading where we would show a mixed padlock. We should look to expanding scope of this pref (or a similar one) in another bug I think.


> I agree with Chris that we should defnitley log to the console from the beginning (or as soon as we turn the pref on anywhere), else this will be very hard to debug.

There is already logging for CSP UIR which seems to have a bug. I can implement my logging to match this logging however it appears to be logging to the wrong window and so only appears in the browser toolbox. I spent all last night trying to fix this with no success.
(Assignee)

Comment 36

a year ago
I tried to fix the logging however I think the issue is more with the fact we are using window rather than documents. I think this should be moved into a new bug especially as Chrome doesn't log for UIR, we might actually prevent developers from upgrading due to console noise.

In this bug I will add a new string for "upgradeInsecureRequest" to be browser specific and leave it at that.

Attempted to improve the logging with:

uint32_t innerWindowId = aLoadInfo->GetInnerWindowID();

nsCOMPtr<nsIDOMDocument> domDoc;
rv = aLoadInfo->GetLoadingDocument(getter_AddRefs(domDoc));
if (NS_SUCCEEDED(rv) && domDoc) {
  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
  nsCOMPtr<nsPIDOMWindowInner> window = doc->GetInnerWindow();
  if (window) {
    innerWindowId = window->WindowID();
  }
}
CSP_LogLocalizedStr("upgradeInsecureRequest",
                    params, ArrayLength(params),
                    EmptyString(), // aSourceFile
                    EmptyString(), // aScriptSample
                    0, // aLineNumber
                    0, // aColumnNumber
                    nsIScriptError::warningFlag, "CSP",
                    innerWindowId);
Comment hidden (mozreview-request)

Comment 38

a year ago
mozreview-review
Comment on attachment 8948410 [details]
Bug 1435733 - Upgrade mixed display content pref.

https://reviewboard.mozilla.org/r/217874/#review227808

Only looked at the console message including localization. thanks for adding that - looks great. r=me
Attachment #8948410 - Flags: review?(ckerschb) → review+

Comment 39

a year ago
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/275404b9ec61
Upgrade mixed display content pref. r=baku,ckerschb,francois,mayhemer

Comment 40

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/275404b9ec61
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Keywords: dev-doc-needed
This setting probably breaks quite a lot of sites which rely on non-HTTPS CDNs.
Is there no way to fallback to HTTP if the upgrading fails?
(Assignee)

Comment 42

a year ago
:RiCON this is currently an experiment. CDNs really shouldn't ever be on HTTP anyway but we are considering a header the site can provide to turn off this upgrade.
(Assignee)

Updated

a year ago
Depends on: 1440701

Comment 43

a year ago
If this "experiment" ever were to hit a stable build without a way for sites to opt out of it, like what happened with the HSTS priming fiasco, that would be highly counterproductive. Any site that relied on a passive content CDN that cannot support HTTPS for either technical or capacity reasons would then be forced to downgrade Firefox sessions to use HTTP for active content as well.
(Assignee)

Comment 44

a year ago
We are in talks with other browsers, as mentioned in Comment 42, we are considering an opt out header however that isn't possible without experimenting with groups of users.

The reality is also that this is very different from priming:
- We are only upgrading display content
- Content won't block page load like scripts and CSS
- There isn't timeouts here for a fallback
- MitM can't force a downgrade here

Priming wasn't a fiasco it helped shape this experiment, we will be implementing more telemetry to measure the success of this and I'm not really sure how this will have the negative impacts you are stating. The only breakage that will happen is media that isn't available over HTTPS, which is becoming rarer as the web becomes HTTPS only.

Comment 45

a year ago
(In reply to Jonathan Kingston [:jkt] from comment #44)
> Priming wasn't a fiasco

It was a fiasco because it violated the HTTP spec by sending HTTPS requests to HTTP sockets on non-standard ports, and because it was enabled by default in Firefox 51 despite repeated assurances that this would not happen, which broke a lot of websites. (It was later disabled with a hotfix.)

> The only breakage that will happen is media that isn't available over HTTPS,
> which is becoming rarer as the web becomes HTTPS only.

That is a rather massive negative impact. We are not all Google or Amazon, and web infrastructure is not in general on a two-year upgrade cycle. HTTPS is still orders of magnitude more expensive processing-wise than HTTP and with significantly higher initial latency, and because the advantages of doing passive content over HTTPS is primarily one of privacy rather than security, it would be rather presumptuous of a browser company to decree that this must be done.

While I absolutely agree that active content must be done over HTTPS for end-user safety, websites need to have the freedom to make trade-offs between the advantages and disadvantages between HTTP and HTTPS for passive content. If an opt-out header must be provided, this is fine, but please don't do a repeat of the aforementioned HSTS priming fiasco.
(Assignee)

Updated

a year ago
Depends on: 1440709
This broke many podcasts on https://deezer.com. Setting security.mixed_content.upgrade_display_content to false make them work again.

I initially thought we had the right behavior compared to Chrome, so I filed a bug there: https://bugs.chromium.org/p/chromium/issues/detail?id=816899. Now I understand there's no right behavior at the moment.
I'm puzzled because https://deezer.com doesn't seem to use any CSP. Is there another way to trigger this behavior?
(Assignee)

Comment 49

a year ago
This is experimental behaviour which has been disabled again (should be off in the next nightly) in Bug 1440709. This doesn't have anything to do with CSP, we are just blanked upgrading all mixed passive content to fix the broken padlock.

We have disabled this, pending more metrics so we can have an ability to work out how we can roll this out. So if we need to have some UX fallback, better escapes for developers etc. Chrome is experimenting in the same area with the eventual goal for a cross browser solution that will be standardised.

We upgraded over 15 million mixed content requests over the last 9 days with minimal breakage reports based on the scale of upgrading. The experiment almost removed any usage of the grey padlock with a yellow triangle which is our ultimate goal here.
> This doesn't have anything to do with CSP.

Then I'm puzzled again because in https://everlong.org/mozilla/testcase-csp.html, when I remove the CSP in the meta, I don't get this behavior.
(Assignee)

Comment 51

a year ago
When "security.mixed_content.upgrade_display_content"=true.

Without the CSP I get the warning:
"Nightly is upgrading an insecure display request ‘http://rf.proxycast.org/1406104647436869632/15275-27.02.2018-ITEMA_21601361-3.mp3’ to use ‘https’"

With the CSP there is no warning but it should still break in the same way.
I just tried again with a new testcase in https://everlong.org/mozilla/testcase-nocsp.html; I see the behavior you describe. I believe there was some caching involved. Thanks for your explanations!
Can we expose this pref to WebExtensions?  There is a precedent for allowing WebExtensions to control certain privacy/security features: https://bugzilla.mozilla.org/show_bug.cgi?format=default&id=1397611.
I accidentally found out about this after thinking that a few sites were simply broken the last few days. It was only by chance that I read a comment that this was actually Nightly’s fault that made me invest in what caused this.

I understand the idea of this but I don’t think it’s a good way to just potentially break pages. The problem is that the end user has no clue why this actually happens.

When automatically upgrading to HTTPS, is there a way we can figure out if those requests actually make it through? Maybe we could show some UI like “To improve your security, resources were upgraded to HTTPS. This might have caused problems displaying this page. Click here to reload the page without upgrading resources.”

We could do this similar to the tracking protection in private mode which is persisted for a site. That way, we could have this feature in a very forward-moving way, while allowing users to actually reach their content.
(Assignee)

Updated

a year ago
Depends on: 1442990
I've added a description of this on our experimental features page:

https://developer.mozilla.org/en-US/Firefox/Experimental_features#Security

Search for "Upgrading mixed display content".

Let me know if the wording makes sense. Thanks!
Flags: needinfo?(jkt)
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 56

11 months ago
"When enabled, it shows a specific color of padlock instead of marking the padlock as mixed on secure pages (padlock with warning triangle)" is incorrect that is the default behaviour.

We are fixing the mixed padlock by changing http:// subresources like images to be https:// against what the website is giving us in the HTML and JS.
This is similar to how HSTS works when the website sends the header to the browser we rewrite the requests, however this is limited just to passive mixed content, where as HSTS will apply to JS and other active content.

Perhaps change this sentence to: "When enabled, the browser shouldn't show a mixed padlock (padlock with warning triangle) as the media requests on the page have been upgraded to using https://"

It might also be worth noting that this might prevent the requests from loading if the media doesn't support HTTPS.
Flags: needinfo?(jkt) → needinfo?(cmills)
(In reply to Jonathan Kingston [:jkt] from comment #56)
> "When enabled, it shows a specific color of padlock instead of marking the
> padlock as mixed on secure pages (padlock with warning triangle)" is
> incorrect that is the default behaviour.
> 
> We are fixing the mixed padlock by changing http:// subresources like images
> to be https:// against what the website is giving us in the HTML and JS.
> This is similar to how HSTS works when the website sends the header to the
> browser we rewrite the requests, however this is limited just to passive
> mixed content, where as HSTS will apply to JS and other active content.
> 
> Perhaps change this sentence to: "When enabled, the browser shouldn't show a
> mixed padlock (padlock with warning triangle) as the media requests on the
> page have been upgraded to using https://"
> 
> It might also be worth noting that this might prevent the requests from
> loading if the media doesn't support HTTPS.

OK, cool, thanks for the clarification! I've updated the wording as advised.
Flags: needinfo?(cmills)

Comment 58

10 months ago
mozreview-review
Comment on attachment 8948410 [details]
Bug 1435733 - Upgrade mixed display content pref.

https://reviewboard.mozilla.org/r/217874/#review247244

::: testing/firefox-ui/tests/functional/security/test_mixed_content_page.py:13
(Diff revision 9)
>  
>  class TestMixedContentPage(PuppeteerMixin, MarionetteTestCase):
>      def setUp(self):
>          super(TestMixedContentPage, self).setUp()
>  
> +        self.marionette.set_pref('security.mixed_content.upgrade_display_content', False)

This should never have passed review for firefox-ui functional tests given that this set preference never gets removed, and following tests will also treat it as disabled.

I will fix that as part of my patch on bug 1414776.
You need to log in before you can comment on or make changes to this bug.