Closed Bug 1340482 Opened 5 years ago Closed 5 years ago

Shape field not traced for non-native shaped objects

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main53+][adv-esr52.1+])

Attachments

(1 file)

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)
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.
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
Track 54+ as sec-high.
https://hg.mozilla.org/integration/mozilla-inbound/rev/846948a41b6c

Please request Aurora/Beta/ESR52 approval on this as soon as possible.
Flags: needinfo?(jcoppeard)
Whiteboard: [checkin on 3/21]
https://hg.mozilla.org/mozilla-central/rev/846948a41b6c
Status: NEW → RESOLVED
Closed: 5 years ago
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.
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.
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 on attachment 8838474 [details] [diff] [review]
trace-object-shape

sec-high uaf fix for esr52
Attachment #8838474 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
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.