Note: There are a few cases of duplicates in user autocompletion which are being worked on.

GC hazard with cross-compartment sNPObjectMemberClass::npobjWrapper

RESOLVED FIXED in Firefox 15



5 years ago
5 years ago


(Reporter: bholley, Assigned: bholley)


({csectype-uaf, sec-high})

16 Branch
csectype-uaf, sec-high

Firefox Tracking Flags

(firefox14 unaffected, firefox15 fixed, firefox16+ fixed, firefox17+ fixed, firefox-esr10 unaffected)


(Whiteboard: [advisory-tracking-])


(1 attachment)

I was reading over my patches for bug 771202, and realized that I screwed this one up. :-(

GetNPObjectWrapper currently returns an unwrapped JSObject*, since its primary consumer is GetNPObject, which just grabs the private (an NPObject*). But it's _also_ used in CreateNPObjectMember when storing |npobjWrapper|:

I thought this was ok at the time, but sNPObjectMemberClass actually has a trace hook NPObjectMember_Trace, which means that npobjWrapper very much needs to be same-compartment with the NPObjectMember. Patch coming right up.
Created attachment 643765 [details] [diff] [review]
Wrap-by-default in GetNPObjectWrapper. v1

Attaching a patch. Flagging bsmedberg for review.
Attachment #643765 - Flags: review?(benjamin)
status-firefox15: --- → unaffected
status-firefox16: --- → affected
status-firefox17: --- → affected
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
Version: unspecified → 16 Branch
I guess this is probably s-s.
Group: core-security
Comment on attachment 643765 [details] [diff] [review]
Wrap-by-default in GetNPObjectWrapper. v1

Yeah. Damn this is complicated.
Attachment #643765 - Flags: review?(benjamin) → review+
Pushed to try here:

Looks green modulo infra orange, so pushing to m-i:

Comment 5

5 years ago
Last Resolved: 5 years ago
status-firefox17: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment on attachment 643765 [details] [diff] [review]
Wrap-by-default in GetNPObjectWrapper. v1

Waiting to hear whether this reduced crashes over in bug 774052, but we should take this on m-c and m-b regardless, because it's an obvious GC hazard. Requesting approval.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 771202
User impact if declined: Potential crashes and security bugs
Testing completed (on m-c, etc.): baked on m-c over the weekend. The crash this may fix is being tracked in bug 774052.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None
Attachment #643765 - Flags: approval-mozilla-beta?
Attachment #643765 - Flags: approval-mozilla-aurora?

Comment 7

5 years ago
Comment on attachment 643765 [details] [diff] [review]
Wrap-by-default in GetNPObjectWrapper. v1

[Triage Comment]
Cleanup for uplifted patch, and low risk. Let's take these on branches.
Attachment #643765 - Flags: approval-mozilla-beta?
Attachment #643765 - Flags: approval-mozilla-beta+
Attachment #643765 - Flags: approval-mozilla-aurora?
Attachment #643765 - Flags: approval-mozilla-aurora+
tracking-firefox16: ? → +
tracking-firefox17: ? → -
tracking-firefox17: - → +
Pushed to m-a:
status-firefox16: affected → fixed
Pushed to m-b:
status-firefox15: unaffected → fixed
status-firefox-esr10: --- → unaffected
status-firefox14: --- → unaffected
Whiteboard: [advisory-tracking-]
Keywords: csec-uaf, sec-high
Group: core-security
What is the visible impact while testing this on a build that reproduces this issue?
You need to log in before you can comment on or make changes to this bug.