All users were logged out of Bugzilla on October 13th, 2018

UAF with dynamically-allocated JSClasses

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jandem, Assigned: njn)

Tracking

({csectype-uaf, sec-high})

unspecified
mozilla53
csectype-uaf, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox50 wontfix, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Patches in bug 1302996 are triggering hard-to-reproduce TI/Ion crashes. I'm not entirely sure yet, but I think what's happening is that we're accessing ObjectGroups for dynamically-allocated Classes.

Looking at crash-stats I see some crashes that look similar [1] (there are other signatures probably).

Something like this would trigger it:

(1) Allocate XPCNativeScriptableShared holding a malloc'd JSClass for one or more JSObjects. All JSObjects have an ObjectGroup associated with them that points to this Class.

(2) This ObjectGroup can be stored in TypeSets etc (sets hold weak references).

(3) When all objects with this JSClass are finalized, the JSClass is freed by XPConnect.

(4) I think at this point the ObjectGroup is not guaranteed to be finalized (due to incremental GC or things holding references to it), and Ion/TI code can still use it to query stuff about the Class ==> UAF.

Unfortunately this is hard to reproduce because we have to trigger an (incremental) GC that collects the objects but not the group. This can definitely happen though.

:njn ran into similar issues with BaseShapes + dynamically allocated JSClasses before.

---------

We could simply ignore these Classes in Ion/TI code. However, the issue is more complicated as the Class is also used as key to map from proto + Class to ObjectGroup etc. A better fix may be something like this:

(1) Add a DYNAMICALLY_ALLOCATED flag to JSClass.

(2) Add a JSAPI to register a dynamic JSClass with the engine. This hands over ownership to SpiderMonkey. We can then stick it in a HashMap that maps the JSClass* to a reference count (1 initially, for the XPConnect reference).

(3) Whenever an ObjectGroup or BaseShape is created for a JSClass with this flag set, we increment the reference count in the table. Hopefully this won't affect perf too much as dynamic JSClasses aren't used that much anymore.

(4) The ObjectGroup and BaseShape finalizers will decrement the reference count.

(5) If this refcount drops to zero, the JS engine (not XPConnect) frees the JSClass.

When we destroy the JSContext, we can assert the map is empty.

Can anyone think of a nicer way to fix this?

---------

[1] https://crash-stats.mozilla.com/signature/?signature=ObjectHasExtraOwnProperty&date=%3E%3D2016-11-02T19%3A10%3A00.000Z&date=%3C2016-12-10T19%3A10%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports
(Reporter)

Updated

2 years ago
Keywords: sec-high
With some careful refactoring we might be able to get away with making these classes non-dynamic. The reasons they need to be dynamic are:
* class name string, which affects stringification and various explicit strcmps throughout the tree. I doubt we have strcmps for the remaining classes, and we could probably rejigger the stringification code if needed.
* flags. At the moment the only dynamic behavior of the flags depends on global vs non-global objects. We could make two static JSClasses for those cases.
* Hooks. We could probably just make all the custom hooks smart enough to detect whether they're actually needed and delegate to the non-custom hooks if they're not. None of them should be perf-sensitive anymore.

Not sure if that's more or less work than explicit handling for dynamic JSClasses, but SM hackers seem to keep making the assumption that JSClasses are static, so it might be better to just make that true.
(Assignee)

Comment 2

2 years ago
Bug 1265271 is the previous bug about removing all dynamic JSClasses.
See Also: → bug 1265271
Note that spidernode uses non-static JSClasses and would require quite some rejiggering to handle that.  Stringification code would be part of it, but there's more there (e.g. is-this-an-instance-of-that checking).
(Reporter)

Comment 4

2 years ago
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> * Hooks. We could probably just make all the custom hooks smart enough to
> detect whether they're actually needed and delegate to the non-custom hooks
> if they're not. None of them should be perf-sensitive anymore.

