1635293 - (CVE-2020-6463) UAF in ANGLE gl::Texture::onUnbindAsSamplerTexture

Status: RESOLVED FIXED

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

Description

[Daniel Veditz [:dveditz]]

The Chrome team has taken a fix from ANGLE to patch a UAF. The patch applies to our source and isn't is one the parts I know we don't use so it probably affects Firefox, too. "Reported by Pawel Wylecial of REDTEAM.PL on 2020-03-26" according to their stable release notes.

Chrome bug (hidden): https://bugs.chromium.org/p/chromium/issues/detail?id=1065186
Patch:
https://chromium.googlesource.com/angle/angle/+/91c39dae9a518706f2635ac8b87f9f5b5ed9001c

Chrome has assigned CVE-2020-6463

[reference: https://chromereleases.googleblog.com/2020/04/stable-channel-update-for-desktop_21.html]

Comment 1

[Daniel Veditz [:dveditz]]

Rating based on chrome bug assuming it affects Firefox too.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

AFAICT that code hasn't changed in awhile and affects all supported branches. Is this something we need to consider patching on Release76 too or is 77 good enough?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Per Slack discussion with Jeff and Dan, we can let this ride the next regularly-scheduled releases.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Here's the commit message:

When a new texture is bound, the texture binding state is updated before
updating the active texture cache. With this ordering, it is possible to delete
the currently bound texture when the binding changes and then use-after-free it
when updating the active texture cache.

Diff:

diff --git a/src/libANGLE/State.cpp b/src/libANGLE/State.cpp
index 207bc39..1baa133 100644
--- a/src/libANGLE/State.cpp
+++ b/src/libANGLE/State.cpp

@@ -1170,14 +1170,14 @@
 
 void State::setSamplerTexture(const Context *context, TextureType type, Texture *texture)
 {
-    mSamplerTextures[type][mActiveSampler].set(context, texture);
-
     if (mProgram && mProgram->getActiveSamplersMask()[mActiveSampler] &&
         IsTextureCompatibleWithSampler(type, mProgram->getActiveSamplerTypes()[mActiveSampler]))
     {
         updateActiveTexture(context, mActiveSampler, texture);
     }
 
+    mSamplerTextures[type][mActiveSampler].set(context, texture);
+
     mDirtyBits.set(DIRTY_BIT_TEXTURE_BINDINGS);
 }

It sounds like WebGL might be affected by this, so I'll try making a webgl testcase and test in an ASAN build.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Based on the commit message, it looks like this is a non-user-controlled UAF at least, almost exactly the same as a dangling reference. That makes this closer to a sec-moderate, IMO.

Specifically:

void Foo(Tex* tex) {
  delete tex;
  [...]
  tex->Bar();
}

As long as nothing between delete and use is targetable, we only have to worry about another thread putting allocating data at tex's address, which is really hard to target I think.
Maybe we still call that sec-high, but it seems less bad. dveditz?

Comment 6

[Kelsey Gilbert [:jgilbert]]

Talking with ANGLE devs this sounds like it may be coming via functionality of ANGLE that we don't use for WebGL. We should audit WebRender though.

Comment 7

[Daniel Veditz [:dveditz]]

sec-moderate sounds appropriate.

Comment 8

[Kelsey Gilbert [:jgilbert]]

ANGLE team now believes this impacts WebGL, at least in Chromium: https://chromium-review.googlesource.com/c/angle/angle/+/2199654

Comment 9

[Kelsey Gilbert [:jgilbert]]

https://bugs.chromium.org/p/chromium/issues/detail?id=1078375

Comment 10

[Julien Cristau [:jcristau]]

What's the status here? Do we still expect a fix for 78?

Comment 11

[Kelsey Gilbert [:jgilbert]]

Taking a fix late in the cycle here seems risky because it's hard to reason about a change like this in ANGLE.
We should try to apply their patch for Nightly probably? I don't feel great about landing this in late beta, but we still have a week so maybe it's worth a try.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Given that this is rated sec-moderate, assuming we believe that to still be the case, it would seem reasonable to me that we land on Nightly79 now to let it start baking and then revisit ESR uplifts for the next cycle (i.e. 68.11/78.1 shipping alongside Fx79 in July).

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1635293 - Cherry-pick ANGLE fix: Update tex cache even if new type is InvalidEnum.

Comment 14

[Kelsey Gilbert [:jgilbert]]

The cherry-pick had conflicts, but it wasn't too bad to resolve them manually. It builds locally, and next step would be to test on CI. Since this is a sec bug, we may or may not want to just have the CI run during landing instead of an early Try run. We'll see when we get sec-approval.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9156848 [details]
Bug 1635293 - Cherry-pick ANGLE fix: Update tex cache even if new type is InvalidEnum.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Fairly tricky, but the change is small enough that's it's clear what to look for.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• 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?: No
• If not, how different, hard to create, and risky will they be?: I haven't made one for esr68 yet, which claims to be affected.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely if it passes CI. If it's gonna break, CI's tests should be enough to break it.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 9156848 [details]
Bug 1635293 - Cherry-pick ANGLE fix: Update tex cache even if new type is InvalidEnum.

sec-approval+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Updating the flags per comment 11 as well. Looks like the patch grafts cleanly to Beta78 (and ESR78 eventually) and only needs a minor rebase for ESR68.

Comment 18

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

https://hg.mozilla.org/integration/autoland/rev/a8706acb5a4ca68ca49f5310f8d28f8d7d92f953
https://hg.mozilla.org/mozilla-central/rev/a8706acb5a4c

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is this ready for ESR68 & ESR78 approval requests? Note that ESR68 will need a bit of a rebased patch.

Comment 20

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Bug-1635293-esr68-Cherry-pick-ANGLE-fix-Update-tex-c.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-moderate, but given ESR's long timeline and that this is fixed in Chrome and trunk for us, we should consider taking this minor fix.
User impact if declined: Confidently sec-moderate, but potentially worse if someone figures out a way to leverage this.
Fix Landed on Version: 79
Risk to taking this patch (and alternatives if risky): Fairly low, small patch, rebase applied to ANGLE cleanly, and landed already without regressions so far.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9156848 [details]
Bug 1635293 - Cherry-pick ANGLE fix: Update tex cache even if new type is InvalidEnum.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: See esr68 request: https://bugzilla.mozilla.org/show_bug.cgi?id=1635293#c20
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch: none

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9156848 [details]
Bug 1635293 - Cherry-pick ANGLE fix: Update tex cache even if new type is InvalidEnum.

Approved for 78.1esr and 68.11esr.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/b1e067197dce

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/fe00c8ed77d6

Comment 25

[Frederik Braun [:freddy]]

Attachment: advisory.txt