1625404 - (CVE-2020-6821) CopyTexSubImage leaves unwritten destination pixels as uninitialized data instead of lazy-initializing

Status: VERIFIED FIXED

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

Description

[Kelsey Gilbert [:jgilbert]]

This the security-bug version of bug 1624451. Unsure if we want to tip our hand and mark that one as a sec bug, since it started public.

webgl.copyTexSubImage supports reading from areas partially (or fully) outside the source resource. Since we would get undefined values (sec issue!) if we read out-of-bounds like this, we find the common subrect between the source resource size and the selected read rect. The WebGL spec defines out-of-bounds pixel reads as if they read zeros.

However, in this case, and with a destination texture image/slice which has never been initialized, and currently contains uninitialized contents[1], we neglect to initialize (oops) the contents of the image/slice before (safely) copying the valid source subrect onto it. This results in a careful copy into part of an otherwise-uninitialized resource, and the whole resource slice then is then marked incorrectly as initialized. (Subsequent reads from the resource will treat it as initialized, leaking the uninitialized contents)

The modern security model for GPU resource contents should generally restrict uninitialized data to within the process, not necessarily just from within the OpenGL context. Unfortunately for us, this means data could leak in from other GL contexts in the process. On Linux/Mac/Android, this should be only WebGL, and maybe Flash/Plugins, and we'll check if e.g. WebRender is affected.[2] Not all drivers we run WebGL on can be called "modern", so I have some concern that leaked contents might be system-wide, especially on older systems.

General exploit:

1. create a large uninitialized texture resource
2. copyTexSubImage from out-of-bounds of a source resource
3. read back the uninitialized/leaked contents
4. Exfil interesting leaked contents?

The hardest part here is (4), because:

• The possible sources of leaks are other GPU resources on the machine, probably (but not certainly) in-process only.
• It's hard to detect interesting contents to leak.
  • Maybe one could try to detect text, but you'll get an exfiltration dump mostly full of reddit.
• Trying to exfiltrate all data is not trivial, because it's all (large) images.

Due to the difficulty of an effective exfil path, I considered sec-medium, but sec-high seems safer because the core exploit up until exfil is easy, and exfil may be easy if you can afford to stream out a ton of images.

Non-minimal testcase: https://www.khronos.org/registry/webgl/sdk/tests/conformance/misc/uninitialized-test.html?webglVersion=1&quiet=0&quick=1
This has a high false-negative rate, so you may need to refresh a number of times before you see a (true-positive) failure. (in red)

[1]: Resource contents in WebGL are generally lazily initialized for performance reasons. On use, we lazily initialize the contents unless we can be sure the work would be redundant.

[2]: jrmuizel, do you know of any other GPU contexts in the client process?

Comment 1

[Kelsey Gilbert [:jgilbert]]

dveditz FYI

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: example-corruption.png

Comment 3

[Kelsey Gilbert [:jgilbert]]

We should probably add a killswitch pref for disabling lazy invalidation in WebGL in general.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1625404 - webgl.copyTexSubImage should not preserve existing dest rect contents.

Previously the corresponding destinations for the out-of-bounds source area are being retained.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Is ESR68 also affected? Patch grafts cleanly, so I guess so?

Comment 6

[Kelsey Gilbert [:jgilbert]]

Oof, didn't expect ESR too, but yes I can repro there. Thanks for the reminder!

Comment 7

[Kelsey Gilbert [:jgilbert]]

Probably thunderbird-68 too, unless they disable WebGL?

Comment 8

[Ryan VanderMeulen [:RyanVM]]

TB68 will get the fix for free if we uplift to ESR68. No need to track separately.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9136246 [details]
Bug 1625404 - webgl.copyTexSubImage should not preserve existing dest rect contents.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Once you see it, you can't unsee it. I did my best to not mention the uninitialized aspect, so that's not obvious. However, any time you add clearing is suspect, if anyone digs into this.
• 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?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Medium-low. This is a rarely-used entrypoint, and if it passes CI, it's probably fine. However, this sometimes-tricky code.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Based on the corruption I've seen (I'll post a testcase) I do think that this is generally constrained to in-process GPU data.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: testcase-copytexsubimage-corruption.html

Comment 12

[Kelsey Gilbert [:jgilbert]]

+Ken, chrome-webgl, though Chrome looks fine.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Testcase 'unaffected' result is one stretched green rectangle in the center of each snapshot.
'Affected' is any other color, or more than one green shape.

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)

