Bug 1402372 (CVE-2017-7845)

heap buffer overflow in VertexBuffer9 (ANGLE)

RESOLVED FIXED in Firefox -esr52

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: omair, Assigned: jgilbert)

Tracking

(5 keywords)

45 Branch
mozilla58
Points:
---
Bug Flags:
sec-bounty +
qe-verify -

Firefox Tracking Flags

(relnote-firefox 57+, firefox-esr5257+ fixed, firefox55 wontfix, firefox56 wontfix, firefox57blocking fixed, firefox58+ unaffected, firefox59 unaffected)

Details

(Whiteboard: [gfx-noted][adv-main57+][adv-esr52.5.1+], crash signature)

Attachments

(3 attachments, 6 obsolete attachments)

Reporter

Description

2 years ago
Posted file D3D9.html (obsolete) —
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

I have attached the same PoC (Bug 1398381) which can be used to trigger a heap overflow in ANGLE. 
(You can disable hardware acceleration and test.)

I have tested this on: 58 nightly build (should be applicable to the current stable build as well)

libglesv2.dll!memcpy + ? (the exact offset is not known) (id: 72d)
libglesv2.dll!rx::VertexBuffer9::storeVertexAttributes + 0x1D4 (id: 791) [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\d3d9\vertexbuffer9.cpp @ 109]]
libglesv2.dll!rx::StreamingVertexBufferInterface::storeDynamicAttribute + 0x204 [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\vertexbuffer.cpp @ 173]]
libglesv2.dll!rx::VertexDataManager::storeDynamicAttrib + 0x240 [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\vertexdatamanager.cpp @ 480]]
libglesv2.dll!rx::VertexDataManager::storeDynamicAttribs + 0x25A [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\vertexdatamanager.cpp @ 400]]
libglesv2.dll!rx::VertexDataManager::prepareVertexData + 0x2DA [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\vertexdatamanager.cpp @ 275]]
libglesv2.dll!rx::Renderer9::applyVertexBuffer + 0x5E [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\d3d9\renderer9.cpp @ 1285]]
libglesv2.dll!rx::Renderer9::genericDrawElements + 0x1A8 [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\d3d9\renderer9.cpp @ 2984]]
libglesv2.dll!rx::Context9::drawElements + 0x47 [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\d3d9\context9.cpp @ 157]]
libglesv2.dll!gl::Context::drawElements + 0x33 (inlined function) [[z:\build\build\src\gfx\angle\src\libangle\context.cpp @ 1654]]
libglesv2.dll!gl::DrawElements + 0xB2 [[z:\build\build\src\gfx\angle\src\libglesv2\entry_points_gles_2_0.cpp @ 791]]
xul.dll!mozilla::gl::GLContext::raw_fDrawElements + 0x1A (inlined function) [[z:\build\build\src\gfx\gl\glcontext.h @ 1101]]
xul.dll!mozilla::gl::GLContext::fDrawElements + 0x26 [[z:\build\build\src\gfx\gl\glcontext.h @ 1115]]
xul.dll!mozilla::WebGLContext::DrawElements + 0x13F [[z:\build\build\src\dom\canvas\webglcontextdraw.cpp @ 811]]
xul.dll!mozilla::dom::WebGLRenderingContextBinding::drawElements + 0xB6 [[z:\build\build\src\obj-firefox\dom\bindings\webglrenderingcontextbinding.cpp @ 16256]]
xul.dll!mozilla::dom::GenericBindingMethod + 0x12A [[z:\build\build\src\dom\bindings\bindingutils.cpp @ 3061]]
xul.dll!js::CallJSNative + 0xA3 (inlined function) [[z:\build\build\src\js\src\jscntxtinlines.h @ 293]]
xul.dll!InternalCall + ? (the exact offset is not known) [[z:\build\build\src\js\src\vm\interpreter.cpp @ 495]]
xul.dll!js::CallFromStack + 0xD (inlined function) [[z:\build\build\src\js\src\vm\interpreter.cpp @ 546]]
xul.dll!js::jit::DoCallFallback + 0x24A [[z:\build\build\src\js\src\jit\baselineic.cpp @ 2589]]
0x2E8E572D08E (no function symbol available)
0x18453FC480 (no function symbol available)
0x18453FC1B0 (no function symbol available)
0xFFFE01216EA6D940 (no function symbol available)
0xEEE4421FD838 (no function symbol available)
0x18453FC430 (no function symbol available)
0x18453FC3E8 (no function symbol available)
0x4 (no function symbol available)
0x18453FC3E8 (no function symbol available)
Group: firefox-core-security → gfx-core-security
Component: Untriaged → Graphics
Product: Firefox → Core
Peter, can you find somebody to take a look?
Flags: needinfo?(howareyou322)
Priority: -- → P2
Whiteboard: [gfx-noted]
Michael, please check this with ANGEL update.
Assignee: nobody → cleu
Flags: needinfo?(howareyou322)
 1:50.79 INFO: Last good revision: 099f695d31326c39595264c34988a0f4b7cbc698 (2015-11-25)
 1:50.79 INFO: First bad revision: c321d84038519dcf1670d59fd2c5c00ad8a85a55 (2015-11-26)
 1:50.80 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=099f695d31326c39595264c34988a0f4b7cbc698&tochange=c321d84038519dcf1670d59fd2c5c00ad8a85a55

