1577573 - webExtension content-script can cause a crash in the content process using exportFunction

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrea Marchesini [:baku]]

This doesn't seem to be a bug in webExtension code. More likely it's in the implementation of 'exportFunction'. Attach to this bug there is a small test case. Load it from about:debugging -> Load temporary extension... -> load any website.

The content script does:

function overrideProp(object, property, original) {
  let newValue = new window.wrappedJSObject.Proxy(original, {
      apply: exportFunction(function(target, thisArg, args) {
        if (ContentScript.syncShouldOverload()) {
          if (data.potentiallyShowContextBanner) {
            ContentScript.syncPotentiallyShowContextBanner();
          }
          return window.wrappedJSObject.Promise.reject(new window.wrappedJSObject.Error("SecurityError"));
        }
        return window.wrappedJSObject.Reflect.apply(target, thisArg, args);
      }, window),
    });

  object[property] = exportFunction(newValue, window);
}

overrideProp(window.navigator.wrappedJSObject, "mozGetUserMedia", window.navigator.wrappedJSObject.mozGetUserMedia);

I'll provide a valid crash report soon.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: testcase - webExtension

Comment 2

[Andrea Marchesini [:baku]]

#0 0x00007fb4776bb073 in FunctionFlags::hasFlags(unsigned short) const (this=0x22, flags=256) at /home/baku/Sources/m/proxy/src/js/src/vm/JSFunction.h:147
#1 0x00007fb4776e22ea in FunctionFlags::hasInferredName() const (this=0x22) at /home/baku/Sources/m/proxy/src/js/src/vm/JSFunction.h:187
#2 0x00007fb4776e2289 in JSFunction::hasInferredName() const (this=0x0) at /home/baku/Sources/m/proxy/src/js/src/vm/JSFunction.h:455
#3 0x00007fb4776b7469 in JSFunction::explicitName() const (this=0x0) at /home/baku/Sources/m/proxy/src/js/src/vm/JSFunction.h:560
#4 0x00007fb477e92a35 in JS_GetFunctionId(JSFunction*) (fun=0x0) at /home/baku/Sources/m/proxy/src/js/src/jsapi.cpp:3440
#5 0x00007fb46ff793bf in xpc::ExportFunction(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>)
(cx=0x563b48e0c790, vfunction=..., vscope=..., voptions=..., rval=...) at /home/baku/Sources/m/proxy/src/js/xpconnect/src/ExportHelpers.cpp:430
#6 0x00007fb46ff7fc74 in SandboxExportFunction(JSContext*, unsigned int, JS::Value*) (cx=0x563b48e0c790, argc=2, vp=0x563b49d519c0) at /home/baku/Sources/m/proxy/src/js/xpconnect/src/Sandbox.cpp:374
#7 0x00007fb4776b437c in CallJSNative(JSContext*, bool ()(JSContext, unsigned int, JS::Value*), JS::CallArgs const&)

Comment 3

[Eric Rahm [:erahm]]

Looks like either an xpconnect or js issue, redirecting to xpconnect for now.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So looking at xpc::ExportFunction, we get a HandleValue vfunction. We testd that it's an object, then do:

    RootedObject funObj(cx, &vfunction.toObject());
...
    funObj = js::CheckedUnwrapStatic(funObj);
    // Check that funObj is not null
    funObj = UncheckedUnwrap(funObj);
    if (!JS::IsCallable(funObj)) {
      JS_ReportErrorASCII(cx, "First argument must be a function");
      return false;
    }
...
    JSFunction* fun = JS_GetObjectFunction(funObj);
    RootedString funName(cx, JS_GetFunctionId(fun));

so that looks like the buggy part: in the testcase here funObj is a callable Proxy. So it tests true for JS::IsCallable, but it's NOT a JSFunction, so the JS_GetObjectFunction call return null, and then JS_GetFunctionId(fun) leads to a null-deref; note the fun=0x0 in frame 4 of the stack in comment 2.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I think we should open this up; it's a null-deref and one that requires somewhat privileged code (at least an extension) to trigger.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1577573. Make sure we actually have a function in exportFunction before we try to get its name. r=mccr8

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8df1e9b9f393
Make sure we actually have a function in exportFunction before we try to get its name.  r=mccr8

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8df1e9b9f393

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8df1e9b9f393