Closed Bug 1371424 Opened 3 years ago Closed 2 years ago

Crash in mozilla::dom::RsaHashedKeyAlgorithm::ToObjectInternal on GC poison value

Categories

(Core :: DOM: Core & HTML, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 55+ fixed
firefox53 --- wontfix
firefox54 + wontfix
firefox55 + fixed
firefox56 + fixed

People

(Reporter: mccr8, Assigned: bzbarsky)

References

Details

(Keywords: crash, csectype-uaf, sec-critical, Whiteboard: [adv-main55+][adv-esr52.3+][post-critsmash-triage])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-ea88e19f-5ceb-4ee0-bfaa-0e7280170607.
=============================================================

37 crashes in the past week, so not very common, but they are mostly crashing on values that look like 0xffffffffbad0bad9.

I think this is the value RelocationOverlay::Relocated, which maybe is some kind of forwarding value in the nursery, so I think there's some kind of incorrect rooting in this code.

There are a number of open intermittent bugs with this crash signature, though none of the bugs have been touched in the last 5 months. But it could be that the volume is too low to merit an orange factor comment.
Jon, what does this poison value indicate?
Flags: needinfo?(jcoppeard)
So the code around that crashing line in KeyAlgorithmBinding.cpp is:

    JS::Rooted<JS::Value> temp(cx);
    Uint8Array const & currentValue = mPublicExponent;
    temp.setObject(*currentValue.Obj());
    if (!MaybeWrapNonDOMObjectValue(cx, &temp)) {
      return false;
    }
    if (!JS_DefinePropertyById(cx, obj, atomsCache->publicExponent_id, temp, JSPROP_ENUMERATE)) {
      return false;
    }

and the crashing line is the MaybeWrapNonDOMObjectValue call.  This suggests that mPublicExponent.Obj() is garbage.

The callsite looks like this:

      RootedDictionary<RsaHashedKeyAlgorithm> rsa(cx);
      converted = mAlgorithm.mRsa.ToKeyAlgorithm(cx, rsa);
      if (converted) {
        converted = ToJSValue(cx, rsa, &val);
      }

so it's correctly rooting the RsaHashedKeyAlgorithm.  And RsaHashedKeyAlgorithm::TraceDictionary does:

  mPublicExponent.TraceSelf(trc);

which should land at http://searchfox.org/mozilla-central/source/dom/bindings/TypedArray.h#45 ... which is buggy!  It traces mTypedObj twice, instead of tracing mWrappedObj one of those times.  That dates back to bug 1020034.  :(

Anyway, Obj() returns mWrappedObj, which is exactly the thing that's not being traced.
I bet this fixes it....
Attachment #8875887 - Flags: review?(continuation)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8875887 [details] [diff] [review]
Trace the right thing in TypedArray's TraceSelf implementation

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Random-ish crashes, but maybe this could be exploitable.
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Very low risk
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1020034
[User impact if declined]: Crashes, maybe exploitable
[Is this code covered by automated tests?]: Kinda sorta
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: Unclear
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It just fixes an obvious typo so we trace
   the right thing.
[String changes made/needed]: None.
Attachment #8875887 - Flags: approval-mozilla-release?
Attachment #8875887 - Flags: approval-mozilla-esr52?
Attachment #8875887 - Flags: approval-mozilla-esr45?
Attachment #8875887 - Flags: approval-mozilla-beta?
Flags: needinfo?(jcoppeard)
Comment on attachment 8875887 [details] [diff] [review]
Trace the right thing in TypedArray's TraceSelf implementation

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

Yikes! I'm surprised we haven't noticed this before. Thanks for fixing this, Boris.
Attachment #8875887 - Flags: review?(continuation) → review+
Comment on attachment 8875887 [details] [diff] [review]
Trace the right thing in TypedArray's TraceSelf implementation

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  I don't actually know.  It's not trivial, but someone who knows our code well enough could figure out that at least they need to exercise typed-array-to-JS webidl conversions and gc at the same time.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  I don't think so.

Which older supported branches are affected by this flaw? All of them;  This was introduced in Firefox 33.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  This is quite easy to backport.  The code _has_ changed some (last in Firefox 46), so an ESR 45 backport would need about 5 mins of work.  Anything newer this should apply to cleanly.

How likely is this patch to cause regressions; how much testing does it need?  Not likely and very little, apart from being able to tell that it fixes the bug.
Attachment #8875887 - Flags: sec-approval?
[Tracking Requested - why for this release]: This seems very low risk so I think we should consider it for a ride along for a dot release for 54 or whatever.
I guess I'd actually rate this critical, seeing Boris's patch. I have no idea how this remained unfixed for so long.
Keywords: sec-highsec-critical
OS: Windows 10 → All
Hardware: x86 → All
So one note is that mTypedObj and mWrappedObj are more or less the same thing: the latter is a cross-compartment wrapper for the former.  This means that we're not talking UAF here in some broad sense, since the actualy typed array is mTypedObj.  But mWrappedObj _can_ get moved and then our pointer will either be poison or maybe end up pointing to some other object, which is not great at all.

critical is probably the right rating.
I searched in Socorro for crashes on addresses containing bad0bad, and this was the only one in high volume (where "high" is around 30) that wasn't a generic GC tracing signature, FWIW.
Track 54+/55+ as sec-critical.
(In reply to Andrew McCreight [:mccr8] from comment #1)
> Jon, what does this poison value indicate?

This indicates that the thing has been moved (by generational or compacting GC) and we are dereferencing a stale pointer to the old location.
Comment on attachment 8875887 [details] [diff] [review]
Trace the right thing in TypedArray's TraceSelf implementation

We're about to ship 54 so I'm giving sec-approval+ for trunk checkin on 6/26, two weeks into the new cycle. We should take it everywhere then.

I agree that a dot release of 54 and ESR52 could include this too.
Attachment #8875887 - Flags: sec-approval? → sec-approval+
Whiteboard: [checkin on 6/26]
Another variant: https://crash-stats.mozilla.com/report/index/ee28d0fc-1344-4b13-9778-8a8800170609
[@ mozilla::dom::RsaHashedKeyAlgorithm::ToObjectInternal const ]
11 in the last week
Crash Signature: [@ mozilla::dom::RsaHashedKeyAlgorithm::ToObjectInternal] → [@ mozilla::dom::RsaHashedKeyAlgorithm::ToObjectInternal] [@ mozilla::dom::RsaHashedKeyAlgorithm::ToObjectInternal const ]
Comment on attachment 8875887 [details] [diff] [review]
Trace the right thing in TypedArray's TraceSelf implementation

sec-crit, let's take it in 55.0b1
Attachment #8875887 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8875887 [details] [diff] [review]
Trace the right thing in TypedArray's TraceSelf implementation

ESR45 is officially EOL.
Attachment #8875887 - Flags: approval-mozilla-esr45?
Comment on attachment 8875887 [details] [diff] [review]
Trace the right thing in TypedArray's TraceSelf implementation

resetting beta approval to work around https://github.com/mozilla/relman-auto-nag/issues/97 since this isn't something we want to land right away
Attachment #8875887 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Comment on attachment 8875887 [details] [diff] [review]
Trace the right thing in TypedArray's TraceSelf implementation

Sec-critical issue, let's take this for the beta 5 or 6 build this week. 
Gerry, you may want to consider this for a dot release.
Flags: needinfo?(gchang)
Attachment #8875887 - Flags: approval-mozilla-esr52?
Attachment #8875887 - Flags: approval-mozilla-esr52+
Attachment #8875887 - Flags: approval-mozilla-beta?
Attachment #8875887 - Flags: approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/f5d25845cb77
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Comment on attachment 8875887 [details] [diff] [review]
Trace the right thing in TypedArray's TraceSelf implementation

Fix a sec-critical. Release54+. Should be in 54.0.1.
Flags: needinfo?(gchang)
Attachment #8875887 - Flags: approval-mozilla-release? → approval-mozilla-release+
Hi Ryan, let's take this fix in esr52.2.1 dot release since this will also be included in 54.0.1
Flags: needinfo?(ryanvm)
Why are we taking this particular sec-critical but not others in a dot release? What makes this one special?
Flags: needinfo?(lhenry)
Flags: needinfo?(gchang)
Change of plans, Dan and I had a chat and we decided not to take this in the dot release.
Flags: needinfo?(ryanvm)
Comment on attachment 8875887 [details] [diff] [review]
Trace the right thing in TypedArray's TraceSelf implementation

We decide not to take this in 54 dot release.
Flags: needinfo?(gchang)
Attachment #8875887 - Flags: approval-mozilla-release+ → approval-mozilla-release-
Can we get this checked into ESR52.3 now?
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/releases/mozilla-esr52/rev/40ce248a8c15
Flags: needinfo?(lhenry)
Flags: needinfo?(bzbarsky)
Target Milestone: --- → mozilla56
Duplicate of this bug: 1376845
Group: dom-core-security → core-security-release
Whiteboard: [adv-main55+][adv-esr52.3+]
Flags: qe-verify-
Whiteboard: [adv-main55+][adv-esr52.3+] → [adv-main55+][adv-esr52.3+][post-critsmash-triage]
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.