Closed Bug 787493 (CVE-2012-4187) Opened 7 years ago Closed 7 years ago

Crash with ASSERTION: insPos too small

Categories

(Core :: DOM: Core & HTML, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox16 + verified
firefox17 + verified
firefox18 + verified
firefox-esr10 16+ verified

People

(Reporter: attekett, Assigned: smaug)

Details

(5 keywords, Whiteboard: [asan][advisory-tracking+])

Attachments

(4 files)

Attached file Repro-file
Reproducible with built from 

https://people.mozilla.com/~choller/firefox/asan/20120831-mozilla-central-linux64-debug-fcc533f691e9+asan.html

OS: Ubuntu 12.04 x86_64

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

ASAN-report:

ASAN:SIGSEGV
==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
.
.
.
Attached file 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.
Severity: normal → critical
Component: General → DOM: Core & HTML
Product: Firefox → Core
Whiteboard: [asan]
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.
I think that this repro-file is much more reliable than the original one.
Attachment #657541 - Attachment mime type: text/plain → text/html
Attachment #657365 - Attachment mime type: text/plain → text/html
I can reproduce the crash using the first testcase but not using the latter one.
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.
Attached patch patchSplinter Review
This fixes the original testcase. No idea about the latter one.
(I hate mutation events.)
Assignee: nobody → bugs
Attachment #657601 - Flags: review?(bzbarsky)
(I could use mutationguard too, but not sure it is worth.)
The 2nd testcase crashes reliably for me in a Linux64 debug build (non-asan).
It crashes in memmove() called from
nsTArray_base<nsTArrayDefaultAllocator>::ShiftData:
(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     }
248
(gdb) p base
$1 = 0x8000ddbc2490 <Address 0x8000ddbc2490 out of bounds>
(gdb) p start
$2 = 4294967288
(gdb) p/x start
$3 = 0xfffffff8
(gdb) 

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 on attachment 657601 [details] [diff] [review]
patch

r=me
Attachment #657601 - Flags: review?(bzbarsky) → review+
This appears to affect ESR-10 as well from code inspection (don't have an esr-10 ASAN build)

http://mxr.mozilla.org/mozilla-esr10/source/content/base/src/nsGenericElement.cpp#4077
Olli is this ready to be landed?
Yes, but this particular bug is so obvious that I'd like to land it as close to next release as possible.
Great, just making sure you were not landing it on purpose. :)
(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 on attachment 657601 [details] [diff] [review]
patch

[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
Attachment #657601 - Flags: approval-mozilla-esr10?
Attachment #657601 - Flags: approval-mozilla-beta?
Attachment #657601 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7748dba7f579
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 657601 [details] [diff] [review]
patch

[Triage Comment]
Approving for Aurora in preparation for Beta consideration.
Attachment #657601 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 657601 [details] [diff] [review]
patch

[Triage Comment]
We've now had the opportunity to discuss in triage - let's land on Beta 16 and ESR10 before Tuesday.
Attachment #657601 - Flags: approval-mozilla-esr10?
Attachment #657601 - Flags: approval-mozilla-esr10+
Attachment #657601 - Flags: approval-mozilla-beta?
Attachment #657601 - Flags: approval-mozilla-beta+
Target Milestone: --- → mozilla18
Thanks for landing
I see you landed also to beta. I assume you'll do also esr.
Yup. I'm just making sure it builds locally because esr10 is a little different...

https://hg.mozilla.org/releases/mozilla-beta/rev/77f02bc310f4
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?
I never used ASAn builds with this. The problem is clear so verifying without asan should be enough.
Thanks :smaug, marking this verified across the board based on my results in comment 25.
Status: RESOLVED → VERIFIED
QA Contact: anthony.s.hughes
Whiteboard: [asan] → [asan][advisory-tracking+]
Alias: CVE-2012-4187
Group: core-security
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.