Closed Bug 765179 Opened 8 years ago Closed 8 years ago

WebGL crash when empty string is passed to getUniformLocation, getAttribLocation or bindAttribLocation [@mozilla::WebGLProgram::MapIdentifier]

Categories

(Core :: Canvas: WebGL, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla15
Tracking Status
firefox14 --- fixed
firefox15 --- fixed
firefox16 --- fixed
firefox-esr10 --- unaffected

People

(Reporter: posidron, Assigned: bjacob)

References

Details

(4 keywords, Whiteboard: [asan][advisory-tracking+] webgl-test-needed)

Attachments

(4 files)

Attached file 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, []);
Attached file callstack
Group: core-security
[] stringifies to "", so this is exactly the same as bug 765161.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 765161
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.
Attachment #633539 - Flags: review?
Boris: is it normal that the DOM bindings silently accept [] as a DOMString parameter and silently convert [] to an empty string?
Oh, I hadn't seen comment 2. OK.
Moving to bug 765161 which is more to the point indeed.
Attachment #633539 - Flags: review?
No valgrind errors with this patch.
> 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 "".
As Jeff points out, this is the real fix, the other patch was just avoiding the problem. For safety, we probably want both.
Attachment #633661 - Flags: review?(jgilbert)
Attachment #633661 - Flags: review?(jgilbert) → review+
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.
Attachment #633539 - Flags: review+
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.
Assignee: nobody → bjacob
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Whiteboard: [asan] → [asan] webgl-test-needed
Target Milestone: --- → mozilla15
Duplicate of this bug: 765161
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
Attachment #633539 - Flags: approval-mozilla-aurora?
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
Attachment #633661 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/cbe259c0ba18
https://hg.mozilla.org/mozilla-central/rev/cebbfb5c0e02
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
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 on attachment 633539 [details] [diff] [review]
check for empty variable names

[Approval Request Comment]
Same as for aurora. This does affect beta as well.
Attachment #633539 - Flags: approval-mozilla-beta?
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.
Attachment #633661 - Flags: approval-mozilla-beta?
Summary: WebGL crash with bindAttribLocation bracket parameter [@mozilla::WebGLProgram::MapIdentifier] → WebGL crash when empty string is passed to getUniformLocation, getAttribLocation or bindAttribLocation [@mozilla::WebGLProgram::MapIdentifier]
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?
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.
(I reproduced on first try on Linux 64-bit)
Attachment #633539 - Flags: approval-mozilla-beta?
Attachment #633539 - Flags: approval-mozilla-beta+
Attachment #633539 - Flags: approval-mozilla-aurora?
Attachment #633539 - Flags: approval-mozilla-aurora+
Attachment #633661 - Flags: approval-mozilla-beta?
Attachment #633661 - Flags: approval-mozilla-beta+
Attachment #633661 - Flags: approval-mozilla-aurora?
Attachment #633661 - Flags: approval-mozilla-aurora+
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.
I can verify that the bug is fixed.
Status: RESOLVED → VERIFIED
Whiteboard: [asan] webgl-test-needed → [asan][advisory-tracking+] webgl-test-needed
Group: core-security
You need to log in before you can comment on or make changes to this bug.