It's possible to get the XPCNativeScriptableShared for a JSObject*, right? In that case we could also replace the Class we currently store in XPCNativeScriptableShared with a Class-like thing that stores just the relevant hooks, and give the static Classes hooks that forward to these hooks?
(In reply to Jan de Mooij [:jandem] from comment #4)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> > * Hooks. We could probably just make all the custom hooks smart enough to
> > detect whether they're actually needed and delegate to the non-custom hooks
> > if they're not. None of them should be perf-sensitive anymore.
> 
> It's possible to get the XPCNativeScriptableShared for a JSObject*, right?
> In that case we could also replace the Class we currently store in
> XPCNativeScriptableShared with a Class-like thing that stores just the
> relevant hooks, and give the static Classes hooks that forward to these
> hooks?

Yeah, that should work too, though it's not obvious to me a priori which would be easier.
With respect to comment 0:

How dynamic are these? Would it be ok to tie their lifetimes to the runtime? If so, you could eliminate the ref counting.

Would it be ok to ask spidermonkey to allocate them, and fill in the contents afterward? If so, you can get rid of the DYNAMICALLY_ALLOCATED flag.
(Reporter)

Comment 7

2 years ago
(In reply to Steve Fink [:sfink] [:s:] from comment #6)
> How dynamic are these? Would it be ok to tie their lifetimes to the runtime?

We could hold strong references to XPCNativeScriptableShared (the thing that contains the Class) from XPCNativeScriptableSharedMap (the thing that maps name + flags to it). I think that would keep them alive as long as the XPCJSContext, but it would use more memory. I don't know enough about XPConnect to know if that's acceptable.

I think as a first step we should try bholley's suggestion to get rid of these dynamic Classes completely. If we can get away with that it does seem like it's the simplest option. We will also have to talk to the SpiderNode people though.
(Reporter)

Comment 8

2 years ago
For stringification, I wonder if we could give the static Classes a GetProperty hook that handles Symbol.toStringTag. Unfortunately toStringTag is not available on ESR45, but we could emulate it with a cx callback.
(In reply to Jan de Mooij [:jandem] from comment #7)
> (In reply to Steve Fink [:sfink] [:s:] from comment #6)
> > How dynamic are these? Would it be ok to tie their lifetimes to the runtime?
> 
> We could hold strong references to XPCNativeScriptableShared (the thing that
> contains the Class) from XPCNativeScriptableSharedMap (the thing that maps
> name + flags to it). I think that would keep them alive as long as the
> XPCJSContext, but it would use more memory. I don't know enough about
> XPConnect to know if that's acceptable.

This would probably be ok if we need to do it. There's a fixed and relatively small number of these things, so provided the hashtables work properly it should be ok to leak them. This could also be a decent more-backportable solution.

> I think as a first step we should try bholley's suggestion to get rid of
> these dynamic Classes completely. If we can get away with that it does seem
> like it's the simplest option. We will also have to talk to the SpiderNode
> people though.

Yeah, getting rid of the dynamism certainly the simplest.
(Assignee)

Comment 10

2 years ago
Disallowing dynamic classes is certainly the simplest option from a conceptual point of view, and the most likely to prevent subtle future problems. So even if it's not the simplest option in terms of the amount of work it seem like the best.
Ok. Trevor, how tractable would it be to move away from dynamic JSClasses on SpiderNode?
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> Ok. Trevor, how tractable would it be to move away from dynamic JSClasses on
> SpiderNode?

I believe all the usage of dynamic JSClass is in the ObjectTeblate code which bz knows better than me, so I'd defer to him.  However I can spend time on that code if necessary so I imagine it can be delt with.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3)
> Note that spidernode uses non-static JSClasses and would require quite some
> rejiggering to handle that.  Stringification code would be part of it, but
> there's more there (e.g. is-this-an-instance-of-that checking).

I guess I'm missing something here, but it seems to me like JS_GetClass() on a template object is the static ObjectTemplate JSClass.  The dynamic usage seems to be used for a object we shove into a reserved slot, its not immediately obvious to me why that object needs to be a JSClass, can it just be a random struct?
Flags: needinfo?(tbsaunde+mozbugs)
(Assignee)

Comment 13

2 years ago
Ignoring the SpiderNode situation (about which I know nothing): AFAICT there are fewer than 40 distinct dynamic JSClasses used in Firefox and they're all known in advance. Providing static instances of all of them seems like it should be possible. I'm happy to do this if it would move this bug forward.

jandem, any idea which dynamic JSClass instance is causing the crashes? I ask because it would make sense to convert the problematic ones first.
Flags: needinfo?(jdemooij)
(Reporter)

Comment 14

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #13)
> jandem, any idea which dynamic JSClass instance is causing the crashes? I
> ask because it would make sense to convert the problematic ones first.

No idea, I can't reproduce the crashes locally. I could add instrumentation and do some Try pushes, but I don't want to draw more attention to this than necessary.

> Providing static instances of all of them seems like it
> should be possible. I'm happy to do this if it would move this bug forward.

OK, so that's yet another option we have :) It seems either (a) holding strong references to these Classes (comment 7) or (b) adding static instances for all of them, are the simplest and safest options when it comes to backporting this. Backporting (b) to ESR45 might be annoying due to Class changes like bug 1261723 that landed since then, but it should be doable.

Would be great if you could work on this!
Flags: needinfo?(jdemooij)
(Assignee)

Comment 15

2 years ago
I am working on a patch that replaces the dynamic js::Class instances with static ones. Each nsIXPCScriptable has a static js::Class associated with it, which is accessible via a new nsIXPCScriptable.getJSClass() method. This assumes that there is an unchanging 1-to-1 association between nsIXPCScriptable instances and js::Class instances, which appears to be true. This change means that XPCNativeScriptableShared and XPCNativeScriptableSharedMap can both be removed. (In the long run, XPCNativeScriptableInfo and XPCNativeScriptableCreateInfo will also be removable, though I'll do that in a follow-up.)

It's working locally. On try runs Mac works well, but I have some bustage on other platforms that I need to debug.
Assignee: nobody → n.nethercote
(Assignee)

Comment 16

2 years ago
Created attachment 8818452 [details] [diff] [review]
Simplify js::Class handling relating to nsIXPCScriptable.  s

mccr8: please review (bholley suggested you for review).

bholley: please super-review.

tbsaunde: please let me know how this change will affect SpiderNode.

There are about 50 kinds of nsIXPCScriptable. Roughly half of these are defined
as distinct subclasses of nsIXPCScriptable via the macros in xpc_map_end.h
(e.g. BackstagePass, nsJSIID, AsyncStatementParams). The other half are defined
as variants of nsDOMClassInfo with distinguishing data in sClassInfoData[]
(e.g. DOMPrototype, CSSStyleRule, XULTreeBuilder).

Currently, every nsIXPCScriptable instance has a dynamic js::Class instance,
created on demand. There's lots of complexity around this: each
nsIXPCScriptable instance is embedded in an XPCNativeScriptableInfo, which has
a pointer to an XPCNativeScriptableShared, which holds flags and the dynamic
js::Class and may be shared by multiple XPCNativeScriptableInfo instances. The
XPCNativeScriptableShared objects are stored in the
XPCNativeScriptableSharedMap.

This patch removes all the dynamic js::Class instances, avoiding the potential
for dangling pointers. Each nsIXPCSCriptable instance now has a static
js::Class, trivially accessible via the new nsIXPCScriptable::GetJSClass()
method. For the nsIXPCScriptable subclasses defined via xpc_map_end.h, the
static js::Class is defined within the GetJSClass() method. For nsDOMClassInfo,
the static js::Class is defined within the nsDOMClassInfoData elements in
sClassInfoData[], via the NS_DEFINE_CLASSINFO_DATA_HELPER macro.

Thanks to nsIXPCScriptable::GetJSClass() there is no longer any need for
XPCNativeScriptableShared and XPCNativeScriptableSharedMap, and they have been
removed. (Likewise for XPCNativeScriptableInfo and
XPCNativeScriptableCreateInfo, though I have left them in for now. They can be
removed in follow-ups that won't require uplifting.)

Some things of note.

- The old code for dynamically creating js::Class instances was in
  XPCNativeScriptableShared's constructor. The equivalent code for statically
  creating these same instances is now in the XPC_MAKE_CLASS macro, which is
  defined in the new header file xpc_make_class.h, and gets used in a bunch of
  places outside XPConnect.

- Because of this a lot of functions and a couple of struct instances in
  XPCWrappedNativeJSOps.cpp had to be made visible outside that file. The
  added declarations for these are also in xpc_make_class.h. I also had to add
  XPCWrappedNative_Trace(), which is an exported wrapper function for
  XPCWrappedNative::Trace(). And I had to make WRAPPER_FLAGS public for the
  same reason.

- I plan to mark js::Class and related types as MOZ_NONHEAP_CLASS in a
  follow-up.

- On Linux64 this change increases the size of libxul.so by 43 KiB. This is
  more than I expected just from the extra static js::Class and js::ClassOp
  instances, but I think it's worthwhile.

- The commit message in the patch is deliberately vague because this is a
  security bug.
Attachment #8818452 - Flags: review?(continuation)
Attachment #8818452 - Flags: review?(bobbyholley)
Attachment #8818452 - Flags: feedback?(tbsaunde+mozbugs)
(Assignee)

Comment 17

2 years ago
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d57ba378c283d088417e82598a759013388cb13
(That try push includes a follow-up patch that enforces non-dynamic allocation of js::Class instances.)
Comment on attachment 8818452 [details] [diff] [review]
Simplify js::Class handling relating to nsIXPCScriptable.  s

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

Seems good conceptually, I'll let mccr8 do the careful review.

More cleanup is possible here, but that can be deferred, probably until we finish the webidl conversion and nix nsIXPCScriptable entirely.
Attachment #8818452 - Flags: review?(bobbyholley) → superreview+
Keywords: csectype-uaf
Comment on attachment 8818452 [details] [diff] [review]
Simplify js::Class handling relating to nsIXPCScriptable.  s

So, I don't see any changes to spider monkey itself here, so this should be completely fine.  As for marking the types as non heap, that shouldn't cause a build problem because SpiderNode isn't using the static analysis plugin at this point.  Of course we should stop doing it and, even now its probably buggy.  So I'll get that done before the next time I pull spidermonkey into spidernode.
Attachment #8818452 - Flags: feedback?(tbsaunde+mozbugs) → feedback+
> finish the webidl conversion and nix nsIXPCScriptable entirely.

Note that Components.interfaces uses nsIXPCScriptable afaict.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #20)
> > finish the webidl conversion and nix nsIXPCScriptable entirely.
> 
> Note that Components.interfaces uses nsIXPCScriptable afaict.

Yeah, I guess there's that and the storage stuff. So it might stick around for a while...
> The commit message in the patch is deliberately vague because this is a security bug.

I don't think it needs to be vague. You are really just implementing bug 1265271, right? And that bug is filed publicly. Just don't say why you want to make all the classes static, though I suppose it is all obvious enough once this is associated with a security bug.
Comment on attachment 8818452 [details] [diff] [review]
Simplify js::Class handling relating to nsIXPCScriptable.  s

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

It looks like XPCNativeScriptableFlags can probably also be deleted in a followup, for what it is worth.

::: js/xpconnect/idl/nsIXPCScriptable.idl
@@ +11,5 @@
>  #ifdef XP_WIN
>  #undef GetClassName
>  #endif
>  
> +#include "js/Class.h"

This is just a pointer, right? Can you forward declare js::Class instead of including it?

::: js/xpconnect/public/xpc_make_class.h
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/* vim: set ts=8 sts=4 et sw=4 tw=99: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Please add include guards for this file.

@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// This file should be used to create js::Class instances for nsIXPCScriptable
> +// instances. This includes any file that uses xpc_map_end.h.

Maybe mention this file in xpc_map_end.h somewhere? If you really have to just include it any time you include that one.

@@ +72,5 @@
> +extern const js::ClassExtension XPC_WN_JSClassExtension;
> +
> +extern const js::ObjectOps XPC_WN_ObjectOpsWithEnumerate;
> +
> +#define XPC_MAKE_CLASS_OPS(_flags) { \

This is pretty ugly, but I can't think of a way to make it better.

::: js/xpconnect/public/xpc_map_end.h
@@ +70,5 @@
> +// virtual
> +const js::Class*
> +XPC_MAP_CLASSNAME::GetJSClass()
> +{
> +    static js::ClassOps classOps = XPC_MAKE_CLASS_OPS(GetScriptableFlags());

These are static but not const. Doesn't that mean that they are writable, and thus won't be shared between processes? That seems bad. (Generally, if you can easily measure the impact your changes might have on how much code we share between processes that would be a nice sanity check.)

::: js/xpconnect/src/XPCJSContext.cpp
@@ -3243,5 @@
>     mIID2NativeInterfaceMap(IID2NativeInterfaceMap::newMap(XPC_NATIVE_INTERFACE_MAP_LENGTH)),
>     mClassInfo2NativeSetMap(ClassInfo2NativeSetMap::newMap(XPC_NATIVE_SET_MAP_LENGTH)),
>     mNativeSetMap(NativeSetMap::newMap(XPC_NATIVE_SET_MAP_LENGTH)),
>     mThisTranslatorMap(IID2ThisTranslatorMap::newMap(XPC_THIS_TRANSLATOR_MAP_LENGTH)),
> -   mNativeScriptableSharedMap(XPCNativeScriptableSharedMap::newMap(XPC_NATIVE_JSCLASS_MAP_LENGTH)),

Please remove the definition of XPC_NATIVE_JSCLASS_MAP_LENGTH from xpcprivate.h.

::: js/xpconnect/src/XPCMaps.cpp
@@ -474,5 @@
> -    // StatsCellCallback() in js/src/vm/MemoryMetrics.cpp.
> -    //
> -    // When the code below is removed (bug 1265271) and there are no longer any
> -    // heap-allocated js::Class instances, the disabled code in
> -    // StatsCellCallback() should be reinstated.

I don't know what this comment is referring to, but is this something that needs to be done, or have a bug filed about?

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +231,5 @@
>      if (siProto && siProto->GetCallback() == sciWrapper.GetCallback()) {
>          wrapper->mScriptableInfo = siProto;
> +        // XPCNativeScriptableInfo uses manual memory management. If we're
> +        // switching over to that of the proto, we need to destroy the one
> +        // we've allocated, and also null out the AutoMarkingPtr, so that it

You can delete the part about the AutoMarkingPtr, as |si| is a raw pointer since I changed some stuff to be refcounted.

@@ +969,5 @@
>      if (HasProto())
>          proto->SystemIsBeingShutDown();
>  
> +    // Back when we had dynamically allocated js::Class and js::ClassOp objects
> +    // we deliberately didn't destroy mScriptableInfo here because those

I'd just delete the stuff about how we used to do things. No need to document what we don't do any more.

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ -949,5 @@
>      XPC_WN_JSOp_Enumerate,
>      nullptr,  // funToString
>  };
>  
> -XPCNativeScriptableShared::XPCNativeScriptableShared(uint32_t aFlags,

It is nice to see all of this horribleness removed.

::: js/xpconnect/src/xpcprivate.h
@@ +1449,3 @@
>  // XPCNativeScriptableInfo is used to hold the nsIXPCScriptable state for a
>  // given class or instance.
> +// XXX: actually, at this point it's just a trivial wrapper for

Please get rid of the old out of date comment. So, make the whole comment something like "XPCNativeScriptableInfo is just a trivial wrapper for nsIXPCScriptable, and it should be removed eventually."

::: js/xpconnect/src/xpcpublic.h
@@ +161,5 @@
>  struct RuntimeStats;
>  
>  } // namespace JS
>  
> +#define WRAPPER_FLAGS (JSCLASS_HAS_PRIVATE | JSCLASS_FOREGROUND_FINALIZE)

This name is really generic, but I guess we don't use macros much any more so hopefully making it public won't cause any issues.
Attachment #8818452 - Flags: review?(continuation)
Attachment #8818452 - Flags: review+
Attachment #8818452 - Flags: feedback?(tbsaunde+mozbugs)
Attachment #8818452 - Flags: feedback+
Comment on attachment 8818452 [details] [diff] [review]
Simplify js::Class handling relating to nsIXPCScriptable.  s

Sorry, I didn't mean to overwrite that.
Attachment #8818452 - Flags: feedback?(tbsaunde+mozbugs)
(Assignee)

Comment 25

2 years ago
> Just don't say why you want to make all the classes static, though I suppose it is all obvious enough
> once this is associated with a security bug.

I was thinking about the sec-approval question "Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?"

The whole point of the patch is the dynamic-to-static conversion. Once you know about that, concluding that a UAF is happening is probably not that hard. But the patch has enough other changes that I think it's not obvious that there is dynamic-to-static conversion happening, so I deliberately didn't mention the conversion in the commit message.
Why not just land the patch under bug 1265271 instead of pointing out it's a security fix at all?
(Assignee)

Comment 27

2 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #26)
> Why not just land the patch under bug 1265271 instead of pointing out it's a
> security fix at all?

That's possible, but we may want to uplift the fix to earlier branches. Uplifting to Aurora and Beta requires only very minor changes to the patch. Uplifting to ESR is significantly harder; I just got a version compiling and starting the browser works, but there were some changes required that I'm quite uncertain about.
(Assignee)

Comment 28

2 years ago
Created attachment 8818788 [details] [diff] [review]
Simplify js::Class handling relating to nsIXPCScriptable.  s
(Assignee)

Updated

2 years ago
Attachment #8818452 - Attachment is obsolete: true
(Assignee)

Comment 29

2 years ago
Created attachment 8818789 [details] [diff] [review]
(aurora/beta) - Simplify js::Class handling relating to nsIXPCScriptable.  s

Here's the slightly modified version that applies to Aurora and Beta.
(Assignee)

Comment 30

2 years ago
Thank you for the fast and good review. I have addressed all the addressable comments.

> > -    // When the code below is removed (bug 1265271) and there are no longer any
> > -    // heap-allocated js::Class instances, the disabled code in
> > -    // StatsCellCallback() should be reinstated.
> 
> I don't know what this comment is referring to, but is this something that
> needs to be done, or have a bug filed about?

It's on my todo list :)
(Assignee)

Comment 31

2 years ago
Comment on attachment 8818788 [details] [diff] [review]
Simplify js::Class handling relating to nsIXPCScriptable.  s

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

I don't know. We've never reproduced the problem, we've only seen crashes from users that look like they fit this pattern. Comment 0 outlines one possible path to a crash. It *is* a UAF crash, so that's bad. But it also seems to depend on GC timing, which presumably would make exploiting it harder and/or non-deterministic.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a
> bulls-eye on the security problem?

No. The key change is that some objects that were dynamically allocated are now static. That's not obvious from the code of the patch itself because there are quite a few supporting changes. The commit message also doesn't mention it, nor do comments.

> Which older supported branches are affected by this flaw?

All of them.

> If not all supported branches, which bug introduced the flaw?

N/A

> Do you have backports for the affected branches? If not, how different, hard to create,
> and risky will they be?

Yes for Aurora and Beta. ESR45 is harder, though if we can backport bug 1251655's patches -- which remove a bunch of related but unused code -- then I think it will be doable.

> How likely is this patch to cause regressions; how much testing does it need?

The bad news is that it's a moderately large change to XPConnect :(  The good news is that it replaces some complex machinery with something simpler (with a ~100 LOC reduction).

This try push looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0cfcbe018634cd7a1ca4db29c84d11a09f0392f

It probably would be good to have it bake in Nightly for a few days before uplifting. We could land it initially using bug 1265271 as cover, or a fresh bug.
Attachment #8818788 - Flags: sec-approval?
(Assignee)

Comment 32

2 years ago
Comment on attachment 8818788 [details] [diff] [review]
Simplify js::Class handling relating to nsIXPCScriptable.  s

Carrying over mccr8's r+.
Attachment #8818788 - Flags: review+
(Assignee)

Comment 33

2 years ago
Comment on attachment 8818789 [details] [diff] [review]
(aurora/beta) - Simplify js::Class handling relating to nsIXPCScriptable.  s

Carrying over mccr8's r+.
(Assignee)

Updated

2 years ago
Attachment #8818789 - Flags: review+
> if we can backport bug 1251655's patches -- which remove a bunch of related but unused code

We don't actually have a guarantee that it's unused, except insofar as we dropped it and no one complained.  Extensions and the like could use that code, in theory.  That makes backporting it to ESR a bit chancy.  :(
(Assignee)

Comment 35

2 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #34)
> > if we can backport bug 1251655's patches -- which remove a bunch of related but unused code
> 
> We don't actually have a guarantee that it's unused, except insofar as we
> dropped it and no one complained.  Extensions and the like could use that
> code, in theory.  That makes backporting it to ESR a bit chancy.  :(

Good to know, thank you for the info. Still, it was removed in 47 and release is now 50, which is a fair amount of time for any problems to have surfaced...

I can certainly live without uplifting this to ESR -- it'll save me some work, for one -- but I don't have much sense of what's typically done for security bugs.
status-firefox50: --- → wontfix
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox52: --- → +
tracking-firefox53: --- → +
Depends on: 1251655
Comment on attachment 8818788 [details] [diff] [review]
Simplify js::Class handling relating to nsIXPCScriptable.  s

sec-approval=dveditz

If we're going to aim for uplift to Firefox 51 (Jan 24) we should get plenty of beta testing on this. I agree with the "bake on Nightly" bit as a sanity check, but try to get it up to beta before the new year so we have plenty of feedback and time to backout if necessary.
Attachment #8818788 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 37

2 years ago
Just to clarify: can I land the patch on Nightly now? If so, here's a possible landing plan:

- Nightly: land today.
- Aurora: uplift on Dec 23
- Beta: uplift on Dec 28

Sound reasonable?
Flags: needinfo?(dveditz)
sec-approval without any qualifiers means you can land it immediately.
Flags: needinfo?(dveditz)
https://hg.mozilla.org/mozilla-central/rev/98eaebf80768
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: javascript-core-security → core-security-release
Comment on attachment 8818789 [details] [diff] [review]
(aurora/beta) - Simplify js::Class handling relating to nsIXPCScriptable.  s

[Triage Comment]
a=dveditz for aurora.

Is this practical to backport to ESR 45?
Attachment #8818789 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 41

2 years ago
 
> Is this practical to backport to ESR 45?

As per comment 31:

> ESR45 is harder, though if we can backport bug 1251655's patches -- which remove a bunch of related but unused code -- then I think it will be doable.

Doable, but it is riskier. bz was a little uncomfortable about it in comment 34. I'd be happy to not have to do the work to backport to ESR 45 :)
(Assignee)

Comment 43

2 years ago
Comment on attachment 8818789 [details] [diff] [review]
(aurora/beta) - Simplify js::Class handling relating to nsIXPCScriptable.  s

Approval Request Comment
[Feature/Bug causing the regression]: none; longstanding bug.

[User impact if declined]: possible crashes and security exploits.

[Is this code covered by automated tests?]: No specific tests. But this code is exercised heavily during normal browser execution.

[Has the fix been verified in Nightly?]: Yes. It landed on mozilla-central 5 days ago and on mozilla-aurora 2 days ago, and there have been no sign of problems.

[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?]: Moderately.

[Why is the change risky/not risky?]: It's a non-trivial change to XPConnect, which is always somewhat risky. But it's a net simplification -- it replaces a mixture of static and dynamic memory allocation for one type with just static memory allocation. It also reduces the number of lines of code by ~100.

[String changes made/needed]: None.
Attachment #8818789 - Flags: approval-mozilla-beta?
Comment on attachment 8818789 [details] [diff] [review]
(aurora/beta) - Simplify js::Class handling relating to nsIXPCScriptable.  s

Looks a little scary but good to get rid of this type of crashes. 
This should make it into the beta 11 build on Monday.
Attachment #8818789 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 45

2 years ago
uplift
https://hg.mozilla.org/releases/mozilla-beta/rev/be751ea19d46

So can we officially call ESR45 wontfix here?
status-firefox51: affected → fixed
tracking-firefox51: --- → ?
(Assignee)

Comment 46

2 years ago
> So can we officially call ESR45 wontfix here?

Ok by me.
status-firefox-esr45: affected → wontfix
tracking-firefox51: ? → +
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
Blocks: 1302996
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.