Based on the corruption I've seen (I'll post a testcase) I do think that this is generally constrained to in-process GPU data.

On my mac I'm seeing some images that come from my Slack instance which is running as a separate app.

Comment 15

[Julien Cristau [:jcristau]]

Please request branch approvals on this.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 9136246 [details]
Bug 1625404 - webgl.copyTexSubImage should not preserve existing dest rect contents.

sec-approval+ to get this into Fx75 release and corresponding ESR. please modify the commit msg as we discussed in Slack.

Coordinate with release managers on when to land on each branch.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9136246 [details]
Bug 1625404 - webgl.copyTexSubImage should not preserve existing dest rect contents.

Beta/Release Uplift Approval Request

• User impact if declined: sec-high: Leaks of system GPU memory (across processes on at least osx 10.14) in WebGL content, with ability to readback
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: STR in bug
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Some risk because this code is dealing with protecting uninitialized memory, and if it still fails in some corner cases, it will allow access to uninitialized GPU memory.
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: sec-high: Leaks of system GPU memory (across processes on at least osx 10.14) in WebGL content, with ability to readback
• Fix Landed on Version: 76
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Some risk because this code is dealing with protecting uninitialized memory, and if it still fails in some corner cases, it will allow access to uninitialized GPU memory.
• String or UUID changes made by this patch: none

Comment 18

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

Landed:
https://hg.mozilla.org/integration/autoland/rev/8717adb8b05b3a2686049a249ff725b6a4bc2b3d

Backed out for causing mochitest failures at dom/canvas/test/webgl-conf/generated/test_conformance__textures__misc__copy-tex-image-and-sub-image-2d.html
https://hg.mozilla.org/integration/autoland/rev/3e2c78fcc266122f060144f65d511db7df9f4ea1

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=8717adb8b05b3a2686049a249ff725b6a4bc2b3d
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=295438451&repo=autoland

Comment 19

[Kelsey Gilbert [:jgilbert]]

This fix was wrong, because some spec text is out of date.

Comment 20

[Kelsey Gilbert [:jgilbert]]

I think Windows 10 (at least) is Unaffected, fwiw.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1625404 - webgl.copyTexSubImage should lazily-initialize unwritten destination pixels.

Comment 22

[Kelsey Gilbert [:jgilbert]]

Green this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99dba9fd231ac2e9ba60fb7db85ec358c5a771b3

Comment 23

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

https://hg.mozilla.org/integration/autoland/rev/bb8d410a3bf7565327dd72dcc311a365529a9cd7
https://hg.mozilla.org/mozilla-central/rev/bb8d410a3bf7

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 9136246 [details]
Bug 1625404 - webgl.copyTexSubImage should not preserve existing dest rect contents.

approved for 75 rc1

Comment 25

[Bogdan Maris, Desktop Test Engineering]

Reproduced the issue on Ubuntu 18.04 64bit using Fx 75.0b9. Verified using latest Nightly 76.0a1 (Ubuntu 18.04 and macOS 10.12 and 10.15) that the demopage and khronos test does not show the behavior which was present on a build without the fix.

Comment 26

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/d8b7e2a1e9c0

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9136246 [details]
Bug 1625404 - webgl.copyTexSubImage should not preserve existing dest rect contents.

This needs a rebased patch for ESR68.

Comment 28

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1625404 - (esr68) webgl.copyTexSubImage should lazily-initialize unwritten destination pixels.

Comment 29

[Kelsey Gilbert [:jgilbert]]

ESR68 backport posted, but I can't build it locally nor can I seem to get mach to push to try...

Comment 30

[Kelsey Gilbert [:jgilbert]]

What's the process here? How do we test this?

Comment 31

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1625404 - (esr68) webgl.copyTexSubImage should lazily-initialize unwritten destination pixels.

