61 bytes, text/plain
12.27 KB, patch
|Details | Diff | Splinter Review|
1.19 KB, patch
|Details | Diff | Splinter Review|
24.66 KB, patch
|Details | Diff | Splinter Review|
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 . . .
Component: General → HTML: Parser
Product: Firefox → Core
Henri, What's going on at this crash site? What's being read, what's going to happen next with the data?
(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.
wchen, can you take a look?
Yeah, I'm on it.
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.
Created attachment 786543 [details] [diff] [review] Save template insertion mode stack information in TreeBuilderState.
Attachment #786543 - Flags: review?(hsivonen)
Created attachment 786545 [details] [diff] [review] Add template insertion mode stack methods to nsAHtml5TreeBuilderState.
Attachment #786545 - Flags: review?(hsivonen)
Attachment #786543 - Flags: review?(hsivonen) → review+
Attachment #786545 - Flags: review?(hsivonen) → review+
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 on attachment 789101 [details] [diff] [review] Generated parser code. This attachment is actually the Java code--not the generated C++ code.
Comment on attachment 789101 [details] [diff] [review] Generated parser code. Sec-approval+ for trunk. After that, please put it on Aurora and Release.
Does this affect ESR17 or B2G?
status-b2g18: --- → ?
status-firefox23: --- → wontfix
status-firefox24: --- → affected
status-firefox25: --- → affected
status-firefox26: --- → affected
status-firefox-esr17: --- → ?
tracking-firefox24: --- → +
tracking-firefox25: --- → +
tracking-firefox26: --- → +
(In reply to Al Billings [:abillings] from comment #12) > Does this affect ESR17 or B2G? No, only gecko 22+ is affected.
status-b2g18: ? → unaffected
status-firefox-esr17: ? → unaffected
OS: Linux → All
Hardware: x86_64 → All
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox26: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
status-firefox24: affected → fixed
status-firefox25: affected → fixed
I missed the lack of security rating on this. We need one for the advisory. Can someone suggest what it might be?
Whiteboard: [asan] → [asan][adv-main24+]
I still need to get a rating for the advisory for this. This is just a read with no crash outside of address sanitizer?
There's no clear evidence of attacker control, though I can't entirely rule it out. Going with sec-moderate for now.
Keywords: csec-uninitialized, sec-moderate
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
Confirmed crash in FF23. Verified fixed in ASan builds of FF24, 25 and 26 from 2013-09-11.
Status: RESOLVED → VERIFIED
status-firefox24: fixed → verified
status-firefox25: fixed → verified
status-firefox26: fixed → verified
You need to log in before you can comment on or make changes to this bug.