Heap-buffer-overflow in nsSVGPathGeometryFrame::DidSetStyleContext

VERIFIED FIXED in Firefox 43, Firefox OS v2.5

Status

()

Core
CSS Parsing and Computation
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Abhishek Arya, Assigned: longsonr)

Tracking

(4 keywords)

Trunk
mozilla45
crash, csectype-nullptr, sec-low, testcase
Points:
---

Firefox Tracking Flags

(firefox42 wontfix, firefox43+ verified, firefox44+ verified, firefox45+ verified, firefox-esr38 wontfix, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 wontfix, b2g-v2.5 fixed, b2g-v2.2r wontfix, b2g-master fixed)

Details

(Whiteboard: [adv-main43-])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8684664 [details]
Testcase

================================================================
==23741==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6250001d3100 at pc 0x7f24a0731498 bp 0x7fff0656c270 sp 0x7fff0656c268
READ of size 4 at 0x6250001d3100 thread T0 (Web Content)
    #0 0x7f24a0731497 in nsSVGPathGeometryFrame::DidSetStyleContext(nsStyleContext*) layout/svg/nsSVGPathGeometryFrame.cpp:156:62
    #1 0x7f24a000ceeb in SetStyleContext layout/generic/nsIFrame.h:551:7
    #2 0x7f24a000ceeb in mozilla::ElementRestyler::RestyleSelf(nsIFrame*, nsRestyleHint, unsigned int*, nsTArray<mozilla::ElementRestyler::SwapInstruction>&) layout/base/RestyleManager.cpp:4127
    #3 0x7f24a0008f8f in mozilla::ElementRestyler::Restyle(nsRestyleHint) layout/base/RestyleManager.cpp:3278:7
    #4 0x7f24a00124b2 in mozilla::ElementRestyler::RestyleContentChildren(nsIFrame*, nsRestyleHint) layout/base/RestyleManager.cpp:4752:13
    #5 0x7f24a000fc90 in mozilla::ElementRestyler::RestyleChildren(nsRestyleHint) layout/base/RestyleManager.cpp:4274:7
    #6 0x7f24a0009636 in mozilla::ElementRestyler::Restyle(nsRestyleHint) layout/base/RestyleManager.cpp:3431:5
    #7 0x7f24a00124b2 in mozilla::ElementRestyler::RestyleContentChildren(nsIFrame*, nsRestyleHint) layout/base/RestyleManager.cpp:4752:13
    #8 0x7f24a000fc90 in mozilla::ElementRestyler::RestyleChildren(nsRestyleHint) layout/base/RestyleManager.cpp:4274:7
    #9 0x7f24a0009636 in mozilla::ElementRestyler::Restyle(nsRestyleHint) layout/base/RestyleManager.cpp:3431:5
    #10 0x7f24a00124b2 in mozilla::ElementRestyler::RestyleContentChildren(nsIFrame*, nsRestyleHint) layout/base/RestyleManager.cpp:4752:13
    #11 0x7f24a000fc90 in mozilla::ElementRestyler::RestyleChildren(nsRestyleHint) layout/base/RestyleManager.cpp:4274:7
    #12 0x7f24a0009636 in mozilla::ElementRestyler::Restyle(nsRestyleHint) layout/base/RestyleManager.cpp:3431:5
    #13 0x7f24a00124b2 in mozilla::ElementRestyler::RestyleContentChildren(nsIFrame*, nsRestyleHint) layout/base/RestyleManager.cpp:4752:13
    #14 0x7f24a000fc90 in mozilla::ElementRestyler::RestyleChildren(nsRestyleHint) layout/base/RestyleManager.cpp:4274:7
    #15 0x7f24a0009636 in mozilla::ElementRestyler::Restyle(nsRestyleHint) layout/base/RestyleManager.cpp:3431:5
    #16 0x7f24a0014a66 in mozilla::ElementRestyler::ComputeStyleChangeFor(nsIFrame*, nsStyleChangeList*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&, nsTArray<mozilla::ElementRestyler::ContextToClear>&, nsTArray<RefPtr<nsStyleContext> >&) layout/base/RestyleManager.cpp:4416:7
    #17 0x7f249fffb9da in mozilla::RestyleManager::ComputeAndProcessStyleChange(nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) layout/base/RestyleManager.cpp:4826:3
    #18 0x7f249fffaf02 in mozilla::RestyleManager::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) layout/base/RestyleManager.cpp:1037:5
    #19 0x7f24a00291af in ProcessOneRestyle layout/base/RestyleTracker.cpp:194:5
    #20 0x7f24a00291af in mozilla::RestyleTracker::DoProcessRestyles() layout/base/RestyleTracker.cpp:352
    #21 0x7f24a0001b95 in ProcessRestyles layout/base/RestyleManager.h:534:7
    #22 0x7f24a0001b95 in mozilla::RestyleManager::ProcessPendingRestyles() layout/base/RestyleManager.cpp:1769
    #23 0x7f24a024e4a4 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) layout/base/nsPresShell.cpp:4087:9
    #24 0x7f249c0e6688 in nsDocument::FlushPendingNotifications(mozFlushType) dom/base/nsDocument.cpp:8196:7
    #25 0x7f249b36dd5e in nsDocLoader::DocLoaderIsEmpty(bool) uriloader/base/nsDocLoader.cpp:675:9
    #26 0x7f249b370148 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) uriloader/base/nsDocLoader.cpp:605:5
    #27 0x7f249b370adc in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) uriloader/base/nsDocLoader.cpp:468:14
    #28 0x7f2499bac8ea in nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) netwerk/base/nsLoadGroup.cpp:634:18
    #29 0x7f249c0ee1e5 in nsDocument::DoUnblockOnload() dom/base/nsDocument.cpp:9062:7
    #30 0x7f249c0edbb0 in nsDocument::UnblockOnload(bool) dom/base/nsDocument.cpp:8990:9
    #31 0x7f249c0bdf5d in nsDocument::DispatchContentLoadedEvents() dom/base/nsDocument.cpp:5133:3
    #32 0x7f249c1902a0 in apply<nsDocument, void (nsDocument::*)()> objdir-ff-asan/dist/include/nsThreadUtils.h:663:5
    #33 0x7f249c1902a0 in nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run() objdir-ff-asan/dist/include/nsThreadUtils.h:870
    #34 0x7f24999d9145 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:964:7
    #35 0x7f2499a5c93c in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:297:10
    #36 0x7f249a3d0e8e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:95:21
    #37 0x7f249a33b6d1 in RunInternal ipc/chromium/src/base/message_loop.cc:234:3
    #38 0x7f249a33b6d1 in RunHandler ipc/chromium/src/base/message_loop.cc:227
    #39 0x7f249a33b6d1 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:201
    #40 0x7f249f8bfacf in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:156:3
    #41 0x7f24a1877383 in XRE_RunAppShell toolkit/xre/nsEmbedFunctions.cpp:787:12
    #42 0x7f249a33b6d1 in RunInternal ipc/chromium/src/base/message_loop.cc:234:3
    #43 0x7f249a33b6d1 in RunHandler ipc/chromium/src/base/message_loop.cc:227
    #44 0x7f249a33b6d1 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:201
    #45 0x7f24a18768a7 in XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:623:7
    #46 0x4dbd14 in content_process_main(int, char**) ipc/contentproc/plugin-container.cpp:237:19
    #47 0x7f2496c29ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
