Bugzilla

Updated

•

7 years ago

Group: core-security → layout-core-security

Component: DOM → CSS Parsing and Computation

Keywords: csectype-uaf

Comment 2

•

7 years ago

Heycam, can you take a look?

Flags: needinfo?(cam)

Keywords: sec-critical

Comment 3

•

7 years ago

I'm taking a look...  NOTE: the testcase requires the domfuzz extension to be installed (which I built locally by running "make" in this directory: https://github.com/MozillaSecurity/domfuzz/tree/master/dom/extension ).

I can reproduce in ESR52 -- the latest esr52 release (no ASAN required).  It just crashes within a second or two, when I load the testcase.[1]

I can also trigger the reported ASAN issue in local opt builds of ESR52 (with & without ASAN).

HOWEVER:
I don't get a crash in stock Nightly, nor in mozilla-central rewound to an arbitrary Firefox 52-era cset, nor in an arbitrary Aurora 52 build that I tested:
https://archive.mozilla.org/pub/firefox/nightly/2016/11/2016-11-30-00-40-19-mozilla-aurora/firefox-52.0a2.en-US.linux-x86_64.tar.bz2

(I can't test Release 52 or Beta 52 because they don't accept unsigned extensions and there's no way to make them do so, AFAICT.)

So this is either something that briefly affected 52 and was subsequently fixed [and ESR52 needs the fix], or it might be a regression that just happened on the 52 branch [and maybe on beta/release but we can't tell because they won't accept the extension which is required to run the testcase].

Comment 4

•

7 years ago

I'm narrowing down the regression range via "hg bisect" in the mozilla-esr52 repo.  Right now I've got a known-good & known-bad endpoints, with a ~1200-cset fix range between them.  That should only require ~10 trials to bisect (which is reasonably fast with the icecc build cluster in MV).

Comment 5

•

7 years ago

hg bisect says this started crashing as of this cset:
  https://hg.mozilla.org/releases/mozilla-esr52/rev/01d67bfe6c81
...which is a "Update configs for release" change.

We can ignore the mozconfig changes there (which shouldn't matter for my local build because I'm using my own mozconfig).

Of the remaining files, I've found with local testing that I can "fix" a build from that ^ cset by reverting its changes to these files (probably not all are necessary):
 browser/confvars.sh
 browser/config/version.txt
 browser/config/version_display.txt
 config/milestone.txt

Bottom-line: I'll bet there's some feature/behavior here that is dependent on "is this a release build" which those ^^ files' changes toggle.  So the real regression was older, and it's possible it's still present on Nightly if I toggle the same thing.

Flags: needinfo?(cam) → needinfo?(dholbert)

Mats Palmgren (inactive)

Comment 6

•

7 years ago

Fwiw, I seem to recall setting xpinstall.signatures.required to false to install DOMFuzz.

Comment 7

•

7 years ago

Yup, thanks -- I had to do that here, & I'm testing in a profile that's got that pref manually set.

I'm definitely seeing terminal output ("DOMFuzzHelper created") in both crashy & non-crashy builds during my testing, too.  So the versioning-related regression isn't due to the extension's installability.

Comment 8

•

7 years ago

Attached patch (not a fix) Patch to make bug reproduce on trunk [like it does on ESR] (obsolete) — Details — Splinter Review

OK, the key file from comment 5 is config/milestone.txt.  For this bug to be reproducible, that file's version number must NOT have a suffix after the version number (so it needs to be e.g. "55.0", not "55.0a1").

For convenience, here's a patch to make that tweak, and to set two extension-related prefs to DomFuzz-friendly defaults.  With this patch, this bug is reproducible in trunk.

Flags: needinfo?(dholbert)

Updated

•

7 years ago

status-firefox53: --- → affected

status-firefox54: --- → affected

status-firefox55: --- → affected

status-firefox-esr52: --- → affected

Comment 9

•

7 years ago

Attached patch (not a fix) Patch to make bug reproduce on trunk [like it does on ESR]. (obsolete) — Details — Splinter Review

Here's a better reproducibility-enhancing patch -- it does one additional thing now: enables a pref to allow popups (since the testcase spawns some & needs you to allow them).

This just removes one manual step from reproducing.  So, current STR:
 1. Apply this patch to trunk and build (ideally with ASAN enabled)
 2. "./mach run" and install domFuzzLite3.xpi built from https://github.com/MozillaSecurity/domfuzz/tree/master/dom/extension and restart your build to complete the extension installation.
 3. "./mach run" and load nils's testcase (I'm using a locally-saved copy)

Occasionally it doens't trigger during the first attempt, but it generally reproduces if I close the popups & reload my main browser window at that point.

Attachment #8876011 - Attachment is obsolete: true

Comment 10

•

7 years ago

Attached file ASAN report + backtrace, for trunk — Details

Here's an ASAN report + backtrace from a trunk build based off of this cset from yesterday: https://hg.mozilla.org/mozilla-central/rev/f4262773c433

Comment 11

•

7 years ago

Attached file testcase 2 (reduced / made more human-readable) — Details

Updated

•

7 years ago

Attachment #8876242 - Attachment description: testcase 2 (reduced) → testcase 2 (reduced / made more human-readable)

Comment 12

•

7 years ago

see_comment_19

OK, here's roughly what's going on here (looking at testcase 2):
 1. In fun0, we create a marquee, and we also resize the window that it's in. (Both of these involve a bit of deferred action.)

 2. During the next refresh driver tick (I think), we flush some deferred marquee action and trigger a call to nsXBLBinding::ExecuteAttachedHandler for the marquee's XBL.

 3. This involves a call to getComputedStyle in the marquee "init()" method:
https://dxr.mozilla.org/mozilla-central/rev/e61060be36424240058f8bef4c5597f401bc8b7e/layout/style/xbl-marquee/xbl-marquee.xml#538-539

 4. Under the hood, that getComputedStyle invocation causes us to create a nsComputedDOMStyle and call UpdateCurrentStyleSources on it.

 5. nsComputedDOMStyle::UpdateCurrentStyleSources starts out by flushing layout, which flushes the resize operation, which triggers the resize handler ("fun1").
   Inside of fun1:
    (a) the testcase calls close() on the window that we're operating inside of, which deletes some stuff.
    (b) the testcase forces a cycle collection, which deletes our nsComputedDOMStyle. (!!! We are hosed at this point.)

 6. After the fun1 & the layout flush completes, we unwind the stack back up to nsComputedDOMStyle::UpdateCurrentStyleSources.

 7. ...and now the next member-var that we touch as we proceed through nsComputedDOMStyle::UpdateCurrentStyleSources will be a UAF, because our "this" object has been deleted.

Assignee

Comment 13

•

7 years ago

Hmm.  So this is the second bug recently in which we ran into the problem that a delayed resize causes script to run during flush.  That said, flush is known to run arbitrary script.  So that part shouldn't be a problem per se.

