Closed Bug 1325406 Opened 3 years ago Closed 3 years ago

FakeRooted doesn't work with ValueOperations

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr52 --- wontfix
firefox53 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

(Keywords: regression, sec-audit, Whiteboard: [adv-main53-][post-critsmash-triage])

Attachments

(1 file, 1 obsolete file)

We have this nice system where ValueOperations/MutableValueOperations and others define interface methods for use with wrapped pointer types like Rooted, Handle etc.  For example, when you call setUndefined() on a Rooted<Value> it extracts a reference to the wrapped value and calls the method on it.

This works through mixin base classes like RootedBase, HandleBase, etc which are specialised for types like Value to provide the interface operations.  The specialisations derive from classes like MutableValueOperations which actually implement the operations.

Currently both Rooted and FakeRooted derive from RootedBase but the specialisation of RootedBase for Value derives MutableValueOperations<Rooted<Value>>.  If you use this with a FakeRooted it will cast your FakeRooted to a Rooted and access it.  These don't have the same representation and the program will crash.

The same probably applies to other combinations of wrapped / wrapper classes.
Marking s-s just in case because there's some weirdness where BarrieredBaseMixins<Value> casts to WriteBarrieredBase even though it could be used with a ReadBarriered (and hence might skip the barrier).
Group: javascript-core-security
Blocks: 1205909
Attached patch bug1325406-rooted-base-classes (obsolete) — Splinter Review
What we need to do is to pass the type of the wrapper class as well as the type of the wrapped thing.

We can also get rid of one layer of indirection by making all the different FooOperation / MutableFooOperation classes into WrapperPtrOperations / MutableWrappedPtrOperations classes templated on wrapped pointer and wrapper class types.  For example, RootedBase now pulls in MutableWarppedPtrOperations by default.  This removes quite a lot of boilerplate.

Any ideas for a better name that WrapperPtrOperations (we already have proxy-related wrappers which are an entirely different concept)?

Also, It's possible that we could remove RootedBase and related classes completely and just pull in MutableWrappedPtrOperations but there are a couple of places where we do use this extra flexibility.

For the barrier issue, I removed the inheritance on BarrieredBase and put it in WriteBarrieredBase and ReadBarriered separately.  I has to put a 'using' delcaration so that use of 'value' referred to BarrieredBase::value and not WrappedPtrOperations<Value>::value.

InnerViewTable previously exposed a bunch of private methods via this mechanism that are only used from JSCompartment.  This seemed unnecessary so I got rid of this and used get() in the few places that needed it.
Assignee: nobody → jcoppeard
Attachment #8821222 - Flags: review?(sphink)
Keywords: sec-audit
Comment on attachment 8821222 [details] [diff] [review]
bug1325406-rooted-base-classes

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

This is terrifying, but I like having RootedBase automatically pull in operations. And that's some awful boilerplate you've managed to kill off.

::: js/public/RootingAPI.h
@@ +113,5 @@
>  struct BarrierMethods {
>  };
>  
> +template <typename Inner, typename Outer>
> +class WrappedPtrOperations {};

The only other thing that comes to mind is

template <typename Element, typename Container> // or Containee?
class PtrContainerOperations {}; // or ContainedPtrOperations?

I used the 'element' term for the 'using ElementType = T;' declarations for the operator== stuff. Probably ought to use the same terminology, whatever it is. Hm... come to think of it, could we now lift ElementType into this class?

::: js/public/SweepingAPI.h
@@ -11,5 @@
>  
> -namespace js {
> -template <typename T>
> -class WeakCacheBase {};
> -} // namespace js

I guess we weren't using that layer.

::: js/src/ds/TraceableFifo.h
@@ -114,5 @@
> -template <typename A, size_t B, typename C>
> -class HandleBase<TraceableFifo<A,B,C>>
> -  : public TraceableFifoOperations<JS::Handle<TraceableFifo<A,B,C>>, A,B,C>
> -{
> -    using TF = TraceableFifo<A,B,C>;

\o/ That's some ugly boilerplate to be killing off.

::: js/src/jsobj.h
@@ +590,5 @@
>      void operator=(const JSObject& other) = delete;
>  };
>  
> +template <typename Outer>
> +template <typename U>

Achievement unlocked! (Two consecutive template <>s.)
Attachment #8821222 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #3)

> template <typename Element, typename Container> // or Containee?
> class PtrContainerOperations {}; // or ContainedPtrOperations?

I've kind of got used to WrappedPtrOperations now.  Naming the template parameters Element and Container is a good idea, I'll do that.

> Hm... come to think of it, could we now lift ElementType into this class?

We'd need everyone who specialised the template to define this themselves though in that case (or pull it in from somewhere else via a base class).  It's probably easier to leave it as it is.
Updated patch with changes to browser templates and a tweak to InitialShapeEntry::needsSweep() to not trigger the read barrier on the tagged proto, just like it already did for the shape.  The later was causing an assert - we now actually trigger the read barrier in a few places where we didn't before.
Attachment #8821222 - Attachment is obsolete: true
Attachment #8824488 - Flags: review+
Comment on attachment 8824488 [details] [diff] [review]
bug1325406-rooted-base-classes

Requesting additional review for browser changes.

The patch is described in comment 2.  From the browser POV there shouldn't be any change at all.  I added the extra template parameter, removed some conversion operations that get supplied by other parts of the system already and for OwningNonNull and RefPtr changed the template that's being specialised from RootedBase to the more generic WrappedPtr so that these methods get pulled in to the other rooting classes as well.
Attachment #8824488 - Flags: review?(continuation)
Comment on attachment 8824488 [details] [diff] [review]
bug1325406-rooted-base-classes

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

I don't understand these changes, but they look benign enough from a browser perspective.
Attachment #8824488 - Flags: review?(continuation) → review+
Comment on attachment 8824488 [details] [diff] [review]
bug1325406-rooted-base-classes

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

Extremely difficult.

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

No.

Which older supported branches are affected by this flaw?

Everything back to 43.

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

Bug 1205054.

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

This will require some rewriting (I think some things have been renamed) but it should be straightforward.

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

Unlikely.
Attachment #8824488 - Flags: sec-approval?
Comment on attachment 8824488 [details] [diff] [review]
bug1325406-rooted-base-classes

As a sec-audit, this doesn't need sec-approval+ to land so I'm clearing the flag.

If we wanted to take it on branches, someone would have to make a case for doing so as the main bugs we port backwards are sec-high and sec-critical rated issues.
Attachment #8824488 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/2e1a0c4886c5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main53-]
Flags: qe-verify-
Whiteboard: [adv-main53-] → [adv-main53-][post-critsmash-triage]
Dan, do you think we should request ESR52 approval on this?
Flags: needinfo?(dveditz)
Doesn't seem like the kind of "definitely severe" vulnerability we take on ESR.
Flags: needinfo?(dveditz)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.