Closed Bug 1540221 (CVE-2019-9817) Opened 2 years ago Closed 2 years ago

Security: Cross-origin theft of images in fillText and CanvasPattern

Categories

(Core :: Canvas: 2D, defect)

68 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla68
Tracking Status
firefox-esr60 67+ verified
firefox66 --- wontfix
firefox67 + verified
firefox68 + verified

People

(Reporter: manhluat93.php, Assigned: baku, NeedInfo)

References

Details

(Keywords: csectype-sop, sec-high, Whiteboard: [adv-main67+][adv-esr60.7+])

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.121 Safari/537.36

Steps to reproduce:

  1. Navigate to https://cyberjutsu.io/files/cross_origin_fillText.html
  2. You can see the Google logo has been stolen, then print it out as the textarea value

Tested on latest Stable && Nightly (Windows 10)

Actual results:

At https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp?redirect_type=single#3527


      if (state->gradientStyles[style]) {  // Gradient
        pattern = GetGradientFor(style);
      } else if (state->patternStyles[style]) {  // Pattern
        pattern = GetPatternFor(style); 
      } else {
        MOZ_ASSERT(false, "Should never reach here.");
        return;
      }
      MOZ_ASSERT(pattern, "No valid pattern.");

      if (style == Style::FILL) {
        params.context->SetPattern(pattern);
      } else {
        params.textStrokePattern = pattern;
      }

it's possible to draw a canvaspattern (which can be loaded with an image) as background of a text.

Due to lacking of checking mWriteonly/Principal/CORs later on, we can extract the whole canvas by calling .toDataURL() for example.

    ctx.font = "10000px Arial"; // <-- make the dot "." to big, to leak the whole image. we can fix it dynamically depends on image's size.
    ctx.fillStyle = pattern; 
    ctx.fillText(".", 0, 200);

Expected results:

canvas.toDataURL() should not be invoked successfully due to invalid CORs.

Group: firefox-core-security → gfx-core-security
Component: Untriaged → Canvas: 2D
Product: Firefox → Core

The patch should be trivial.

We can call |CanvasUtils::DoDrawImageSecurityCheck| once the fillstyle is a canvaspattern.

For example:
https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#286

