1176270 - (CVE-2015-4488) StyleAnimationValue::operator= uses objects after delete on self-assignment

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[q1]

On self-assignment, StyleAnimationValue::operator= deletes an object, then creates a new object from the deleted object's freed memory.

3558: StyleAnimationValue&
3559: StyleAnimationValue::operator=(const StyleAnimationValue& aOther)
3560: {
3561:   FreeValue();
3562:   mUnit = aOther.mUnit;
3563:   switch (mUnit) {
...
3586:     case eUnit_Calc:
3587:     case eUnit_ObjectPosition:
3588:       MOZ_ASSERT(IsCSSValueUnit(mUnit),
3589:                  "This clause is for handling nsCSSValue-backed units");
3590:       MOZ_ASSERT(aOther.mValue.mCSSValue, "values may not be null");
3591:       mValue.mCSSValue = new nsCSSValue(*aOther.mValue.mCSSValue);
3592:       break;
...
3637: };

3786: void
3787: StyleAnimationValue::FreeValue()
3788: {
3789:   if (IsCSSValueUnit(mUnit)) {
3790:     delete mValue.mCSSValue;
3791:   } else if (IsCSSValueListUnit(mUnit)) {
3792:     delete mValue.mCSSValueList;
3793:   } else if (IsCSSValueSharedListValue(mUnit)) {
3794:     mValue.mCSSValueSharedList->Release();
3795:   } else if (IsCSSValuePairUnit(mUnit)) {
3796:     delete mValue.mCSSValuePair;
3797:   } else if (IsCSSValueTripletUnit(mUnit)) {
3798:     delete mValue.mCSSValueTriplet;
3799:   } else if (IsCSSRectUnit(mUnit)) {
3800:     delete mValue.mCSSRect;
3801:   } else if (IsCSSValuePairListUnit(mUnit)) {
3802:     delete mValue.mCSSValuePairList;
3803:   } else if (IsStringUnit(mUnit)) {
3804:     MOZ_ASSERT(mValue.mString, "expecting non-null string");
3805:     mValue.mString->Release();
3806:   }
3807: }

operator = mishandles all of the cases in which an object is deleted or Released by FreeValue, using the deleted/Released object *aOther.mValue.* as the source for the creation of a new object.

Comment 1

[q1]

In the case of eUnit_Transform and eUnit_UnparsedString, instead of creating a new object, the code AddRefs the Released object, which can be a deleted object.

Comment 2

[q1]

(This is 38.0.1\layout\style\StyleAnimationValue.cpp/.h)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Thanks for the bug report!  I suspect we don't ever *actually* self-assign these structs, though I'm not 100% sure.

Anyway, we definitely should handle this case, for robustness/safety.  Here's a simple patch, handling self-assignment with an early-return, as recommended in (2) of https://isocpp.org/wiki/faq/assignment-operators#self-assignment-how

(I don't think there's a more graceful/efficient way to make self-assignment simply Just Work with the existing code, due to the need to clean up our previous contents.)

Comment 4

[Daniel Holbert [:dholbert]]

Looks like this issue goes back to 2009, from bug 522852:
  http://hg.mozilla.org/mozilla-central/rev/2072cf8f65b4#l2.542

We probably should take this patch on all supported branches, to be on the safe side.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8624818 [details] [diff] [review]
fix v1

r=dbaron; this is the standard way to handle this

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: dummy patch for auditing

I did an audit locally, to see if we can ever get self-assignment (i.e. if this is possible to exploit).

I've convinced myself that it's not possible, and hence that we're safe.

I'm attaching the auditing patch that I used to convince myself.  I just replaced the operator= decl with "operator=(...) = delete;", so that each usage of the reassignment operator would turn into a build error. And then I checked the conditions around each usage, and proved to myself that none of these instances can be a self-assignment. Then I commented out the assignment (so I could proceed with the build), and waited for the next build error.

This patch builds, which means I found all the usages.

Comments are included next to each commented-out assignment, indicating why I'm sure we're safe at that spot. (Please feel free to sanity-check my reasoning.)

Comment 7

[Daniel Holbert [:dholbert]]

(Note that my auditing patch deletes the (implicitly-defined) reassignment operator for "ValueWrapper" in nsSMILCSSValueType.   That implicitly-defined reassignment operator uses the StyleAnimationValue assignment operator, because ValueWrapper has a StyleAnimationValue member-var.)

Comment 8

[Daniel Holbert [:dholbert]]

So I don't think this is actually exploitable, per comment 6.

But to be on the safe side, I still think we should keep this bug hidden & go through the sec-approval process, and that we should backport the patch to affected branches, in the unlikely event that I made a mistake in my auditing, or that my trunk auditing doesn't cover some other usage of this assignment operator in an older still-supported Firefox release.  (Plus, it's an extremely-safe fix, which can only improve safety/stability.)

