Closed Bug 1595491 Opened 4 years ago Closed 2 years ago

Make <embed> and <object> behave more like <iframe>

Categories

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

defect

Tracking

()

RESOLVED FIXED
105 Branch
Tracking Status
firefox105 --- fixed

People

(Reporter: annevk, Assigned: sefeng)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: [orb:m1])

Attachments

(2 files)

Chrome and Safari appear to create a browsing context more often than Firefox for the embed and object elements. Firefox already resizes for image/svg+xml, so that logic could be reused for image documents (though see bug 1594986).

Plugins might have to remain a special case, but Firefox could start honoring X-Frame-Options and CSP's frame-ancestors for all responses that go through embed and object elements. If honoring those headers seems agreeable, perhaps we should split that out as it's easier to do and it'll make it harder for folks to make mistakes with Fetch Metadata (bug 1508292).

See also https://github.com/whatwg/fetch/pull/948 for context.

Flags: needinfo?(bzbarsky)

I have no opinions on the header bits; I see no reasons to not honor them, honestly, apart from possible compat issues.

As far as creating a browsing context for the image case, we could do that, but we'd need to do a bit more work than we do for SVG. Specifically, we'd need to:

  1. Get rid of things like margins and padding around the image in the image document (at least when loaded via object/embed).
  2. Possibly make it possible to have browsing contexts that don't show up in the parent's frames[n] and don't show up by name. This depends on whether we'd want to make the existence of the browsing context visible or not.
  3. Fix UI code that shows image context menu options on <object data="some-image">, assuming we show those, to work with the new setup.

Obviously doing this plus dropping plug-ins would let us significantly simplify nsObjectLoadingContent, to the point where it would almost go away as a concept.

Flags: needinfo?(bzbarsky)
Depends on: 1595762
Type: defect → enhancement
Depends on: 1643611
Depends on: 1686040

As per bug 1686040 comment 5 our current setup for these elements is problematic as too much data can enter the attacker process. It seems that in Chrome and Safari these elements always end up instantiating documents (e.g., https://software.hixie.ch/utilities/js/live-dom-viewer/saved/9009) and we should consider doing the same. Getting there incrementally is acceptable though. E.g., allowing sniffed images to the attacker process and nothing else could be a reasonable middle ground, if that would be easier to do.

Blocks: orb
Depends on: plugin-cleanup
Component: DOM: Core & HTML → DOM: Navigation
Type: enhancement → defect
Summary: Investigate making <embed> and <object> behave more like <iframe> → Make <embed> and <object> behave more like <iframe>
Priority: -- → P3
Assignee: nobody → afarre
Severity: normal → --

Latest try is here, and it's starting to come together (if I haven't done anything drastically wrong in the latest update): https://treeherder.mozilla.org/#/jobs?repo=try&revision=81e119a921f15b4e55f02cc31b4b67d29e3b51cd

One thing that's failing though is 315920-18d.html, and I talked briefly with Emilio about this, and he graciously offered to take a quick look!

I'll look at it as well though, but after that, it's review time!

Flags: needinfo?(emilio)

Turns out there is another bug that is happening due to me not handling nsImageLoadingContent correctly (this is probably the same, it also has got to do with nsImageLoadingContent).

But that's it. And in the end I wouldn't be surprised if 315920-18d.html also is an nsImageLoadingContent thing. It feels like it has got to do with -moz-broken which is handled through ImageState in nsImageLoadingContent.

It turned out to not be exactly that, but it does have with -moz-broken to do. The thing that's missing is a final call to nsObjectLoadingContent::NotifyStateChanged when loading is complete, to trigger a restyle due to nsObjectLoadingContent state changing. Easy enough to change.

Flags: needinfo?(emilio)

By making image loading in <embed> and <object> behave more like when
an <iframe> loads an image, we can make sure that the synthetic
document generated is process switched if the image is cross
origin. This is done by making image loading in nsObjectLoadingContent
follow the document loading path.

We also make sure that we pass the image size back to the embedder
element to not get stuck with the intrinsic size.

To avoid named targeting being able to target these synthetic
documents, as well as showing up in Window.frames and being counted
in Window.length, we keep a filtered list of non-synthetic browsing
contexts for that use-case.

This feature is controlled by two prefs:

  • browser.opaqueResponseBlocking.syntheticBrowsingContext

    This triggers the creation of synthetic documents for images loaded
    in <object> or embed.

  • browser.opaqueResponseBlocking.syntheticBrowsingContext.filter

    This turns on the filtering of synthetic browsing contexts in named
    targeting, Window.length and Window.frames.

Flags: needinfo?(emilio)
Whiteboard: [orb:m1]
Flags: needinfo?(emilio)

I haven't had a chance to try your patches yet, but I wanted to early-flag a possible a11y concern. I think this might result in images embedded with <embed> or <object> being exposed in the a11y tree as being inside a frame, where this isn't the case for <img> and isn't currently the case for <embed> or <object> either. This does have some consequences for screen reader users: some additional verbosity; and VoiceOver treats frames as additional containers that users have to "interact with" (explicitly move inside) to see what's inside.

I'm not super familiar with how embed and object work. Why might an author use embed/object to embed an image instead of img? Given that this is about cross-origin process switches, is that currently an issue for img or is it not relevant there for some reason?