confirmed :(
Baku, can you take a look?

Assignee: nobody → amarchesini
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: sec-bounty?
Keywords: sec-high

This is a 'known' issue. We have web-platform tests, (disabled of course) here:
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/html/semantics/embedded-content/the-canvas-element

I have a patch to fix them all. Would be really great if somebody can help me checking if we have covered all the possible scenarios. We don't have enough tests and the few we have, if WPT, are disabled.

Hi folks,

I don't think those tests are covering my case.

Since the bug is placed in |Canvas2D::fillText| method
which invoking the actual vulnerable function |DrawText|

( https://dxr.mozilla.org/mozilla-central/rev/3ebf6f2a81387d7087ef235c3ce60452baf550f3/dom/canvas/CanvasRenderingContext2D.cpp#3527,3580,3588,3590,3616 )

The bug exists in this function only. Other places (which the test-cases already reach) are not vulnerable to this.

To confirm,

https://github.com/servo/servo/search?q=path%3Atests%2Fwpt%2Fweb-platform-tests%2Fhtml%2Fsemantics%2Fembedded-content%2Fthe-canvas-element%2F+fillText&unscoped_q=path%3Atests%2Fwpt%2Fweb-platform-tests%2Fhtml%2Fsemantics%2Fembedded-content%2Fthe-canvas-element%2F+filltext

No match found.

It means the test-cases won't recognize this vulnerability.

Comment on attachment 9055137 [details]
Bug 1540221 - Setting fillStyle to a pattern of an unclean canvas makes the canvas origin-unclean, r=aosmond

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Impossible. Here we are exposing images which should be protected by cross-origin.
  • 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?:
  • 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?: Easy. I hope.
  • How likely is this patch to cause regressions; how much testing does it need?: There are good tests. I think this patch should not introduce regressions.
Attachment #9055137 - Flags: sec-approval?

(In reply to Andrea Marchesini [:baku] from comment #7)

Comment on attachment 9055137 [details]
Bug 1540221 - Setting fillStyle to a pattern of an unclean canvas makes the canvas origin-unclean, r?aosmond

Security Approval Request

  • Which older supported branches are affected by this flaw?:

Which older branches are affected?

Flags: needinfo?(amarchesini)

Which older branches are affected?

All. this is not a regression. This was a missing feature. There were WPTs too, disabled since ever.

Flags: needinfo?(amarchesini)

sec-approval+ for trunk. We'll want this on beta and ESR60 as well.

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

That got backed out for WPT failures in /html/semantics/embedded-content/the-canvas-element/security.pattern.fillStyle.sub.html

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

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&tochange=6f0b0ab56a478e99853a465ceaa7a5d8c7e2b1b8&fromchange=aa0feb50773a391760802da8121742653321d1ce&selectedJob=239548783
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=239548783&repo=autoland

TEST-UNEXPECTED-FAIL | /html/semantics/embedded-content/the-canvas-element/security.pattern.fillStyle.sub.html | redirected to same-origin HTMLVideoElement: Setting fillStyle to an origin-unclear pattern makes the canvas origin-unclean - assert_throws: function "function() { canvas.toDataURL(); }" did not throw

Sorry for the late notice.

Flags: needinfo?(amarchesini)

Right. I forgot to submit security.pattern.fillStyle.sub.html.ini. As any other 'HTMLVideoElement redirect' test, this one fails too. See: https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/testing/web-platform/meta/2dcontext/imagebitmap/createImageBitmap-origin.sub.html.ini

Flags: needinfo?(amarchesini)
Attachment #9055137 - Attachment description: Bug 1540221 - Setting fillStyle to a pattern of an unclean canvas makes the canvas origin-unclean, r?aosmond → Bug 1540221 - Setting fillStyle to a pattern of an unclean canvas makes the canvas origin-unclean, r=aosmond
Regressions: 1544785

Looks like this was successfully landed on the 16th but never got resolved :
https://hg.mozilla.org/mozilla-central/rev/3fd42eb67a3f

Andrea, please request Beta/ESR60 approval on this when you get a chance. It grafts cleanly to Beta as-landed but needs a rebased patch for ESR60.

Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(amarchesini)
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Flags: sec-bounty? → sec-bounty+

(In reply to Andrea Marchesini [:baku] from comment #4)

This is a 'known' issue. We have web-platform tests, (disabled of course) here:
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/html/semantics/embedded-content/the-canvas-element

Did we file a security bug when we turned these off?

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

Comment on attachment 9055137 [details]
Bug 1540221 - Setting fillStyle to a pattern of an unclean canvas makes the canvas origin-unclean, r=aosmond

Beta/Release Uplift Approval Request

  • User impact if declined: cross origin images can be read using canvas objects.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • 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): This patch seems stable in nightly and it has good tests. I think this is a low risk patch to be uplift.
  • String changes made/needed: none
Flags: needinfo?(amarchesini)
Attachment #9055137 - Flags: approval-mozilla-beta?

(In reply to Daniel Veditz [:dveditz] from comment #15)

(In reply to Andrea Marchesini [:baku] from comment #4)

This is a 'known' issue. We have web-platform tests, (disabled of course) here:
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/html/semantics/embedded-content/the-canvas-element

Did we file a security bug when we turned these off?

No. There are several problems with requiring/expecting that:

  • I (and others on my team) who are responsible for maintining the sync with upstream don't have the correct expertise or appropriate resources to identify which wpt failures may represent security issues. Even in "obvious" cases (like if the test is evidently intended to check SOP correctness) there are many reasons for failure other than a security issue.

  • Most tests aren't disabled if they fail. Typically disabling is only used in cases where the test is too unstable to run in CI. In the case that the test is marked as expected:FAIL there is generally no review whatsoever, and these are just as likely to represent security issues as other tests.

The way that we need to deal with this is for platform teams to actively monitor the wpt tests that fail (or otherwise aren't passing) in their components. https://jgraham.github.io/wptdash is intended to help with this, and the intent is that we will be able to add annotations to the test results linking them with bugs, so teams are able to triage the Firefox failures (or at least the Firefox-only failures) and so keep the number of untriaged issues to zero, just like we do for incoming bug reports. As a side effect that should allow us to identify potential security issues faster.

So I fully agree that we should be more actively looking at the results of these tests, not only for security bugs, and there are efforts ongoing to make that more workable for platform teams.

Flags: needinfo?(james)

Comment on attachment 9055137 [details]
Bug 1540221 - Setting fillStyle to a pattern of an unclean canvas makes the canvas origin-unclean, r=aosmond

Looks minmal, uplift approved for 67 beta 14, thanks.

Attachment #9055137 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: gfx-core-security → core-security-release
QA Whiteboard: [qa-triaged]

Hi, I tried reproducing this issue but the Steps were a bit confusing, and the Results even more so.

In an older version of Firefox I can see 2 images plus the Text for the Leaked image.
In newer versions of Firefox, where the fix is supposed to be I only see 1 Google logo and no text in the Leaked section.

However on Chrome I do not see any images or text in the leak section.

What are the correct Expected Results ? can someone help us out with that ?

Flags: needinfo?(manhluat93.php)
Flags: needinfo?(amarchesini)

Chrome implementation seems to be broken. The spec says that we should not expose the content, but it can still be shown.

Flags: needinfo?(amarchesini)

So the expected results should be to display 1 Google image but no code in the leak section right ??

Flags: needinfo?(amarchesini)

(In reply to Rares Doghi from comment #22)

So the expected results should be to display 1 Google image but no code in the leak section right ??

4.12.5.6 Security with canvas elements
https://html.spec.whatwg.org/multipage/canvas.html#security-with-canvas-elements

"Information leakage can occur if scripts from one origin can access information (e.g. read pixels) from images from another origin (one that isn't the same).

To mitigate this, bitmaps used with canvas elements and ImageBitmap objects are defined to have a flag indicating whether they are origin-clean. All bitmaps start with their origin-clean set to true. The flag is set to false when cross-origin images are used.

The toDataURL(), toBlob(), and getImageData() methods check the flag and will throw a "SecurityError" DOMException rather than leak cross-origin data."

This has nothing to do with the displaying of the canvas content.

Flags: needinfo?(amarchesini)

Do you think this is a safe uplift for ESR as well?

Flags: needinfo?(amarchesini)

This test is covered by several tests and it has not introduced any regressions in nightly.
The code is relatively easy, so yes, I think it's OK to uplift it.

Flags: needinfo?(amarchesini)

The changes fail to apply on ESR60, conflicts with at least bug 1500768. Please provide patches for ESR.

Flags: needinfo?(amarchesini)

I don't think we need to uplift to esr60 because there, we don't have clean-origin at all. Fixing this would require the uplift bug 1502799 too.

Flags: needinfo?(amarchesini)

The testcase from #c0 reproduces on ESR60 - what can we do to fix this bug there?

Flags: needinfo?(amarchesini)

Uplifting this patch would require bug 1502799 (which might include bug 1526218 and bug 1528909)

Flags: needinfo?(amarchesini)

Dan, do you have an opinion here? We've fixed this for 67 but the work seems a bit complex for esr uplift. Do you think it is OK here to wait for esr68 or should we go for an uplift here and in the 3 bugs mentioned in comment 29?

Flags: needinfo?(dveditz)

[Tracking Requested - why for this release]:

The two regressions in comment 29 are small/safe-looking patches, and this patch isn't too bad, so the question is how risky is the patch in bug 1502799 to back-port? It's longish but looks pretty straightforward (a lot of passing a new argument around).

Given how easy this attack is to pull off once the bug is known I don't see how we could skip fixing a sec-high bug on ESR-60.

Flags: needinfo?(dveditz)

baku, given comment 32, could you put together patches and request uplift for esr60 on whatever would be needed?

Flags: needinfo?(amarchesini)
Whiteboard: [adv-main67+]

Liz, do you want 1 single big patch or you are OK with them to be separate? I can rebase them in case they don't apply to esr60.

Flags: needinfo?(amarchesini)
Alias: CVE-2019-9817

Hi, I have retested this issue in Beta 67, Esr 60.7.0 as well as Nightly 68.0a1 (2019-05-16) and after comparing to release 66 I can confirm that this issue is Verified as fixed.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Whiteboard: [adv-main67+] → [adv-main67+][adv-esr60.7+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.