1154598 - heap-use-after-free at assign_assuming_AddRef

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Aki Helin]

Attachment: ff-uaf-assign_assuming_AddRef.xhtml

ASan spots a use after free at least in current tinderbox builds when the attached page is opened. Filing under XPCOM, since that's where the memory is touched erroneously, but also sounds like a DOM node event being handled after it got deleted.


==5661==ERROR: AddressSanitizer: heap-use-after-free on address 0x611000182f80 at pc 0x7fe623afbdc8 bp 0x7ffef0d49f70 sp 0x7ffef0d49f68
READ of size 8 at 0x611000182f80 thread T0 (Web Content)
    #0 0x7fe623afbdc7 in assign_assuming_AddRef /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/build/../glue/nsCOMPtr.h:336
    #1 0x7fe623c0fa28 in operator= /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/netwerk/base/../../dist/include/nsCOMPtr.h:850
    #2 0x7fe623c0fbfc in non-virtual thunk to nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/netwerk/base/Unified_cpp_netwerk_base1.cpp:785
    #3 0x7fe623c4f15d in OnStateStop /builds/slave/m-cen-l64-asan-000000000000000/build/src/netwerk/base/nsInputStreamPump.cpp:721
    #4 0x7fe623c4d2e9 in OnInputStreamReady /builds/slave/m-cen-l64-asan-000000000000000/build/src/netwerk/base/nsInputStreamPump.cpp:440
    #5 0x7fe623a727c9 in Run /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/io/nsStreamUtils.cpp:91
    #6 0x7fe623aaec74 in ProcessNextEvent /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:866
    #7 0x7fe623b10c0a in NS_ProcessNextEvent /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:265
    #8 0x7fe624368da9 in Run /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessagePump.cpp:99
    #9 0x7fe6242f6aec in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:233
    #10 0x7fe628dc44b7 in Run /builds/slave/m-cen-l64-asan-000000000000000/build/src/widget/nsBaseAppShell.cpp:164
    #11 0x7fe62a953872 in XRE_RunAppShell /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:738
    #12 0x7fe6242f6aec in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:233
    #13 0x7fe62a952ee2 in XRE_InitChildProcess /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:575
    #14 0x48d252 in content_process_main /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:236
    #15 0x7fe6216bdeac in ?? ??:0
    #16 0x48c5ac in _start ??:0

Comment 1

[Boris Zbarsky [:bzbarsky]]

So in a debug build I get:

###!!! ASSERTION: aRequest should be pending!: 'mDeferRequests.Contains(aRequest) || mLoadingAsyncRequests.Contains(aRequest) || mNonAsyncExternalScriptInsertedRequests.Contains(aRequest) || mXSLTRequests.Contains(aRequest) || mPreloads.Contains(aRequest, PreloadRequestComparator()) || mParserBlockingRequest', file ../../../mozilla/dom/base/nsScriptLoader.cpp, line 1554

followed by:

Hit MOZ_CRASH(element wasn't found in this list!) at ../../dist/include/mozilla/LinkedList.h:477
#1  0x00000001031311c4 in mozilla::LinkedListElement<nsScriptLoadRequest>::removeFrom (this=0x117d74c58, aList=@0x1267842c0) at LinkedList.h:220
#2  0x000000010312a5d5 in nsScriptLoadRequestList::Steal (this=0x1267842c0, aElem=0x117d74c50) at nsScriptLoader.h:141
#3  0x000000010311787e in nsScriptLoader::PrepareLoadedRequest () at nsScriptLoader.cpp:1561
#4  0x0000000103116bdd in nsScriptLoader::OnStreamComplete () at nsScriptLoader.cpp:1412

so it's posible this is a regression from bug 1149235...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, definitely.  So what happens is that nsScriptLoader::ParsingComplete does:

  mLoadingAsyncRequests.Clear();

after which we have a still-loading nsScriptLoadRequest which is not in mLoadingAsyncRequests.

And the key is that the XML is malformed, so we forcibly terminate the parser, so we reach that Clear() code.

Still not sure how this used to work before bug 1149235.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The answer is: it didn't quite.  We hit the "aRequest should be pending" assert before then too.

It's just that we didn't use to depend on not hitting it.

I'm going to put in a pair of belt-and-suspenders fixes here: go back to not depending on not hitting the assert, and fixing things so we don't hit the assert.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Keep better track of our script requests

Comment 5

[Boris Zbarsky [:bzbarsky]]

Note that I can't guarantee this fixes the UAF, but I'm pretty sure it does.  In particular, I would expect that the messup with the lists dropped an extra ref on the nsScriptLoadRequest, and nsBaseChannel::OnStopRequest is what's currently dropping the last ref (so with the bug is running after the object has been deleted).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8592975 [details] [diff] [review]
Keep better track of our script requests

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Probably not very easily; at least have to figure out how to trigger the aTerminated condition.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  No, but only because I didn't include the test.. which I would like to land with the patch, actually.

Which older supported branches are affected by this flaw?  None; this is m-c only.

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

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

How likely is this patch to cause regressions; how much testing does it need?  I'm a bit worried about whether something expected script load requests to still be handled even after parser termination somehow.  I don't see how it could have happened, but would like as much bake time as possible for this to make sure.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8592975 [details] [diff] [review]
Keep better track of our script requests

Trunk-only issues don't need sec-approval.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Alright.  https://hg.mozilla.org/integration/mozilla-inbound/rev/c4819139b744 with the test included.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4819139b744

Comment 13

[Jim Mathies [:jimm]]

Now that this is fixed can we open it up and add crash signatures to it?