Closed Bug 1635293 (CVE-2020-6463) Opened 1 year ago Closed 11 months ago

UAF in ANGLE gl::Texture::onUnbindAsSamplerTexture

Categories

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

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla79
Tracking Status
firefox-esr68 79+ fixed
firefox-esr78 79+ fixed
firefox75 --- wontfix
firefox76 - wontfix
firefox77 + wontfix
firefox78 + wontfix
firefox79 + fixed

People

(Reporter: dveditz, Assigned: jgilbert)

References

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main79+][adv-ESR78.1+][adv-esr68.11+][reporter in comment 0])

Attachments

(3 files)

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]

Rating based on chrome bug assuming it affects Firefox too.

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?

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

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

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.

Assignee: nobody → jgilbert
Severity: -- → S2
Flags: needinfo?(jgilbert)
Priority: -- → P1

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?

Keywords: sec-highsec-moderate
Flags: needinfo?(dveditz)

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.

sec-moderate sounds appropriate.

Flags: needinfo?(dveditz)

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

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

Flags: needinfo?(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.

Flags: needinfo?(jgilbert)

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

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

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

sec-approval+

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

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.

Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

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

Flags: needinfo?(jgilbert)

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

Attachment #9161789 - Flags: approval-mozilla-esr68?

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
Flags: needinfo?(jgilbert)
Attachment #9156848 - Flags: approval-mozilla-esr78?
Attachment #9161789 - Flags: approval-mozilla-esr78?

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.

Attachment #9156848 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
Attachment #9161789 - Flags: approval-mozilla-esr78?
Attachment #9161789 - Flags: approval-mozilla-esr68?
Attachment #9161789 - Flags: approval-mozilla-esr68+
Alias: CVE-2020-6463
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main79+]
Whiteboard: [post-critsmash-triage][adv-main79+] → [post-critsmash-triage][adv-main79+][adv-ESR78.1+]
Whiteboard: [post-critsmash-triage][adv-main79+][adv-ESR78.1+] → [post-critsmash-triage][adv-main79+][adv-ESR78.1+][adv-esr68.11+][reporter in comment 0]
Attached file advisory.txt
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.