1438988 - Use after free in mozJSComponentLoader::ImportInto()

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[mozillabugs]

mozJSComponentLoader::ImportInto() [1] (js\xpconnect\loader\mozJSComponentLoader.cpp) can leave a pointer to a deleted object lying around. When it is called again, it can use the object.

The bug centers around the |nsAutoPtr| |newEntry| on line 1111. Line 1150 extracts the raw pointer from |newEntry| and puts it into |mInProgressImports| as a |ModuleEntry *|. But line 1153 returns if |rv| indicates an error, without first removing |info.Key()| (which indexes the pointer from |newEntry|) from |mInProgressImports|. [2]

The return then deletes the object pointed to by |newEntry|, but leaves a pointer to this object in |mInProgressImports|. When |ImportInto()| gets called again for the module that that pointer represented, line 1112 extracts the pointer from |mInProgressImports| and uses it, passing it to lines 1185-1199. The code typically crashes reading a trash address on a call chain from line 1199.

1091: nsresult
1092: mozJSComponentLoader::ImportInto(const nsACString& aLocation,
1093:                                  HandleObject targetObj,
1094:                                  JSContext* callercx,
1095:                                  MutableHandleObject vp)
1096: {
1097:     vp.set(nullptr);
1098: 1
1099:     nsresult rv;
...
1111:     nsAutoPtr<ModuleEntry> newEntry;
1112:     if (!mImports.Get(info.Key(), &mod) && !mInProgressImports.Get(info.Key(), &mod)) {
...
1148: 
1149:         mLocations.Put(newEntry->resolvedURL, new nsCString(info.Key()));
1150:         mInProgressImports.Put(info.Key(), newEntry);
1151: 
1152:         rv = info.EnsureURI();
1153:         NS_ENSURE_SUCCESS(rv, rv);
1154:         RootedValue exception(callercx);
1155:         rv = ObjectForLocation(info, sourceFile, &newEntry->obj,
1156:                                &newEntry->thisObjectKey,
1157:                                &newEntry->location, true, &exception);
1158: 
1159:         mInProgressImports.Remove(info.Key());
1160: 
1161:         if (NS_FAILED(rv)) {
1162:             if (!exception.isUndefined()) {
1163:                 // An exception was thrown during compilation. Propagate it
1164:                 // out to our caller so they can report it.
1165:                 if (!JS_WrapValue(callercx, &exception))
1166:                     return NS_ERROR_OUT_OF_MEMORY;
1167:                 JS_SetPendingException(callercx, exception);
1168:                 return NS_OK;
1169:             }
1170: 
1171:             // Something failed, but we don't know what it is, guess.
1172:             return NS_ERROR_FILE_NOT_FOUND;
1173:         }
...
1183:     }
1184: 
1185:     MOZ_ASSERT(mod->obj, "Import table contains entry with no object");
1186:     vp.set(mod->obj);
1187: 
1188:     if (targetObj) {
1189:         // cxhelper must be created before jsapi, so that jsapi is destroyed and
1190:         // pops any context it has pushed before we report to the caller context.
1191:         JSCLContextHelper cxhelper(callercx);
1192: 
1193:         // Even though we are calling JS_SetPropertyById on targetObj, we want
1194:         // to ensure that we never run script here, so we use an AutoJSAPI and
1195:         // not an AutoEntryScript.
1196:         dom::AutoJSAPI jsapi;
1197:         jsapi.Init();
1198:         JSContext* cx = jsapi.cx();
1199:         JSAutoCompartment ac(cx, mod->obj);

You can force ImportInto() to use the destroyed object as follows. Attach a debugger to FF and set a BP on line 1153. Wait for a BP at which |info.mLocation.mData| contains "resource://gre/modules/DownloadCore.jsm". Force |rv| to -1 and proceed. The next time ImportInto() is called for that module, FF will crash with a read or write beyond bounds on a call chain from line 1199. (Note that if you're using the debug build, you'll need to skip some asserts).

I don't know whether this bug practically can be manifested in the wild.


[1] This code is slightly different in trunk; ImportInto() has been renamed to Import(), but still has the same bug.

[2] Later code (line 1159) removes the pointer from |mInProgressImports|, but that's too late.

Comment 1

[Daniel Veditz [:dveditz]]

Without legacy add-ons the only code loading components in this way is our own so hard to see how it could be exploited, and add-ons already needed to have been trusted given the powers they had.

Comment 2

[Kris Maglione [:kmag]]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> Without legacy add-ons the only code loading components in this way is our
> own so hard to see how it could be exploited, and add-ons already needed to
> have been trusted given the powers they had.

Even if we were giving untrusted code access to the import API, it's hard to imagine how this could be exploited. The only way EnsureURI can fail is on OOM or if the module URI is invalid. But, OOM in this case would be unlikely and hard to exploit. And, is in fact impossible because:

EnsureURI can only fail if the URI hasn't already been resolved and cached. But that's not possible at this point, because we check ResolvedURI() before we get to this point, which has already called EnsureURI(), and returns failure if EnsureURI fails.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Cleanup some error handling logic

The error handling logic here would fail to remove a stale raw pointer from
mInProgressImports if the EnsureURI() call failed. Fortunately, it's not
actually possible for EnsureURI() to fail in this case, because the
EnsureResolvedURI() call above already implies EnsureURI(). That said, we're
better off structuring this code to ensure that we never leave a value in
mInProgressImports after we exit the scope.

Comment 4

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc0c79890cb95ea48d83cd358b19e8f90e030cc
Bug 1438988: Cleanup some error handling logic. r=mccr8

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9fc0c79890cb