Closed Bug 1497715 Opened 6 years ago Closed 6 years ago

Figure if/how SVG-as-an-image should live in a separate process for Fission

Categories

(Core :: Graphics: ImageLib, enhancement, P3)

Product:
Core
Component:
Graphics: ImageLib
Type:
enhancement

Tracking

()

Status:
RESOLVED WONTFIX
Tracking Flags:
Tracking Status
firefox64 --- affected

People

(Reporter: dholbert, Unassigned)

References

(Blocks 1 open bug)

Details

Authors can reference cross-origin SVG files via <img> and e.g. as CSS backgrounds. This will end up spawning a SVG document that imagelib wraps (via VectorImage / SVGDocumentWrapper in imagelib). I imagine this cross-origin document rendering will need some special consideration/handling for Fission.
Do we support scripting inside of SVG as an <img>? If we don't does that still mean we have work to do with this for Fission? Or at least a Fission MVP.
(In reply to Ryan Hunt [:rhunt] from comment #1) > Do we support scripting inside of SVG as an <img>? No. We support SVG/CSS animations, but no scripting. > If we don't does that still mean we have work to do with this for Fission? > Or at least a Fission MVP. (I don't know the answer to this.)
One attack that's worth considering here (which might help answer comment 1): consider an untrusted page, which references a cross-origin private SVG file as an image. e.g.: > == somewhere on evil.com ==: > <img src="https://your-silly-bank.com/svg-that-contains-your-balance.svg"> If an attacker can get a user to visit evil.com, and if that SVG image's internal document ends up being stored/rendered in the same content process that is rendering evil.com, then one can imagine there being JS on evil.com that can use some sneaky same-process memory-stealing technique to learn about the contents of that purportedly-private SVG image.
(In reply to Daniel Holbert [:dholbert] from comment #3) > One attack that's worth considering here (which might help answer comment > 1): consider an untrusted page, which references a cross-origin private SVG > file as an image. e.g.: > > > == somewhere on evil.com ==: > > <img src="https://your-silly-bank.com/svg-that-contains-your-balance.svg"> > > If an attacker can get a user to visit evil.com, and if that SVG image's > internal document ends up being stored/rendered in the same content process > that is rendering evil.com, then one can imagine there being JS on evil.com > that can use some sneaky same-process memory-stealing technique to learn > about the contents of that purportedly-private SVG image. That's true, I hadn't considered that. I think one difficulty to moving cross origin SVG-as-an-image out of process is that you can use drawImage to render that SVG to a canvas. The canvas will be tainted and we won't allow JS to readback the pixel data, but that pixel data currently lives in the same process as the canvas. Which would be vulnerable to an attacker still. Not sure what our options are here. Move the canvas pixel data OOP as well and broker requests to get the raw pixels?
Good point. This seems like a general concern for all cross-origin images (not just SVG) -- it seems like ideally the canvas-pixel-data for any "tainted" canvas should be stored OOP, for maximal protection. That probably wants its own bug hanging off of bug 1496596. But the reason I filed *this* bug was that I was slightly worried we might have assertions for e.g.... "All documents that have $ORIGIN should be rendered in DedicatedProcessFor($ORIGIN)" ...any such assertions might not hold up for imagelib-internal documents, at least not right away. (Though it's conceivable we might want to make them hold up.)
We do not intend to protect images from a.com from being loaded into a content process for b.com. Nor SVGs. For more check out CORB (Bug 1459357). The short version is we want to prevent documents (HTML, JSON, XML) from a.com being loaded into b.com's process. (We'll also protect pdfs, but they inherit that protection from being included via an <embed> context that puts them in a separate process.) The exception to this is if an image (or other resource, such as JS, CSS, etc) is protected with a new CORP header (Bug 1459573). But we haven't implemented CORP yet. The other potential exception to this is if there is something special about how complicated SVGs are. But since we don't allow scripting via them, I suspect the answer is no.

Daniel, Ryan, so is this WONTFIX?

And how about comment 5, do we need a separate bug for storing canvas-pixel-data for any tainted canvas OOP, or is that also not something we plan to do?

Flags: needinfo?(rhunt)
Flags: needinfo?(dholbert)
Summary: Figure out how to handle SVG-as-an-image with fission → Figure if/how SVG-as-an-image should live in a separate process for Fission

I guess other than the security issues another reason that we may want images (SVG or otherwise) to be OOP is to share memory as part of the MemShrink effort. But then we probably want to gather telemetry or other evidence to show whether that would result in a significant enough win to be worth the effort. And probably that should be considered in a separate bug to avoid clutter with the security related discussion here.

(In reply to Jonathan Watt [:jwatt] from comment #8)

I guess other than the security issues another reason that we may want
images (SVG or otherwise) to be OOP is to share memory as part of the
MemShrink effort. But then we probably want to gather telemetry or other
evidence to show whether that would result in a significant enough win to be
worth the effort. And probably that should be considered in a separate bug
to avoid clutter with the security related discussion here.

It might be worth doing, but I think this bug can be closed as WONTFIX.

Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(rhunt)
Resolution: --- → WONTFIX

I filed bug 1531568 on comment 5 (specifically for the tainted-canvas scenario), FWIW.

Flags: needinfo?(dholbert)
You need to log in before you can comment on or make changes to this bug.