896159 - Bad svg length object crash in nsSVGElement::WillChangeLength

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Abhishek Arya]

Attachment: Testcase

NS_NOTREACHED("Unknown unit type"); in nsSVGLength2::GetUnitScaleFactor, followed by NS_ASSERTION(aAttrEnum < info.mLengthCount, "aAttrEnum out of range");, followed by corrupted object crash in nsAttrAndChildArray::SetAndTakeAttr [new (&ATTRS(mImpl)[i].mName) nsAttrName(aLocalName);]

==6958==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f98aadc2b90 at pc 0x7f98a5d33568 bp 0x7fff3f89e7d0 sp 0x7fff3f89e7c8
READ of size 8 at 0x7f98aadc2b90 thread T0
    #0 0x7f98a5d33567 in nsSVGElement::WillChangeLength(unsigned char) content/svg/content/src/nsSVGElement.cpp:1604
    #1 0x7f98a5d492a0 in nsSVGLength2::ConvertToSpecifiedUnits(unsigned short, nsSVGElement*) content/svg/content/src/nsSVGLength2.cpp:312
    #2 0x7f98a38b220d in NS_InvokeByIndex xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
    #3 0x7f98a5e5897d in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) js/xpconnect/src/XPCWrappedNative.cpp:2795
    #4 0x7f98a5e68f8f in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1315
    #5 0x7f98a7a81bc4 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:225
    #6 0x7f98a7a72f1c in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2502
    #7 0x7f98a7a6343b in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:434
    #8 0x7f98a7a83b05 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:618
    #9 0x7f98a8302907 in EvalKernel(JSContext*, JS::CallArgs const&, EvalType, js::AbstractFramePtr, JS::Handle<JSObject*>, unsigned char*) js/src/builtin/Eval.cpp:330
    #10 0x7f98a8302f34 in js::DirectEval(JSContext*, JS::CallArgs const&) js/src/builtin/Eval.cpp:452
    #11 0x7f98a7f69ea3 in js::ion::DoCallFallback(JSContext*, js::ion::BaselineFrame*, js::ion::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/ion/BaselineIC.cpp:6991
    #12 0x7f9894a36732 in
    #13 0x611000376b37 in
    #14 0x7f9894a2efff in
    #15 0x7f98a7f9ea66 in EnterBaseline(JSContext*, js::ion::EnterJitData&) js/src/ion/BaselineJIT.cpp:107
    #16 0x7f98a7f9f5df in js::ion::EnterBaselineAtBranch(JSContext*, js::StackFrame*, unsigned char*) js/src/ion/BaselineJIT.cpp:188
    #17 0x7f98a7a7c6a3 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:1610
    #18 0x7f98a7a6343b in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:434
    #19 0x7f98a7a83b05 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:618
    #20 0x7f98a7a83e18 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:654
    #21 0x7f98a7c3aa38 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) js/src/jsapi.cpp:5549
    #22 0x7f98a53ed6bb in nsJSContext::EvaluateString(nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, bool, JS::Value*) dom/base/nsJSEnvironment.cpp:1278
    #23 0x7f98a53b03bd in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) dom/base/nsGlobalWindow.cpp:10237
    #24 0x7f98a53986a3 in nsGlobalWindow::RunTimeout(nsTimeout*) dom/base/nsGlobalWindow.cpp:10481
    #25 0x7f98a53af733 in nsGlobalWindow::TimerCallback(nsITimer*, void*) dom/base/nsGlobalWindow.cpp:10728
    #26 0x7f98a387a3f6 in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:627
    #27 0x7f98a37ad29a in NS_ProcessNextEvent(nsIThread*, bool) objdir-ff-asan/xpcom/build/nsThreadUtils.cpp:238
    #28 0x7f98a29c656c in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:81
    #29 0x7f98a391ab99 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:219
    #30 0x7f98a66364bc in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #31 0x7f98a26cad59 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3921
    #32 0x7f98a26cbbc9 in XRE_main toolkit/xre/nsAppRunner.cpp:4123
    #33 0x4282a4 in main browser/app/nsBrowserApp.cpp:272
    #34 0x7f98acc8676c in ?? ??
0x7f98aadc2b90 is located 40 bytes to the right of global variable _ZN7mozilla3dom25SVGTextPositioningElement15sNumberListInfoE (content/svg/content/src/SVGTextPositioningElement.cpp) (0x7f98aadc2b60) of size 8
Shadow bytes around the buggy address:
  0x0ff3955b0520: 00 00 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 f9 f9
  0x0ff3955b0530: f9 f9 f9 f9 00 00 f9 f9 f9 f9 f9 f9 00 00 00 00
  0x0ff3955b0540: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff3955b0550: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff3955b0560: 00 00 00 00 00 00 00 00 f9 f9 f9 f9 00 f9 f9 f9
