Last Comment Bug 888820 - (CVE-2013-1720) Heap-buffer-overflow READ in nsHtml5TreeBuilder::resetTheInsertionMode()
(CVE-2013-1720)
: Heap-buffer-overflow READ in nsHtml5TreeBuilder::resetTheInsertionMode()
Status: VERIFIED FIXED
[asan][adv-main24+]
: csectype-uninitialized, regression, sec-moderate
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla26
Assigned To: William Chen [:wchen] (Vacation until 8/25)
:
Mentors:
Depends on:
Blocks: 818976
  Show dependency treegraph
 
Reported: 2013-06-30 22:37 PDT by Atte Kettunen
Modified: 2014-11-19 20:11 PST (History)
10 users (show)
dveditz: sec‑bounty+
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
verified
+
verified
unaffected
unaffected


Attachments
Repro-file (61 bytes, text/plain)
2013-06-30 22:37 PDT, Atte Kettunen
no flags Details
Save template insertion mode stack information in TreeBuilderState. (12.27 KB, patch)
2013-08-06 15:16 PDT, William Chen [:wchen] (Vacation until 8/25)
hsivonen: review+
Details | Diff | Splinter Review
Add template insertion mode stack methods to nsAHtml5TreeBuilderState. (1.19 KB, patch)
2013-08-06 15:17 PDT, William Chen [:wchen] (Vacation until 8/25)
hsivonen: review+
Details | Diff | Splinter Review
Generated parser code. (24.66 KB, patch)
2013-08-12 12:53 PDT, William Chen [:wchen] (Vacation until 8/25)
abillings: approval‑mozilla‑aurora+
abillings: approval‑mozilla‑beta+
abillings: sec‑approval+
Details | Diff | Splinter Review

Description Atte Kettunen 2013-06-30 22:37:59 PDT
Created attachment 769587 [details]
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] 2013-07-10 10:48:31 PDT
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) 2013-08-06 02:49:48 PDT
(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) 2013-08-06 02:50:23 PDT
wchen, can you take a look?
Comment 4 William Chen [:wchen] (Vacation until 8/25) 2013-08-06 11:10:36 PDT
Yeah, I'm on it.
Comment 5 William Chen [:wchen] (Vacation until 8/25) 2013-08-06 15:15:51 PDT
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] (Vacation until 8/25) 2013-08-06 15:16:36 PDT
Created attachment 786543 [details] [diff] [review]
Save template insertion mode stack information in TreeBuilderState.
Comment 7 William Chen [:wchen] (Vacation until 8/25) 2013-08-06 15:17:29 PDT
Created attachment 786545 [details] [diff] [review]
Add template insertion mode stack methods to nsAHtml5TreeBuilderState.
Comment 8 William Chen [:wchen] (Vacation until 8/25) 2013-08-12 12:53:42 PDT
Created attachment 789101 [details] [diff] [review]
Generated parser code.
Comment 9 William Chen [:wchen] (Vacation until 8/25) 2013-08-12 12:56:29 PDT
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) 2013-08-13 03:35:28 PDT
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] 2013-08-13 11:21:28 PDT
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] 2013-08-13 11:22:31 PDT
Does this affect ESR17 or B2G?
Comment 13 William Chen [:wchen] (Vacation until 8/25) 2013-08-13 11:26:00 PDT
(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] (Vacation until 8/25) 2013-08-13 14:42:01 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/14ff70627709
Comment 15 Ryan VanderMeulen [:RyanVM] 2013-08-14 06:22:15 PDT
https://hg.mozilla.org/mozilla-central/rev/14ff70627709
Comment 17 Al Billings [:abillings] 2013-08-22 16:36:26 PDT
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] 2013-08-27 17:16:01 PDT
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] 2013-08-27 17:22:31 PDT
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] 2013-09-09 15:35:42 PDT
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:] 2013-09-16 14:27:26 PDT
Confirmed crash in FF23.
Verified fixed in ASan builds of FF24, 25 and 26 from 2013-09-11.

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