Bug 787493 (CVE-2012-4187)

Crash with ASSERTION: insPos too small

VERIFIED FIXED in Firefox 16

Status

()

Core
DOM: Core & HTML
--
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Atte Kettunen, Assigned: smaug)

Tracking

(5 keywords)

Trunk
mozilla18
x86_64
Linux
assertion, crash, csectype-wildptr, sec-critical, testcase
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox16+ verified, firefox17+ verified, firefox18+ verified, firefox-esr1016+ verified)

Details

(Whiteboard: [asan][advisory-tracking+])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
Created attachment 657365 [details]
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
.
.
.
(Reporter)

Comment 1

5 years ago
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.

Updated

5 years ago
Severity: normal → critical
Component: General → DOM: Core & HTML
Keywords: assertion, crash, testcase
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.
(Reporter)

Comment 3

5 years ago
Created attachment 657541 [details]
More reliable repro-file

I think that this repro-file is much more reliable than the original one.
(Assignee)

Updated

5 years ago
Attachment #657541 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

5 years ago
Attachment #657365 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 4

5 years ago
I can reproduce the crash using the first testcase but not using the latter one.
(Reporter)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 657601 [details] [diff] [review]
patch

This fixes the original testcase. No idea about the latter one.
(I hate mutation events.)
Assignee: nobody → bugs
Attachment #657601 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

5 years ago
(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+
status-firefox-esr10: --- → ?
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox16: --- → +
tracking-firefox17: --- → +
tracking-firefox18: --- → +
Keywords: csec-wildptr, sec-critical
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
status-firefox-esr10: ? → affected
tracking-firefox-esr10: ? → 16+

Updated

5 years ago
tracking-firefox-esr10: 16+ → ---
Olli is this ready to be landed?
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 16

5 years ago
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?
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/mozilla-central/rev/7748dba7f579
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
tracking-firefox-esr10: --- → ?
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+

Updated

5 years ago
tracking-firefox-esr10: ? → 16+
status-firefox18: affected → fixed
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
https://hg.mozilla.org/releases/mozilla-aurora/rev/01d038d3d45c
status-firefox17: affected → fixed
(Assignee)

Comment 21

5 years ago
Thanks for landing
(Assignee)

Comment 22

5 years ago
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
status-firefox16: affected → fixed
https://hg.mozilla.org/releases/mozilla-esr10/rev/03d28718012f
status-firefox-esr10: affected → fixed
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?
(Assignee)

Comment 26

5 years ago
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
status-firefox-esr10: fixed → verified
status-firefox16: fixed → verified
status-firefox17: fixed → verified
status-firefox18: fixed → 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.