1559715 - (CVE-2019-11742) Cross-origin image stealing using SVG filters and canvas

Status: VERIFIED FIXED

(Core :: SVG, defect, P1)

Description

[Paul Stone]

Attachment: proof of concept

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:69.0) Gecko/20100101 Firefox/69.0

Steps to reproduce:

Cross-origin images introduce a 'taint' into things like canvas and SVG filters to prevent image stealing. However, images in SVG load slightly differently than normal <img> elements. Images in SVG documents will display from the cache first, then re-display after revalidating with the server. It seems that the taint isn't introduced until the server has responded (e.g. with a 304 Not Modified). Before this happens, it's possible to use the image in ways that would normally fail or cause a security exception. Normal HTML images don't seem to behave in this way - they don't display at all until they've been revalidated.

The attached testcase has an SVG filter that uses feImage to load a cross-origin image. It than uses CanvasRenderingContext2D.filter to apply the filter to a canvas and toDataURI to copy the image data. On first load toDataURI call causes an error because the image isn't cached. If you refresh the page, toDataURI works and gets a copy of the cross-origin image, due to the tainting delay.

Actual results:

Can use a cross-origin image from SVG without triggering tainting

Expected results:

Taint should apply even when image is displayed from cache, before revalidation

Comment 1

[Paul Stone]

The attached testcase doesn't seem to load properly for me - it goes into a weird redirect loop. The same testcase is at http://dev.jigawatt.co.uk/dev/svgimgsteal/ which seems to work ok.

Comment 2

[Paul Stone]

To clarify, it looks like this only applies to feImage, not normal SVG images. SVGFEImageElement::OutputIsTainted returns false if the load isn't complete yet, even though the cached image data is available to the filter.

Comment 3

[Andrew McCreight [:mccr8]]

Daniel, maybe you could do some initial triage on this? Thanks.

Comment 4

[Daniel Holbert [:dholbert]]

The PoC looks valid & concerning to me.

Comment 5

[Frederik Braun [:freddy]]

Prematurely assigning to baku, who has worked on the other taing flag propagation bugs we've had in the past months.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Paul Stone from comment #1)

The attached testcase doesn't seem to load properly for me - it goes into a weird redirect loop.

Same for me -- specifically, the first load works, but subsequent (re)loads trigger a redirect-failure error page. This is a recent bugzilla regression, and :dylan is now aware & hopefully will fix it before long.

For the time being, the bugzilla attachment isn't directly usable as a PoC, but it reproduces the bug if you download it and run it locally (or use the version from comment 1).

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 9072463 [details]
proof of concept

(I filed bug 1560405 on the bugzilla redirect issue, BTW.)

Comment 8

[Daniel Holbert [:dholbert]]