0x6250001d3100 is located 0 bytes to the right of 8192-byte region [0x6250001d1100,0x6250001d3100)
allocated by thread T0 (Web Content) here:
    #0 0x4b6458 in __interceptor_malloc _asan_rtl_
    #1 0x7f24a80feeed in PL_ArenaAllocate nsprpub/lib/ds/plarena.c:210:27
    #2 0x7f249ff5d9ae in nsPresArena::Allocate(unsigned int, unsigned long) layout/base/nsPresArena.cpp:165:3
    #3 0x7f249fe5aa70 in AllocateByObjectID layout/base/nsPresArena.h:65:12
    #4 0x7f249fe5aa70 in AllocateByObjectID layout/base/nsIPresShell.h:255
    #5 0x7f249fe5aa70 in operator new layout/style/nsRuleNode.h:151
    #6 0x7f249fe5aa70 in SetStyleData layout/style/nsRuleNode.h:302
    #7 0x7f249fe5aa70 in nsRuleNode::PropagateDependentBit(nsStyleStructID, nsRuleNode*, void*) layout/style/nsRuleNode.cpp:1693
    #8 0x7f249fe59ef6 in nsRuleNode::WalkRuleTree(nsStyleStructID, nsStyleContext*) layout/style/nsRuleNode.cpp:2385:5
    #9 0x7f249fec5f44 in GetStyleTextReset<true> objdir-ff-asan/layout/style/nsStyleStructList.h:95:1
    #10 0x7f249fec5f44 in DoGetStyleTextReset<true> objdir-ff-asan/layout/style/nsStyleStructList.h:95
    #11 0x7f249fec5f44 in nsStyleContext::StyleTextReset() objdir-ff-asan/layout/style/nsStyleStructList.h:95
    #12 0x7f249fedf82a in nsStyleContext::ApplyStyleFixups(bool) layout/style/nsStyleContext.cpp:594:36
    #13 0x7f249ff0242d in NS_NewStyleContext(nsStyleContext*, nsIAtom*, nsCSSPseudoElements::Type, nsRuleNode*, bool) layout/style/nsStyleContext.cpp:1218:5
    #14 0x7f249ff0d483 in nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, nsCSSPseudoElements::Type, mozilla::dom::Element*, unsigned int) layout/style/nsStyleSet.cpp:954:14
    #15 0x7f249ff12529 in nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&) layout/style/nsStyleSet.cpp:1395:10
    #16 0x7f24a00943e6 in nsCSSFrameConstructor::ResolveStyleContext(nsStyleContext*, nsIContent*, nsFrameConstructorState*) layout/base/nsCSSFrameConstructor.cpp:4756:16
    #17 0x7f24a0090650 in ResolveStyleContext layout/base/nsCSSFrameConstructor.cpp:4725:10
    #18 0x7f24a0090650 in ResolveStyleContext layout/base/nsCSSFrameConstructor.cpp:4741
    #19 0x7f24a0090650 in nsCSSFrameConstructor::AddFrameConstructionItems(nsFrameConstructorState&, nsIContent*, bool, nsCSSFrameConstructor::InsertionPoint const&, nsCSSFrameConstructor::FrameConstructionItemList&) layout/base/nsCSSFrameConstructor.cpp:5369
    #20 0x7f24a007b023 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) layout/base/nsCSSFrameConstructor.cpp:10371:9
    #21 0x7f24a00846e0 in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsStyleDisplay const*, nsIContent*, nsContainerFrame*, nsContainerFrame*, nsStyleContext*, nsContainerFrame**, nsFrameItems&, nsIFrame*, PendingBinding*) layout/base/nsCSSFrameConstructor.cpp:11409:3
    #22 0x7f24a00816f7 in nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*) layout/base/nsCSSFrameConstructor.cpp:2530:5
    #23 0x7f24a00a1801 in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) layout/base/nsCSSFrameConstructor.cpp:7420:7
    #24 0x7f24a02344ce in PresShell::Initialize(int, int) layout/base/nsPresShell.cpp:1672:7
    #25 0x7f249c017a58 in nsContentSink::StartLayout(bool) dom/base/nsContentSink.cpp:1246:19
    #26 0x7f249f5cf74b in nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int, bool) dom/xml/nsXMLContentSink.cpp:1023:5
    #27 0x7f249b432200 in nsExpatDriver::HandleStartElement(char16_t const*, char16_t const**) parser/htmlparser/nsExpatDriver.cpp:385:19
    #28 0x7f24a1ffe617 in doContent parser/expat/lib/xmlparse.c:2410:11
    #29 0x7f24a1ff2da1 in contentProcessor parser/expat/lib/xmlparse.c:2066:27
    #30 0x7f24a1ff2da1 in doProlog parser/expat/lib/xmlparse.c:4047
    #31 0x7f24a1fe7fe8 in prologProcessor parser/expat/lib/xmlparse.c:3781:10
    #32 0x7f24a1fe7fe8 in prologInitProcessor parser/expat/lib/xmlparse.c:3598
    #33 0x7f24a1fe62a3 in MOZ_XML_Parse parser/expat/lib/xmlparse.c:1524:17
    #34 0x7f249b438bfc in ParseBuffer parser/htmlparser/nsExpatDriver.cpp:1018:16
    #35 0x7f249b438bfc in nsExpatDriver::ConsumeToken(nsScanner&, bool&) parser/htmlparser/nsExpatDriver.cpp:1116
    #36 0x7f249b447640 in nsParser::Tokenize(bool) parser/htmlparser/nsParser.cpp:1942:16
    #37 0x7f249b443376 in nsParser::ResumeParse(bool, bool, bool) parser/htmlparser/nsParser.cpp:1463:41
    #38 0x7f249b448d71 in nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) parser/htmlparser/nsParser.cpp:1840:12
    #39 0x7f2499b5c606 in nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) netwerk/base/nsBaseChannel.cpp:844:17
    #40 0x7f2499ba5015 in nsInputStreamPump::OnStateTransfer() netwerk/base/nsInputStreamPump.cpp:601:22

