Shape field not traced for non-native shaped objects

RESOLVED FIXED in Firefox -esr52

Status

()

Core
JavaScript: GC
RESOLVED FIXED
9 months ago
24 days ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

({csectype-uaf, sec-high})

unspecified
mozilla55
csectype-uaf, sec-high
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox51 wontfix, firefox52 wontfix, firefox-esr5253+ fixed, firefox53+ fixed, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [adv-main53+][adv-esr52.1+])

Attachments

(1 attachment)

Created attachment 8838474 [details] [diff] [review]
trace-object-shape

Since we introduced ShapedObject, JSObject::traceChildren() doesn't trace the shape if the object is shaped but not native.  This might cause UAF if a shape is swept while still in use (although mostly objects are marked by a different path), or if such an object is moved by compacting GC.
Attachment #8838474 - Flags: review?(sphink)
Keywords: csectype-uaf, sec-high
Attachment #8838474 - Flags: review?(sphink) → review+
Comment on attachment 8838474 [details] [diff] [review]
trace-object-shape

[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 50.

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

Bug 1284634.

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

Should be trivial.

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

Unlikely.
Attachment #8838474 - Flags: sec-approval?
We've had our last beta. I think this is too late for 52, which affects sec-approval, but it is a small change. I'm asking release management for their opinion.
status-firefox51: --- → wontfix
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → affected
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
Let's aim to fix this in 53.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
sec-approval+ for checkin on 3/21, two weeks into the next cycle. We're a week from final release of 52 right now.
Whiteboard: [checkin on 3/21]
Attachment #8838474 - Flags: sec-approval? → sec-approval+
[Tracking Requested - why for this release]: sec-high
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?

Comment 6

9 months ago
Track 54+ as sec-high.
tracking-firefox54: ? → +
tracking-firefox53: ? → +
https://hg.mozilla.org/integration/mozilla-inbound/rev/846948a41b6c

Please request Aurora/Beta/ESR52 approval on this as soon as possible.
status-firefox52: affected → wontfix
status-firefox55: --- → affected
tracking-firefox52: ? → ---
tracking-firefox55: --- → ?
tracking-firefox-esr52: --- → ?
Flags: needinfo?(jcoppeard)
Whiteboard: [checkin on 3/21]

Comment 8

8 months ago
https://hg.mozilla.org/mozilla-central/rev/846948a41b6c
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8838474 [details] [diff] [review]
trace-object-shape

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1284634.
[User impact if declined]: Possible crash / security vulnerability.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[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?]: This is a simple change to trace objects' shapes in some circumstances when it wasn't being traced before.
[String changes made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8838474 - Flags: approval-mozilla-beta?
Attachment #8838474 - Flags: approval-mozilla-aurora?
Comment on attachment 8838474 [details] [diff] [review]
trace-object-shape

[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: 55.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Attachment #8838474 - Flags: approval-mozilla-esr52?
Tracking 55+, as well as for esr52.
tracking-firefox55: ? → +
tracking-firefox-esr52: ? → 52+
Shouldn't the esr52 tracking be 53+ since that's the corresponding Fx release we're hoping to ship this with?
Flags: needinfo?(mozillamarcia.knous)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Shouldn't the esr52 tracking be 53+ since that's the corresponding Fx
> release we're hoping to ship this with?

Yes, sorry I will correct that.
tracking-firefox-esr52: 52+ → 53+
Flags: needinfo?(mozillamarcia.knous)
Comment on attachment 8838474 [details] [diff] [review]
trace-object-shape

Fix a  sec-high. Aurora54+ & Beta53+.
Attachment #8838474 - Flags: approval-mozilla-beta?
Attachment #8838474 - Flags: approval-mozilla-beta+
Attachment #8838474 - Flags: approval-mozilla-aurora?
Attachment #8838474 - Flags: approval-mozilla-aurora+

Comment 15

8 months ago
uplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0fff3bdd02a7
status-firefox54: affected → fixed

Comment 16

8 months ago
uplift
https://hg.mozilla.org/releases/mozilla-beta/rev/3d0b8766a2a5
status-firefox53: affected → fixed
Comment on attachment 8838474 [details] [diff] [review]
trace-object-shape

sec-high uaf fix for esr52
Attachment #8838474 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 18

8 months ago
uplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/512604631b23
status-firefox-esr52: affected → fixed
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main53+][adv-esr52.1+
Whiteboard: [adv-main53+][adv-esr52.1+ → [adv-main53+][adv-esr52.1+]
Setting qe-verify- based on Jon's assessment on manual testing needs and the fact that this fix has automated coverage (see Comment 17).
Flags: qe-verify-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.