WebGL: stack-buffer-underflow [@mozilla::detail::GuardObjectNotificationReceiver::GuardObjectNotificationReceiver]

VERIFIED FIXED in Firefox 30, Firefox OS v1.2

Status

()

--
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: posidron, Assigned: kamidphish)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla30
x86_64
Mac OS X
crash, csectype-bounds, sec-critical, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox27 wontfix, firefox28 wontfix, firefox29+ wontfix, firefox30+ verified, firefox-esr2430+ verified, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, seamonkey2.26 unaffected)

Details

(Whiteboard: [adv-main30+][adv-esr24.6+])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 8369836 [details]
testcase

WebGL: copyTexImage2D: the maximum texture size for level 268435456 is 16384

The stack for the stack-buffer-underflow for an optimized build seems mangled.

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/53489b3e14f1
(Reporter)

Comment 1

5 years ago
Created attachment 8369837 [details]
callstack
Haha, fun! Thanks!
Assignee: nobody → dglastonbury
status-firefox29: --- → affected
status-firefox30: --- → affected
tracking-firefox30: --- → +
Any updates on this security bug, Dan?
status-firefox28: --- → ?
Flags: needinfo?(dglastonbury)
It's fixed in 966624, which I'm in the process of trying to get landed.
Flags: needinfo?(dglastonbury)
Fixed by Bug 966624.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox28: ? → ---
Resolution: --- → FIXED
status-firefox30: affected → fixed
Target Milestone: --- → mozilla30
This is a sec-critical that affects Firefox 29. Does it go back further? We should get this backported to Aurora.

Ideally, things that affect multiple versions and which are security issues get sec-approval+ before going in. https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: needinfo?(dglastonbury)
status-firefox28: --- → ?
status-firefox-esr24: --- → ?
This likely goes back further, but we should check.
Matt, can you or someone else in QA check with older builds?
Flags: needinfo?(dglastonbury) → needinfo?(mwobensmith)
I tested the following, and they are all affected:
 * aurora
 * ff-esr24
 * ff28
We should get this on Aurora, Beta, and ESR24.
status-firefox27: --- → wontfix
status-firefox28: ? → affected
status-firefox-esr24: ? → affected
Flags: needinfo?(mwobensmith)
tracking-firefox29: --- → +
tracking-firefox-esr24: --- → 29+
I just found this again. Is there a reason we didn't backport this?
Flags: needinfo?(dglastonbury)
I tested Aurora debug and see no crash and the following in the console:
  "Error: WebGL: copyTexImage2D: for cube map, width must equal height"

I noticed the reported crash stack was using ASAN so I'll recompile with ASAN and repeat the test.
Aurora: Ran with non-ASAN and ASAN debug builds. No stack underflow. JavaScript console contains:
    "Error: WebGL: copyTexImage2D: for cube map, width must equal height"
We are building our final FF29 build on Monday - this has to be able to land safely to the branch (will be on mozilla-release) before that build.  Please get patches ready and tested by then.
OK we're past the time of safely landing this to branch with time to ensure it's not going to pose risk to quality/stability of the release.  Since this is fixed in 30 we will see it go to GA at that time.  We will need an ESR patch for this though - can someone prepare that and nominate for uplift?
status-firefox28: affected → wontfix
status-firefox29: affected → wontfix
tracking-firefox-esr24: 29+ → 30+
Dan, ESR24 lives: http://hg.mozilla.org/releases/mozilla-esr24/, though you probably already used it.
Milan: I have already been working on a patch for ESR 24. I'll upload for review from Jeff.
Flags: needinfo?(dglastonbury)
Created attachment 8409952 [details] [diff] [review]
Fix incorrect usage of UpdateWebGLErrorAndClearGLError()

Error checking in CopyTexImage2D appears to be incorrect. GetError()
should be used instead. Updated all occurences where
UpdateWebGLErrorAndClearGLError() appears to be used incorrectly.
Attachment #8409952 - Flags: review?(jgilbert)
Created attachment 8410010 [details] [diff] [review]
Fix incorrect usage of UpdateWebGLErrorAndClearGLError()

