Manipulation with Offscreen Canvas allows bypassing tainting restrictions
Categories
(Core :: Graphics: Canvas2D, defect, P1)
Tracking
()
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)
32.96 KB,
application/x-zip-compressed
|
Details | |
3.92 KB,
text/html
|
Details | |
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-beta+
tjr
:
sec-approval+
|
Details | Review |
20.57 KB,
patch
|
dmeehan
:
approval-mozilla-release-
diannaS
:
approval-mozilla-esr115+
|
Details | Diff | Splinter Review |
20.62 KB,
patch
|
diannaS
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
228 bytes,
text/plain
|
Details |
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;
}
Updated•2 years ago
|
Comment 1•2 years ago
|
||
For the avoidance of doubt, does your example work if run in an http/https context, too, or only from file
?
Reporter | ||
Comment 2•2 years ago
|
||
(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•2 years ago
|
||
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.
Updated•2 years ago
|
Comment 4•2 years ago
|
||
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?
Updated•2 years ago
|
Comment 5•2 years ago
|
||
(In reply to :Gijs (he/him) from comment #4)
Created attachment 9334978 [details]
UntaintingFFDemo.htmlThis 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?
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 6•2 years ago
|
||
Assignee | ||
Comment 7•2 years ago
|
||
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
Updated•2 years ago
|
Comment 8•2 years ago
|
||
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.
Updated•2 years ago
|
Assignee | ||
Comment 9•2 years ago
|
||
Okay, I will hold off until June 18th.
Reporter | ||
Comment 10•2 years ago
|
||
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•2 years ago
|
||
(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.
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 12•2 years ago
|
||
(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•2 years ago
|
||
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•2 years ago
|
||
When would you like us to land this in Nightly?
Comment 15•2 years ago
|
||
(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 17•2 years ago
|
||
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.
Updated•2 years ago
|
Comment 19•2 years ago
|
||
Assignee | ||
Comment 20•2 years ago
|
||
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
Assignee | ||
Comment 21•2 years ago
|
||
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•2 years ago
•
|
||
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•2 years ago
|
||
Comment 24•2 years ago
•
|
||
This will need a rebased patch and approval request for ESR102 also.
Comment 25•2 years ago
|
||
Comment on attachment 9336639 [details]
Bug 1833876.
Approved for 116.0b8
Assignee | ||
Comment 26•2 years ago
|
||
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•2 years ago
|
||
uplift |
Updated•2 years ago
|
Assignee | ||
Comment 28•2 years ago
|
||
It appears there are a few more dependencies I need to pull in for 102esr.
Comment 29•2 years ago
|
||
Comment on attachment 9344456 [details] [diff] [review]
Patch for release/esr115
Approved for 115.1esr
Assignee | ||
Comment 30•2 years ago
|
||
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•2 years ago
|
||
uplift |
Updated•2 years ago
|
Comment 32•2 years ago
|
||
Comment on attachment 9344884 [details] [diff] [review]
Patch for esr102
Approved for 102.14esr
Comment 33•2 years ago
|
||
uplift |
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 34•2 years ago
|
||
Updated•1 year ago
|
Updated•1 year ago
|
Comment 35•8 months ago
|
||
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
Description
•