In the sequence of events above, the really problematic bit from my point of view is that we collect the nsComputedDOMStyle.  That should really not happen.  Why is it happening?

Specifically, we should have a strong ref to the nsComputedDOMStyle from the object which is the "this" value of the binding call.  Bindings depend on this all over the place.

A horrible thought: Is it possible that the problem is that in this case the "this" value in the JS CallArgs in CSS2PropertiesBinding::get_width is an Xray (from the XBL scope to the content scope) and it gets hueyfixed by all the collection that's happening and hence drops its ref to the underlying DOM JS reflector, which then gets collected and calls Release on the C++ thing?

This seems scarily plausible, and if that's what's going on it means that any call via Xrays that can trigger untrusted code execution on the other side (which is lots of them!) is vulnerable to variants of this problem....

Assignee

Comment 14

•

7 years ago

ccing some xpconnect/bindings people.

Component: CSS Parsing and Computation → DOM

Comment 15

•

7 years ago

Yeah, that sounds right to me.

Comment 16

•

7 years ago

Could UnwrapObject take a mutable handle for obj? Then the rooted value would be the actual reflector for |self|. Though presumably that would mess up the actual |getter| call...

Assignee

Comment 17

•

7 years ago

OK, dholbert poked at this directly in rr, and in fact in CSS2PropertiesBinding::get_width "obj" is a dead object proxy in this case.  So comment 16 explains exactly what's going on here.

We need to fix this in the bindings.  I'll think a bit about how best to do that, but I suspect the right answer is to stick the underlying object in a Rooted if we have a wrapper.  We'd need to do this for "this", for arguments, and wherever else we don't do the "is a member" strong-ref bits in binding code.

Flags: needinfo?(bzbarsky)

Assignee

Comment 18

•

7 years ago

> Could UnwrapObject take a mutable handle for obj?

It could take one in addition to the arg, for example.  But that's the general idea, yes.

I do expect an extra Rooted on the stack is cheaper than addref/release on DOM objects, so it's worth sticking with the current setup where we rely on the JS object for ownership.  And of course for owned objects this is really the only way to make it work.

Flags: needinfo?(bzbarsky)

Assignee

Updated

•

7 years ago

Flags: needinfo?(bzbarsky)

Comment 19

•

7 years ago