Shadow bytes around the buggy address:
  0x0c4a800325d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a800325e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a800325f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a80032600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a80032610: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c4a80032620:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a80032630: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a80032640: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a80032650: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a80032660: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a80032670: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==23741==ABORTING
CC'ing some SVG folks in case it's SVG-specific, but could just be a CSS thing.
Group: core-security → layout-core-security
Flags: needinfo?(dholbert)
Keywords: sec-high
This testcase crashes non-ASAN builds, too, with a null deref.

We crash at this line:
> 150 /* virtual */ void
> 151 nsSVGPathGeometryFrame::DidSetStyleContext(nsStyleContext* aOldStyleContext)
> 152 {
> 153   nsSVGPathGeometryFrameBase::DidSetStyleContext(aOldStyleContext);
> 154 
> 155   if (aOldStyleContext) {
> 156     float oldOpacity = aOldStyleContext->PeekStyleDisplay()->mOpacity;
https://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGPathGeometryFrame.cpp?rev=e8c7dfe727cd#156

The null-deref happens because PeekStyleDisplay() returns null, so PeekStyleDisplay()->mOpacity is a null-deref.

Incidentally, we only have 1 other call to PeekStyleDisplay() in our codebase, and in that instance, we null-check it before using it. And nsStyleContext.cpp has some documentation implying that it's totally fine for PeekStyle* to return null.

So, we definitely should null-check this pointer before we use it here.

(I'm not adept enough at reading ASAN output to be sure if this null-deref is the only problem here, or if ASAN is reporting something subtly different in the output in comment 0.)
Flags: needinfo?(dholbert)
For the record, this chunk of nsSVGPathGeometryFrame was added in 2013, for bug 901955, here:
 https://hg.mozilla.org/mozilla-central/rev/1d64db569ebf#l3.12
Created attachment 8685560 [details] [diff] [review]
fix v1: null-check the result of PeekStyleDisplay

This fixes the null-deref, and I think correctly adjusts the logic around it to work correctly.

(We're dereferencing this pointer to find out if opacity changed; and if we've got a null pointer for our old style data, I'm just making the code assume the opacity has changed.)

(This might actually be overly pessimistic -- if PeekStyleDisplay returns null, I *think* that means no one ever inspected the old opacity, which maybe means we don't need to care about invalidating anything. But I'm not sure that we can get away with skipping the invalidation; hence, I'm taking the safe route.)
Created attachment 8685567 [details] [diff] [review]
dholbert's initial patch

(now with a commit message)

Tagging roc for review, since he was the reviewer on bug 901955, and I believe jwatt (patch author on that bug) is out for a little while.
Attachment #8685560 - Attachment is obsolete: true
Attachment #8685567 - Flags: review?(roc)
I can't actually reproduce any ASAN warning output, using our ASAN builds on treeherder from a recent inbound revision:
 https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=344a9c9a8454

The debug ASAN build crashes, but doesn't have any ASAN output:
   https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux64-asan-debug/1447176396/
The opt ASAN build doesn't even crash (it must take a different codepath somehow):
   https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux64-asan/1447175675/
(Assignee)

Comment 7

2 years ago
Created attachment 8685586 [details] [diff] [review]
fix

not sure this is better or worse but I'll attach it given that I was working on this till I saw dholbert's patch.
Comment on attachment 8685567 [details] [diff] [review]
dholbert's initial patch

Oh, I didn't notice the additional "PeakStyleSVG" call below this (which longsonr's patch catches).

Canceling review on my patch, as it should probably handle that as well before it's review-worthy. (Or maybe longsonr's is better.)
Attachment #8685567 - Flags: review?(roc)
(Ah, but the existing code does already correctly null-check PeakStyleSVG; longsonr's patch just caches that result to avoid repeated calls.)
Attachment #8685567 - Attachment description: fix v1a: null-check the result of PeekStyleDisplay → dholbert's initial patch
Comment on attachment 8685586 [details] [diff] [review]
fix

I'll r+ longsonr's patch, if longsonr is up for having it reviewed.

Per my parentheticals in comment 4, I'm think we're likely-fine to skip the invalidation in this "old opacity was never inspected" case.  (and we do skip it, in longsonr's patch)

This is an extreme edge case anyway, as evidenced by the fact that we've been using PeekStyleDisplay() without a null-check here for 2 years without noticing any trouble.
Flags: needinfo?(longsonr)
Attachment #8685586 - Flags: review+
(Assignee)

Comment 11

2 years ago
Comment on attachment 8685586 [details] [diff] [review]
fix

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

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? all of them

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?
Flags: needinfo?(longsonr)
Attachment #8685586 - Flags: sec-approval?
(Assignee)

Comment 12

2 years ago
unlikely to cause regressions, it's just a null check.
Sec-approval+ for trunk. We'll want patches made and nominated for Aurora, Beta, and ESR38.
status-firefox42: --- → wontfix
status-firefox43: --- → affected
status-firefox44: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox43: --- → +
tracking-firefox44: --- → +
tracking-firefox45: --- → +
tracking-firefox-esr38: --- → 43+
Attachment #8685586 - Flags: sec-approval? → sec-approval+
Abishek: firstly, thanks for filing the bug! (and your many other bugs)

Secondly: I'm confused, because I'm still unable to reproduce any ASAN error here. I tried both ASAN builds linked in comment 6, as well as this most-recent ASAN nightly (which crashes when I load your testcase):
https://archive.mozilla.org/pub/firefox/nightly/2015/11/2015-11-10-03-02-05-mozilla-central/firefox-45.0a1.en-US.linux-x86_64-asan.tar.bz2
...and I only get crashes, with no ASAN errors.  So I haven't been able to reproduce anything worse than a nullptr deref here (which is what the attached patches are targeted at fixing).

So I'm wondering:
 1) Can you reproduce the ASAN error with e.g. that ^ ASAN build or another Mozilla-provided ASAN build?
 2) Can you still reproduce the ASAN error with your own ASAN build?
 3) Does longsonr's attached patch fix the ASAN error for you?

(and 4) Perhaps you have a different version of this testcase and *that* one triggers the ASAN errors, and you accidentally attached the wrong one?)
Flags: needinfo?(inferno)
Assignee: nobody → longsonr
Status: NEW → ASSIGNED
(Assignee)

Comment 15

2 years ago
patch will also fix bug 1223281
status-b2g-v2.0: --- → wontfix
status-b2g-v2.0M: --- → wontfix
status-b2g-v2.1: --- → wontfix
status-b2g-v2.1S: --- → wontfix
status-b2g-v2.2: --- → affected
status-b2g-v2.2r: --- → affected
status-b2g-v2.5: --- → affected
status-b2g-master: --- → affected
(Assignee)

Comment 16

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8c552ad326f
(Reporter)

Comment 17

2 years ago
1) Testcase is correct.
2) Weirdly, it (In reply to Daniel Holbert [:dholbert] from comment #14)
> Abishek: firstly, thanks for filing the bug! (and your many other bugs)
> 
> Secondly: I'm confused, because I'm still unable to reproduce any ASAN error
> here. I tried both ASAN builds linked in comment 6, as well as this
> most-recent ASAN nightly (which crashes when I load your testcase):
> https://archive.mozilla.org/pub/firefox/nightly/2015/11/2015-11-10-03-02-05-
> mozilla-central/firefox-45.0a1.en-US.linux-x86_64-asan.tar.bz2
> ...and I only get crashes, with no ASAN errors.  So I haven't been able to
> reproduce anything worse than a nullptr deref here (which is what the
> attached patches are targeted at fixing).
> 
> So I'm wondering:
>  1) Can you reproduce the ASAN error with e.g. that ^ ASAN build or another
> Mozilla-provided ASAN build?
>  2) Can you still reproduce the ASAN error with your own ASAN build?
>  3) Does longsonr's attached patch fix the ASAN error for you?
> 
> (and 4) Perhaps you have a different version of this testcase and *that* one
> triggers the ASAN errors, and you accidentally attached the wrong one?)