=>0x0ff3955b0570: f9 f9[f9]f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff3955b0580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff3955b0590: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff3955b05a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff3955b05b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff3955b05c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
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 righ 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
  ASan internal:         fe
==6958==ABORTING

Comment 1

[Cameron McCormack (:heycam)]

I think this is because the LengthInfo returned by SVGTextPathElement does not include an entry for textLength="", which is handled on SVGTextContentElement.  A similar issue will exist for lengthAdjust="".

We need a way for the objects returned from GetLengthInfo, GetEnumInfo, etc. to be able to include attribute info from superclasses.

Comment 2

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #1)
> We need a way for the objects returned from GetLengthInfo, GetEnumInfo, etc.
> to be able to include attribute info from superclasses.

I think that's too much trouble, actually.  I'm just going to duplicate the attribute information in SVGTextPathElement.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: patch

How about this?  It moves the nsSVGLength2 for textLength (and the nsSVGEnum for lengthAdjust) down to each text content element subclass.  I had to add the virtual LengthAttributes() and EnumAttributes() methods so that the TextLength() and LengthAdjust() Web IDL methods on SVGTextContentElement could get at the nsSVGLength2/nsSVGEnum arrays.

Comment 4

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=10b491a59044

Comment 5

[Jonathan Watt [:jwatt]]

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

Review of attachment 778935 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I guess this is what we should do.

By the way, what's with the removal of blank lines at the end of files? Can you change your editor not to do that? I've noticed the same thing happening in other patches of yours on other bugs.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #5)
> By the way, what's with the removal of blank lines at the end of files? Can
> you change your editor not to do that? I've noticed the same thing happening
> in other patches of yours on other bugs.

Looks like that's a "bugzilla diff viewer not correctly interpreting git diffs" bug, not an actual thing that Cameron's editor is removing.

His patch ends with:
> -- 
> 1.7.10.2 (Apple Git-33)

where "--" is some sort of git delimeter, not a line-removal.

If you 'hg qimport' his patch and then 'hg qdiff content/svg/content/src/SVGTextPathElement.h' you'll see that no line is actually removed at the end of that file.

Comment 7

[Jonathan Watt [:jwatt]]

Ah, good. Thanks, Daniel.

Comment 8

[Cameron McCormack (:heycam)]

I run my git patches through git-patch-to-hg-patch before attaching them, but I guess that doesn't remove that bit at the end.  (Since git patch format is as an email, it's an email signature.)  I found a git option to turn it off, though:

  [format]
          signature = ""

so it shouldn't show up again.

Comment 9

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3b66a9d34dd

Comment 10

[Jonathan Watt [:jwatt]]

Great, thanks!

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e3b66a9d34dd

Comment 12

[Daniel Veditz [:dveditz]]

Security bugs should not land without a sec-approval, unless they're known to be sec-low or recent trunk-only regressions. The policy is https://wiki.mozilla.org/Security/Bug_Approval_Process

Please request retroactive sec-approval? on the patch and answer the questions. Primarily we're trying to answer the questions "how bad is this?" and "how far back do we have to worry about it?".

Comment 13

[Daniel Holbert [:dholbert]]

[tagging heycam for needinfo, to address comment 12]