I can't reproduce this bug in release, so this seems to be a regression. I'm working on tracking down a regression range now.
[EDIT: actually I can reproduce in release 67, but it's just much flakier/harder-to-trigger than in Nightly; more in next comment]

Comment 9

[Daniel Holbert [:dholbert]]

Hmm, actually I suspect this didn't so much "regress", but rather just started reproducing reliably instead of infrequently.

"Regression range" from mozregression, using a locally-saved version of the attached PoC:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=74b2c66643c90b1a7f161136bdf0ce8fa1178396&tochange=87b426177fadd80991f6234939daeb80f9b17130
--> bug 1525356

Before that range, this reproduces very infrequently -- most of the time the top and bottom of the testcase are blank, and only the occasional reload will have the image at top + bottom.

After that range, this reproduces 100% reliably on the first reload.

(So: I'm guessing this wasn't so much a "regression" but rather a race condition being fixed, which happens to make this bug easier to trigger.)

Comment 10

[Daniel Holbert [:dholbert]]

We probably need to update this chunk of code (which is invoked when we set the canvas's filter):
https://searchfox.org/mozilla-central/rev/da14c413ef663eb1ba246799e94a240f81c42488/dom/canvas/CanvasRenderingContext2D.cpp#2382-2392
...to analyze the filter and see if there's an feImage (or more specifically, a cross-origin feImage).

We do have a bool there "sourceGraphicIsTainted", but that doesn't get set at all based on the filter. We should perhaps make that an outparam and do some filter analysis to adjust that boolean, to reflect the fact that the filter itself may be tainted (not just the source graphic).

Comment 11

[Daniel Holbert [:dholbert]]

(i.e. the fix here may want to go inside of nsFilterInstance::GetFilterDescription, perhaps by making its aFilterInputIsTainted arg into an in/outparam and updating it if we find a feImage)

Comment 12

[Daniel Holbert [:dholbert]]

Anyway: I'll step back at this point since :baku's assigned -- hopefully comment 10-11 gives some ideas here.

baku, if you can't get to this, then jwatt or longsonr or I could take a look instead. Or possibly mstange, who worked on bug 1307740 which added a previous incarnation of this code including some of the taintedness stuff in the context here.

Comment 13

[Andrea Marchesini [:baku]]

I suspect that the fix should be done at an imgRequest cache level. It's important to cache if the image had cross-origin redirects.
At the moment I don't know when I can work on this bug. Maybe next week, but I'm not 100% sure.
If somebody else wants to take this bug, it would be great. Let me NI aosmond.

Comment 14

[Jonathan Watt [:jwatt]]

i'm about to go on two week's PTO so I'm not going to get to this either. Also needinfo'ing dholbert so he's aware.

Comment 15

[Markus Stange [:mstange]]

(In reply to Daniel Holbert [:dholbert] from comment #10)

We do have a bool there "sourceGraphicIsTainted", but that doesn't get set at all based on the filter. We should perhaps make that an outparam and do some filter analysis to adjust that boolean, to reflect the fact that the filter itself may be tainted (not just the source graphic).

A mechanism for looking at the filter primitives already exists, and comment 2 pointed it out correctly: The fix probably needs to go into SVGFEImageElement::OutputIsTainted. More specifically, we need to find out which of the return false branches is taken and do something about still-loading cross-origin images.

Comment 16

[Markus Stange [:mstange]]

The code that propagates filter taintedness back to the canvas is after the call to RenderFilterDescription.

Comment 17

[Robert Longson [:longsonr]]

Looks like currentRequest->GetImageStatus(&status) is returning a status of 0. mIsValidating is true so we end up in here: https://searchfox.org/mozilla-central/source/image/imgRequestProxy.cpp#712, returning imgIRequest::STATUS_NONE

That suggests that the caller (canvas) is calling too early somehow, i.e. before the filter has completed but after it has started painting. If we changed the logic to return true at that point it would always fail which seems wrong too.

Comment 18

[Robert Longson [:longsonr]]

the caller javascript should wait (if it is to work properly), so I guess https://searchfox.org/mozilla-central/rev/0671407b7b9e3ec1ba96676758b33316f26887a4/dom/svg/SVGFEImageElement.cpp#254 should simply return true if the load is not complete rather than false if it is.

Comment 19

[Paul Stone]

re: comment 18, returning true while a cached image is revalidating would presumably cause a same-origin image to taint the canvas. Properly written JavaScript should wait until the image has loaded, but if you don't you'll get weird intermittent behaviour where the canvas is sometimes tainted for no obvious reason.

A fix for this bug should either:

• prevent a cached image from being painted at all (I think this is what normal HTML/SVG images do, but I need to check)
• or, check the origin of the cached image (but I suspect this would be tricky to do correctly)

Comment 20

[Paul Stone]

I double-checked the behaviour of cached <img> and SVG <image> while they're revalidating:

• <img> will not display a cached copy at all, until revalidated
• SVG <image> will display while revalidating, but won't draw to canvas until revalidated

Comment 21

[Robert Longson [:longsonr]]

On reflection that call should be removed altogether.

Comment 22

[Robert Longson [:longsonr]]

Attachment: 1559715.txt

Comment 23

[Robert Longson [:longsonr]]

If we agree this is the way forward I'll convert it to phabricator.

Comment 24

[Robert Longson [:longsonr]]

Attachment: Bug 1559715 r=dholbert

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 9072463 [details]
proof of concept

BTW the bugzilla-hosted testcase now reproduces the issue just fine (because bug 1560405 in bugzilla itself has been fixed).

Comment 26

[Robert Longson [:longsonr]]

Comment on attachment 9075889 [details]
Bug 1559715 r=dholbert

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Seems fairly straightforward, we can infer we need an FEImage filter and that somehow canvas taintedness is involved as is whether we think we've painted something. If you wanted to have an example of a change that couldn't avoid painting a bullseye on what to do to exploit it, this is it. There's not much I can do with a check-in comment to hide it and there's no tests landing in this patch.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: Bug 941887
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: This patch should work everywhere. And we're going to need to land it everywhere too.
• How likely is this patch to cause regressions; how much testing does it need?: Really simple, should be safe.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

I think this needs to land late in the cycle (which just began). I'm going to give Sec-approval+ for landing on August 9 (yes, a month) since it seems like this will be pretty obvious once we check in. We'll want it on beta as well (and ESR60) at that time.

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to Al Billings [:abillings] from comment #27)

I think this needs to land late in the cycle (which just began). I'm going to give Sec-approval+ for landing on August 9 (yes, a month)

Thanks, Al. I can take an action to make sure we land & request uplift on August 9th. I added a calendar-event to ping me and jwatt to remind us.

We'll want it on beta as well (and ESR60) at that time.

Yup. And also ESR68.

Comment 29

[Frederik Braun [:freddy]]

Paul, thanks again for reporting this!
We expect this specific issue to be resolved when Firefox 69 is released.

As an open source project, we assume that people monitor our check-ins. This is why we only land security patches shortly before the end of a release cycle.
Please see https://wiki.mozilla.org/Security/Bug_Approval_Process if you want to know more about how we fix security bugs.

Comment 30

[Boris Zbarsky [:bzbarsky]]

<img> will not display a cached copy at all, until revalidated

Fwiw, it certainly used to. Maybe the behavior changed at some point...

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:longsonr, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 32

[Frederik Braun [:freddy]]

This will need about 9 more days before it can land as mentioned in the sec-approval in comment 27.

Comment 33

[Daniel Holbert [:dholbert]]

Comment on attachment 9075889 [details]
Bug 1559715 r=dholbert

Beta/Release Uplift Approval Request

• User impact if declined: Information disclosure. (cross-origin image reading)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Load https://bugzilla.mozilla.org/attachment.cgi?id=9072463 or http://dev.jigawatt.co.uk/dev/svgimgsteal/

2. Refresh the page a few times.

BUGGY RESULTS: After you've refreshed, two copies of the image show up (one under "Canvas" and one under "data URI").
EXPECTED RESULTS:

• Only the upper image should show up. Nothing should appear under "data URI".
• In Web Console (ctrl+shift+K), you should see an entry saying something like SecurityError: The operation is insecure with a link to line 23 of the testcase.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It simply removes an early permissive return for not-yet-loaded images during our "should this image cause the canvas to be tainted (unreadable-by-the-page)" security checklist. After the patch, we'll now perform the same checks on images regardless of whether they're loaded or not, which seems intuitively correct and strictly safer than what we were doing before.
• String changes made/needed: None.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is sec:high.
• User impact if declined: Information disclosure. (cross-origin image reading)
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It simply removes an early permissive return for not-yet-loaded images during our "should this image cause the canvas to be tainted (unreadable-by-the-page)" security checklist. After the patch, we'll now perform the same checks on images regardless of whether they're loaded or not, which seems intuitively correct and strictly safer than what we were doing before.
• String or UUID changes made by this patch: None.

Comment 34

[Daniel Holbert [:dholbert]]

Is this code covered by automated tests?: Yes

(Note: I'm referring to the existing automated tests added in e.g. bug 1307740. This patch doesn't include additional automated tests.)

Has the fix been verified in Nightly?: No

This is "no" because it hasn't landed in Nightly yet -- see comment 27. However, I have verified the fix in a local build.

Comment 35

[Daniel Holbert [:dholbert]]

(note: as shown in my current bugzilla nickname, I'll be on vacation for the next few days, but I'm planning on being available for some chunk of Friday morning [Aug 9th, the requested landing-day] and landing/uplifting this during that time-window.)

Comment 36

[Daniel Holbert [:dholbert]]

Al: it's the landing day but it seems we still need beta/ESR approvals. (Probably should've requested them earlier; I forgot that separate approval would be needed until a few days ago.)

How should I proceed?

(Should I go ahead and land on Nightly to get this baking/tested there sooner rather than later, and assume branch uplift approvals are coming within a few days, and land on branches when those approvals arrive?)

Comment 37

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] (mostly AFK Aug 8th-12th) from comment #36)

(Should I go ahead and land on Nightly to get this baking/tested there sooner rather than later

mccr8 confirms in IRC that m-c landing doesn't need to be gated on uplift approvals for situations like this, so I'll proceed with a m-c landing.

Comment 38

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/autoland/rev/8c8c8cc13b27c828124880b7726109f4aee3aeef

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8c8c8cc13b27

Comment 40

[Daniel Bodea [:danibodea]]

I can confirm the fix in Nightly v70.0a1 from 2019-08-12 and reproduced in Beta v69.0b12 with STR in the uplift request. Waiting for uplifts.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9075889 [details]
Bug 1559715 r=dholbert

Approved for 69.0b13, 68.1esr, and 60.9esr.

Comment 42

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a9f7c1cd4c0

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/5b8102992d77

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/656ab502f280c80d473daa224c1ae014b15a4044

Comment 45

[Daniel Bodea [:danibodea]]

I have verified the fix in Beta v69.0b13 and ESR v68.1.0esr. Can't find a build for ESR60. Will verify later on.

Comment 46

[Daniel Bodea [:danibodea]]

I have also verified in ESR60 v60.9.0esr (64-bit); build ID: 20190812200839 on Windows 10.