Comment 32

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 33

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jeff Gilbert [:jgilbert] from comment #30)

What's the process here? How do we test this?

Looking green on Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d0a8043b8e358381fe806e20d33af8ed52dd8f4

Comment 34

[Kelsey Gilbert [:jgilbert]]

Rarely am I so glad to see tests come back green. :)

Comment 35

[Kelsey Gilbert [:jgilbert]]

The advisory isn't quite accurate, though this does get a little tricky to describe accurately:

Uninitialized GPU memory is leaked and could be read following a call to WebGL's copyTexSubImage2D

When reading from out-of-bounds regions of a source resource with WebGL's <code>copyTexSubImage2D</code> method, the specification requires that the destination pixels that correspond to the out-of-bounds regions (of the source resource) are left unchanged. When the contents of the destination resource were marked for lazy initialization, destination pixels that correspond to the out-of-bounds regions (of the source resource) were marked as initialized without being either written to or lazily clearing the previously-uninitialized contents to zero, leading to potentially sensitive data disclosure.

Comment 36

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 37

[Bogdan Maris, Desktop Test Engineering]

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #25)

Reproduced the issue on Ubuntu 18.04 64bit using Fx 75.0b9. Verified using latest Nightly 76.0a1 (Ubuntu 18.04 and macOS 10.12 and 10.15) that the demopage and khronos test does not show the behavior which was present on a build without the fix.

Also verified that Fx 75.0 RC is not affected by this issue anymore on the same platforms as above.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9137332 [details]
Bug 1625404 - (esr68) webgl.copyTexSubImage should lazily-initialize unwritten destination pixels.

Approved for 68.7esr.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/6a1c567e654b1ee83425a517ad410067a086f06a

Comment 40

[Kelsey Gilbert [:jgilbert]]

If Ken's name in the advisory is for identifying the security issue, Ken's original bug did not include the security aspect until the investigation I did above, though it did point out the fishy test result that precipitated my investigation. I'm not sure what the format for these advisories is.

Comment 41

[Tom Ritter [:tjr]]

(In reply to Jeff Gilbert [:jgilbert] from comment #40)

If Ken's name in the advisory is for identifying the security issue, Ken's original bug did not include the security aspect until the investigation I did above, though it did point out the fishy test result that precipitated my investigation. I'm not sure what the format for these advisories is.

I'm not sure either. Perhaps co-credit is appropriate. I'll see what Dan thinks....

Comment 42

[Bogdan Maris, Desktop Test Engineering]

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #37)

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #25)

Reproduced the issue on Ubuntu 18.04 64bit using Fx 75.0b9. Verified using latest Nightly 76.0a1 (Ubuntu 18.04 and macOS 10.12 and 10.15) that the demopage and khronos test does not show the behavior which was present on a build without the fix.

Also verified that Fx 75.0 RC is not affected by this issue anymore on the same platforms as above.

68.7.0esr is also verified as well.

Comment 43

[Daniel Veditz [:dveditz]]

(In reply to Tom Ritter [:tjr] (OOTO until 5/1?) from comment #41)

(In reply to Jeff Gilbert [:jgilbert] from comment #40)

Ken's original bug did not include the security aspect until the investigation I did above, though it did point out the fishy test result that precipitated my investigation.

I'm not sure either. Perhaps co-credit is appropriate. I'll see what Dan thinks....

Most times in this situation--where the original bug was not called out as a sec bug and a moz developer realizes the implications--we just throw it into the roll-up and credit the Mozilla investigator. Ken's original does point out suspicious results, and he points at a part of the spec that's talking about not reading outside the defined area. That might be close enough to co-credit him. But Jeff should get primary credit since he had the insight.

I wouldn't feel bad if we just threw this into the roll-up -- pretty easy to slurp images from other processes if you know this bug exists, and not everyone upgrades quickly. Hard to do much with the leaked data, though: easiest to recognize interesting bits and pieces with a human looking at them, but that would involve shipping massive amounts of data back to the attacker to sift through.

Comment 44

[Kelsey Gilbert [:jgilbert]]

Can I close bug 1624451 now, referencing this one?

Comment 45

[Daniel Veditz [:dveditz]]

Yes, please.