[How scary is this? It looks to me like the we'd end up calling nsSVGElement::WillChangeLength() with an out-of-bounds aAttrEnum value, and that function makes a call to:
  WillChangeValue(*GetLengthInfo().mLengthInfo[aAttrEnum].mName);
and since aAttrEnum is out of bounds, we'll be passing a bogus nsIAtom* to WillChangeValue().  And then I'm not sure what it does with that nsIAtom, but I'm willing to believe it could be coaxed to do something evil if the nsIAtom is bogus.]

Comment 14

[Cameron McCormack (:heycam)]

(In reply to Daniel Veditz [:dveditz] from comment #12)
> Security bugs should not land without a sec-approval, unless they're known
> to be sec-low or recent trunk-only regressions. The policy is
> https://wiki.mozilla.org/Security/Bug_Approval_Process
> 
> Please request retroactive sec-approval? on the patch and answer the
> questions. Primarily we're trying to answer the questions "how bad is this?"
> and "how far back do we have to worry about it?".

Sorry Daniel.  I think I thought I was safe given the wiki page says a fix can be landed without approval if:

  2. The bug is a recent regression on mozilla-central

as it was a result of a change checked in two weeks before I landed the fix (bug 569722), and within the same release.  Is that too long?

Comment 15

[Cameron McCormack (:heycam)]

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

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

It's not obvious to me looking at the patch how to construct an exploit.

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

Not IMO.

Which older supported branches are affected by this flaw?

None.

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

N/A

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

N/A

How likely is this patch to cause regressions; how much testing does it need?

I don't think there was a great chance that it would cause regressions; I did manual testing at the time to check that the SVG DOM text properties did the right thing on <textPath> elements.

Comment 16

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

So this only affects trunk? If so, it doesn't need sec-approval to go in. Please confirm that it doesn't affect Aurora or Beta.

Otherwise, this needs a security rating before it can be checked in.

Comment 17

[Cameron McCormack (:heycam)]

At the time the patch landed, it only affected trunk.  (When m-c was Firefox 25.)

Comment 18

[Daniel Holbert [:dholbert]]

[I think dveditz was primarily interested in getting this tagged with a security rating]

Just talked to heycam about this, and we agree it was a sec-critical flavor of bug.

I was slightly off track in Comment 13 -- the evil stuff happens a bit later than the WillChangeValue.  In Comment 0, the "corrupted object crash" happens during nsAttrAndChildArray::SetAndTakeAttr, which presumably we call several levels deep during ConvertToSpecifiedUnits().  Comment 0 specifically mentions a line that constructs a nsAttrName object from "aLocalName", which is likely the bogus nsIAtom* -- and nsAttrName's constructor addrefs the nsIAtom* that you pass in.

So, this bug meant we were addrefing a bogus pointer (albeit a pointer whose address might be derived from the data segment and hence may not be attacker-controllable...)  Anyway, this is nasty enough to merit a sec-critical rating, I think (and heycam agrees).

Comment 19

[Daniel Holbert [:dholbert]]

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

[Canceling sec-approval, since (as abillings points out) this didn't actually need it, and since we have a severity rating now which is what I think dveditz was really getting at.]

Comment 20

[Daniel Holbert [:dholbert]]

We should check in a testcase for this at some point, too -- marking "in-testsuite?".  I think we can do that (and un-hide this bug) any time, right? (since no users are actually affected at this point, per comment 17.)

Comment 21

[Daniel Veditz [:dveditz]]

(In reply to Cameron McCormack (:heycam) from comment #14)
> I thought I was safe given the wiki page says a fix can be landed without approval if:
> 
>   2. The bug is a recent regression on mozilla-central
> 
> as it was a result of a change checked in two weeks before I landed the fix
> (bug 569722), and within the same release.  Is that too long?

Not too long, no. It's nice to have evidence to that effect in the bug so we don't freak out when our bug queries pick up security fixes that have landed with unknown status on older branches.

> Which older supported branches are affected by this flaw?
>     None.
> If not all supported branches, which bug introduced the flaw?
>     N/A

For the future, those are the key questions and that's a nonsense answer: either you know when the bug was introduced or you can't say that older branches aren't affected. Pinning it to a specific older checkin helps because sometimes an older branch isn't affected /now/ but then we later backport whatever it was that caused the problem. By linking the bugs we can avoid reintroducing known regressions.

But you had already said it was caused by the landing for bug 569722 in the previous comment so maybe you thought that question was asking something else. Any suggestions for rewording?

Comment 22

[Cameron McCormack (:heycam)]

(In reply to Daniel Veditz [:dveditz] from comment #21)
> Not too long, no. It's nice to have evidence to that effect in the bug so we
> don't freak out when our bug queries pick up security fixes that have landed
> with unknown status on older branches.

I'll make sure to do that in future.

> > Which older supported branches are affected by this flaw?
> >     None.
> > If not all supported branches, which bug introduced the flaw?
> >     N/A
> 
> For the future, those are the key questions and that's a nonsense answer:
> either you know when the bug was introduced or you can't say that older
> branches aren't affected. Pinning it to a specific older checkin helps
> because sometimes an older branch isn't affected /now/ but then we later
> backport whatever it was that caused the problem. By linking the bugs we can
> avoid reintroducing known regressions.
>
> But you had already said it was caused by the landing for bug 569722 in the
> previous comment so maybe you thought that question was asking something
> else. Any suggestions for rewording?

You're right I think I misread the question.  I thought it was asking "if there is a set of older supported branches that are affected by this flaw, and that isn't the whole set of supported older branches, then which bug introduced the flaw?".  The empty set is still a set, of course.

How about this wording:

  If there is at least one supported branch that is not affected by this flaw, which bug introduced the flaw?

Maybe a bit repetitive.  But it sounds less like a follow-on from the previous question.

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #22)
> How about this wording:
> 
>   If there is at least one supported branch that is not affected by this
> flaw, which bug introduced the flaw?

(FWIW, I prefer the existing text; the qualifier in that suggested text is harder for me to parse. I had to read it several times to make sure I was interpreting it properly.)

Strawman alternate suggestion:
 "Which bug's patch introduced the flaw? (Skip this if all supported branches are affected.)"