(In reply to Daniel Holbert [:dholbert] from comment #12)
>  3. This involves a call to getComputedStyle in the marquee "init()" method: [...]
> 
>  4. Under the hood, that getComputedStyle invocation causes us to create a
> nsComputedDOMStyle and call UpdateCurrentStyleSources on it.

Off-topic slightly at this point, but let me correct myself ^^ slightly here:
The marquee JS code actually calls "getComputedStyle().width", and both parts (getComputedStyle and .width) are important for the bug here.
 - The getComputedStyle() creates the nsComputedDOMStyle
 - And then the .width is what triggers the problematic call into "UpdateCurrentStyleSources".

This is why, at the point where we crash, mozilla::dom::CSS2PropertiesBinding::get_width is in the backtrace, as bz mentions.

Comment 20

•

7 years ago

I looked over all of the calls to CheckedUnwrap for places where the output isn't rooted and then we pull something out of the output. I filed bug 1371865, bug 1371863, and bug 1371853 for some possible variations on this issue.

Comment 21

•

7 years ago

I didn't look at any of the calls in js/src, because I assume they are less likely to pull C++ stuff out and call methods on them. I did skim over the many call sites in generated DOM bindings code, but it all looked okay.

Comment hidden (obsolete)

For the record, I did some minimal digging to see if I could figure out why stock trunk builds are unaffected -- i.e. why bug only reproduces if you have a "clean" version number (e.g. "55.0") in milestone.txt.

Referencing my overview of what goes wrong in comment 12, it seems like stock trunk builds take a different path in step 5:
> 5. nsComputedDOMStyle::UpdateCurrentStyleSources starts out by flushing layout,
> which flushes the resize operation, which triggers the resize handler ("fun1").

For a stock trunk build (with milestone.txt=55.0a1), we don't trigger the resize handler at this point. Instead, the resize handler is triggered in a subsequent refresh driver tick.  So, this means the close() + CC are delayed -- they don't happen mid-marquee-JS-running.

I'm tentatively guessing that we have some delayed-event-dispatching code that is guarded by a check for NIGHTLY_BUILD and/or RELEASE_OR_BETA. Or something like that.

Comment 23

•

7 years ago

Attached file dholbert's build of domFuzzLite3.xpi (extension required for testcases to reproduce the crash) — Details

For reference/convenience, here's the domFuzzLite3.xpi extension that I'm using for testing here (which I built locally from the domfuzz github repo as noted previously).

Liz Henry (:lizzard) (relman/hg->git project)

Updated

•

7 years ago

Attachment #8876333 - Attachment description: dholbert's build of domFuzzLite3.xpi (extension required by testcases) → dholbert's build of domFuzzLite3.xpi (extension required for testcases to reproduce the crash)

Comment hidden (obsolete)

Hmm, I'm not aware of us changing resize event dispatching, and code seems to agreehttp://searchfox.org/mozilla-central/rev/12afa8cc9cea4a141494f767541b411845eef7a0/layout/base/PresShell.cpp#4131

Landing bug 1149555 never succeeded.

Updated

•

7 years ago

status-firefox53: affected → wontfix

status-firefox56: --- → affected

tracking-firefox54: --- → +

tracking-firefox55: --- → +

tracking-firefox56: --- → +

Comment 25

•

7 years ago

last-one-should-be-true

(In reply to Daniel Holbert [:dholbert] from comment #22)
> For the record, I did some minimal digging to see if I could figure out why
> stock trunk builds are unaffected -- i.e. why bug only reproduces if you
> have a "clean" version number (e.g. "55.0") in milestone.txt

Poked around a bit more, and it's simpler than I thought. The key difference between crashy "milestone.txt-tweaked" trunk builds vs non-crashy trunk builds is: e10s.

In release builds (or builds with milestone.txt adjusted to make them look like a release), I think the domFuzz extension makes us disable e10s by default.  Whereas, in a stock nightly build, we keep e10s enabled regardless of the extension's presence. And unsusprisingly, window-resize events get queued up & dispatched a bit differently depending on e10s.

SO: bottom line, to trigger the issue with this particular testcase, e10s *must be disabled*.

I verified that I can reproduce the ASAN error report in a stock nightly build (no tweaks needed to milestone.txt), with the domfuzz extension installed & with these prefs set:
  browser.tabs.remote.autostart.2      false    # Disable e10s
  dom.disable_open_during_load         false    # Allow popups (to make testcase more automated)
  xpinstall.signatures.required        false    # Allow domfuzz extension (which is unsigned)
  extensions.allow-non-mpc-extensions  false    # Allow domfuzz extension (which is not a webextension)

Comment 26

•

7 years ago

Attached patch (not a fix) Patch to make bug reproduce on trunk, by setting some prefs [you also need domFuzzLite3.xpi] — Details — Splinter Review

Here's an updated patch to make this bug reproduce easily on trunk, just by setting some about:config pref defaults as noted in prev. comment -- no longer any milestone.txt tweak needed here to trigger this.

I confirmed that I could reproduce this bug with "testcase 2" on the second attempt [it's not 100% reliable], with this patch applied, in an ASAN debug build. [Though likely any build will do & may just crash slightly differently.]

Updated

•

7 years ago

Attachment #8876221 - Attachment is obsolete: true

Updated

•

7 years ago

Attachment #8876851 - Attachment description: (not a fix) Patch to make bug reproduce on trunk, by setting some prefs → (not a fix) Patch to make bug reproduce on trunk, by setting some prefs [you also need domFuzzLite3.xpi]

Assignee

Comment 27

•

7 years ago

Attached patch Work in progress (obsolete) — Details — Splinter Review

This makes unwrapping much safer.  It fixes or audits all the UNWRAP_OBJECT consumers and some UnwrapObject<> ones.  What's left is the codegen bits, especially the non-owning union conversions.  I'm really not sure how to handle those yet...

I do expect we'll want to land these changes together with the fixes for the various bugs mccr8 filed as a result of his followup audit (see comment 20).

Assignee

Updated

•

7 years ago

Assignee: nobody → bzbarsky

Status: NEW → ASSIGNED

Assignee

Comment 28

•

7 years ago

Attached patch WIP with some updates; still missing the non-owning union bits (obsolete) — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8876987 - Attachment is obsolete: true

Comment 29

•

7 years ago

(In reply to Daniel Holbert [:dholbert] from comment #25)
> I verified that I can reproduce the ASAN error report in a stock nightly
> build (no tweaks needed to milestone.txt), with the domfuzz extension
> installed & with these prefs set:
>   browser.tabs.remote.autostart.2      false    # Disable e10s
>   dom.disable_open_during_load         false    # Allow popups (to make testcase more automated)
>   xpinstall.signatures.required        false    # Allow domfuzz extension (which is unsigned)
>   extensions.allow-non-mpc-extensions  false    # Allow domfuzz extension (which is not a webextension)

Sorry, I mistyped here -- that final pref ("extensions.allow-non-mpc-extensions") needs to be manually set to *true* (not false) in order for the domfuzz extension to be activated.

My "patch to make bug reproduce" (attachment 8876851 [details] [diff] [review]) sets it correctly -- I just mistyped in comment 25.

Assignee

Comment 30

•

7 years ago

Attached patch Compiling patch that should fix this bug (obsolete) — Details — Splinter Review

I have been trying to reproduce the crash and failing so far, but I believe this should fix it.  If someone who can reproduce could test, that would be awesome.

I'm still working on the related issues mccr8 filed, because I think we should land them all at once.

Attachment #8876988 - Attachment is obsolete: true

Comment 31

•

7 years ago

Comment on attachment 8877298 [details] [diff] [review]
Compiling patch that should fix this bug

My initial testing seems to confirm that bz's patch fixes the bug.

I've got a debug not-ASAN build which crashed from this bug ~40% of the time (2/5 attempts today), and when I applied bz's patch, I couldn't reproduce the bug on 8 attempts.  I also broke in with gdb and confirmed that we were traversing the codepath that was previously crashy (marquee construtor calling get_width, which flushes layout, which fires the resize event, which triggers cycle collection -- all in one synchronous nested callstack), and nonetheless we don't crash.

So I think I can confirm that this patch does indeed fix this bug on trunk.  I'm rebuilding with ASAN now, for extra verification that there's no latent ASAN-only funny business.

Attachment #8877298 - Flags: feedback+

Comment 32

•

7 years ago

(My ASAN build results seem to confirm that bz's patch fixes this, too.  Without his patch, I crashed on my 3rd or 4th attempt. With his patch, I was able to let the testcase run ~15 times without crashing.)

Updated

•

7 years ago

status-firefox54: affected → wontfix

tracking-firefox54: + → ---

tracking-firefox-esr52: --- → 55+

Assignee

Comment 33

•

7 years ago

OK, I posted patches in bug 1371865 and bug 1371853, but I feel pretty strongly that we should fix these all at once, so maybe I should dup them here and just land those patches with this bug#...

I _definitely_ want to do that with bug 1371863 because the patches for that are pretty tied in to the bindings patches we need for this bug.

Assignee

Updated

•

7 years ago

Blocks: 1371863

Comment 34

•

7 years ago

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #33)
> OK, I posted patches in bug 1371865 and bug 1371853, but I feel pretty
> strongly that we should fix these all at once, so maybe I should dup them
> here and just land those patches with this bug#...

Yeah, we should definitely fix them all at once. Duping them over makes sense: this patch is already pretty big, so making it a little larger won't hurt.

Assignee

Comment 35

•

7 years ago

Fwiw, this patch is actually multiple changesets in my tree; I'll be posting it in that form for review and then rollups for branch backports...  So yeah, I'll move over the other bits here.

Assignee

Updated

•

7 years ago

Updated

•

7 years ago

Blocks: 1371865

Assignee

Updated

•

7 years ago

Blocks: 1371853

Assignee

Comment 37

•

7 years ago

Attached patch part 1. Pass maybe-mutable Value handles, not JSObject*, into CastableObjectUnwrapper (obsolete) — Details — Splinter Review

The idea is that CastableObjectUnwrapper will want to have a MutableHandle for
the thing it's unwrapping whenever its target is a raw pointer.  Since we have
convenient MutableHandle<Value> in most cases, it's easier to switch
CastableObjectUnwrapper to working with MutableHandle<Value> or Handle<Value>
instead of trying to get MutableHandle<JSObject*> in the right places.

There are basically two changes here:

1) Make CastableObjectUnwrapper work with at thing that looks like a Value.
2) Change various callsites to pass in MutableHandle<Value>, in addition to a
   Handle<Value>, into the JS-to-C++ conversion templates.  The
   MutableHandle<Value> is passed as ${maybeMutableVal}.  It may not actually
   end up being a MutableHandle in some cases.

The reason for passing both a Handle and a MutableHandle is that when the thing
we actually have is a Rooted named "foo" the Handle will be "foo" but the
MutableHandle is most naturally written as "&foo".  This is not a problem if
you're just passing it through, but if you want to test whether it's an object,
say, you have a problem.  Writing "foo.isObject()" is ok, but "&foo.isObject()"
is not, and neither is "(&foo).isObject()".  This could be worked around by
passing the MutableHandle as "JS::MutableHandle<JS::Value>(&foo)" or something,
and then "JS::MutableHandle<JS::Value>(&foo).isObject()" does work.  But it
makes the code very hard to read.

So we just pass both things along; ${val} should be used for readonly access and
${maybeMutableVal} any time you really want a MutableHandle.

Attachment #8877893 - Flags: review?(peterv)

Assignee

Comment 38

•

7 years ago

Attached patch part 2. Change union conversions for non-owning unions to pass a MutableHandle through to the underlying conversion code — Details — Splinter Review

Attachment #8877894 - Flags: review?(peterv)

Assignee

Comment 39

•

7 years ago

Attached patch part 3. Change UnwrapObject<> and the UNWRAP_OBJECT macro to allow passing in mutable object or value handles for the thing being unwrapped, and do so at various callsites (obsolete) — Details — Splinter Review

I did audit all UNWRAP_OBJECT callers to make sure that the lifetimes of all the
temporary Rooted or the RefPtrs they unwrap into are long enough.

Attachment #8877895 - Flags: review?(peterv)

Assignee

Comment 40

•

7 years ago

Attached patch part 4. Stop using UnwrapArg to unwrap this values — Details — Splinter Review

Attachment #8877896 - Flags: review?(peterv)

Assignee

Comment 41

•

7 years ago

Attached patch part 5. Use a safer implementation of IsFileList — Details — Splinter Review

Attachment #8877897 - Flags: review?(peterv)

Assignee

Comment 42

•

7 years ago

Attached patch part 6. Root the unwrapped object in PRE_HELPER_STUB — Details — Splinter Review

This has review from mccr8 in bug 1371865

Assignee

Comment 43

•

7 years ago

Attached patch part 7. Root the unwrapped object in XPCConvert code — Details — Splinter Review

This has review from mccr8 in bug 1371853

Assignee

Comment 44

•

7 years ago

Attached patch part 8. Get rid of nsIXPConnect::GetNativeOfWrapper — Details — Splinter Review

Most of these changes are just replacements of GetNativeOfWrapper with
UnwrapReflectorToISupports, which is all it did under the hood.

The other changes are as follows:

* In nsDOMClassInfo, we really care whether we have a window, so we can just
  UNWRAP_OBJECT to the Window interface, since Window is always on Web IDL
  bindings now.  Also, the weird compartment check hasn't been needed ever since
  GetNativeOfWrapper stopped returning things off the passed-in object's
  prototype chain (Firefox 22, bug 658909).
* The only use of do_QueryWrapper was to get a Window in nsDocument; again we
  can UNWRAP_OBJECT.
* In XPCJSRuntime, we again just want to check for a Window, so UNWRAP_OBJECT.

Attachment #8877900 - Flags: review?(peterv)

Assignee

Comment 45

•

7 years ago

Attached patch part 9. Make UnwrapReflectorToISupports return already_AddRefed<nsISupports> (obsolete) — Details — Splinter Review

The main reason to not do this would be performance (avoiding the
addref/release), but there are two main mitigating factors:

1)  All calls to UnwrapReflectorToISupports that pass in a Web IDL object
    already do the addref (and in fact QI).  So this only affects the
    XPCWrappedNative case.

2)  The vast majority of the callers proceed to QI on the pointer anyway, and a
    second addref is cheap; it's the first addref after a CC that can be
    expensive on a cycle-collected object.

Going through the changes one by one:

* In GlobalObject::GetAsSupports, we do have a change that slightly slows down
  precisely in the XPCWrappedNative global case.  That's the message managers
  and the backstagepass.  And this really only affects calls to Web IDL statics
  from those globals.

* In UnwrapArgImpl we're talking about a Web IDL method taking an "external
  interface" type, and the UnwrapReflectorToISupports call is immediately
  followed by QI anyway.

* In UnwrapXPConnectImpl we're talking about the case when we have a
  non-WebIDL-object implementation of a Web IDL interface.  Again, this is the
  message manager globals, for EventTarget.  And we have a QI call immediately
  after the UnwrapReflectorToISupports.

* In the generated HasInstance hook for EventTarget we will be slightly slower
  when the LHS of the instanceof is an XPCWrappedNative.  And not much slower,
  because again there's an immediate QI.

* In InstallXBLField we're never going to have an XPCWrappedNative as thisObj;
  it's always an Element in practice.  So this is no more expensive than before.

* In sandbox's GetPrincipalOrSOP we now have an extra addref.  But it was
  followed by various QIs anyway.

* In XPCConvert::JSValToXPCException we have an extra addref if someone throws
  an XPCWrappedNative, which is fairly unlikely; our actual Exception objects
  are on Web IDL bindings.  Plus we have an immediate QI.

* In xpc::HasInstance we have an extra addred if the LHS of instanceof is an
  XPCWrappedNative.  But, again, there's an immediated QI after the
  UnwrapReflectorToISupports.

* In xpcJSWeakReference::Init we are likely doing an extra addref, but again
  immediately followed by QI.

I think it's worth making this change just to remove the footgun and that the
perf impact, if any, is pretty minimal.

Attachment #8877901 - Flags: review?(peterv)

Assignee

Comment 46

•

7 years ago

Attached patch Rollup patch for convenience — Details — Splinter Review

Flags: needinfo?(bzbarsky)

Assignee

Comment 47

•

7 years ago

Attached patch Part 3 diff -w, for possible review convenience — Details — Splinter Review

Assignee

Comment 48

•

7 years ago

Attached patch Rollup patch for beta (obsolete) — Details — Splinter Review

Assignee

Comment 49

•

7 years ago

Attached patch Rollup patch for 54 release (obsolete) — Details — Splinter Review

Assignee

Comment 50

•

7 years ago

Attached patch Rollup patch for ESR52 (obsolete) — Details — Splinter Review

Assignee

Comment 51

•

7 years ago

Comment on attachment 8877895 [details] [diff] [review]
part 3.  Change UnwrapObject<> and the UNWRAP_OBJECT macro to allow passing in mutable object or value handles for the thing being unwrapped, and do so at various callsites

Requesting sec-approval here, because this is the heart of the fix, but it should really apply to all of the patches.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Hard to say.  It's pretty clear from the patch that the problem is a rooting/ownership issue in bindings.  The new invariants described in the comments give an idea of what is wrong with the current code (violating those invariants by not providing a mutable handle _and_ not providing a strong ref).  So let's posit that based on the patch an attacker could figure out that they need to invoke a DOM API and then cause it to reenter script and cause things to be collected.  Also let's posit that an attacker knows enough about our code to figure out that wrappers are involved and hence that the DOM API call needs to be a cross-compartment one.  That leaves the problems of (1) figuring out that the call needs to be across Xrays, (2) figuring out that the hueyfix needs to be triggered, (3) finding a way to trigger an API call via Xrays that then reenters the untrusted script, (4) actually managing to trigger the hueyfix, (5) exploiting the resulting dead object.  I expect #5 is easy for someone who does this for a living, #4 is not too hard if you just do a bunch of allocation and event loop spinning.  #3 could take some work.  #1 and #2 also need some inspiration, but hard to say how much.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  To some extent.  Per above, it's clear from the comments that the problem is with GC/rooting stuff, and I _think_ it's fairly clear that the issue is with wrappers.  I carefully avoided mentioning Xrays or hueyfix anywhere in the comments, so that part is not pointed out.  And of course I didn't mention the specific API involved here (i.e. the answer to #3 above).

That said, the commit messages and comments in these patches could probably use a once-over from someone other than me to see whether we could remove something from them without ending up with security-critical code that's not documented as critical and could get changed by people who don't realize it is.

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

If not all supported branches, which bug introduced the flaw?  I guess bug 695480 landed before webidl bindings, so let's call it webidl bindings.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Backports to beta 55, release 54, ESR 52 are attached.  Risk is... hard to say.  I ended up changing a bunch of code to resolve this whole class of issues in various guises.  We _could_ probably land just parts 1-3 or 1-4 on the older branches to mitigate risk, but I think the later parts are actually not very risky...

How likely is this patch to cause regressions; how much testing does it need?  It's hard to say.  I'm touching some pretty critical code here.  I have compiled it on Mac and Linux, but I haven't even done a try push for Windows and tests because I'm not sure about exposing these patches on try too early...  Ideally I would give this as much bake time as I could.  I _think_ this is fairly safe as fixes for this bug go, but that's not a very high bar.

Attachment #8877895 - Flags: sec-approval?

Assignee

Comment 52

•

7 years ago

I'm not going to worry about branch approvals until the reviews and sec-approval are done, because that may affect the branch patches.

Updated

•

7 years ago

Group: layout-core-security → dom-core-security

Comment 53

•

7 years ago

Comment on attachment 8877895 [details] [diff] [review]
part 3.  Change UnwrapObject<> and the UNWRAP_OBJECT macro to allow passing in mutable object or value handles for the thing being unwrapped, and do so at various callsites

I'm going to say we should check this in the week of July 10, which is four weeks into our eight week (this time) product cycle, just to reduce the potential exposure window. I'm willing to have a discussion if this seems overly paranoid though.

Attachment #8877895 - Flags: sec-approval? → sec-approval+

Updated

•

7 years ago

Whiteboard: [checkin on 7/10]

Assignee

Comment 54

•

7 years ago

That doesn't seem unreasonable.  I just wish I had a way to check whether this actually compiles on all platforms and passes tests before then...

Cameron McCormack (:heycam)

Comment 55

•

7 years ago

I wish we had a truly private way to run tests and log results for viewing by the submitting dev.

Comment 56

•

7 years ago

That would be nice.  I filed bug 1373889.

Comment 57

•

7 years ago

Comment on attachment 8877893 [details] [diff] [review]
part 1.  Pass maybe-mutable Value handles, not JSObject*, into CastableObjectUnwrapper

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

::: dom/bindings/Codegen.py
@@ +4255,5 @@
>      """
> +    A class for unwrapping an object stored in a JS Value (or
> +    MutableHandle<Value> or Handle<Value>) named by the "source"
> +    argument based on the passed-in descriptor and storing it in a
> +    variable called by the name in the "target" argument.

Could you add some explanation for mutableSource here?

Attachment #8877893 - Flags: review?(peterv) → review+

Updated

•

7 years ago

Attachment #8877894 - Flags: review?(peterv) → review+

Comment 58

•

7 years ago

Comment on attachment 8877895 [details] [diff] [review]
part 3.  Change UnwrapObject<> and the UNWRAP_OBJECT macro to allow passing in mutable object or value handles for the thing being unwrapped, and do so at various callsites

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

::: dom/base/StructuredCloneHolder.cpp
@@ +450,5 @@
>                                                       JSStructuredCloneWriter* aWriter,
>                                                       JS::Handle<JSObject*> aObj)
>  {
> +  // We can just keep passing the same Rooted to all the callees, because all of
> +  // our callees want to CheckedUnwrap it as needed.

I don't completely understand these comments (here and below). Is what is happening that the first call to UNWRAP_OBJECT will put the result of CheckedUnwrap into obj? And then subsequent calls to UNWRAP_OBJECT will just use the unwrapped object?
I thought about doing the CheckedUnwrap call here instead. Having unwrap change the passed in JSObject is a bit spooky, but then again I really prefer to keep CheckedUnwrap calls in binding code if possible :-/.

::: dom/base/nsDOMWindowUtils.cpp
@@ +3135,5 @@
>      *_retval = -1;
>      return NS_OK;
>    }
>  
> +  JS::Rooted<JSObject*> obj(aCx, aFile.toObjectOrNull());

Maybe add a comment here too? And wherever we pass it in multiple times?

::: dom/bindings/BindingUtils.h
@@ +254,5 @@
> +    return NS_ERROR_XPC_SECURITY_MANAGER_VETO;
> +  }
> +  MOZ_ASSERT(!js::IsWrapper(unwrappedObj));
> +  domClass = GetDOMClass(unwrappedObj);
> +  if (!domClass) {

Hmm, the duplication of this code is unfortunate. I don't suppose we could call UnwrapObjectInternal on unwrappedObj here?

@@ +278,5 @@
>    return NS_ERROR_XPC_BAD_CONVERT_JS;
>  }
>  
> +struct MutableObjectHandleWrapper {
> +  MutableObjectHandleWrapper(JS::MutableHandle<JSObject*> aHandle)

Make this explicit?

@@ +299,5 @@
> +  JS::MutableHandle<JSObject*> mHandle;
> +};
> +
> +struct MutableValueHandleWrapper {
> +  MutableValueHandleWrapper(JS::MutableHandle<JS::Value> aHandle)

explicit?

::: js/xpconnect/src/XPCComponents.cpp
@@ +1717,2 @@
>          Exception* e;
> +        *bp = NS_SUCCEEDED(UNWRAP_OBJECT(Exception, &obj, e)) ||

IS_INSTANCE_OF?

Attachment #8877895 - Flags: review?(peterv) → review+

Assignee

Comment 59

•

7 years ago

> Is what is happening that the first call to UNWRAP_OBJECT will put the result of CheckedUnwrap into obj?

Oh, good catch.  That _was_ what was happening when I wrote that comment, before I realized that it would cause problems with IDL overloads (e.g. the case when we had an interface overloaded with a DOMString: the interface unwrap attempt would CheckedUnwrap and then the string conversion would no longer get Xrays) and changed UNWRAP_OBJECT to only modify the mutable handle when the unwrap succeeds.  I'll just take this comment out entirely, because at this point it's just confusing.

> Having unwrap change the passed in JSObject is a bit spooky

I agree, but I don't really have any concrete better ideas.... if you do, I'm totally open to changing how this stuff works!

> Hmm, the duplication of this code is unfortunate

I agree.  I couldn't figure out a way to keep from duplicating it while preserving that "don't write to the inout param unless unwrap succeeds" property, but that's because I wasn't thinking sufficiently well, clearly!

> I don't suppose we could call UnwrapObjectInternal on unwrappedObj here?

Hmm... with mayBeWrapper forced to false, now that we have such a thing?  That's a good point.  I think we can in fact do that.  I think I didn't have the mayBeWrapper bit yet when I initially refactored this method to avoid the inout param writes; I added it later when I realized I neded UNWRAP_NON_WRAPPER_OBJECT.

> Make this explicit?

Good catch, both places.

> IS_INSTANCE_OF?

Also good catch; I think I changed that bit before I decided to introduce IS_INSTANCE_OF...  I can actually undo most of the changes to this method if I use IS_INSTANCE_OF here.

Updated

•

7 years ago

Attachment #8877897 - Flags: review?(peterv) → review+

Updated

•

7 years ago

Attachment #8877896 - Flags: review?(peterv) → review+

Updated

•

7 years ago

Attachment #8877900 - Flags: review?(peterv) → review+

Comment 60

•

7 years ago

Comment on attachment 8877901 [details] [diff] [review]
part 9.  Make UnwrapReflectorToISupports return already_AddRefed<nsISupports>

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

I'm working on the message managers again, so hopefully some of this will be simpler soon.

::: js/xpconnect/src/nsXPConnect.cpp
@@ -783,5 @@
>      }
>  
>      // Try DOM objects.
>      nsCOMPtr<nsISupports> canonical =
>          do_QueryInterface(mozilla::dom::UnwrapDOMObjectToISupports(reflector));

This is safe because we never end up calling into JS for a QI to nsISupports. Right?

Attachment #8877901 - Flags: review?(peterv) → review+

Assignee

Comment 61

•

7 years ago

> This is safe because we never end up calling into JS for a QI to nsISupports. Right?

Right, plus if the pointer is non-null we know we have an actual webidl object, not XPCWrappedJS.  Those should never call into JS for QI at all, except for XBL-implemented interfaces on elements.

I'll add a comment.

Assignee

Comment 62

•

7 years ago

Attached patch Addresses review comments on part 3; could use another look — Details — Splinter Review

Attachment #8880535 - Flags: review?(peterv)

Assignee

Comment 63

•

7 years ago

Attached patch Part 1 updated to review comments — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8877893 - Attachment is obsolete: true

Assignee

Comment 64

•

7 years ago

Attached patch Part 3 updated to review comments (obsolete) — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8877895 - Attachment is obsolete: true

Assignee

Comment 65

•

7 years ago

Attached patch Part 9 updated to review comments — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8877901 - Attachment is obsolete: true

Assignee

Comment 66

•

7 years ago

Attached patch Updated esr52 rollup fix (obsolete) — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8877906 - Attachment is obsolete: true

Assignee

Comment 67

•

7 years ago

Attached patch Updated release channel rollup fix (obsolete) — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8877905 - Attachment is obsolete: true

Assignee

Comment 68

•

7 years ago

Attached patch Updated beta channel rollup fix (obsolete) — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8877904 - Attachment is obsolete: true

Assignee

Updated

•

7 years ago

Attachment #8877298 - Attachment is obsolete: true

Comment 69

•

7 years ago

Comment on attachment 8880535 [details] [diff] [review]
Addresses review comments on part 3; could use another look

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

::: dom/bindings/BindingUtils.h
@@ +255,5 @@
>    }
>    MOZ_ASSERT(!js::IsWrapper(unwrappedObj));
> +  // Recursive call is OK, because now we're using false for mayBeWrapper and
> +  // we never reach this code if that boolean is false, so can't keep calling
> +  // ourselves.

Could we static_assert that?

@@ -268,5 @@
> -    // It's very important to not update "obj" with the "unwrappedObj" value
> -    // until we know the unwrap has succeeded.  Otherwise, in a situation in
> -    // which we have an overload of object and primitive we could end up
> -    // converting to the primitive from the unwrappedObj, whereas we want to do
> -    // it from the original object.

This comment still has value, no?

Attachment #8880535 - Flags: review?(peterv) → review+

Updated

•

7 years ago

Flags: sec-bounty?

Updated

•

7 years ago

Blocks: 1371630

Assignee

Comment 70

•

7 years ago

> Could we static_assert that?

I don't quite see how: static_assert doesn't consider reachability.  That is, a static_assert in unreachable code still gets compiled and hence asserted at compile-time.

> This comment still has value, no?

Indeed.  I will restore it.

Assignee

Comment 71

•

7 years ago

Attached patch Updated esr52 rollup fix (obsolete) — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8880598 - Attachment is obsolete: true

Assignee

Comment 72

•

7 years ago

Attached patch Updated beta channel rollup fix (obsolete) — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8880618 - Attachment is obsolete: true

Assignee

Comment 73

•

7 years ago

Attached patch Updated release channel rollup fix (obsolete) — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8880599 - Attachment is obsolete: true

Assignee

Comment 74

•

7 years ago

Comment on attachment 8884915 [details] [diff] [review]
Updated esr52 rollup fix

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible UAF leading to bad things.
Fix Landed on Version: 56.
Risk to taking this patch (and alternatives if risky): Medium to high risk.  We
   do not yet have compat data for this, because it's just landing on tip.  We
   don't even have CI results yet.  I think the patches are correct, and they
   do fix the bug.  I believe they are principled enough and unobservable
   enough from web content that they should not cause compat issues.  But I
   don't have more than about 70% confidence in that.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Attachment #8884915 - Flags: approval-mozilla-esr52?

Assignee

Comment 75

•

7 years ago

Comment on attachment 8884916 [details] [diff] [review]
Updated beta channel rollup fix

Approval Request Comment
[Feature/Bug causing the regression]: WebIDL bindings.
[User impact if declined]: Possible UAF leading to bad things.
[Is this code covered by automated tests?]: Generally, yes: this is changing all
   of our binding code.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Medium to high.
[Why is the change risky/not risky?]: See comment 74.
[String changes made/needed]: None.

Attachment #8884916 - Flags: approval-mozilla-beta?

Assignee

Comment 76

•

7 years ago

Comment on attachment 8884917 [details] [diff] [review]
Updated release channel rollup fix

I'm not sure whether we want this on release, but in case we do, here's the patch and approval request.  See comment 75 for the approval request boilerplate.

Attachment #8884917 - Flags: approval-mozilla-release?

Assignee

Comment 77

•

7 years ago

Attached patch Part 3 updated to tip (obsolete) — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8880596 - Attachment is obsolete: true

Assignee

Comment 78

•

7 years ago

Attached patch Part 3 really merged to tip — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8884918 - Attachment is obsolete: true

Ryan VanderMeulen [:RyanVM]

Assignee

Comment 79

•

7 years ago

   https://hg.mozilla.org/integration/mozilla-inbound/rev/516a1c0c080e7dce29df440a61f0e8ae0f88d36e
   https://hg.mozilla.org/integration/mozilla-inbound/rev/9c3018012352ca80f3ce4d132e6ed61cb2c813ba
   https://hg.mozilla.org/integration/mozilla-inbound/rev/d58add6deeb122dc92760eb100c7068306be6a7f
   https://hg.mozilla.org/integration/mozilla-inbound/rev/2338abec8626ded38b149b611f2ecfc7aa2466c8
   https://hg.mozilla.org/integration/mozilla-inbound/rev/19c7f37f992073c3f0b539062e0ef009b48d7675
   https://hg.mozilla.org/integration/mozilla-inbound/rev/a32e94e98f03c23290f74f892bc545b7d4e025f6
   https://hg.mozilla.org/integration/mozilla-inbound/rev/9491622034f17872aec1732dd23005bfae368f32
   https://hg.mozilla.org/integration/mozilla-inbound/rev/e9505b57cab715e24506db8d59a9dc4568ed490d
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c298a314c6345b03609ed298437aa508aa7e1c

Updated

•

7 years ago

Whiteboard: [checkin on 7/10]

Assignee

Comment 80

•

7 years ago

Attached patch Minor update to fix gc hazards — Details — Splinter Review

Attachment #8885007 - Flags: review?(peterv)

Assignee

Comment 81

•

7 years ago

    https://hg.mozilla.org/integration/mozilla-inbound/rev/64ce651bf43d15e711de6e752d80702cd991b336

to fix the gc hazards.

Assignee

Comment 82

•

7 years ago

Attached patch ESR52 patch updated to fix rooting hazard — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8884915 - Attachment is obsolete: true

Attachment #8884915 - Flags: approval-mozilla-esr52?

Assignee

Comment 83

•

7 years ago

Attached patch release patch updated to fix rooting hazard — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8884917 - Attachment is obsolete: true

Attachment #8884917 - Flags: approval-mozilla-release?

Assignee

Comment 84

•

7 years ago

Attached patch beta patch updated to fix rooting hazard — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8884916 - Attachment is obsolete: true

Attachment #8884916 - Flags: approval-mozilla-beta?

Assignee

Updated

•

7 years ago

Attachment #8885065 - Flags: approval-mozilla-esr52?

Assignee

Updated

•

7 years ago

Attachment #8885066 - Flags: approval-mozilla-release?

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Assignee

Updated

•

7 years ago

Attachment #8885067 - Flags: approval-mozilla-beta?

Comment 85

•

7 years ago

https://hg.mozilla.org/mozilla-central/rev/516a1c0c080e
https://hg.mozilla.org/mozilla-central/rev/9c3018012352
https://hg.mozilla.org/mozilla-central/rev/d58add6deeb1
https://hg.mozilla.org/mozilla-central/rev/2338abec8626
https://hg.mozilla.org/mozilla-central/rev/19c7f37f9920
https://hg.mozilla.org/mozilla-central/rev/a32e94e98f03
https://hg.mozilla.org/mozilla-central/rev/9491622034f1
https://hg.mozilla.org/mozilla-central/rev/e9505b57cab7
https://hg.mozilla.org/mozilla-central/rev/f1c298a314c6
https://hg.mozilla.org/mozilla-central/rev/64ce651bf43d

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox56: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla56

Ionuț Goldan [:igoldan]

Updated

•

7 years ago

Depends on: 1379880

Assignee

Updated

•

7 years ago

Blocks: 1378110

Julien Cristau [:jcristau] (back April 22)

Updated

•

7 years ago

Attachment #8885007 - Flags: review?(peterv) → review+

Comment 86

•

7 years ago

Comment on attachment 8885067 [details] [diff] [review]
beta patch updated to fix rooting hazard

sec-critical, seems to stick on trunk; beta55+

Attachment #8885067 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Carsten Book [:Tomcat]

Comment 87

•

7 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/e3d205dc31f3

status-firefox55: affected → fixed

Liz Henry (:lizzard) (relman/hg->git project)

Updated

•

7 years ago

Group: dom-core-security → core-security-release

Gerry Chang [:gchang]

Comment 88

•

7 years ago

Comment on attachment 8885066 [details] [diff] [review]
release patch updated to fix rooting hazard

Since status-firefox54 was marked as won't fix, Release54-.

Attachment #8885066 - Flags: approval-mozilla-release? → approval-mozilla-release-

Comment 89

•

7 years ago

Comment on attachment 8885065 [details] [diff] [review]
ESR52 patch updated to fix rooting hazard

Sec-critical, sounds bad, let's land this on esr since it's going to release with 55.

Attachment #8885065 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Ryan VanderMeulen [:RyanVM]

Comment 90

•

7 years ago

uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/0a86729d653e

status-firefox-esr52: affected → fixed

Updated

•

7 years ago

No longer blocks: 1378110

Assignee

Comment 93

•

7 years ago

Right, for bounty purposes: this bug report was awesome.  It pointed out a longstanding architectural problem and it was quite easy to go from the provided information to the problem determination.  If we have to have security bug reports, this is the kind I want.  ;)

Andrei Vaida [:avaida]

Updated

•

7 years ago

Flags: qe-verify+

Updated

•

7 years ago

Flags: sec-bounty? → sec-bounty+

Updated

•

7 years ago

Alias: CVE-2017-7801

Whiteboard: [adv-main55+][adv-esr52.3+]

Andrei Vaida [:avaida]

Comment 94

•

7 years ago

I've reproduced the crash on an affected build (asan 52.2esr, 20170608175922) using the test case from Comment 0 on Ubuntu 16.04 x64.

The crash is no longer ocurring on Ubuntu 16.04 x64 using:

      - asan 52.2esr (20170726030943)


Unfortunately I couldn't make the domFuzzLite3 add-on work with 55 and 56: tried setting xpinstall.signatures.required;false and extensions.allow-non-mpc-extensions;true and it still got disabled. Also tried loading it temporarily through about:debugging, with no success.


I tried reproducing the crash with the following regular asan builds instead and I was unable to:

      - asan 55.0b13 (20170726032849)
      - asan 56.0a1 (20170726020448)

Nils, do you have any tips on how I could make the add-on work with newer Fx versions? Thanks!

status-firefox-esr52: fixed → verified

Flags: needinfo?(nils)

Tyson Smith [:tsmith]

Comment 95

•

7 years ago

(In reply to Andrei Vaida [:avaida], Desktop Release QA – please ni? me from comment #94)
> Nils, do you have any tips on how I could make the add-on work with newer Fx
> versions? Thanks!

Andrei, you can use ffpuppet[1]. It is a python module that the fuzzing team uses to automate launching the browser. You will need the unpacked extension which can be found here[2]. 

Then to launch the browser you would the command: python ffpuppet.py path/to/browser/firefox -e <extension_path> -p <your_prefs_file> -d

You can use the prefs file available here[3] or your own custom prefs.js

Hopefully that helps.

[1] https://github.com/MozillaSecurity/ffpuppet
[2] https://github.com/MozillaSecurity/domfuzz/tree/master/dom
[3] https://github.com/MozillaSecurity/ffpuppet/blob/master/prefs/fuzzing-no-e10s.prefs.js

Andrei Vaida [:avaida]

Comment 96

•

7 years ago

Thank you, Tyson - it works. 

I've managed to verify the fix on Ubuntu 16.04 x64 using the following versions as well:

        - asan 55.0b13 (20170726032849)
        - asan 56.0a1 (20170726020448)

Status: RESOLVED → VERIFIED

status-firefox55: fixed → verified

status-firefox56: fixed → verified

Flags: qe-verify+

Flags: needinfo?(nils)

Markus Stange [:mstange]

Updated

•

7 years ago

Blocks: 1371269

Nathan Froyd [:froydnj]

Comment 98

•

7 years ago

So looking at this and the related bugs for some stats I'm collecting...why weren't some of the rooting problems here caught by our hazard analysis?  Is it because proxies were involved, and we're just looking at the possibility of garbage collection happening in the hazard analysis, and not considering things like dead proxies getting swapped in underneath us?

Flags: needinfo?(continuation)

Flags: needinfo?(bzbarsky)

Steve Fink [:sfink] [:s:]

Assignee

Comment 99

•

7 years ago

The fundamental code pattern in this bug was, in pseudo-C++-code:

  void doStuff(HandleObject obj) {
    nsISupports* foo;
    if (IsProxy(obj)) {
      foo = Unwrap(obj)->GetFoo();
    } else {
      foo = obj->GetFoo();
    }

    // Use foo and assume it stays alive even across GC.
  }

From the point of view of rooting analysis, there is no problem here.  "obj" is rooted.  Unwrap(obj) is not rooted, but its liveness lifetime does not cross a possible GC.  The nsISupports* _does_ have a liveness lifetime that crosses possible GC, but it's not a GCThing, so the rooting analysis doesn't care about it.

The fact that in the IsProxy() case the valid lifetime of "foo" matches the valid lifetime of Unwrap(obj) is not something the rooting analysis has a way to derive...

Now if we ignore the dead proxy bit, then everything is still ok: "obj" is rooted, it keeps Unwrap(obj) rooted, and that keeps "foo" alive.  But the dead proxy thing means that "it keeps Unwrap(obj) rooted" can become false, and then you lose.  But that was a problem that was missed by manual analysis, not the automated static analysis, because the automatic analysis didn't have a concept of "it keeps Unwrap(obj) rooted" in the first place; as far as it's concerned, Unwrap(obj) is _not_ rooted, but also not used across GC...

Flags: needinfo?(continuation)

Flags: needinfo?(bzbarsky)

Comment 100

•

7 years ago

Let me try repeating the problematic path that I understood: Unwrap(obj) returns an unrooted GC pointer that we call GetFoo() on, and its return value's (foo's) lifetime is contained within the unwrapped obj's lifetime, and foo is held live across a later GC.

So a hypothetical analysis would need to (1) somehow know that the lifetime of GetFoo's return value is contained within its 'this', and (2) do some amount of dataflow analysis to know that foo gets assigned an nsISupports* with a GC-controlled lifetime. Neither smells totally crazy, but I would worry about complexity and false positives.

unwrapped->GetFoo() is really something like XPCWrappedNative::Get(unwrapped), right?

If people are interested in this, please file a bug to at least get it on record. (Preferably clearing whatever misconceptions I have in the above.) I haven't thought too hard about the feasibility. And my current leaning would be to resuscitate mccr8's DOM iterator analysis before working on this, but opinions are welcome.

Steve Fink [:sfink] [:s:]

Assignee

Comment 101

•

7 years ago

> and its return value's (foo's) lifetime is contained within the unwrapped obj's lifetime

Conceptually, yes.  On the C++ language level, no.  The logic we used to have looks kinda like this:

  void* GetFoo(JSObject* obj) {
    if (IsWrapper(obj)) {
      return GetFoo(Unwrap(obj));
    }

    return GetReservedSlot(obj, 0).toPrivate();
  }

or:

  void* GetFoo(JSObject* obj) {
    if (IsWrapper(obj)) {
      return GetReservedSlot(Unwrap(obj), 0).toPrivate();
    }

    return GetReservedSlot(obj, 0).toPrivate();
  }

but in either case the void* is outliving the JSObject* returned by Unwrap(), from a static-analysis point of view....

> unwrapped->GetFoo() is really something like XPCWrappedNative::Get(unwrapped), right?

It's dom::UnwrapDOMObject; see http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/dom/bindings/BindingUtils.h#155-164

But yes, conceptually pretty similar to XPCWrappedNative::Get.

Comment 102

•

7 years ago

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #101)
> > and its return value's (foo's) lifetime is contained within the unwrapped obj's lifetime
> 
> Conceptually, yes.  On the C++ language level, no.
> ...
> in either case the void* is outliving the JSObject* returned by
> Unwrap(), from a static-analysis point of view....

Right, exactly. I think we're saying the same thing.

But I should have paid more attention to the dead proxy bit. It's true that the case above is funky because obj can cease keeping Unwrap(obj) alive. But actually, the analysis doesn't know or care that obj is rooted. The HandleObject is meaningless to it. It just sees it as JSObject**, which doesn't require rooting.

So come to think of it, this is an interior pointer. obj->GetFoo() and Unwrap(obj)->GetFoo() are pretty much the same thing -- both return an interior pointer, and the analysis at least currently has no reason to believe that the interior pointer is going to survive the GC. Neither obj nor Unwrap(obj) is rooted in its point of view, though note that the analysis presently doesn't handle interior pointers anyway.

As is, this looks like a recipe for a massive number of false positives. It would at *least* need to know that HandleObject points to a rooted pointer. But I doubt that would be enough. Consider

  foo(HandleObject obj) {
    cobj = (CStruct*) obj.getPrivate();
    foo = GetFoo(cobj->GetSomeJSObjectMember());
    gc();
    use(foo);
  }

This is safe, because obj is rooted so everything else is going to stay alive. But the hypothetical analysis would scream because foo is an interior pointer in the temporary returned by cobj->GetSomeJSObjectMember(), and it has no reason to believe that it will stay alive. (And note this case also shows that treating HandleObject as rooted isn't all that helpful; obj might have a weak pointer to cobj, so you can't tell from this code whether obj really keeps cobj and cobj->mSomeJSObjectMember alive.)

Never mind, I'm not seeing much potential for the analysis to catch this case after all. The best I see is if we treated Unwrap() as returning an extra-volatile pointer that propagates its brittleness on any arbitrary call to the call's return value, and then check whether any brittle values are live across a GC. (It would be extra volatile because of the dead proxy possibility.) But that still seems like a lot of false positives and false negatives.

Well, the false negatives could be handled by saying if you Unwrap and then GC, you shouldn't touch any pointers at all. I won't be the judge of how problematic that might be.

Assignee

Comment 103

•

7 years ago

> This is safe, because obj is rooted so everything else is going to stay alive.

Maybe, maybe not.  Just because cobj has a JS object member, doesn't mean it traces it.  It could instead clear it out on finalization.  For an example, see nsWrapperCache when not preserving the wrapper.

> so you can't tell from this code whether obj really keeps cobj and cobj->mSomeJSObjectMember alive.

Yep.