1371853 - Possible native rooting issue in XPCConvert::JSObject2NativeInterface()

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

This method contains a call to CheckedUnwrap:
  JSObject* inner = js::CheckedUnwrap(src, /* stopAtWindowProxy = */ false);
then an XPCWN is gotten off of inner, an nsISupports is gotten from that XPCWN, then we call QI on it. If the QI is implemented in JS (is that possible?) and src != inner, then the XPCWN or the nsISupports might get destroyed. |inner| should probably at least be rooted, if not the XPCWN and |iface|.

Comment 1

[Andrew McCreight [:mccr8]]

There's similar issues with the CheckedUnwrap call in XPCConvert::JSValToXPCException().

Comment 2

[Boris Zbarsky [:bzbarsky]]

> If the QI is implemented in JS (is that possible?)

Yes, if it's a double-wrapped XPCWrappedJS.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Just for posterity, the failure mode is if entering JS causes us to turn src into a DeadObjectWrapper, thereby effectively leaving "inner" not rooted and possibly killing the XPCWN hanging off it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Root the unwrapped object in XPCConvert code

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8877718 [details] [diff] [review]
Root the unwrapped object in XPCConvert code

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not very.  For one thing, the set of objects that pages can access that are actually XPCWrappedNative is pretty limited.  But also, it's not immediately clear how "unwrapped" could fail to be rooted.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  Not any more so than the actual change.

Which older supported branches are affected by this flaw?  All of them.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  This should be pretty easy and safe to backport.

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, moving this patch to bug 1371259.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed by bug 1371259.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Minusing this for advisories since Nils' bug will get advisory credit.