Closed Bug 1371863 Opened 7 years ago Closed 7 years ago

Questionable looking calls to UnwrapReflectorToISupports()

Categories

(Core :: XPConnect, enhancement, P1)

Product:
Core
Component:
XPConnect
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox-esr52 55+ fixed
firefox54 --- wontfix
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: mccr8, Assigned: bzbarsky)

References

Details

(Keywords: sec-audit, Whiteboard: [adv-main55-][adv-esr52.3-][post-critsmash-triage]) 

UnwrapReflectorToISupports calls CheckedUnwrap, but does not root the output. It then extracts a native from the output. Some of the callers, such as UnwrapArgImpl, call QI on the result without rooting it. I don't know if that is possibly dangerous.
See Also: → CVE-2017-7801
Yes, that's dangerous.  I'll fix at least some of these over in bug 1371259.
Depends on: CVE-2017-7801
So what really bothers me about this is that this function, and nsXPConnect::GetNativeOfWrapper, are more or less footguns in their current form.  And I'm not sure how to best make them not be footguns.  :(
You could make it return already_AddRefed<nsISupports>. (Or the more modern nsCOMPtr<nsISupports>) That's how most of the callers use it.
Hmm.  I was going to say I though it would be a perf problem, but it might not be one.  I'll look through the callers carefully to make sure...
I'm giving this to you, Boris, but I know you're working on this mostly in another bug. If this one isn't covered there, please let me know. Thanks.
Assignee: nobody → bzbarsky
Priority: -- → P1
Marking sec-high per comment 1.
Keywords: sec-audit → csectype-uaf, sec-high
The other bug completely fixes this one; it implements the suggestion from comment 3.
Keywords: csectype-uaf, sec-high → sec-audit
Fixed by bug 1371259.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Group: dom-core-security → core-security-release
status-firefox54: --- → wontfix
status-firefox55: --- → fixed
status-firefox56: --- → fixed
status-firefox-esr52: --- → fixed
Target Milestone: --- → mozilla56
tracking-firefox-esr52: --- → 55+
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
You need to log in before you can comment on or make changes to this bug.