You'll be able to see whether there's a difference by looking in the Firefox Accessibility Inspector. I'll also try to play with this when I get a chance.

We can probably work around this in the a11y engine by exposing a generic role for OuterDocAccessible in this case which screen readers should ignore.

Having said all of this, Chrome doesn't seem to handle images embedded with embed/object very well with regard to the a11y tree at all, so this certainly isn't going to be a major web compat issue or anything like that.

I tried the patches and confirmed my suspicion here. It's a bit worse than I thought because setting the title attribute on the embed to provide alternative text doesn't expose the alternative text on the image inside the frame, which means that if you embed an image like this, there's no way to provide alternative text for it. I'll think on possible solutions.

Thanks for the comments James!

So the most common reason to embed images instead of using <img> is(/was) for when you had an image format only available in plugins. Which is why there are still images in <object> and <embed> out there. The reason for why we want to load cross origin images loaded in <embed> or <object> is that we want to block an attack vector for spectre et al. By not having to consider <object> and <embed> we can simplify the ORB implementation significantly.

As for providing alternative text, it would be possible to set the alt attribute on the image in the image document somehow I'm sure. But I wonder how this is done for <iframe> today. If you do <iframe src="image.png" title="A title></iframe>, what do you get then? I checked, you get the behavior you describe in comment 8. So this change won't at least make it worse than it already is for <iframe>. And the upside is that if we fix it in ImageDocument.cpp we'll fix it for all cases.

The plan is to land this on nightly only to begin with. Would it be ok with you to have a slightly broken nightly for a11y of embed/object? If so I'll submit a follow-up for fixing this before it rides to beta.

Flags: needinfo?(jteh)

(In reply to Andreas Farre [:farre] from comment #10)

The reason for why we want to load cross origin images loaded in <embed> or <object> is that we want to block an attack vector for spectre et al.

Why is this not an issue for <img>? Is it because the data supplied and allowed in that case is limited, whereas embed/object (by virtue of supporting so many types) have a much greater potential for spectre, etc. attacks?

As for providing alternative text, it would be possible to set the alt attribute on the image in the image document somehow I'm sure.

I guess that would currently involve passing data over IPDL, but yeah, it seems possible. With the new a11y caching architecture we're developing, this would be easier; we could probably handle this entirely in the parent process.

But I wonder how this is done for <iframe> today. If you do <iframe src="image.png" title="A title></iframe>, what do you get then? I checked, you get the behavior you describe in comment 8. So this change won't at least make it worse than it already is for <iframe>.

Ah. Somehow, I didn't realise you could embed images with iframe. Yuck... but you're right, you get the same problem there.

The plan is to land this on nightly only to begin with. Would it be ok with you to have a slightly broken nightly for a11y of embed/object? If so I'll submit a follow-up for fixing this before it rides to beta.

That seems totally reasonable, especially as i doubt this will be a "real" problem in the wild. If an author is actually thinking about a11y, it's not unreasonable that they'd switch to using img instead of embed or object for semantic reasons if nothing else.

Flags: needinfo?(jteh)

(In reply to James Teh [:Jamie] from comment #11)

Why is this not an issue for <img>? Is it because the data supplied and allowed in that case is limited, whereas embed/object (by virtue of supporting so many types) have a much greater potential for spectre, etc. attacks?

For <embed> and <object> it's mainly because they're already container elements, and can have browsing context, like they do whey documents are embedded. And it removes a whole bunch of special casing, and even the actual need to block any responses. For <img> (and other media elements and <script>) we need to look at the responses and block them if needed, we can't isolate them in any other way. If you're interested there's more info here: https://github.com/annevk/orb

That seems totally reasonable, especially as i doubt this will be a "real" problem in the wild. If an author is actually thinking about a11y, it's not unreasonable that they'd switch to using img instead of embed or object for semantic reasons if nothing else.

This is wonderful. I'll file a bug about it.

Blocks: 1778503
Assignee: afarre → sefeng
Severity: -- → S3
Pushed by sefeng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/91d87de145e3
Part 1: Make <embed> and <object> behave more like <iframe>. r=smaug,emilio
https://hg.mozilla.org/integration/autoland/rev/3687b7153c44
Part 2: Adapt test files to <embed> and <object> behaving more like <iframe>. r=smaug

Backed out 2 changesets (bug 1595491) for causing build bustages in docshell/base/BrowsingContext.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/eecfa46043c20f261e0f6e825609fcd658d49ea3

Push with failures

Failure log

/builds/worker/checkouts/gecko/docshell/base/BrowsingContext.cpp:742:26: error: unused variable 'context' [-Werror=unused-variable]
Flags: needinfo?(sefeng)
Flags: needinfo?(sefeng)

Also R2 failure Log <--> R2

Pushed by sefeng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c07b32cdfb47
Part 1: Make <embed> and <object> behave more like <iframe>. r=smaug,emilio
https://hg.mozilla.org/integration/autoland/rev/3c894371eace
Part 2: Adapt test files to <embed> and <object> behaving more like <iframe>. r=smaug
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch
Regressions: 1785227
Regressions: 1785921
Regressions: 1785933
Blocks: 1801664
Whiteboard: [orb:m1] → [orb:m1][sp3]
Whiteboard: [orb:m1][sp3] → [orb:m1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: