1833876 - (CVE-2023-4045) Manipulation with Offscreen Canvas allows bypassing tainting restrictions

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P1)

Description

[Max Vlasov]

Attachment: The working demos of the Canvas restriction bypassing

The attached demo (A modified example from the demo page https://developer.mozilla.org/en-US/docs/Web/API/Canvas_API/Tutorial/Pixel_manipulation_with_canvas ) tries to overcome the restriction that doesn't allow accessing the canvas pixel data when a locally accessed image was drawn on it.

Steps to reproduce:

• Extract the archive locally (Windows 7 in my case).
• Open file .... main_entry\UntaintingFFDemo.htm
• Click at Grayscale/Invated/Sepia.

What happened:

• the switching works without limitations in Firefox showing the modified images. So the pixels of the image from the local exposed_image folder is accessed by the script.

What expected

• no change in canvas, possible security-related errors in the console related to the security restrictions.

I thought that it was a feature, not a bug, but In Chrome ( Version 109.0.5414.120 (Official Build) (64-bit) ) they are not drawn with the console error

Uncaught DOMException: Failed to execute 'getImageData' on 'OffscreenCanvasRenderingContext2D': The canvas has been tainted by cross-origin data. ... at GetUnrestrictedImageData

The code fragment that "launders" the image is this

function GetUnrestrictedImageData(restrImage)
{
  var img = new Image(restrImage.width, restrImage.height);
  
  var offscreen = new OffscreenCanvas(restrImage.width, restrImage.height);
  var gctx = offscreen.getContext("2d");
  
  gctx.drawImage(restrImage, 0, 0);
  var testImageData = gctx.getImageData(0, 0, offscreen.width, offscreen.height);
  return testImageData;
}

Comment 1

[:Gijs (he/him)]

For the avoidance of doubt, does your example work if run in an http/https context, too, or only from file?

Comment 2

[Max Vlasov]

(In reply to :Gijs (he/him) from comment #1)

For the avoidance of doubt, does your example work if run in an http/https context, too, or only from file?

Short answer: I don't know. Long answer: I'm not sure, I thought that it might affect server logic too. My guess is that for this test one should emulate CORS-related environment possible, so on domain1 some script tries to access images on another domain with trick. Currently I'm not able to set up this quickly.

Comment 3

[:Gijs (he/him)]

I appear to be able to reproduce by using https://bugzilla.mozilla.org/images/index/firefox-beta.svg which doesn't appear to be served with CORS headers, also when serving the testcase using http (from localhost, in my case, but I don't imagine it matters). If I use a regular canvas I get a security error, but for OffscreenCanvas it works. I'll upload a self-contained testcase.

Comment 4

[:Gijs (he/him)]

Attachment: UntaintingFFDemo.html

This is a somewhat altered version to allow easily seeing the difference between offscreen canvas and "normal" canvas.

:jfkthame, do you know who on the graphics team could investigate this?

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to :Gijs (he/him) from comment #4)

Created attachment 9334978 [details]
UntaintingFFDemo.html

This is a somewhat altered version to allow easily seeing the difference between offscreen canvas and "normal" canvas.

:jfkthame, do you know who on the graphics team could investigate this?

Andrew, maybe you could take a look? Or suggest who to redirect to?

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1833876.

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9336639 [details]
Bug 1833876.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It should be clear that we have limitations on our previous tainting checks, and thus very easy to extract data that we should have denied from a canvas.
• 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?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Bug 1443925 is not on ESR102, so it will require simplification of the patch for uplift since we cannot use the nsIPrincipal off the main thread. This just means we fail in a few corner cases, unlikely to be significant.

Bug 1816189 introduced minor conflicts, trivial to resolve, that we will have trouble with when uplifting to release.

• How likely is this patch to cause regressions; how much testing does it need?: It has the potential to cause some compat issues since we now restrict access where we did not before; hopefully Chrome should already have those same restrictions, so content authors should not be relying on it.
• Is Android affected?: Yes

Comment 8

[Tom Ritter [:tjr]]

I think it would be better to hold this until midway through the cycle, it's pretty obvious what the issue is from the patch, and it's also very easy to develop this into an exploit. I'm going to set a reminder with the tag 'disclosure' (because I can't use a 'misc' tag) but that's just to land it then. I'm going add the sec-approval flag but ask you not to land it until then.

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Okay, I will hold off until June 18th.

Comment 10

[Max Vlasov]

As the OP I keep monitoring this page. As of recently auto-updated 115.0b2, the issue with the local variant is still there. Probably this is expected, I might interpret some plans here incorrectly. I suppose that I will see the difference in some upcoming public version as the rest of the public. But If it's 116 then I'll be probably out of luck to check since I'm still on Windows 7 so as the recent news suggested I will be forced to ESR.

Comment 11

[:Gijs (he/him)]

(In reply to Max Vlasov from comment #10)

As the OP I keep monitoring this page. As of recently auto-updated 115.0b2, the issue with the local variant is still there. Probably this is expected, I might interpret some plans here incorrectly. I suppose that I will see the difference in some upcoming public version as the rest of the public. But If it's 116 then I'll be probably out of luck to check since I'm still on Windows 7 so as the recent news suggested I will be forced to ESR.

This isn't on any branches right now. We're holding off landing this to make the patch gap shorter, as the 115 release is still 4 weeks away, but right now the plan is to also uplift to 115 beta.

If you have a local build you could try the patch locally, but that may not work on Windows 7 - I can't tell quickly from reading the docs.

Comment 12

[Donal Meehan [:dmeehan]]

(In reply to Andrew Osmond [:aosmond] (he/him) from comment #9)

Okay, I will hold off until June 18th.

Is this ready land and request beta and esr102 uplift?

Comment 13

[Donal Meehan [:dmeehan]]

Too late for 115 since this never landed
:aosmond RE: Comment 8, this needs to wait until later in the Fx117 cycle before landing and requesting uplift to esr/beta to ship with Fx116

Comment 14

[Jim Mathies [:jimm]]

When would you like us to land this in Nightly?

Comment 15

[Donal Meehan [:dmeehan]]

(In reply to Jim Mathies [:jimm] from comment #14)

When would you like us to land this in Nightly?

The request in Comment 8 was midway through the nightly cycle. Sometime during the week of July 17th?

Comment 16

[Jim Mathies [:jimm]]

FYI, I'll plug something into my cal on this.

Comment 17

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

28 days ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-disclosure 2023-06-18] .

aosmond, please refer to the original comment to better understand the reason for the reminder.

Comment 18

[Jim Mathies [:jimm]]

We will land this this week.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7f910004e86
r=lsalzman

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9336639 [details]
Bug 1833876.

Beta/Release Uplift Approval Request

• User impact if declined: Sec issue, may be able to access OffscreenCanvas data or data via OffscreenCanvas that should not be accessible
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Excellent coverage via WPT to avoid regressions, patch mostly mirrors existing code for main thread HTMLCanvasElement
• String changes made/needed:
• Is Android affected?: Yes

Comment 21

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Patch for release/esr115

ESR/Release Approval Request Comment
[Feature/Bug causing the regression]: Bug 1746750, shipped for Zoom domains only in bug 1751721 (release 99), shipped in general in bug 1779009 (release 105)
[User impact if declined]: Sec issue, may be able to access OffscreenCanvas data or data via OffscreenCanvas that should not be accessible
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Excellent WPT test coverage, mirror existing code for HTMLCanvasElement with OffscreenCanvas
[String changes made/needed]: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 22

[Donal Meehan [:dmeehan]]

Comment on attachment 9344456 [details] [diff] [review]
Patch for release/esr115

We aren't planning any more Fx115 dot releases this cycle. This fix will ship in 116/115.1esr/102.14esr going out on August 1.

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/a7f910004e86

Comment 24

[Ryan VanderMeulen [:RyanVM]]

This will need a rebased patch and approval request for ESR102 also.

Comment 25

[Dianna Smith [:diannaS]]

Comment on attachment 9336639 [details]
Bug 1833876.

Approved for 116.0b8

Comment 26

[Andrew Osmond [:aosmond] (he/him)]

I'll attach a patch for 102esr / request uplift provided my try comes back green:
https://treeherder.mozilla.org/jobs?repo=try&revision=da4cbf4c2e36dfb98cbef094847b6d8edc33ff32

It will also require bug 1779315 as a dependency.

Comment 27

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/77bff5d4ace2

Comment 28

[Andrew Osmond [:aosmond] (he/him)]

It appears there are a few more dependencies I need to pull in for 102esr.

Comment 29

[Dianna Smith [:diannaS]]

Comment on attachment 9344456 [details] [diff] [review]
Patch for release/esr115

Approved for 115.1esr

Comment 30

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Patch for esr102

Green try (you can also pull from this as the patch is identical): https://treeherder.mozilla.org/jobs?repo=try&revision=2a36886747f94d73c367dc7ea809899941634706

ESR Approval Request Comment
[Feature/Bug causing the regression]: Bug 1746750, shipped for Zoom domains only in bug 1751721 (release 99)
[User impact if declined]: Sec issue
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Bug 1779315, Bug 1779521, Bug 1780042
[Is the change risky?]: No
[Why is the change risky/not risky?]: We have very good test coverage.
[String changes made/needed]: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 31

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/282fb45e9af3

Comment 32

[Dianna Smith [:diannaS]]

Comment on attachment 9344884 [details] [diff] [review]
Patch for esr102

Approved for 102.14esr

Comment 33

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr102/rev/67f308759495

Comment 34

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt

Comment 35

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons