Last Comment Bug 765179 - WebGL crash when empty string is passed to getUniformLocation, getAttribLocation or bindAttribLocation [@mozilla::WebGLProgram::MapIdentifier]
: WebGL crash when empty string is passed to getUniformLocation, getAttribLocat...
Status: VERIFIED FIXED
[asan][advisory-tracking+] webgl-test...
: crash, regression, sec-critical, testcase
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla15
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
: 765161 (view as bug list)
Depends on:
Blocks: 658170
  Show dependency treegraph
 
Reported: 2012-06-15 03:43 PDT by Christoph Diehl [:posidron]
Modified: 2012-07-20 18:30 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed
unaffected


Attachments
testcase (1.60 KB, text/html)
2012-06-15 03:43 PDT, Christoph Diehl [:posidron]
no flags Details
callstack (6.70 KB, text/plain)
2012-06-15 03:43 PDT, Christoph Diehl [:posidron]
no flags Details
check for empty variable names (905 bytes, patch)
2012-06-15 08:28 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
check for empty string in SplitLastSquareBracket (1.09 KB, patch)
2012-06-15 14:04 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Christoph Diehl [:posidron] 2012-06-15 03:43:09 PDT
Created attachment 633467 [details]
testcase

This bug is related to bug 765161. 

A square bracket value as the third parameter of bindAttribLocation() will result in a crash. An empty string '' will also lead to a crash. Although this bug is most likely caused by the same issue described in bug 765161, I have created a separate bug for this particular method.

gl.bindAttribLocation(pg, 1, []);
Comment 1 Christoph Diehl [:posidron] 2012-06-15 03:43:42 PDT
Created attachment 633469 [details]
callstack
Comment 2 Boris Zbarsky [:bz] 2012-06-15 07:43:53 PDT
[] stringifies to "", so this is exactly the same as bug 765161.

*** This bug has been marked as a duplicate of bug 765161 ***
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 08:28:07 PDT
Created attachment 633539 [details] [diff] [review]
check for empty variable names

This should fix it, although i am running in Valgrind at the moment to confirm.

When the testcase calls BindAttribLocation with [] as the attribute name string parameter, the string that the DOM bindings pass to WebGLContext::BindAttribLocation is an empty string (but not a null string). So IsEmpty() is the right check for that. Doing it in ValidateGLSLVariableName() allows to fix this bug also in other functions that take such strings and do this long-identifier-mapping work on them: GetAttribLocation and GetUniformLocation.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 08:29:07 PDT
Boris: is it normal that the DOM bindings silently accept [] as a DOMString parameter and silently convert [] to an empty string?
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 08:31:06 PDT
Oh, I hadn't seen comment 2. OK.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 08:31:33 PDT
Moving to bug 765161 which is more to the point indeed.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 08:37:01 PDT
No valgrind errors with this patch.
Comment 8 Boris Zbarsky [:bz] 2012-06-15 08:50:02 PDT
> Boris: is it normal that the DOM bindings silently accept [] as a DOMString parameter 

Yes.  When something is passed to a string parameter, the normal ToString() algorithm from the JS spec is applied; for an empty array that produces "".
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 14:04:49 PDT
Created attachment 633661 [details] [diff] [review]
check for empty string in SplitLastSquareBracket

As Jeff points out, this is the real fix, the other patch was just avoiding the problem. For safety, we probably want both.
Comment 10 Jeff Gilbert [:jgilbert] 2012-06-15 14:09:12 PDT
Comment on attachment 633539 [details] [diff] [review]
check for empty variable names

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

This is good to have even just as an early-out.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 15:08:37 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/cbe259c0ba18
http://hg.mozilla.org/integration/mozilla-inbound/rev/cebbfb5c0e02

Reopening just because the patch review ended up happening on this bug rather than the other one.
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 15:09:36 PDT
*** Bug 765161 has been marked as a duplicate of this bug. ***
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 15:10:55 PDT
Comment on attachment 633539 [details] [diff] [review]
check for empty variable names

[Approval Request Comment]
Bug caused by (feature/regressing bug #): at least the crash on empty string has occurred ever since we did long identifier mapping (Gecko 13).
User impact if declined: serious crash, security sensitive
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): no risk, trivial patch checking for empty strings
String or UUID changes made by this patch: none
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 15:11:10 PDT
Comment on attachment 633661 [details] [diff] [review]
check for empty string in SplitLastSquareBracket

[Approval Request Comment]
Bug caused by (feature/regressing bug #): at least the crash on empty string has occurred ever since we did long identifier mapping (Gecko 13).
User impact if declined: serious crash, security sensitive
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): no risk, trivial patch checking for empty strings
String or UUID changes made by this patch: none
Comment 16 Daniel Veditz [:dveditz] 2012-06-16 09:45:20 PDT
I'm not sure what "sec-other" signifies here, that means "not s security bug, hidden for some other reason" but this looks like it's either a security bug or not sensitive at all.

If you were clever enough to arrange memory blocks (see upcoming blackhat talk on jemalloc) so that the block allocated next to string ended in a ']' then SplitLastSquareBracket would back up into some other block's memory until you found a '[' (or crashed harmlessly on an access violation, but we're already assuming a clever attacker who arranged a ']' at the end of the block). Then it'd slurp up the contents into the bracketPart (could the attacker read this back?) and stomp a null in this other block. Potentially exploitable but wouldn't be easy.

Shouldn't we try to get this into Beta as well?
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2012-06-16 10:44:44 PDT
Comment on attachment 633539 [details] [diff] [review]
check for empty variable names

[Approval Request Comment]
Same as for aurora. This does affect beta as well.
Comment 18 Benoit Jacob [:bjacob] (mostly away) 2012-06-16 10:45:13 PDT
Comment on attachment 633661 [details] [diff] [review]
check for empty string in SplitLastSquareBracket

[Approval Request Comment]
Same as for aurora. This does affect beta as well.
Comment 19 Al Billings [:abillings] 2012-06-18 06:33:03 PDT
I just tried the testcase on OS X with the current Aurora build (6-18) and the last released Firefox and I cannot get a crash with it. Is there a particular video card or environment needed to trigger this issue?
Comment 20 Benoit Jacob [:bjacob] (mostly away) 2012-06-18 06:43:59 PDT
No, it should be independent of the video card, but since it's undefined behavior depending on the contents of memory beyond the end of an allocated buffer, it's not too surprising that this doesn't reproduce everywhere.
Comment 21 Benoit Jacob [:bjacob] (mostly away) 2012-06-18 06:44:20 PDT
(I reproduced on first try on Linux 64-bit)
Comment 22 Al Billings [:abillings] 2012-06-19 04:47:06 PDT
All right. Well, I have OS X and virtual machines of others, which may or may not help. I tried in Windows 7 and Ubuntu 12.04 64-bit and could not reproduce the crash in these either. I guess someone in QA will have to verify this fix.
Comment 23 Christoph Diehl [:posidron] 2012-06-19 05:01:44 PDT
I can verify that the bug is fixed.

Note You need to log in before you can comment on or make changes to this bug.