Closed Bug 1833876 (CVE-2023-4045) Opened 2 years ago Closed 2 years ago

Manipulation with Offscreen Canvas allows bypassing tainting restrictions

Categories

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

Firefox 114
x86_64
Unspecified
defect

Tracking

()

RESOLVED FIXED
117 Branch
Tracking Status
firefox-esr102 116+ fixed
firefox-esr115 116+ fixed
firefox113 --- wontfix
firefox114 --- wontfix
firefox115 --- wontfix
firefox116 + fixed
firefox117 + fixed

People

(Reporter: max.vlasov, Assigned: aosmond)

Details

(Keywords: csectype-sop, reporter-external, sec-high, Whiteboard: [adv-main116+][adv-ESR102.14+][adv-ESR115.1+])

Attachments

(6 files)

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;
}
Group: firefox-core-security → gfx-core-security
Component: Security → Graphics: Canvas2D
Product: Firefox → Core

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

Flags: needinfo?(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.

Flags: needinfo?(max.vlasov)

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Manipulation with Offscreen Canvas allows bypassing restrictions for locally accessed images → Manipulation with Offscreen Canvas allows bypassing restrictions
Summary: Manipulation with Offscreen Canvas allows bypassing restrictions → Manipulation with Offscreen Canvas allows bypassing tainting restrictions
Attached file 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?

Flags: needinfo?(jfkthame)
Keywords: csectype-sop

(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?

Flags: needinfo?(jfkthame) → needinfo?(aosmond)
Keywords: sec-high
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Flags: needinfo?(aosmond)
Attached file Bug 1833876.

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
Attachment #9336639 - Flags: sec-approval?

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.

Whiteboard: [reminder-disclosure 2023-06-18]
Attachment #9336639 - Flags: sec-approval? → sec-approval+

Okay, I will hold off until June 18th.

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.

(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.

Severity: -- → S1
Priority: -- → P1
Severity: S1 → S2
Priority: P1 → --
Priority: -- → P1

(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?

Flags: needinfo?(aosmond)

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

When would you like us to land this in Nightly?

Flags: needinfo?(aosmond) → needinfo?(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?

Flags: needinfo?(dmeehan)

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

Flags: needinfo?(aosmond)

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.

Flags: needinfo?(aosmond)
Whiteboard: [reminder-disclosure 2023-06-18]

We will land this this week.

Flags: needinfo?(aosmond)

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
Attachment #9336639 - Flags: approval-mozilla-beta?

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.

Attachment #9344456 - Flags: approval-mozilla-release?
Attachment #9344456 - Flags: approval-mozilla-esr115?

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.

Attachment #9344456 - Flags: approval-mozilla-release? → approval-mozilla-release-
Group: gfx-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch

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

Flags: needinfo?(aosmond)

Comment on attachment 9336639 [details]
Bug 1833876.

Approved for 116.0b8

Attachment #9336639 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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.

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

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

Approved for 115.1esr

Attachment #9344456 - Flags: approval-mozilla-esr115? → approval-mozilla-esr115+
Attached patch Patch for esr102Splinter Review

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.

Flags: needinfo?(aosmond)

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

Approved for 102.14esr

Attachment #9344884 - Flags: approval-mozilla-beta+
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Whiteboard: [adv-main116+][adv-ESR102.14+][adv-ESR115.1+]
Group: core-security-release
Alias: CVE-2023-4045

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

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: