787493 - (CVE-2012-4187) Crash with ASSERTION: insPos too small

Status: VERIFIED FIXED

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

Description

[Atte Kettunen]

Attachment: 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
.
.
.

Comment 1

[Atte Kettunen]

Attachment: 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)]

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]

Attachment: More reliable repro-file

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

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I can reproduce the crash using the first testcase but not using the latter one.

Comment 5

[Atte Kettunen]

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][[e-mail]]]

Attachment: patch

This fixes the original testcase. No idea about the latter one.
(I hate mutation events.)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(I could use mutationguard too, but not sure it is worth.)

Comment 8

[Mats Palmgren (inactive)]

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 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 657601 [details] [diff] [review]
patch

r=me

Comment 11

[Daniel Veditz [:dveditz]]

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

Comment 12

[Andrew McCreight [:mccr8]]

Olli is this ready to be landed?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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]]

Great, just making sure you were not landing it on purpose. :)

Comment 15

[Alex Keybl [:akeybl]]

(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][[e-mail]]]

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

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/7748dba7f579

Comment 18

[Alex Keybl [:akeybl]]

Comment on attachment 657601 [details] [diff] [review]
patch

[Triage Comment]
Approving for Aurora in preparation for Beta consideration.

Comment 19

[Alex Keybl [:akeybl]]

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.

Comment 20

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/01d038d3d45c

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Thanks for landing

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

I see you landed also to beta. I assume you'll do also esr.

Comment 23

[Andrew McCreight [:mccr8]]

Yup. I'm just making sure it builds locally because esr10 is a little different...

https://hg.mozilla.org/releases/mozilla-beta/rev/77f02bc310f4

Comment 24

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/03d28718012f

Comment 25

[u279076]

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][[e-mail]]]

I never used ASAn builds with this. The problem is clear so verifying without asan should be enough.

Comment 27

[u279076]

Thanks :smaug, marking this verified across the board based on my results in comment 25.