Two WebGL-related pushes in that range:
https://hg.mozilla.org/mozilla-central/rev/bd2c8c547222
https://hg.mozilla.org/mozilla-central/rev/7d1c223f397c
Status: UNCONFIRMED → NEW
Crash Signature: [@ memcpy | rx::VertexBuffer9::storeVertexAttributes ]
Has Regression Range: --- → yes
Ever confirmed: true
Keywords: crash, regression
Version: 57 Branch → 45 Branch
Updating ANGLE to newer version is still affected by this issue.

And Chrome does not crash, so there must be something wrong in our Webgl context.
I observed what chrome do when they load the same webpage, it seems  that the DrawElements is not legal and will be blocked as INVALID_OPERATION, I add same validation code in our WebGL code, the crash does disappear after this modification.
Attachment #8913132 - Flags: feedback?(jgilbert)
Assignee

Comment 8

2 years ago
How about this?
Attachment #8913506 - Flags: review?(cleu)
Comment on attachment 8913506 [details] [diff] [review]
0001-Bug-1402372-Always-check-that-we-have-a-non-zero-ver.patch

Review of attachment 8913506 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, this should block illegal Draw calls.
Attachment #8913506 - Flags: review?(cleu) → review+
Assignee

Updated

2 years ago
Severity: normal → critical
Component: Graphics → Canvas: WebGL
Priority: P2 → P1
Assignee

Updated

2 years ago
Attachment #8913132 - Flags: feedback?(jgilbert) → feedback-
Assignee

Comment 10

2 years ago
Comment on attachment 8913506 [details] [diff] [review]
0001-Bug-1402372-Always-check-that-we-have-a-non-zero-ver.patch

This has issues still.
Attachment #8913506 - Flags: review-
Assignee

Comment 11

2 years ago
Easy enough to construct a non-security bug in parallel: bug 1404196

We can do sec approval for uplift and what-not in this bug.
Comment on attachment 8913506 [details] [diff] [review]
0001-Bug-1402372-Always-check-that-we-have-a-non-zero-ver.patch

Review of attachment 8913506 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, this should block illegal Draw calls.
Attachment #8913506 - Flags: review+
Assignee

Updated

2 years ago
Assignee: cleu → jgilbert
Assignee

Comment 13

2 years ago
So the parallel bug is leaving open a potential case: instanced attribs with generic (non-array) attrib values.
I'll make a testcase for this.
Flags: sec-bounty?
tracking as sec-critical.
Has STR: --- → yes
Keywords: testcase
Assignee

Comment 15

2 years ago
Fixed in 58: https://bugzilla.mozilla.org/show_bug.cgi?id=1404196
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Assignee

Comment 16

2 years ago
This was actually the d3d9 version of bug 1376399.
Assignee

Updated

2 years ago
Attachment #8913132 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8913506 - Attachment is obsolete: true
Assignee

Comment 18

2 years ago
We're going to want to fix this in esr52 and probably release, when we get a chemspill opportunity.
Target Milestone: --- → mozilla58
Marking this affected for 57 so that it will show up on relman and security triage queries. 

We also could uplift now, for an RC2 build later this week. 

