1204061 - (CVE-2015-7199) Missing status checks in AddWeightedPathSegLists and SVGPathSegListSMILType::Interpolate cause memory-safety bugs

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[q1]

AddWeightedPathSegLists (dom\svg\SVGPathSegListSMILType.cpp) calls SetLength on a FallibleTArray without checking the result in release builds. If FF is almost out of memory for some other reason and/or a page includes code that invokes this function with an SVG path with an unusually-large number of segments, SetLength could invisibly fail. AddWeightedPathSegLists would then overwrite memory that it doesn't own.

There is also a similar bug in SVGPathSegListSMILType::Interpolate in the same module.

The bugs are latent because the functions in which they occur don't seem to be called by anything in the codebase (please verify).


Details:
--------

The bug in AddWeightedPathSegLists is in lines 267-68: The assert message on line 268 is misleading...

236:static void
237:AddWeightedPathSegLists(double aCoeff1, const SVGPathDataAndInfo& aList1,
238:                        double aCoeff2, const SVGPathDataAndInfo& aList2,
239:                        SVGPathDataAndInfo& aResult)
...
266:  if (aResult.IsIdentity()) {
267:    DebugOnly<bool> success = aResult.SetLength(aList2.Length());
268:    MOZ_ASSERT(success, "infallible nsTArray::SetLength should succeed");
269:    aResult.SetElement(aList2.Element()); // propagate target element info!
270:  }
271:
272:  SVGPathDataAndInfo::iterator resultIter = aResult.begin();
273:
274:  while ((!iter1 || iter1 != end1) &&
275:         iter2 != end2) {
276:    AddWeightedPathSegs(aCoeff1, iter1,
277:                        aCoeff2, iter2,
278:                        resultIter);

...because SVGPathDataAndInfo::SetLength actually resizes a FallibleTArray (dom\svg\SVGPathData.h):

239: class SVGPathDataAndInfo final : public SVGPathData
240: {
...
283:  using SVGPathData::SetLength;
284:  using SVGPathData::begin;
...
293: };

76: class SVGPathData
77: {
...
204:  bool SetLength(uint32_t aLength) {
205:    return mData.SetLength(aLength);
206:  }
...
224:  iterator begin() { return mData.Elements(); }
...
227:  FallibleTArray<float> mData;
228:};

So if SetLength fails, SVGPathSegListSMILType.cpp line 272 derives an iterator to memory that it has no right to use, and the function call on lines 276-78 writes into that memory.

This bug is still present in 9/10/2015's Aurora (http://hg.mozilla.org/releases/mozilla-aurora/file/136110a23c39/dom/svg/SVGPathSegListSMILType.cpp , http://hg.mozilla.org/releases/mozilla-aurora/file/136110a23c39/dom/svg/SVGPathData.h ).

Comment 1

[Daniel Holbert [:dholbert]]

Looks like this was regressed by bug 853554, which replaced SVGPathData's (infallible) nsTArray with a FallibleTArray.  (SVGPathDataAndInfo derives from SVGPathData, as shown in comment 0.)  I'm guessing we probably missed the existence of the SVGPathDataAndInfo subclass when doing sanity-checking on this array's usages in that bug.

The misleading assert mentioned in comment 0 here was correct until bug 853554 landed (at which point it became invalid/misleading).

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to q1 from comment #0)
> The bugs are latent because the functions in which they occur don't seem to
> be called by anything in the codebase (please verify).

Unfortunately, they are indeed called, via SMIL animation of path-data, I believe. So this is a bug that can be triggered in the wild (in low-memory conditions), and can cause uninitialized data reads in a way that's exposed to the web.  So, definitely a potential for data leakage there. Not sure yet if this can cause arbitrary code execution too.

<DETAILS on how these methods are called, if you're curious>
The way these methods are called is a bit obscure.  We use nsSMILValues to represent intermediate values (in this case, intermediate path data on a path that is being animated).  Each nsSMILValue is a tagged union, with a pointer to a singleton nsISMILType object. That nsISMILType singleton has Add, Interpolate, etc. methods which we invoke from nsSMILValue to manipulate its data and perform those operations.  The class here (SVGPathSegListSMILType) is one such nsISMILType singleton, and it's passed in to new nsSMILValues in e.g. GetBaseValue and ValueFromString functions in SVGAnimatedPathSegList.cpp, here:
 http://mxr.mozilla.org/mozilla-central/source/dom/svg/SVGAnimatedPathSegList.cpp?rev=b779fae94e88#160
(...and that class is our internal representation of a path data segment list as its exposed via the DOM, IIRC).
</DETAILS>

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Looks like this was regressed by bug 853554

(This means this affects all supported branches, BTW; that bug's patch landed in Firefox 22, according to flags on the bug.)

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Unfortunately, they are indeed called, via SMIL animation of path-data, I
> believe. So this is a bug that can be triggered in the wild (in low-memory
> conditions), and can cause uninitialized data reads in a way that's exposed
> to the web.

Sorry, I was wrong - since the variable in question is a *result* variable (which we're assigning a value to), this bug will actually manifest as an out-of-bounds *write*, not an out-of-bounds read.  I think that counts as sec-critical.

(The write actually happens in AddWeightedPathSegs, which sets a particular entry in the FallibleTArray, 'aResultSeg'. It'll also happen in the write to *aResult in ConvertPathSegmentData, when called via Interpolate, which [as hinted in comment 0] also has an unchecked SetLength call.)

Comment 5

[Daniel Holbert [:dholbert]]

I'll plan on spinning up a patch for this tomorrow (just adding correct error-handling for these SetLength calls). But if someone else is up for taking this, and has cycles to write a patch before I do, please feel free to take it.

(I'm just getting back from a several-week vacation, so I have a bit of a backlog and would be happy to have one less to-do item. :))

Comment 6

[Robert Longson [:longsonr]]

Attachment: fix

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8660724 [details] [diff] [review]
fix

Looks good, thanks! Just needs a commit message (strawman suggestion: "Handle SetLength failure in SVGPathSegListSMILType"), and sec-approval, and then we can get this landed.

Comment 8

[Daniel Holbert [:dholbert]]

(Happily, this seems very backport-friendly, both from a bitrot perspective and from a likelihood-of-breaking-anything perspective)

Comment 9

[Robert Longson [:longsonr]]

Comment on attachment 8660724 [details] [diff] [review]
fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Difficult to run out of memory in exactly the right way.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Kind of obvious it's triggered by out of memory but how to get that out of memory is not so obvious.

Which older supported branches are affected by this flaw? all of them

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

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

How likely is this patch to cause regressions; how much testing does it need? Unlikely. Could do with a try landing before the real landing just to check it doesn't cause any existing test failures.

Comment 10

[q1]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> (In reply to q1 from comment #0)
> > The bugs are latent because the functions in which they occur don't seem to
> > be called by anything in the codebase (please verify).
> 
> Unfortunately, they are indeed called, via SMIL animation of path-data, I
> believe. So this is a bug that can be triggered in the wild (in low-memory
> conditions)....

Ack! I'll edit the title to remove "latent".

> <DETAILS on how these methods are called, if you're curious>
> The way these methods are called is a bit obscure.  We use nsSMILValues to
> represent intermediate values (in this case, intermediate path data on a
> path that is being animated).  Each nsSMILValue is a tagged union, with a
> pointer to a singleton nsISMILType object. That nsISMILType singleton has
> Add, Interpolate, etc. methods which we invoke from nsSMILValue to
> manipulate its data and perform those operations.  The class here
> (SVGPathSegListSMILType) is one such nsISMILType singleton, and it's passed
> in to new nsSMILValues in e.g. GetBaseValue and ValueFromString functions in
> SVGAnimatedPathSegList.cpp, here:
>  http://mxr.mozilla.org/mozilla-central/source/dom/svg/
> SVGAnimatedPathSegList.cpp?rev=b779fae94e88#160
> (...and that class is our internal representation of a path data segment
> list as its exposed via the DOM, IIRC).
> </DETAILS>

I see. I used DXR to conclude that there were no callers, so this points out a DXR shortcoming. In mitigation, VS 2015 Intellisense can't find the callers either, and it takes about 100 times as long to decide....

Comment 11

[Daniel Holbert [:dholbert]]

(FWIW, to make things more concrete, the place where e.g. this Interpolate() method is called is here:
118 nsresult
119 nsSMILValue::Interpolate(const nsSMILValue& aEndVal,
120                          double aUnitDistance,
121                          nsSMILValue& aResult) const
122 {
[...]
133   return mType->Interpolate(*this, aEndVal, aUnitDistance, aResult);

http://mxr.mozilla.org/mozilla-central/source/dom/smil/nsSMILValue.cpp?rev=b779fae94e88#133

Here, mType has declared type "nsISMILType*", and it *could* have concrete type SVGPathSegListSMILType. I don't know offhand if DXR or intellisense can be smart enough to figure this out - seems like virtual inheritance / virtual function calls could be hard for static analysis tools to be sure about.)

Anyway, we can take further static-analysis discussion elsewhere, maybe to a DXR bug if one is actually merited [not sure if one is].

Comment 12

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

Sec-approval+ to checkin on Oct 6, two weeks into the next cycle.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Al Billings [:abillings] from comment #12)
> Sec-approval+ to checkin on Oct 6, two weeks into the next cycle.

Hooray, that's today! (just about)

longsonr, go ahead & land this -- or I'm happy to land it for you if you'd like.

Comment 14

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3699de434198

Comment 15

[Robert Longson [:longsonr]]

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

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Adding tracking for 44 since this is a regression.

Comment 17

[Daniel Holbert [:dholbert]]

(though as noted in comment 1 & comment 3, this regressed ages ago, in bug 853554 / mozilla22.)

longsonr, mind requesting uplift approval? (For at least aurora/beta/esr38; not sure about others)

Comment 18

[Daniel Holbert [:dholbert]]

(RyanVM says esr31 is EOL, and high/crit bugs have auto-approval on b2g branches, so no approval requests are needed there IIUC.)

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 20

[Robert Longson [:longsonr]]

Comment on attachment 8660724 [details] [diff] [review]
fix

Approval Request Comment
[Feature/regressing bug #]: bug 853554
[User impact if declined]: security hole
[Describe test coverage new/current, TreeHerder]: existing reftests/mochitests cover this but this bug itself has no tests as it's about OOM protection
[Risks and why]: pretty low given current test coverage
[String/UUID change made/needed]: none

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8660724 [details] [diff] [review]
fix

Fix a critical issue, taking it. Should be in 38 beta 5.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/21ba3bb55cb0

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/546253af4e59

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/13c02a20d317

Comment 25

[Carsten Book [:Tomcat]]

this failed to apply to 2.2 could you take a look ?

grafting 307178:546253af4e59 "Bug 1204061 - check return values from some methods r=dholbert, a=sylvestre"
merging dom/svg/SVGPathSegListSMILType.cpp
warning: conflicts during merge.
merging dom/svg/SVGPathSegListSMILType.cpp incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue

Comment 26

[Robert Longson [:longsonr]]

I can't really, perhaps Daniel could help.

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: fix, backported to mozilla-b2g37_v2_2

Thanks for the backport-landings, tocmat.

Fortunately, the bitrot issues on 2.2 aren't functional -- they're just from bug 1127201, which was a mass s/NS_ABORT_IF_FALSE/MOZ_ASSERT/. This patch has that bitrot addressed.

(I'm assuming 2.2 is http://hg.mozilla.org/releases/mozilla-b2g37_v2_2/ - that's what I based this patch on top of.)

Comment 28

[Daniel Holbert [:dholbert]]

(I verified that mozilla-b2g37_v2_2 builds successfully with comment 27's backport-patch applied, too.)

Comment 29

[Carsten Book [:Tomcat]]

thanks daniel, landed this on 2.2 as https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7bc753230036

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/7bc753230036