Closed Bug 1625404 (CVE-2020-6821) Opened 1 year ago Closed 1 year ago

CopyTexSubImage leaves unwritten destination pixels as uninitialized data instead of lazy-initializing

Categories

(Core :: Canvas: WebGL, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla76
Tracking Status
firefox-esr68 75+ verified
firefox74 - wontfix
firefox75 + verified
firefox76 + verified

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main75+][adv-esr68.7+])

Attachments

(6 files, 2 obsolete files)

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?

Flags: needinfo?(jmuizelaar)

dveditz FYI

Flags: needinfo?(dveditz)
Attached image example-corruption.png

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

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

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

Group: core-security
Flags: needinfo?(jgilbert)

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

Flags: needinfo?(jgilbert)

Probably thunderbird-68 too, unless they disable WebGL?

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

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.
Attachment #9136246 - Flags: sec-approval?

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.

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

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.

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

Flags: needinfo?(dveditz)

Please request branch approvals on this.

Flags: needinfo?(jgilbert)

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.

Attachment #9136246 - Flags: sec-approval? → sec-approval+
Depends on: 1624451
Attachment #9136246 - Attachment description: Bug 1625404 - webgl.copyTexSubImage should zero out-of-bounds read areas. → Bug 1625404 - webgl.copyTexSubImage should not preserve existing dest rect contents.

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
Flags: needinfo?(jgilbert)
Attachment #9136246 - Flags: approval-mozilla-esr68?
Attachment #9136246 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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

Summary: CopyTexSubImage reads out-of-bounds source pixels as uninitialized data instead of zeros → CopyTexSubImage leaves unwritten destination pixels as uninitialized data instead of lazy-initializing

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

Group: gfx-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76

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

approved for 75 rc1

Attachment #9136246 - Flags: approval-mozilla-beta? → approval-mozilla-release+

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 on attachment 9136246 [details]
Bug 1625404 - webgl.copyTexSubImage should not preserve existing dest rect contents.

This needs a rebased patch for ESR68.

Flags: needinfo?(jgilbert)
Attachment #9136246 - Flags: approval-mozilla-esr68?

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

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

Flags: needinfo?(jgilbert) → needinfo?(ryanvm)
Attachment #9137325 - Attachment is obsolete: true
Whiteboard: [adv-main75+] → [adv-main75+][adv-esr68.7+]

(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

Flags: needinfo?(ryanvm)

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

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.

QA Whiteboard: [qa-triaged]

(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 on attachment 9137332 [details]
Bug 1625404 - (esr68) webgl.copyTexSubImage should lazily-initialize unwritten destination pixels.

Approved for 68.7esr.

Attachment #9137332 - Flags: approval-mozilla-esr68+

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.

Flags: needinfo?(jmuizelaar) → needinfo?(tom)

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

Flags: needinfo?(tom) → needinfo?(dveditz)

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+

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

Flags: needinfo?(dveditz)
Alias: CVE-2020-6821

Can I close bug 1624451 now, referencing this one?

Flags: needinfo?(dveditz)

Yes, please.

Flags: needinfo?(dveditz)
Blocks: 1624451
No longer depends on: 1624451
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.