nsTArray copies JS::ObjectPtr with memmove

RESOLVED FIXED in Firefox -esr52

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

({csectype-uaf, sec-high})

unspecified
mozilla58
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr5257+ fixed, firefox55 wontfix, firefox56 wontfix, firefox57+ fixed, firefox58+ fixed)

Details

(Whiteboard: [adv-main57+][adv-esr52.5+][post-critsmash-triage])

Attachments

(3 attachments)

nsTArray.h is missing the template magic to make sure it copies JS::ObjectPtr via construction/destruction rather than memmove.  This is bad because ObjectPtr contains a Heap<JSObject*> and this can be referenced directly by the store buffer.  memmove does not update store buffer pointers.  This has probably been causing crashes for a while.
Patch to add the MOZ_NON_MEMMOVABLE attribute to classes that can be pointed to by the storebuffer.

Also adds a move constructor to ObjectPtr that clears the source object.  This is necessary so you can move live instances without the destructor of the source object asserting because finalize() hasn't been called.
Attachment #8908602 - Flags: review?(sphink)
Patch to add JS::ObjectPtr to the list of things that nsTArray must copy with constuctors/destructors rather than using memcpy, plus a little refactoring.
Attachment #8908603 - Flags: review?(nfroyd)
Attachment #8908603 - Flags: review?(nfroyd) → review+
Comment on attachment 8908603 [details] [diff] [review]
bug1400003-nstarray-changes

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Very difficult.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

Everything back to FF33.

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

This was an omission in bug 877762 but it's only been a problem since bug 990729.

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

This should be trivial.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.
Attachment #8908603 - Flags: sec-approval?
This is too late to go into Firefox 56 so I'm not going to approve trunk checkin yet to limit exposure. This will be approved to land around October 12.
Comment on attachment 8908602 [details] [diff] [review]
bug1400003-js-changes

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

Uh... wow.

jonco++
Attachment #8908602 - Flags: review?(sphink) → review+
Comment on attachment 8908602 [details] [diff] [review]
bug1400003-js-changes

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

Whoa, wait. This patch sets MOZ_NON_MEMMOVABLE for Heap. Doesn't ObjectPtr need it too?
Attachment #8908602 - Flags: review+
(In reply to Steve Fink [:sfink] [:s:] from comment #6)
ObjectPtr contains Heap<JSObject*> so should be treated as MOZ_NON_MEMMOVABLE.

I did a try build to check this with the MOZ_NON_MEMMOVABLE attribute on Heap<T> but without the changes to nsTArray:

https://treeherder.mozilla.org/logviewer.html#?job_id=131248949&repo=try&lineNumber=11202
(In reply to Jon Coppeard (:jonco) from comment #7)
> (In reply to Steve Fink [:sfink] [:s:] from comment #6)
> ObjectPtr contains Heap<JSObject*> so should be treated as
> MOZ_NON_MEMMOVABLE.
> 
> I did a try build to check this with the MOZ_NON_MEMMOVABLE attribute on
> Heap<T> but without the changes to nsTArray:
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=131248949&repo=try&lineNumber=11202

Ah! That makes sense, I just didn't think of it.
Attachment #8908602 - Flags: review+
Priority: -- → P1
sec-approval for trunk checkin on October 9, two weeks into the cycle. Do not check this that date, please.

We'll want a beta patch made and nominated for 57 and ESR52 at that time.
Whiteboard: [checkin on 10/9]
Attachment #8908603 - Flags: sec-approval? → sec-approval+
"Do not check this in before that date" rather (that is, October 9).
Whiteboard: [checkin on 10/9]
https://hg.mozilla.org/mozilla-central/rev/6ad5b916c965

Please request Beta and ESR52 approval on this when you get a chance. Note that it grafts cleanly to Beta but will require a rebased patch for ESR52.
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(jcoppeard)
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
jonco or nfroyd, can you help out with uplift requests & a patch for ESR ? We could get this into tomorrow's beta 8 build if it lands today.
Flags: needinfo?(nfroyd)
Comment on attachment 8908603 [details] [diff] [review]
bug1400003-nstarray-changes

This approval request is for both patches in this bug.  Do the patches need to landed simultaneously on Beta and ESR52?  I can do this uplift request for Beta, and could probably do the rebase/uplift request for ESR52, but would feel slightly more comfortable if jonco did the rebase/uplift request for ESR52.

Approval Request Comment
[Feature/Bug causing the regression]: bug 990729/bug 877762
[User impact if declined]: getting pwned
[Is this code covered by automated tests?]: Yes?
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Straightforward code changes; jonco also said it was not risky :P
[String changes made/needed]: None
Flags: needinfo?(nfroyd)
Attachment #8908603 - Flags: approval-mozilla-beta?
Comment on attachment 8908603 [details] [diff] [review]
bug1400003-nstarray-changes

Thanks Nathan, we'll do our best not to get pwned!
Attachment #8908603 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: javascript-core-security → core-security-release
Sorry for the delay!  Here's a patch for esr52.
Flags: needinfo?(jcoppeard)
Attachment #8918255 - Flags: review+
Comment on attachment 8918255 [details] [diff] [review]
bug1400003-esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bug.
User impact if declined: Possible crash / security vulnerability.
Fix Landed on Version: 58
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Attachment #8918255 - Flags: approval-mozilla-esr52?
Comment on attachment 8918255 [details] [diff] [review]
bug1400003-esr52

Sec-high, ESR52.5+
Attachment #8918255 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main57+][adv-esr52.5+]
Flags: qe-verify-
Whiteboard: [adv-main57+][adv-esr52.5+] → [adv-main57+][adv-esr52.5+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.