1575515 - mozilla::jsipc::JavaScriptBase::RecvDOMInstanceOf doesn't validate argument leading to out of bounds read

Status: RESOLVED INVALID

(Core :: JavaScript Engine, task)

Description

I found what looks like an issue using a static analysis tool to trace dataflow from IPC. RecvDOMInstanceOf, receives a integer argument depth which ultimately is used as an offset to when reading an array.

mozilla::jsipc::JavaScriptBase::RecvDOMInstanceOf takes an argument called depth , which is passed to WrapperAnswer::RecvDOMInstanceOf which then calls mozilla::dom::InterfaceHasInstance where the value is used as index (depth & prototypeID both come from IPC) :

bool InterfaceHasInstance(JSContext* cx, int prototypeID, int depth,
                          JS::Handle<JSObject*> instance, bool* bp) {
  const DOMJSClass* domClass = GetDOMClass(js::UncheckedUnwrap(instance));

  MOZ_ASSERT(!domClass || prototypeID != prototypes::id::_ID_Count,
             "Why do we have a hasInstance hook if we don't have a prototype "
             "ID?");

  *bp = (domClass && domClass->mInterfaceChain[depth] == prototypeID);
  return true;
}

At a quick skim, it looks like an out of bounds read, but the value isn't returned directly so not sure if really is a security issue. prototypeID is controlled by IPC so maybe could be infoleak through trial and error but it looks like prototypeID is constrained so maybe not. Anyways, I'm mostly filing this as I would like to make sure this from someone who knows this code better. And then either add a fix, or mark as false positive.

Comment 1

[Jan de Mooij [:jandem]]

Hm I wonder if this (CPOW?) code is still reachable, it could be automation only IIRC.

Comment 2

[Jan de Mooij [:jandem]]

There is an IsInAutomation() check here: https://searchfox.org/mozilla-central/rev/03853a6e87c4a9405fce1de49e5d03b9e7a7a274/js/ipc/WrapperAnswer.cpp#856-858

Comment 3

[Kris Maglione [:kmag]]

It should be automation-only, yes. We still use CPOWs outside of automation, but only for object identity when passing objects from the child to the parent and back to the child. They can't actually be used as cross-process wrappers.

Also, we should never send CPOWs from the parent to the child, so there wouldn't be much opportunity to exploit this anyway.

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Ah ok, thanks for the explanation! I'll mark as false positive.

Comment 5

[Tom S [:evilpie]]

It might still make sense to fix this, but like Jan said after bug 1465911 there is hopefully no way to exploit this.