1371865 - PRE_HELPER_STUB calls method on native without rooting the reflector

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

This is a variation on bug 1371259 for older XPConnect bindings.

This macro does:
  JSObject* unwrapped = js::CheckedUnwrap(obj, false);
  ...
  XPCWrappedNative* wrapper = XPCWrappedNative::Get(unwrapped);
  ...
  wrapper->GetScriptable()->(some function call)

The basic problem is that obj being rooted does not imply that unwrapped is rooted, so we cannot be sure that wrapper and its scriptable is rooted. At least, as far as I can tell.

This macro is used in XPC_WN_Helper_GetProperty and four other similar methods.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> The basic problem is that obj being rooted does not imply that unwrapped is rooted

To be clear, this is because obj could get swapped with a dead object wrapper.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Root the unwrapped object in PRE_HELPER_STUB

I didn't put anything in comments or the commit message explaining _why_ this should be rooted.  That's annoying, but I don't see a good way to have that documentation in there without exposing the real underlying problem.  :(

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Root the unwrapped object in PRE_HELPER_STUB

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8877717 [details] [diff] [review]
Root the unwrapped object in PRE_HELPER_STUB

[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 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8877717 [details] [diff] [review]
Root the unwrapped object in PRE_HELPER_STUB

Review of attachment 8877717 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing these bugs.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, moving this patch to bug 1371259.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed by bug 1371259.