Following discussion with Jeff on IRC, make CopyTexSubImage2D_base
return bool for success. Check this result as well as for real GL
errors.
Attachment #8410010 - Flags: review?(jgilbert)
Attachment #8409952 - Attachment is obsolete: true
Attachment #8409952 - Flags: review?(jgilbert)
Attachment #8410010 - Flags: review?(jgilbert) → review+
Dan - We're approaching the end of the latest ESR24 cycle. Can you confirm that the ESR24 patch is ready to land and, if so, land it ASAP?
Flags: needinfo?(dglastonbury)
(In reply to Lawrence Mandel [:lmandel] from comment #20)
> Dan - We're approaching the end of the latest ESR24 cycle. Can you confirm
> that the ESR24 patch is ready to land and, if so, land it ASAP?

Given Jeff's r+, I believe it's ready to land. I'll check it still applies and refresh if necessary.
Flags: needinfo?(dglastonbury)
Comment on attachment 8410010 [details] [diff] [review]
Fix incorrect usage of UpdateWebGLErrorAndClearGLError()

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Ability to underflow stack by passing bad parameters to WebGL functions 
Fix Landed on Version: 966624
Risk to taking this patch (and alternatives if risky): Low risk. Patch adds checking for error code and doesn't call into OpenGL if there's an error.
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8410010 - Flags: approval-mozilla-esr24?
Comment on attachment 8410010 [details] [diff] [review]
Fix incorrect usage of UpdateWebGLErrorAndClearGLError()

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

ESR approval granted. Please land this week.
Attachment #8410010 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
The patch from bug 966624 is going to need some serious rebasing for b2g28/b2g26/esr24.
status-b2g-v1.2: --- → affected
status-b2g-v1.3: --- → affected
status-b2g-v1.3T: --- → affected
status-b2g-v1.4: --- → fixed
Flags: needinfo?(dglastonbury)
Keywords: branch-patch-needed
status-firefox-esr24: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/803260835c58
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8f952940d1c1

Should this test land on m-c still or is coverage already sufficient there as-is?
status-b2g-v1.2: affected → fixed
status-b2g-v1.3: affected → fixed
Flags: needinfo?(dglastonbury)
Flags: in-testsuite?
Keywords: branch-patch-needed
status-b2g-v1.3T: affected → fixed
Whiteboard: [adv-main30+][adv-esr24.6+]
Confirmed crash on 2014-02-11, Fx30.
Verified fixed on release candidate builds of Fx24.6.0esr and Fx30.
Status: RESOLVED → VERIFIED
status-firefox30: fixed → verified
status-firefox-esr24: fixed → verified
Ryan,
  I'll discuss it with :jgilbert.
Flags: needinfo?(dglastonbury)
I've checked the code and testcase with Aurora and Nightly and in those code bases, I introduced ValidateTexImage which errors and causes an early out in CopyTexImage2D so the bad value never reaches the driver, as was possible in ESR 24.
Per testcase, SeaMonkey 2.26 (based on gecko 29) is unaffected:

o2 = document.createElement('canvas').getContext("webgl");

/* o2 == undefined */
status-seamonkey2.26: --- → unaffected
Ryan - I don't see a commit comment for 1.3T or 1.4. You changed those flags. Can you confirm that this bug was fixed on those branches?
Flags: needinfo?(ryanvm)
It landed on trunk for mozilla30, hence v1.4. v1.3 merges to v1.3t daily, hence 1.3T.
Flags: needinfo?(ryanvm)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #32)
> It landed on trunk for mozilla30, hence v1.4. v1.3 merges to v1.3t daily,
> hence 1.3T.

Right. Missed that timing while looking at a large pile of bugs. Thanks.
Group: core-security
You need to log in before you can comment on or make changes to this bug.