Since I haven't started the ESR build yet, it could make ESR as well if we land patches tomorrow morning. 
Ritu, dveditz, fyi, depending on sec-approval and how risky this looks for 57 we can either plan this for dot release, or get it in this week.
Flags: needinfo?(rkothari)
Flags: needinfo?(dveditz)
Without sec-approval, risk assessment and how easily this can be exploited, my guess is this will likely not be in 57 RC. Let's wait to hear back from Dan and the other info from the uplift request.
Flags: needinfo?(rkothari)
Attachment #8925741 - Flags: review?(cleu) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #16)
> This was actually the d3d9 version of bug 1376399.

Should I worry that the ANGLE update in bug 1371190 seemed to revert the d3d11 fix in that bug back to passing '0' instead of '1'? It's calling a different set of methods now, though:

  gl::Error Context11::drawArrays(const gl::Context *context, GLenum mode, GLint first, GLsizei count)
  {
      ANGLE_TRY(prepareForDrawCall(context, mode));
      return mRenderer->drawArrays(context, mode, first, count, 0);
  }

(Similar in the other places we changed.)
Flags: needinfo?(jgilbert)
(In reply to Ritu Kothari (:ritu) from comment #20)
> Without sec-approval, risk assessment and how easily this can be exploited,
> my guess is this will likely not be in 57 RC. Let's wait to hear back from
> Dan and the other info from the uplift request.

I'm not at all worried about taking this fix in 57 -- it's identical to the patch we took in bug 1376399 except for d3d9 instead of d3d11. I am a little worried about whether we regressed that other fix (see comment21) and might have to fix that, too, but I don't have the hardware to test it right now.
Flags: needinfo?(dveditz)
This hurts. We initially found and fixed the d3d11 version on our own, but didn't identify it as a security bug. That means we didn't think about backporting it to ESR-52 (bug 1398381) and we didn't look at the parallel code for d3d9 (this bug).
Flags: sec-bounty? → sec-bounty+
Assignee

Comment 24

2 years ago
(In reply to Daniel Veditz [:dveditz] from comment #21)
> (In reply to Jeff Gilbert [:jgilbert] from comment #16)
> > This was actually the d3d9 version of bug 1376399.
> 
> Should I worry that the ANGLE update in bug 1371190 seemed to revert the
> d3d11 fix in that bug back to passing '0' instead of '1'? It's calling a
> different set of methods now, though:
> 
>   gl::Error Context11::drawArrays(const gl::Context *context, GLenum mode,
> GLint first, GLsizei count)
>   {
>       ANGLE_TRY(prepareForDrawCall(context, mode));
>       return mRenderer->drawArrays(context, mode, first, count, 0);
>   }
> 
> (Similar in the other places we changed.)

Yes, my god. New rule: No ANGLE patches without regression tests.
Flags: needinfo?(jgilbert)
Assignee

Comment 25

2 years ago
Though actually I believe we're safe now that we skip making the actual call if `!vertCount||!instanceCount`, and always validate non-instanced draw calls as instanceCount=1 in FF58.
Assignee

Comment 26

2 years ago
(In reply to Daniel Veditz [:dveditz] from comment #22)
> (In reply to Ritu Kothari (:ritu) from comment #20)
> > Without sec-approval, risk assessment and how easily this can be exploited,
> > my guess is this will likely not be in 57 RC. Let's wait to hear back from
> > Dan and the other info from the uplift request.
> 
> I'm not at all worried about taking this fix in 57 -- it's identical to the
> patch we took in bug 1376399 except for d3d9 instead of d3d11. I am a little
> worried about whether we regressed that other fix (see comment21) and might
> have to fix that, too, but I don't have the hardware to test it right now.

I do confirm that the d3d11 bug remains fixed in FF58, and the ANGLE patch(es) are not needed there, because of our new validation. The D3D11 bug is also fixed in FF57 release build 4, but this D3D9 version still repros there, as expected.
Assignee

Comment 27

2 years ago
Comment on attachment 8925741 [details] [diff] [review]
0001-Bug-1402372-Fix-instancing-count-for-non-instanced-c.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-crit
User impact if declined: Poorly written WebGL code might crash.
Fix Landed on Version: 58+
Risk to taking this patch (and alternatives if risky): Very low, since we've taken this already for d3d11.
String or UUID changes made by this patch: none

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

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
This is more of a vector than a vuln, so it'd probably still be tough. The vector here is clear though.

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?
ESR52, 57

If not all supported branches, which bug introduced the flaw?
ANGLE update

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes.

How likely is this patch to cause regressions; how much testing does it need?
Our CI tests cover this.
Attachment #8925741 - Flags: sec-approval?
Attachment #8925741 - Flags: approval-mozilla-release?
Attachment #8925741 - Flags: approval-mozilla-esr52?
57 already shipped. Unless this is chemspill worthy, we're not going to take it on 57 and re-release.

This is fixed in 58 and, therefore, fixed in 59, right? So we're only talking about ESR52 for 52.6 (next release) at this point?
Assignee

Comment 29

2 years ago
(In reply to Al Billings [:abillings] from comment #28)
> 57 already shipped. Unless this is chemspill worthy, we're not going to take
> it on 57 and re-release.
> 
> This is fixed in 58 and, therefore, fixed in 59, right? So we're only
> talking about ESR52 for 52.6 (next release) at this point?

I'm theoretically OK with letting this go unfixed in 57, but it would be nice to deploy this fix if we chemspill for another reason.
Ritu - FYI as 57 release owner, something we may want to consider for a dot release.
Flags: needinfo?(rkothari)
Note also we support ESR52 through the end of June 2018 (52.8.0). So that may weigh more heavily in favor of taking this in ESR at some point, whether that's for 52.5.1 or 52.6.0.
Thanks Liz! I'll add this to my 57.x dot release ride-alongs though Al's comment 28 makes me feel it might be a wontfix for all of 57. Let's review risks, reward, security team's opinion when we are closer to doing a dot release for 57.
Flags: needinfo?(rkothari)
Group: gfx-core-security → core-security-release
Comment on attachment 8925741 [details] [diff] [review]
0001-Bug-1402372-Fix-instancing-count-for-non-instanced-c.patch

Clearing sec-approval since this is really a branch issue at this point.
Attachment #8925741 - Flags: sec-approval?
Comment on attachment 8925741 [details] [diff] [review]
0001-Bug-1402372-Fix-instancing-count-for-non-instanced-c.patch

DVeditz agrees that this would be a low-risk ride-along for 57.0.1. Release57+
Attachment #8925741 - Flags: approval-mozilla-release? → approval-mozilla-release+
Comment on attachment 8925741 [details] [diff] [review]
0001-Bug-1402372-Fix-instancing-count-for-non-instanced-c.patch

Fix for sec-critical issue, let's take this for ESR 52.5.1.
Attachment #8925741 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Alias: CVE-2017-7845
Whiteboard: [gfx-noted] → [gfx-noted][adv-main57+][adv-esr52.5.1+]
Assignee

Updated

2 years ago
See Also: → 1376399
Jeff, this is hitting WinXP debug asserts on ESR52.
https://treeherder.mozilla.org/logviewer.html#?job_id=148283375&repo=mozilla-esr52

Is this something that should block the 52.5.1 release?
Flags: needinfo?(jgilbert)
(In reply to Jeff Gilbert [:jgilbert] from comment #27)
> How likely is this patch to cause regressions; how much testing does it need?
> Our CI tests cover this.

Since this patch has automated coverage, I'm flagging it as not needing manual testing.

Jeff, if you think otherwise, please ni? me.
Flags: qe-verify-

Comment 40

2 years ago
Looking at the PoC I got wondering whether one actually hits the problematic code path if one just allows WebGL in minimum capability mode (by setting `webgl.min_capability_mode` to `true`) and disables things like extensions (by setting `webgl.disable-extensions`, to `true`). Or is the bug in question generic enough that it hits the users as soon as any kind of WebGL is allowed?
We should probably block 52.5.1 while we sort this out; may need to back out some stuff out.
We will not push ESR52.5.1 until the test failure is all clear, I just emailed r-d.
Note that we're also shipping this in 57.0.1 and I believe Al's intending to publish an advisory for it. We should probably delay that then too.

To recap, Windows XP debug (and *only* Windows XP debug) is hitting asserts in the mochitest-gl test suite. We aren't seeing them on opt builds or on other Windows platforms. What isn't clear to me is what test coverage we have for this code path anywhere outside of ESR52 - are our Win7 machines (which have SP1 and no platform update) also hitting the d3d9 path, or are they on d3d11?

It's really unclear what the severity is here, especially with opt tests passing. FWIW, when I run 52.5.1 locally on my Win10 machine with webgl.angle.try-d3d11 set to false (which IIUC forces d3d9 mode on ANGLE), I was able to run various WebGL demos on https://experiments.withgoogle.com/chrome?tag=WebGL without any obvious issues.
Assignee

Comment 44

2 years ago
I'm trying solutions, but having a hell of a time building esr52 or pushing to try over git.
Flags: needinfo?(jgilbert)
Assignee

Comment 45

2 years ago
I'll be on a flight, and can't build esr52 anyways at the moment.
Can someone build and test this?
Attachment #8925741 - Attachment is obsolete: true
Flags: needinfo?(milan)
Assignee

Updated

2 years ago
Attachment #8933419 - Flags: review?(kvark)
I'll attempt to run this through Try. Jeff, can you please summarize what the issue is here so we have a better idea of what your patch is fixing? Does it affect all branches? Is it a real user-facing issue?
Flags: needinfo?(jgilbert)
Comment on attachment 8933419 [details] [diff] [review]
0001-Bug-1402372-Always-use-instanced-drawing-when-enable.patch

Try says this doesn't work :(
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c21121e38f60df9f13ba73e6228a68c4037e0e55&group_state=expanded

Assertions are still there on XP and it actually causes new failures in /test_2_conformance__rendering__point-no-attributes.html on Win7/Win8.
Attachment #8933419 - Flags: feedback-
Comment on attachment 8933419 [details] [diff] [review]
0001-Bug-1402372-Always-use-instanced-drawing-when-enable.patch

Sorry, I misunderstood that this patch depends on the earlier patch being backed out. Here's a new Try run for that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f2a2d8bec27350c016debfcf0a7a42520822f07&group_state=expanded
Attachment #8933419 - Flags: feedback- → feedback?(ryanvm)
Assignee

Comment 49

2 years ago
This should fix the no-attrib drawing tests.
Flags: needinfo?(jgilbert)
Attachment #8933467 - Flags: review?(kvark)
Attachment #8933419 - Flags: review?(kvark) → review+
Comment on attachment 8933467 [details] [diff] [review]
0002-Bug-1402372-Always-check-for-active-vertex-attrib-di.patch

Review of attachment 8933467 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLContextDraw.cpp
@@ +998,5 @@
>      }
>  
> +    if (mActiveProgramLinkInfo->attribs.size() && !hasPerVertex) {
> +        /* http://www.khronos.org/registry/gles/extensions/ANGLE/ANGLE_instanced_arrays.txt
> +         *  If all of the enabled vertex attribute arrays that are bound to active

The reasoning doesn't seem strong to me: one could fetch per-vertex positions from textures or generate in code with `gl_VertexID`, so the case should be valid.
Attachment #8933467 - Flags: review?(kvark) → review+
Assignee

Comment 51

2 years ago
(In reply to Dzmitry Malyshau [:kvark] from comment #50)
> Comment on attachment 8933467 [details] [diff] [review]
> 0002-Bug-1402372-Always-check-for-active-vertex-attrib-di.patch
> 
> Review of attachment 8933467 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLContextDraw.cpp
> @@ +998,5 @@
> >      }
> >  
> > +    if (mActiveProgramLinkInfo->attribs.size() && !hasPerVertex) {
> > +        /* http://www.khronos.org/registry/gles/extensions/ANGLE/ANGLE_instanced_arrays.txt
> > +         *  If all of the enabled vertex attribute arrays that are bound to active
> 
> The reasoning doesn't seem strong to me: one could fetch per-vertex
> positions from textures or generate in code with `gl_VertexID`, so the case
> should be valid.

The logic for the restrictions comes pretty directly from that spec. It's to appease D3D9 (I believe) which requires at least one non-instanced vertex attrib.
Attachment #8933419 - Attachment is obsolete: true
Attachment #8933419 - Flags: feedback?(ryanvm)
That push won't give you the buildbot jobs you need for WinXP. Here's a push that has those:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00b6f7856f21783fd70f66452d9e06f4ef8ad7bd
Attachment #8933419 - Attachment is obsolete: false
Except that only has the second patch. Here's one that has both on top of a backout of the first.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=865188c01c4d10a7fc6c551b5543014fff97a0b6
Comment on attachment 8933467 [details] [diff] [review]
0002-Bug-1402372-Always-check-for-active-vertex-attrib-di.patch

Failing on all platforms:
https://treeherder.mozilla.org/logviewer.html#?job_id=148977700&repo=try

Failing on Win7+:
https://treeherder.mozilla.org/logviewer.html#?job_id=148977484&repo=try

The Try push was ESR52 tip with a backout of comment 37, attachment 8933419 [details] [diff] [review], and attachment 8933467 [details] [diff] [review] applied on top.
Attachment #8933467 - Flags: feedback-
Ryan, from the email thread, we want to backout the fix. 
If you agree, could you please do that?
Flags: needinfo?(ryanvm)

Comment 59

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #58)
> Backed out from release and ESR52. We'll target Fx58 and ESR 52.6.0 for
> shipping this fix instead.
> 
> https://hg.mozilla.org/releases/mozilla-release/rev/
> 15bb6c3fb5875c7f39aef036dd161df1407b5ee3
> https://hg.mozilla.org/releases/mozilla-esr52/rev/
> c05ea3d4321bce6c8d6ab8460eec44b277710f1b

I guess I am confused but it seems 57.0.1 got out and includes the fix. So, "wontfix" seems wrong. Or am I missing something?
Flags: needinfo?(ryanvm)
We're building 57.0.2 with it backed out.
Flags: needinfo?(ryanvm)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #60)
> We're building 57.0.2 with it backed out.

I argued against doing this.
Assignee

Comment 62

2 years ago
Attachment #8911233 - Attachment is obsolete: true
Assignee

Comment 63

2 years ago
Of note is that we updated ANGLE in 51, 55, and 58. The update in 55 is a point of uncertainty between release57 and esr52.
Assignee

Comment 64

2 years ago
Here's a testcase that'll show whether 0-attrib rendering works. (or asserts)
Assignee

Comment 65

2 years ago
FWIW, release57 asserts in my local build, running webgl-torture-attribs. After finishing here, I will look at why release57 wasn't seeing asserts in our CI testcases for this.
Assignee

Comment 66

2 years ago
As such, I'm encouraged to ship the old fix with a fix-up of fixing the assert.
Assignee

Comment 67

2 years ago
r+'d previously
Attachment #8933419 - Attachment is obsolete: true
Attachment #8933467 - Attachment is obsolete: true
Attachment #8934859 - Flags: review+
Assignee

Comment 69

2 years ago
This fix doesn't seem to regress on webgl-torture-attribs compared to esr52.5.0.
Flags: needinfo?(rkothari)
Do 58/59 need this revised patch still?
Flags: needinfo?(milan) → needinfo?(jgilbert)
Updating the tracking flag to blocking since we held back 57.0.2/esr52.5.1 on this one. Thanks Jeff for pushing these fixes to m-r/m-esr52.
Assignee

Comment 73

2 years ago
ANGLE rejects the bad call in 58+.
Flags: needinfo?(jgilbert)

Comment 74

2 years ago
Jeff, is the code path for this bug generic enough that users are affected as soon as they have any WebGL enabled or would it be fine for those that only have minimum capability mode on and no extensions and WebGL2 allowed? From looking at the PoC and various patch proposals in this bug it seems to me having all those things set/disabled would mitigate the problem. I am still trying to understand the impact of this bug for Tor Browser users. (see my comment 40)
Flags: needinfo?(jgilbert)
Our team can confirm that the initial issue is not reproducible anymore using 57.0.2-build2 (20171206182557)and 52.5.2esr-build2 (20171206101620) on Windows 10 x64, Windows 7 x64 and Ubuntu 16.04 x64. 
On macOS 10.13	(and Mac OS X 10.11.6) the 2 above mentioned builds trigger @ GLEngine@0x147a9d and @ mozilla::WebGLContext::GetError crashes (with and without hardware acceleration enabled) when using the testcase from comment 64 (the one from comment 62 is only opening a blank page). The 52.5.0esr (20171107091003) build has the same behavior. 
Any thoughts about this?
Added the follow up in the release notes with "Fix a regression with WebGL and D3D9 - Windows only" as wording.
Assignee

Comment 77

2 years ago
(In reply to Georg Koppen from comment #74)
> Jeff, is the code path for this bug generic enough that users are affected
> as soon as they have any WebGL enabled or would it be fine for those that
> only have minimum capability mode on and no extensions and WebGL2 allowed?
> From looking at the PoC and various patch proposals in this bug it seems to
> me having all those things set/disabled would mitigate the problem. I am
> still trying to understand the impact of this bug for Tor Browser users.
> (see my comment 40)

All users who are using instancing. I don't remember if instancing is supported by min-cap mode. (Instancing has ~100% support, otherwise)
Flags: needinfo?(jgilbert)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.