Bug 1176270 (CVE-2015-4488)

StyleAnimationValue::operator= uses objects after delete on self-assignment

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: q1, Assigned: dholbert)

Tracking

({sec-low})

Trunk
mozilla41
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox39 wontfix, firefox40 fixed, firefox41 fixed, firefox-esr31 wontfix, firefox-esr3840+ fixed, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [b2g-adv-main2.2+][post-critsmash-triage][adv-main40+][adv-esr38.2+])

Attachments

(2 attachments)

Reporter

Description

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

Comment 1

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

Updated

4 years ago
Component: Untriaged → Layout
Product: Firefox → Core
Reporter

Comment 2

4 years ago
(This is 38.0.1\layout\style\StyleAnimationValue.cpp/.h)
Assignee

Updated

4 years ago
Assignee: nobody → dholbert
Version: 38 Branch → Trunk
Assignee

Comment 3

4 years ago
Posted patch fix v1Splinter Review
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.)
Attachment #8624818 - Flags: review?(dbaron)
Assignee

Comment 4

4 years ago
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 on attachment 8624818 [details] [diff] [review]
fix v1

r=dbaron; this is the standard way to handle this
Attachment #8624818 - Flags: review?(dbaron) → review+
Assignee

Comment 6

4 years ago
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.)
Assignee

Comment 7

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

Comment 8

4 years ago
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.)
Keywords: sec-low
Assignee

Comment 9

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

Comment 10

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

Comment 12

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

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

Comment 13

4 years ago
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.
Attachment #8624818 - Flags: approval-mozilla-esr31?
Attachment #8624818 - Flags: approval-mozilla-beta?
Attachment #8624818 - Flags: approval-mozilla-aurora?
Attachment #8624818 - Flags: approval-mozilla-esr38?
Attachment #8624818 - Flags: approval-mozilla-b2g37?
We could still take this for beta for the RC build if it gets sec approval.
Assignee

Comment 15

4 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/2afd5728f13a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Wontfix for 39. This could still make it into 40.
Attachment #8624818 - Flags: approval-mozilla-beta?
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.
Attachment #8624818 - Flags: approval-mozilla-esr38?
Attachment #8624818 - Flags: approval-mozilla-esr38+
Attachment #8624818 - Flags: approval-mozilla-esr31?
Attachment #8624818 - Flags: approval-mozilla-esr31-
Attachment #8624818 - Flags: approval-mozilla-aurora?
Attachment #8624818 - Flags: approval-mozilla-aurora+
Comment on attachment 8624818 [details] [diff] [review]
fix v1

Approving and please NI if causing any side effect.
Attachment #8624818 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Ryan, can you confirm the patch has made it into the 2.2 release?
Flags: needinfo?(ryanvm)
Whiteboard: [b2g-adv-main2.2+]
Comment 21 right above yours and status-b2g-v2.2: fixed say that it did.
Flags: needinfo?(ryanvm)
Whiteboard: [b2g-adv-main2.2+] → [b2g-adv-main2.2+][post-critsmash-triage]
Whiteboard: [b2g-adv-main2.2+][post-critsmash-triage] → [b2g-adv-main2.2+][post-critsmash-triage][adv-main40+][adv-esr38.2+]
Alias: CVE-2015-4488
Group: core-security
You need to log in before you can comment on or make changes to this bug.