Bug 1204061 (CVE-2015-7199)

Missing status checks in AddWeightedPathSegLists and SVGPathSegListSMILType::Interpolate cause memory-safety bugs

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: q1, Assigned: longsonr)

Tracking

({sec-critical})

40 Branch
mozilla44
Points:
---
Bug Flags:
sec-bounty +
in-testsuite -

Firefox Tracking Flags

(firefox40 wontfix, firefox41 wontfix, firefox42+ fixed, firefox43+ fixed, firefox44+ fixed, firefox-esr31 affected, firefox-esr3842+ fixed, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master fixed, thunderbird_esr38 fixed)

Details

(Whiteboard: [b2g-adv-main-2.5+][post-critsmash-triage[adv-main42+][adv-esr38.4+])

Attachments

(2 attachments)

Reporter

Description

4 years ago
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 ).
Group: core-security → dom-core-security
Group: dom-core-security → layout-core-security
Component: DOM → SVG
Flags: sec-bounty?
Flags: needinfo?(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).
(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>
(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.)
Flags: needinfo?(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.)
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. :))
Flags: needinfo?(dholbert)
Assignee

Comment 6

4 years ago
Posted patch fixSplinter Review
Flags: needinfo?(dholbert)
Attachment #8660724 - Flags: review?(dholbert)
Assignee

Updated

4 years ago
Assignee: nobody → longsonr
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.
Attachment #8660724 - Flags: review?(dholbert) → review+
(Happily, this seems very backport-friendly, both from a bitrot perspective and from a likelihood-of-breaking-anything perspective)
Assignee

Comment 9

4 years ago
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.
Attachment #8660724 - Flags: sec-approval?
Reporter

Comment 10

4 years ago
(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....
Reporter

Updated

4 years ago
Summary: Missing status checks in AddWeightedPathSegLists and SVGPathSegListSMILType::Interpolate cause latent memory-safety bugs → Missing status checks in AddWeightedPathSegLists and SVGPathSegListSMILType::Interpolate cause memory-safety bugs
(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].
Sec-approval+ to checkin on Oct 6, two weeks into the next cycle.
Whiteboard: [checkin on 10/6]
Attachment #8660724 - Flags: sec-approval? → sec-approval+
Attachment #8660724 - Attachment description: one less to-do item → fix
(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.
Flags: needinfo?(longsonr)
Whiteboard: [checkin on 10/6] → [checkin on 10/6][b2g-adv-main-2.5+]
Assignee

Comment 15

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4c2f277aeae
Flags: needinfo?(longsonr) → in-testsuite-
Whiteboard: [checkin on 10/6][b2g-adv-main-2.5+] → [b2g-adv-main-2.5+]
Adding tracking for 44 since this is a regression.
(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)
Flags: needinfo?(longsonr)
(RyanVM says esr31 is EOL, and high/crit bugs have auto-approval on b2g branches, so no approval requests are needed there IIUC.)
https://hg.mozilla.org/mozilla-central/rev/f4c2f277aeae
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: layout-core-security → core-security-release
Assignee

Comment 20

4 years ago
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
Flags: needinfo?(longsonr)
Attachment #8660724 - Flags: approval-mozilla-esr38?
Attachment #8660724 - Flags: approval-mozilla-beta?
Attachment #8660724 - Flags: approval-mozilla-aurora?
Comment on attachment 8660724 [details] [diff] [review]
fix

Fix a critical issue, taking it. Should be in 38 beta 5.
Attachment #8660724 - Flags: approval-mozilla-esr38?
Attachment #8660724 - Flags: approval-mozilla-esr38+
Attachment #8660724 - Flags: approval-mozilla-beta?
Attachment #8660724 - Flags: approval-mozilla-beta+
Attachment #8660724 - Flags: approval-mozilla-aurora?
Attachment #8660724 - Flags: approval-mozilla-aurora+
Flags: sec-bounty? → sec-bounty+
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
Flags: needinfo?(longsonr)
Assignee

Comment 26

4 years ago
I can't really, perhaps Daniel could help.
Flags: needinfo?(longsonr) → needinfo?(dholbert)
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.)
Flags: needinfo?(dholbert) → needinfo?(cbook)
(I verified that mozilla-b2g37_v2_2 builds successfully with comment 27's backport-patch applied, too.)
thanks daniel, landed this on 2.2 as https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7bc753230036
Flags: needinfo?(cbook)
Whiteboard: [b2g-adv-main-2.5+] → [b2g-adv-main-2.5+][post-critsmash-triage]
Whiteboard: [b2g-adv-main-2.5+][post-critsmash-triage] → [b2g-adv-main-2.5+][post-critsmash-triage[adv-main42+][adv-esr38.4+]
Alias: CVE-2015-7199
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.