1540221 - (CVE-2019-9817) Security: Cross-origin theft of images in fillText and CanvasPattern

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Luat Nguyen]

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.

Comment 1

[Luat Nguyen]

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

Comment 2

[Luat Nguyen]

Attachment: cross_origin_fillText.html

Comment 3

[Frederik Braun [:freddy]]

confirmed :(
Baku, can you take a look?

Comment 4

[Andrea Marchesini [:baku]]

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.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: Bug 1540221 - Setting fillStyle to a pattern of an unclean canvas makes the canvas origin-unclean, r=aosmond

Comment 6

[Luat Nguyen]

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 7

[Andrea Marchesini [:baku]]

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.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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

Comment 9

[Andrea Marchesini [:baku]]

Which older branches are affected?

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

Comment 10

[Al Billings [:abillings - ex-MoCo]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/aa0feb50773a

Comment 12

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

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.

Comment 13

[Andrea Marchesini [:baku]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 15

[Daniel Veditz [:dveditz]]

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

Comment 16

[Andrea Marchesini [:baku]]

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

Comment 17

[James Graham [:jgraham]]

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

Comment 18

[Pascal Chevrel:pascalc]

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.

Comment 19

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

https://hg.mozilla.org/releases/mozilla-beta/rev/3a1b53518d5f

Comment 20

[Rares Doghi, Desktop QA]

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 ?

Comment 21

[Andrea Marchesini [:baku]]

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

Comment 22

[Rares Doghi, Desktop QA]

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

Comment 23

[Andrea Marchesini [:baku]]

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

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 25

[Andrea Marchesini [:baku]]

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.

Comment 26

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

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

Comment 27

[Andrea Marchesini [:baku]]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Frederik Braun [:freddy]]

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

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfixing for esr60, in that case.

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 32

[Daniel Veditz [: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.

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 34

[Andrea Marchesini [:baku]]

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.

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

Separate is fine!

Comment 36

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

https://hg.mozilla.org/releases/mozilla-esr60/rev/ebd350549d83

Comment 37

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

Backed out for bustage (more info in bug 1502799):

https://hg.mozilla.org/releases/mozilla-esr60/rev/9d3b9a48a91710e8bf4eb36c5181e391c7c14abe

Comment 38

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

https://hg.mozilla.org/releases/mozilla-esr60/rev/64ee63facd4ff96b3e8590cff559d7e97ac6b061

Comment 39

[Rares Doghi, Desktop QA]

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.