Comment 9

[Daniel Holbert [:dholbert]]

RyanVM & dveditz confirm that I should skip sec-approval and just go ahead and land, actually, given that this is sec-low / extremely-unlikely-to-be-exploitable (per comment 6).

(sec-approval process is only for high/crit)

Comment 10

[q1]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Created attachment 8624886 [details] [diff] [review]
> dummy patch for auditing
> 
> I did an audit locally, to see if we can ever get self-assignment (i.e. if
> this is possible to exploit).
> 
> I've convinced myself that it's not possible, and hence that we're safe.
> ...
> Comments are included next to each commented-out assignment, indicating why
> I'm sure we're safe at that spot. (Please feel free to sanity-check my
> reasoning.)

This looks believable. Thank you for checking this out.

Comment 11

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2afd5728f13a

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to q1 from comment #10)
> This looks believable. Thank you for checking this out.

Of course! (Thank you for noticing & filing!)

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8624818 [details] [diff] [review]
fix v1

Approval Request Comment
========================
[Feature/regressing bug #]: This code dates back to bug 522852 (though per comment 6, it's unlikely that this can actually cause any problems).

[User impact if declined]: We'll be left with an unsafe coding pattern here. (AFAIK, there's no actual harm, per comment 6, but it's possible I missed something and there might be some way to trigger this case.)

[Describe test coverage new/current, TreeHerder]: Patch has landed on inbound and is baking there. We have good test coverage for various types of animations. And I don't think the patch *actually* affects our behavior, per comment 6, since we never invoke this code in a way that would be unsafe. (currently)

[Risks and why]: This is as low-risk as it gets. This is adding a safe early-return for a case that we initially missed, to handle a scenario that we don't actually ever hit right now. So, I don't think this even affects current behavior. (But to the extent that it does affect behavior at all, this patch makes us strictly safer/more stable.)

[String/UUID change made/needed]: None

For ESR31:
==========
[If this is not a sec:{high,crit} bug, please state case for ESR consideration:] Trivial patch, which fixes up an unsafe coding pattern & which (to the extent that it has any effect at all) makes us strictly safer/more stable.  Might as well just take it on all supported branches, including ESR.

Comment 14

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

We could still take this for beta for the RC build if it gets sec approval.

Comment 15

[Daniel Holbert [:dholbert]]

I was told not to get sec-approval (per comment 9), given that this is sec-low. For the same reason, let's not scramble to rush this into beta.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2afd5728f13a

Comment 17

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

Wontfix for 39. This could still make it into 40.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8624818 [details] [diff] [review]
fix v1

Taking it for esr 38 because we are in the stability period but not taking it for esr 31 as it is not a critical sec issue.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3bd6ba33cb82
https://hg.mozilla.org/releases/mozilla-esr38/rev/ddf4fce739c8

Comment 20

[Josh Cheng [:josh]]

Comment on attachment 8624818 [details] [diff] [review]
fix v1

Approving and please NI if causing any side effect.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3a4c0af882b5

Comment 22

[Christiane Ruetten [INACTIVE]]

Ryan, can you confirm the patch has made it into the 2.2 release?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment 21 right above yours and status-b2g-v2.2: fixed say that it did.