1) Testcase is correct.
2) Weirdly, it only reproduces only on compute engine vm, not locally. i tried all ways like same xvfb env, multiple tabs, etc. nothing worked.
3) So, i updated to trunk and applied Robert's patch. Since it reproduced on vm, i retried with this build and crash is now gone. so, patch indeed fixes this.
Flags: needinfo?(inferno)
Interesting -- I wonder if that means there's an ASAN bug involved here, which only reproduces on the compute engine vm (& generates a false positive heap-buffer-overflow report).

Thanks for verifying that the patch fixes it, in any case!  That indicates that this is indeed a nullptr deref (since the fix is just a null-check). Hence, this is probably sec-low.
Keywords: sec-high → crash, sec-low, testcase
(Assignee)

Comment 19

2 years ago
Comment on attachment 8685586 [details] [diff] [review]
fix

Approval Request Comment
[Feature/regressing bug #]: bug 901955
[User impact if declined]: crash with unusual svg markup (3 bugs filed includin this one)
[Describe test coverage new/current, TreeHerder]: added test from a non-security sensitive bug in patch.
[Risks and why]: very low, just a null check
[String/UUID change made/needed]: none
Attachment #8685586 - Flags: approval-mozilla-beta?
Attachment #8685586 - Flags: approval-mozilla-aurora?
Comment on attachment 8685586 [details] [diff] [review]
fix

Crash fix, ok to uplift to aurora and beta.
Attachment #8685586 - Flags: approval-mozilla-beta?
Attachment #8685586 - Flags: approval-mozilla-beta+
Attachment #8685586 - Flags: approval-mozilla-aurora?
Attachment #8685586 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/a8c552ad326f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
this faild to apply to aurora:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r a8c552ad326f
grafting 314479:a8c552ad326f "Bug 1222812 - add a null check in case there is no old style. r=dholbert"
merging layout/svg/crashtests/crashtests.list
warning: conflicts during merge.
merging layout/svg/crashtests/crashtests.list incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

could you take a look, thanks!
Flags: needinfo?(longsonr)
crashtest.list conflicts are common, since every crasher adds a new entry to it.

I can post a new patch; the new line just needs to be inserted into aurora's crashtest.list at the appropriate sorted spot.
Flags: needinfo?(longsonr) → needinfo?(dholbert)
Created attachment 8687800 [details] [diff] [review]
fix, backported to aurora & beta
Attachment #8685567 - Attachment is obsolete: true
Flags: needinfo?(dholbert) → needinfo?(cbook)
Attachment #8685586 - Attachment description: my version → fix
Comment on attachment 8687800 [details] [diff] [review]
fix, backported to aurora & beta

(This applies cleanly to beta as well.)
Attachment #8687800 - Attachment description: fix, backported to aurora → fix, backported to aurora & beta
https://hg.mozilla.org/releases/mozilla-aurora/rev/57c6a522abc1
status-firefox44: affected → fixed

Updated

2 years ago
Flags: needinfo?(cbook)
Highly unlikely we care about a sec-low for older B2G releases. Probably true for ESR38 as well, but OTOH, it's a pretty trivial patch, so I'll refrain from making that call :)
status-b2g-v2.2: affected → wontfix
status-b2g-v2.2r: affected → wontfix
https://hg.mozilla.org/releases/mozilla-beta/rev/d35d09b0b24f
status-firefox43: affected → fixed
Group: layout-core-security → core-security-release
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/57c6a522abc1
status-b2g-v2.5: affected → fixed
status-b2g-master: affected → fixed
status-firefox-esr38: affected → wontfix
tracking-firefox-esr38: 43+ → ---
Whiteboard: [adv-main43+]
I was able to reproduce this issue on Firefox 45.0a1 (2015-11-08) using Windows 10 64-bit.

Verified fixed on Firefox 43 beta 9 (20151203163240), Firefox 44.0a2 (2015-12-07) and Firefox 45.0a1 (2015-12-07) under Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 12.04 64-bit.
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified
status-firefox44: fixed → verified
status-firefox45: fixed → verified
Alias: CVE-2015-7213
Keywords: csectype-nullptr
Alias: CVE-2015-7213
Whiteboard: [adv-main43+] → [adv-main43-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.