Closed Bug 1358663 Opened 7 years ago Closed 3 years ago

Mixed content in <picture> isn't blocked correctly

Categories

(Core :: DOM: Security, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: xiaoyin.l, Unassigned)

References

(Blocks 1 open bug, )

Details

(Keywords: sec-low, Whiteboard: [domsecurity-backlog1])

Attachments

(1 file)

According to the Mixed Content spec Section 3.1 [1], images loaded via <picture> are not optionally-blockable, meaning that they are blockable mixed content. But I found that in Firefox 53, although insecure images loaded in <picture> are not displayed to users, it does send the HTTP request to the server, which I think violates the spec, and unnecessarily leaks info to man-in-the-middle attackers.

To reproduce:
Download the attached "mixed.html" and upload it to some online server that supports HTTPS. Open WireShark and start capturing traffic. Then launch Firefox and load the PoC from Internet over HTTPS. You can see in WireShark that the request to http://www.opera.com/favicon.ico is sent. Note that if you then refresh the page mixed.html (by Ctrl+F5), it is blocked correctly, but if it's your first visit, the request is sent.

The reason why I think this bug has a theoretical security impact, even if we allow mixed content in <img>, is that web developers may rely on the fact that insecure images in <picture> are blocked as active mixed content. For example, a site allows users to embed images on webpages by providing external URLs, http or https. These URLs are linked to in <picture> elements. Later the site upgrades to HTTPS, and the site's developers want to block insecure images while allow images over https (i.e. developers prefer missing images than leaking info to MITM attackers). To achieve this goal, they don't have to manually remove http images or do anything else (CSP, etc.), knowing that insecure images should be blocked by browsers as described in [1]. However in Firefox, these insecure images are loaded anyway (though not displayed), which compromises user's privacy. (Hopefully I have explained clearly why I think this is a browser vulnerability rather than web developer's mistake.)

Other browsers have similar issues. Chrome 58 has the same behavior as Firefox, and Chromium team has confirmed this issue.[2] Edge 15 loads insecure images and display them. MSRC has opened a case for it. So please don't disclose this bug until Google and Microsoft finish their fix.

[1] https://w3c.github.io/webappsec-mixed-content/#category-optionally-blockable
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=713440
Group: core-security → dom-core-security
As expected, the problem in terms of the actual behavior is image preload.

nsHtml5TreeOpExecutor::PreloadImage calls nsIDocument::ResolvePreloadImage and then nsIDocument::MaybePreLoadImage.  The latter is only passed a URI, crossorigin attribute, and referrer policy.

ResolvePreloadImage knows about <picture> and <source> elements and will return a URI from a <source>, as relevant, if the <img> is inside a <picture>.

But MaybePreLoadImage always passes nsIContentPolicy::TYPE_INTERNAL_IMAGE_PRELOAD as the internal policy type.  contrast this with the normal load codepath, which passes nsIContentPolicy::TYPE_IMAGESET for the srcset-or-picture case (which it does NOT distinguish, btw) and passes nsIContentPolicy::TYPE_INTERNAL_IMAGE for the <img src> case.  We probably want to have different preload types for the different situations here; ResolvePreloadImage should then have an outparam that indicates the right preload type or something.

Note that this would still fail in the following situation:

  <script>document.write("<picture><source srcset='stuff'>")</script>
  <img src="whatever">
  <script>document.write("</picture>")</script>

because in that case the preload scanner will think the <img> is not in a <picture> but it actually is.  Of course the same thing would happen if the site started off with an <img> on its own in markup and then moved it inside <picture>...

I should note that per the mixed content spec, <img srcset> should be optionally-blockable, apparently, though I expect we treat it as blockable.  Or maybe it doesn't say what it means?  Because what it _says_ normatively contradicts its informative note: <picture> still loads its image via <img>, so per the normative text should be optionally-blockable.  I've raised https://github.com/w3c/webappsec-mixed-content/issues/8 on the spec.
Given the problematic situation explained by Boris in comment 1, I think there is not an easy way forward with this bug. Ultimately I think we should treat <img>, <img srcset> and (if possible) <picture> the same. Looking at our implementation it makes me wonder why we treat TYPE_IMAGE and TYPE_IMAGESET differently in nsMixedContentBlocker.cpp. Kate is about to modernize our mixed content blocker implementation. CC'ing Kate to take a look.
Blocks: 1295777
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
> Given the problematic situation explained by Boris in comment 1

To be clear, that situation is not something I think we should worry about too much, if you mean the document.write() usage.

> Looking at our implementation it makes me wonder why we treat TYPE_IMAGE and TYPE_IMAGESET differently in nsMixedContentBlocker.cpp

Because the spec says to.

I don't think this bug, excluding the document.write insanity, is particularly hard to fix.  Comment 1 outlines a simple enough fix, imo: ResolvePreloadImage should output the preload type in addition to the URI, which should then be passed as an arg to PreloadImage.
Keywords: sec-low
Whiteboard: [domsecurity-backlog1] → [domsecurity-backlog1] Embargo until Google and MS unhide their bugs
Chrome's bug is public.
Group: dom-core-security
Whiteboard: [domsecurity-backlog1] Embargo until Google and MS unhide their bugs → [domsecurity-backlog1]

This is no longer reproducible in Firefox 90.
The requests to http://www.opera.com/favicon.ico are blocked by the MCB and do not show up in wireshark.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: