Last Comment Bug 787493 - (CVE-2012-4187) Crash with ASSERTION: insPos too small
: Crash with ASSERTION: insPos too small
: assertion, crash, csectype-wildptr, sec-critical, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla18
Assigned To: Olli Pettay [:smaug]
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2012-08-31 11:05 PDT by Atte Kettunen
Modified: 2014-07-24 13:44 PDT (History)
9 users (show)
dveditz: sec‑bounty+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Repro-file (625 bytes, text/html)
2012-08-31 11:05 PDT, Atte Kettunen
no flags Details
Whole asan-report (12.70 KB, text/plain)
2012-08-31 11:28 PDT, Atte Kettunen
no flags Details
More reliable repro-file (3.84 KB, text/html)
2012-09-01 01:32 PDT, Atte Kettunen
no flags Details
patch (981 bytes, patch)
2012-09-01 14:15 PDT, Olli Pettay [:smaug]
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Atte Kettunen 2012-08-31 11:05:47 PDT
Created attachment 657365 [details]

Reproducible with built from

OS: Ubuntu 12.04 x86_64

The reprofile is not stable and it might take few tries to reproduce.


==11689== ERROR: AddressSanitizer crashed on unknown address 0x7f00dca7af80 (pc 0x7efff2512520 sp 0x7fffdf81b7d8 bp 0x7fffdf81b820 T0)
AddressSanitizer can not provide additional info. ABORTING
    #0 0x7efff2512520 in ?? /build/buildd/eglibc-2.15/string/../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S:2577
    #1 0x7effe7e9c1bf in nsTArray_base<nsTArrayDefaultAllocator>::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int, unsigned long) /builds/slave/try-lnx64-dbg/build/../../../dist/include/nsTArray-inl.h:247
    #2 0x7effe8cb2b47 in nsRefPtr<nsHTMLOptionElement>* nsTArray<nsRefPtr<nsHTMLOptionElement>, nsTArrayDefaultAllocator>::ReplaceElementsAt<nsHTMLOptionElement*>(unsigned int, unsigned int, nsHTMLOptionElement* const*, unsigned int) /builds/slave/try-lnx64-dbg/build/../../../../dist/include/nsTArray.h:724
    #3 0x7effe8ca6367 in nsHTMLOptionCollection::InsertOptionAt(nsHTMLOptionElement*, unsigned int) /builds/slave/try-lnx64-dbg/build/content/html/content/src/nsHTMLSelectElement.h:68
    #4 0x7effe8ca5392 in nsHTMLSelectElement::InsertOptionsIntoListRecurse(nsIContent*, int*, int) /builds/slave/try-lnx64-dbg/build/content/html/content/src/nsHTMLSelectElement.cpp:338
    #5 0x7effe8ca4d8e in nsHTMLSelectElement::InsertOptionsIntoList(nsIContent*, int, int, bool) /builds/slave/try-lnx64-dbg/build/content/html/content/src/nsHTMLSelectElement.cpp:215
    #6 0x7effe8ca65fb in nsHTMLSelectElement::WillAddOptions(nsIContent*, nsIContent*, int, bool) /builds/slave/try-lnx64-dbg/build/content/html/content/src/nsHTMLSelectElement.cpp:455
    #7 0x7effe8ca3881 in nsSafeOptionListMutation /builds/slave/try-lnx64-dbg/build/content/html/content/src/nsHTMLSelectElement.cpp:73
    #8 0x7effe8ca4b51 in nsHTMLSelectElement::InsertChildAt(nsIContent*, unsigned int, bool) /builds/slave/try-lnx64-dbg/build/content/html/content/src/nsHTMLSelectElement.cpp:191
Comment 1 Atte Kettunen 2012-08-31 11:28:41 PDT
Created attachment 657372 [details]
Whole asan-report

Also before that ASAN-report there is following assertion

###!!! ASSERTION: insPos too small: 'insPos >= 1', file /builds/slave/try-lnx64-dbg/build/content/base/src/nsINode.cpp, line 1798

There is also few warnings you can see in the attached full ASAN-report.
Comment 2 Mounir Lamouri (:mounir) 2012-08-31 18:36:12 PDT
It's really hard to reproduce this actually. I've been able to do it once but no more in a few dozen tries.
I'm not really familiar with the involved code too. I don't know who is though.
Comment 3 Atte Kettunen 2012-09-01 01:32:13 PDT
Created attachment 657541 [details]
More reliable repro-file

I think that this repro-file is much more reliable than the original one.
Comment 4 Olli Pettay [:smaug] 2012-09-01 11:18:03 PDT
I can reproduce the crash using the first testcase but not using the latter one.
Comment 5 Atte Kettunen 2012-09-01 11:25:53 PDT
For my laptop that latter file is almost 100% sure. There might be some race-condition. You could try to change the amount of the duplicated line inside the crash-function in the testcase, or try to change the for-loop rounds in that function in the first testcase.
Comment 6 Olli Pettay [:smaug] 2012-09-01 14:15:01 PDT
Created attachment 657601 [details] [diff] [review]

This fixes the original testcase. No idea about the latter one.
(I hate mutation events.)
Comment 7 Olli Pettay [:smaug] 2012-09-01 14:29:20 PDT
(I could use mutationguard too, but not sure it is worth.)
Comment 8 Mats Palmgren (:mats) 2012-09-01 17:56:14 PDT
The 2nd testcase crashes reliably for me in a Linux64 debug build (non-asan).
It crashes in memmove() called from
(gdb) list
240         start *= elemSize;
241         newLen *= elemSize;
242         oldLen *= elemSize;
243         num *= elemSize;
244         char *base = reinterpret_cast<char*>(mHdr + 1) + start;
245         memmove(base + newLen, base + oldLen, num);
246       }
247     }
(gdb) p base
$1 = 0x8000ddbc2490 <Address 0x8000ddbc2490 out of bounds>
(gdb) p start
$2 = 4294967288
(gdb) p/x start
$3 = 0xfffffff8

If something happened to be allocated at |base + newLen| then it's
potentially exploitable?

The patch fixed it for me.

There's about 2000 crash-stats reports in the past 4 weeks that match
"memmove | nsTArray_base<nsTArrayDefaultAllocator>::ShiftData".
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-09-04 21:26:34 PDT
Comment on attachment 657601 [details] [diff] [review]

Comment 11 Daniel Veditz [:dveditz] 2012-09-06 16:57:18 PDT
This appears to affect ESR-10 as well from code inspection (don't have an esr-10 ASAN build)
Comment 12 Andrew McCreight [:mccr8] 2012-09-13 16:51:09 PDT
Olli is this ready to be landed?
Comment 13 Olli Pettay [:smaug] 2012-09-13 17:03:12 PDT
Yes, but this particular bug is so obvious that I'd like to land it as close to next release as possible.
Comment 14 Andrew McCreight [:mccr8] 2012-09-13 17:07:28 PDT
Great, just making sure you were not landing it on purpose. :)
Comment 15 Alex Keybl [:akeybl] 2012-09-19 17:54:47 PDT
(In reply to Olli Pettay [:smaug] from comment #13)
> Yes, but this particular bug is so obvious that I'd like to land it as close
> to next release as possible.

Now's the time to land and nominate for uplift without putting the FF16 release at risk.
Comment 16 Olli Pettay [:smaug] 2012-09-20 02:15:19 PDT
Comment on attachment 657601 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): NA
User impact if declined: Possible security sensitive crash
Testing completed (on m-c, etc.): tested only locally
Risk to taking this patch (and alternatives if risky): The patch is effectively just a null check
String or UUID changes made by this patch: NA
Comment 17 Olli Pettay [:smaug] 2012-09-20 11:03:50 PDT
Comment 18 Alex Keybl [:akeybl] 2012-09-20 15:46:30 PDT
Comment on attachment 657601 [details] [diff] [review]

[Triage Comment]
Approving for Aurora in preparation for Beta consideration.
Comment 19 Alex Keybl [:akeybl] 2012-09-21 15:51:15 PDT
Comment on attachment 657601 [details] [diff] [review]

[Triage Comment]
We've now had the opportunity to discuss in triage - let's land on Beta 16 and ESR10 before Tuesday.
Comment 20 Andrew McCreight [:mccr8] 2012-09-22 10:21:09 PDT
Comment 21 Olli Pettay [:smaug] 2012-09-22 10:48:51 PDT
Thanks for landing
Comment 22 Olli Pettay [:smaug] 2012-09-22 10:52:27 PDT
I see you landed also to beta. I assume you'll do also esr.
Comment 23 Andrew McCreight [:mccr8] 2012-09-22 10:58:06 PDT
Yup. I'm just making sure it builds locally because esr10 is a little different...
Comment 24 Andrew McCreight [:mccr8] 2012-09-22 11:08:50 PDT
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-26 13:27:27 PDT
Both testcases reliably crash Firefox 18.0a1 Nightly debug 2012-09-01 (non-asan) for me.

Neither testcase crashes:
 * Firefox 18.0a1 Nightly debug (non-asan) 2012-09-21
 * Firefox 17.0a2 Aurora debug (non-asan) 2012-09-23
 * Firefox 16.0 Beta debug (non-asan) 2012-09-23
 * Firefox 10.0.8esrpre esr10 debug (non-asan) 2012-09-23

Question, is that sufficient to call this verified, or does the verification require checking against ASan builds?
Comment 26 Olli Pettay [:smaug] 2012-09-26 18:00:42 PDT
I never used ASAn builds with this. The problem is clear so verifying without asan should be enough.
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-27 14:37:48 PDT
Thanks :smaug, marking this verified across the board based on my results in comment 25.

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