888820 - (CVE-2013-1720) Heap-buffer-overflow READ in nsHtml5TreeBuilder::resetTheInsertionMode()

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Atte Kettunen]

Attachment: Repro-file

Tested on:

OS: Ubuntu 12.04
Firefox: ASAN debug-build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-dbg-asan/1372636004/


ASAN-report:

==8023== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fdd5ba3d03c at pc 0x7fdd88de045b bp 0x7fdd67267480 sp 0x7fdd67267478
READ of size 4 at 0x7fdd5ba3d03c thread T16
    #0 0x7fdd88de045a in nsHtml5TreeBuilder::resetTheInsertionMode() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/parser/html/nsHtml5TreeBuilder.cpp:3247
    #1 0x7fdd88de39e6 in nsHtml5TreeBuilder::eof() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/parser/html/nsHtml5TreeBuilder.cpp:529
    #2 0x7fdd88dc4cf2 in nsHtml5StreamParser::ParseAvailableData() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/parser/html/nsHtml5StreamParser.cpp:1293
    #3 0x7fdd88dc8a24 in nsHtml5StreamParserContinuation::Run() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/parser/html/nsHtml5StreamParser.cpp:1355
    #4 0x7fdd8a83e1fb in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/xpcom/threads/nsThread.cpp:626
    #5 0x7fdd8a782cd1 in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:238
    #6 0x7fdd8a83bfec in nsThread::ThreadFunc(void*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/xpcom/threads/nsThread.cpp:264
    #7 0x7fdd944c8926 in _pt_root /builds/slave/m-cen-l64-dbg-asan-00000000000/build/nsprpub/pr/src/pthreads/ptthread.c:204
    #8 0x43f54a in __asan::AsanThread::ThreadStart() ??:0
0x7fdd5ba3d03c is located 4 bytes to the left of 256-byte region [0x7fdd5ba3d040,0x7fdd5ba3d140)
allocated by thread T0 here:
    #0 0x43c620 in __interceptor_malloc ??:0
    #1 0x7fdd92a2c547 in moz_xmalloc /builds/slave/m-cen-l64-dbg-asan-00000000000/build/memory/mozalloc/mozalloc.cpp:54
    #2 0x7fdd88ddf7fb in operator new[](unsigned long) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/../../dist/include/mozilla/mozalloc.h:213
    #3 0x7fdd88dded1e in nsHtml5TreeBuilder::startTokenization(nsHtml5Tokenizer*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/parser/html/nsHtml5TreeBuilder.cpp:77
    #4 0x7fdd88dc32ca in nsHtml5StreamParser::OnStartRequest(nsIRequest*, nsISupports*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/parser/html/nsHtml5StreamParser.cpp:880
    #5 0x7fdd89487c1c in nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/uriloader/base/nsURILoader.cpp:262
    #6 0x7fdd873eaca0 in nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/netwerk/base/src/nsBaseChannel.cpp:720
.
.
.

Comment 1

[Daniel Veditz [:dveditz]]

Henri, What's going on at this crash site? What's being read, what's going to happen next with the data?

Comment 2

[Henri Sivonen (:hsivonen)]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> Henri, What's going on at this crash site? What's being read, what's going
> to happen next with the data?

http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeBuilder.cpp#3247 reads a stored tree builder mode from an array that represents the stack of modes that got remembered when handling <template> elements. The value is next used as the actual tree builder mode, which is the main state variable that the tree builder uses the switch statement on. The most obvious guess is that the templateModePtr index is not actually a valid index of the current templateModeStack array.

Comment 3

[Henri Sivonen (:hsivonen)]

wchen, can you take a look?

Comment 4

[William Chen [:wchen]]

Yeah, I'm on it.

Comment 5

[William Chen [:wchen]]

This bug is caused because the TreeBuilderState does not store the template insertion mode stack information, thus when the tree builder loads a state, we can end up in the template contents mode with an empty template insertion mode stack. This bug is present in all version of Gecko since the feature was implemented in bug 818976.

(In reply to Daniel Veditz [:dveditz] from comment #1)
> Henri, What's going on at this crash site? What's being read, what's going
> to happen next with the data?

In addition to what Henry has said, we read a random number off the heap (index -1 of a heap allocated array), this number is used as the parser mode in switch statements. It looks like in most cases the parser will just end up ignoring some tokens and maybe output error messages which means the tree that is output will not match the markup.

Comment 6

[William Chen [:wchen]]

Attachment: Save template insertion mode stack information in TreeBuilderState.

Comment 7

[William Chen [:wchen]]

Attachment: Add template insertion mode stack methods to nsAHtml5TreeBuilderState.

Comment 8

[William Chen [:wchen]]

Attachment: Generated parser code.

Comment 9

[William Chen [:wchen]]

Comment on attachment 789101 [details] [diff] [review]
Generated parser code.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No

Which older supported branches are affected by this flaw?
aurora, beta, release

If not all supported branches, which bug introduced the flaw?
Bug 818976

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No backports, but patch should apply cleanly on other branches.

How likely is this patch to cause regressions; how much testing does it need?
Not likely. Making sure that the newly added assertion does not fire when opening the repro file in this bug should be sufficient.

Comment 10

[Henri Sivonen (:hsivonen)]

Comment on attachment 789101 [details] [diff] [review]
Generated parser code.

This attachment is actually the Java code--not the generated C++ code.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 789101 [details] [diff] [review]
Generated parser code.

Sec-approval+ for trunk. After that, please put it on Aurora and Release.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Does this affect ESR17 or B2G?

Comment 13

[William Chen [:wchen]]

(In reply to Al Billings [:abillings] from comment #12)
> Does this affect ESR17 or B2G?

No, only gecko 22+ is affected.

Comment 14

[William Chen [:wchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14ff70627709

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14ff70627709

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ae4fd73f675
https://hg.mozilla.org/releases/mozilla-beta/rev/e37e3047baae

Comment 17

[Al Billings [:abillings - ex-MoCo]]

I missed the lack of security rating on this. We need one for the advisory. Can someone suggest what it might be?

Comment 18

[Al Billings [:abillings - ex-MoCo]]

I still need to get a rating for the advisory for this. This is just a read with no crash outside of address sanitizer?

Comment 19

[Daniel Veditz [:dveditz]]

There's no clear evidence of attacker control, though I can't entirely rule it out. Going with sec-moderate for now.

Comment 21

[Daniel Veditz [:dveditz]]

Although we don't feel this meets the normal severity level of our bug bounty program we'd still like to offer a reduced award of $1000 as a thank you since it's unlikely we would have found it on our own, and given the nature of the problem we can't rule out more severe symptoms/results

Comment 22

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash in FF23.
Verified fixed in ASan builds of FF24, 25 and 26 from 2013-09-11.