Closed Bug 1493497 Opened 2 years ago Closed 1 year ago

Crash in gl::Framebuffer::Framebuffer

Categories

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

Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- unaffected
firefox64 --- wontfix
firefox65 + fixed
firefox66 + fixed

People

(Reporter: jseward, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main65+])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is
report bp-e594a59a-df9f-461f-9690-a6c4f0180922.
=============================================================

This crash happened in two different installations in the Windows
nightly 20180920100522.

Top 10 frames of crashing thread:

0 libglesv2.dll gl::Framebuffer::Framebuffer gfx/angle/checkout/src/libANGLE/Framebuffer.cpp:668
1 libglesv2.dll gl::Context::makeCurrent gfx/angle/checkout/src/libANGLE/Context.cpp:657
2 libglesv2.dll egl::Display::makeCurrent gfx/angle/checkout/src/libANGLE/Display.cpp:815
3 libglesv2.dll egl::Display::destroyContext gfx/angle/checkout/src/libANGLE/Display.cpp:909
4 libglesv2.dll egl::DestroyContext gfx/angle/checkout/src/libGLESv2/entry_points_egl.cpp:388
5 xul.dll void mozilla::gl::GLContextEGL::~GLContextEGL gfx/gl/GLContextProviderEGL.cpp:328
6 xul.dll void mozilla::gl::GLContextEGL::~GLContextEGL gfx/gl/GLContextProviderEGL.cpp:316
7 xul.dll mozilla::layers::CopyableCanvasRenderer::~CopyableCanvasRenderer gfx/layers/CopyableCanvasRenderer.cpp:53
8 xul.dll void mozilla::layers::WebRenderCanvasRendererAsync::~WebRenderCanvasRendererAsync gfx/layers/wr/WebRenderCanvasRenderer.cpp:32
9 xul.dll void mozilla::layers::WebRenderCanvasData::~WebRenderCanvasData gfx/layers/wr/WebRenderUserData.cpp:316

=============================================================
Flags: needinfo?(jgilbert)
There are 8 crashes (from 3 installations) in nightly 64 starting with buildid 20180920100522. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1489279.

[1] https://hg.mozilla.org/mozilla-central/rev?node=7aff94b0bba6
Component: Graphics: Layers → Canvas: WebGL
Keywords: regression
Priority: -- → P3
This is spiking in 64.0 release, crash addresses point at UAF.
Group: gfx-core-security
Keywords: csectype-uaf
Jessie, could you please help with that?
Thanks
Flags: needinfo?(jbonisteel)
Priority: P3 → P1
Assignee: nobody → sotaro.ikeda.g
Flags: needinfo?(jbonisteel)
From the crash stack, it seems that egl::Surface was already destroyed. All crashes happened in Display::destroyContext() in GLContextEGL::~GLContextEGL(). Further, Context::makeCurrent() was called in Display::destroyContext(). The makeCurrent() is called only when destroying gl::Context is not current context.

https://dxr.mozilla.org/mozilla-central/source/gfx/angle/checkout/src/libANGLE/Display.cpp#880

In Context::makeCurrent(), it seems that thread->getCurrentDrawSurface() is already destroyed surface.
I found one problem in SharedSurface_ANGLEShareHandle::~SharedSurface_ANGLEShareHandle().

It destroys EGLSurface just by calling eglDestroySurface(). The ~SharedSurface_ANGLEShareHandle() does not clear the destroying EGLSurface from current context. It could cause UAF.

https://dxr.mozilla.org/mozilla-central/source/gfx/gl/SharedSurfaceANGLE.cpp#122
Attachment #9032356 - Flags: review?(jgilbert)
Attachment #9032356 - Flags: review?(jgilbert) → review+
Attachment #9032356 - Attachment description: patch - Clear EGLSurfaceOverride during destroying EGLSurface in SharedSurface_ANGLEShareHandle destructor. → patch - Clear EGLSurfaceOverride during destroying EGLSurface in SharedSurface_ANGLEShareHandle.
Attachment #9032356 - Attachment description: patch - Clear EGLSurfaceOverride during destroying EGLSurface in SharedSurface_ANGLEShareHandle. → patch - Clear EGLSurfaceOverride correctly in SharedSurface_ANGLEShareHandle.
Comment on attachment 9032356 [details] [diff] [review]
patch - Clear EGLSurfaceOverride correctly in SharedSurface_ANGLEShareHandle.

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: It is not easy. I tried to reproduce the problem locally with WebGL. But normal WebGL usages did not hit the crash code path.

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?: firefox64

If not all supported branches, which bug introduced the flaw?: Bug 1489279

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: It is easy to back port.

How likely is this patch to cause regressions; how much testing does it need?: It is not likely to cause the regression. It is a simple fix to address UAF. Current auto tests already cover the code. And there is no specific STR to hit the problem.
Attachment #9032356 - Flags: sec-approval?
Keywords: sec-high
sec-approval+ for trunk. We'll want this on beta as well.
Attachment #9032356 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/41d9d168940d
Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Depends on: 1516787
Let's wait on nominating this for Beta uplift until bug 1516787 has gotten some investigation.

Bug 1516787 has hardly any crash instances on 66. Why is the uplift to beta on this bug gated on that bug?

Flags: needinfo?(ryanvm)

Because it wasn't clear what the volume was at the time of the initial report and I didn't want to risk introducing a bigger regression. Anyway, Jeff is back from PTO tomorrow, so hopefully it'll be a moot point and we can get this + the follow-up in b10 on Thursday.

Flags: needinfo?(ryanvm)

Please nominate this for Beta approval when you get a chance.

Flags: needinfo?(sotaro.ikeda.g)

Comment on attachment 9032356 [details] [diff] [review]
patch - Clear EGLSurfaceOverride correctly in SharedSurface_ANGLEShareHandle.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1489279

User impact if declined: It might cause the crash.

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: Bug 1516787

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Fix is relatively simple, though the patch caused a regression(Bug 1516787). It is already addressed.

String changes made/needed: None

Flags: needinfo?(sotaro.ikeda.g)
Attachment #9032356 - Flags: approval-mozilla-beta?

Comment on attachment 9032356 [details] [diff] [review]
patch - Clear EGLSurfaceOverride correctly in SharedSurface_ANGLEShareHandle.

[Triage Comment]
Fixes a sec-high WebGL crash. Approved for 65.0b12.

Attachment #9032356 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main65+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.