Closed Bug 1634982 Opened 4 years ago Closed 4 years ago

heap-use-after-free in [@ mozilla::net::ParentProcessDocumentChannel::DisconnectDocumentLoadListener]

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla78
Tracking Flags:
Tracking Status
thunderbird_esr68 --- unaffected
firefox-esr68 --- unaffected
firefox75 --- unaffected
firefox76 --- unaffected
firefox77 + fixed
firefox78 --- fixed

People

(Reporter: tsmith, Assigned: jya)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage][sec-survey])

Attachments

(1 file)

Bug 1634982 - Don't call DocumentChannelBridgeDisconnected when already being disconnected from the DocumentLoadListener. r?mattwoodrow
47 bytes, text/x-phabricator-request
dveditz
: approval-mozilla-beta+
dveditz
: sec-approval+
Details | Review

Found with m-c 20200502-71cb60d53ecd

The reducer is currently reducing the test case I will attach it when it is complete.

==28699==ERROR: AddressSanitizer: heap-use-after-free on address 0x611000eda190 at pc 0x7f97320c7697 bp 0x7ffd6a9d4a10 sp 0x7ffd6a9d4a08
READ of size 8 at 0x611000eda190 thread T0
    #0 0x7f97320c7696 in assign_assuming_AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:66:17
    #1 0x7f97320c7696 in operator= /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:168:5
    #2 0x7f97320c7696 in mozilla::net::ParentProcessDocumentChannel::DisconnectDocumentLoadListener() /gecko/netwerk/ipc/ParentProcessDocumentChannel.cpp:172:25
    #3 0x7f97320d9730 in DisconnectChildListeners /gecko/netwerk/ipc/ParentProcessDocumentChannel.h:45:5
    #4 0x7f97320d9730 in non-virtual thunk to mozilla::net::ParentProcessDocumentChannel::DisconnectChildListeners(nsresult, nsresult) /gecko/netwerk/ipc/ParentProcessDocumentChannel.h
    #5 0x7f973209b17f in mozilla::net::DocumentLoadListener::DisconnectChildListeners(nsresult, nsresult) /gecko/netwerk/ipc/DocumentLoadListener.cpp:730:29
    #6 0x7f97320e8415 in mozilla::net::ParentProcessDocumentOpenInfo::DisconnectChildListeners() /gecko/netwerk/ipc/DocumentLoadListener.cpp:208:10
    #7 0x7f97320d7047 in mozilla::net::ParentProcessDocumentOpenInfo::OnStartRequest(nsIRequest*) /gecko/netwerk/ipc/DocumentLoadListener.cpp:185:9
    #8 0x7f9731e2c23c in mozilla::net::nsHttpChannel::CallOnStartRequest() /gecko/netwerk/protocol/http/nsHttpChannel.cpp:1997:27
    #9 0x7f9731e46701 in mozilla::net::nsHttpChannel::ContinueProcessNormal(nsresult) /gecko/netwerk/protocol/http/nsHttpChannel.cpp:3117:8
    #10 0x7f9731e3f72d in mozilla::net::nsHttpChannel::ProcessNormal() /gecko/netwerk/protocol/http/nsHttpChannel.cpp:3054:10
    #11 0x7f9731e3d839 in mozilla::net::nsHttpChannel::ContinueProcessResponse3(nsresult) /gecko/netwerk/protocol/http/nsHttpChannel.cpp
    #12 0x7f9731e3cce4 in mozilla::net::nsHttpChannel::ContinueProcessResponse2(nsresult) /gecko/netwerk/protocol/http/nsHttpChannel.cpp:2707:10
    #13 0x7f9731e37fbc in mozilla::net::nsHttpChannel::ContinueProcessResponse1() /gecko/netwerk/protocol/http/nsHttpChannel.cpp:2680:10
    #14 0x7f9731e36b77 in mozilla::net::nsHttpChannel::ProcessResponse() /gecko/netwerk/protocol/http/nsHttpChannel.cpp:2573:10
    #15 0x7f9731e87e18 in mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*) /gecko/netwerk/protocol/http/nsHttpChannel.cpp:7791:31
    #16 0x7f973137b3d4 in nsInputStreamPump::OnStateStart() /gecko/netwerk/base/nsInputStreamPump.cpp:518:21
    #17 0x7f973137ab7a in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /gecko/netwerk/base/nsInputStreamPump.cpp:427:21
    #18 0x7f9731046d76 in nsInputStreamReadyEvent::Run() /gecko/xpcom/io/nsStreamUtils.cpp:94:20
    #19 0x7f97310e4e56 in nsThread::ProcessNextEvent(bool, bool*) /gecko/xpcom/threads/nsThread.cpp:1200:14
    #20 0x7f97310ef5bc in NS_ProcessNextEvent(nsIThread*, bool) /gecko/xpcom/threads/nsThreadUtils.cpp:481:10
    #21 0x7f97323fa9af in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /gecko/ipc/glue/MessagePump.cpp:87:21
    #22 0x7f97322e79d7 in RunInternal /gecko/ipc/chromium/src/base/message_loop.cc:315:10
    #23 0x7f97322e79d7 in RunHandler /gecko/ipc/chromium/src/base/message_loop.cc:308:3
    #24 0x7f97322e79d7 in MessageLoop::Run() /gecko/ipc/chromium/src/base/message_loop.cc:290:3
    #25 0x7f9739582588 in nsBaseAppShell::Run() /gecko/widget/nsBaseAppShell.cpp:137:27
    #26 0x7f973cf2694b in nsAppStartup::Run() /gecko/toolkit/components/startup/nsAppStartup.cpp:271:30
    #27 0x7f973d13e1c1 in XREMain::XRE_mainRun() /gecko/toolkit/xre/nsAppRunner.cpp:4608:22
    #28 0x7f973d1400f1 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /gecko/toolkit/xre/nsAppRunner.cpp:4750:8
    #29 0x7f973d140de3 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /gecko/toolkit/xre/nsAppRunner.cpp:4804:21
    #30 0x55a71d40c1c7 in do_main /gecko/browser/app/nsBrowserApp.cpp:217:22
    #31 0x55a71d40c1c7 in main /gecko/browser/app/nsBrowserApp.cpp:331:16
    #32 0x7f97550aab96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
    #33 0x55a71d36052c in _start (/home/worker/builds/m-c-20200502214527-fuzzing-asan-opt/firefox+0xa352c)

0x611000eda190 is located 208 bytes inside of 240-byte region [0x611000eda0c0,0x611000eda1b0)
freed by thread T0 here:
    #0 0x55a71d3d8c7d in free /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:123:3
    #1 0x7f97320c4d34 in Release /gecko/netwerk/ipc/DocumentChannel.cpp:50:1
    #2 0x7f97320c4d34 in Release /gecko/netwerk/ipc/ParentProcessDocumentChannel.cpp:18:1
    #3 0x7f97320c4d34 in non-virtual thunk to mozilla::net::ParentProcessDocumentChannel::Release() /gecko/netwerk/ipc/ParentProcessDocumentChannel.cpp
    #4 0x7f973209acf1 in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:50:40
    #5 0x7f973209acf1 in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:381:36
    #6 0x7f973209acf1 in assign_assuming_AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:69:7
    #7 0x7f973209acf1 in operator= /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:168:5
    #8 0x7f973209acf1 in mozilla::net::DocumentLoadListener::DocumentChannelBridgeDisconnected() /gecko/netwerk/ipc/DocumentLoadListener.cpp:691:26
    #9 0x7f97320c75bb in mozilla::net::ParentProcessDocumentChannel::DisconnectDocumentLoadListener() /gecko/netwerk/ipc/ParentProcessDocumentChannel.cpp:171:26
    #10 0x7f97320d9730 in DisconnectChildListeners /gecko/netwerk/ipc/ParentProcessDocumentChannel.h:45:5
    #11 0x7f97320d9730 in non-virtual thunk to mozilla::net::ParentProcessDocumentChannel::DisconnectChildListeners(nsresult, nsresult) /gecko/netwerk/ipc/ParentProcessDocumentChannel.h
    #12 0x7f973209b17f in mozilla::net::DocumentLoadListener::DisconnectChildListeners(nsresult, nsresult) /gecko/netwerk/ipc/DocumentLoadListener.cpp:730:29
    #13 0x7f97320e8415 in mozilla::net::ParentProcessDocumentOpenInfo::DisconnectChildListeners() /gecko/netwerk/ipc/DocumentLoadListener.cpp:208:10
    #14 0x7f97320d7047 in mozilla::net::ParentProcessDocumentOpenInfo::OnStartRequest(nsIRequest*) /gecko/netwerk/ipc/DocumentLoadListener.cpp:185:9
    #15 0x7f9731e2c23c in mozilla::net::nsHttpChannel::CallOnStartRequest() /gecko/netwerk/protocol/http/nsHttpChannel.cpp:1997:27
    #16 0x7f9731e46701 in mozilla::net::nsHttpChannel::ContinueProcessNormal(nsresult) /gecko/netwerk/protocol/http/nsHttpChannel.cpp:3117:8
    #17 0x7f9731e3f72d in mozilla::net::nsHttpChannel::ProcessNormal() /gecko/netwerk/protocol/http/nsHttpChannel.cpp:3054:10
    #18 0x7f9731e3d839 in mozilla::net::nsHttpChannel::ContinueProcessResponse3(nsresult) /gecko/netwerk/protocol/http/nsHttpChannel.cpp
    #19 0x7f9731e3cce4 in mozilla::net::nsHttpChannel::ContinueProcessResponse2(nsresult) /gecko/netwerk/protocol/http/nsHttpChannel.cpp:2707:10
    #20 0x7f9731e37fbc in mozilla::net::nsHttpChannel::ContinueProcessResponse1() /gecko/netwerk/protocol/http/nsHttpChannel.cpp:2680:10
    #21 0x7f9731e36b77 in mozilla::net::nsHttpChannel::ProcessResponse() /gecko/netwerk/protocol/http/nsHttpChannel.cpp:2573:10
    #22 0x7f9731e87e18 in mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*) /gecko/netwerk/protocol/http/nsHttpChannel.cpp:7791:31
    #23 0x7f973137b3d4 in nsInputStreamPump::OnStateStart() /gecko/netwerk/base/nsInputStreamPump.cpp:518:21
    #24 0x7f973137ab7a in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /gecko/netwerk/base/nsInputStreamPump.cpp:427:21
    #25 0x7f9731046d76 in nsInputStreamReadyEvent::Run() /gecko/xpcom/io/nsStreamUtils.cpp:94:20
    #26 0x7f97310e4e56 in nsThread::ProcessNextEvent(bool, bool*) /gecko/xpcom/threads/nsThread.cpp:1200:14

previously allocated by thread T0 here:
    #0 0x55a71d3d8efd in malloc /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:145:3
    #1 0x55a71d40efdd in moz_xmalloc /gecko/memory/mozalloc/mozalloc.cpp:52:15
    #2 0x7f9732081209 in operator new /builds/worker/workspace/obj-build/dist/include/mozilla/cxxalloc.h:33:10
    #3 0x7f9732081209 in mozilla::net::DocumentChannel::CreateDocumentChannel(nsDocShellLoadState*, mozilla::net::LoadInfo*, unsigned int, nsIInterfaceRequestor*, unsigned int, bool, bool) /gecko/netwerk/ipc/DocumentChannel.cpp:186:9
    #4 0x7f973c6f6270 in nsDocShell::DoURILoad(nsDocShellLoadState*, nsIDocShell**, nsIRequest**) /gecko/docshell/base/nsDocShell.cpp:9713:15
    #5 0x7f973c66e565 in nsDocShell::InternalLoad(nsDocShellLoadState*, nsIDocShell**, nsIRequest**) /gecko/docshell/base/nsDocShell.cpp:8977:8
    #6 0x7f973c70b0c3 in nsDocShell::OnLinkClickSync(nsIContent*, nsIURI*, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, nsIPrincipal*, nsIContentSecurityPolicy*) /gecko/docshell/base/nsDocShell.cpp:12073:17
    #7 0x7f973c728538 in OnLinkClickEvent::Run() /gecko/docshell/base/nsDocShell.cpp:11795:17
    #8 0x7f97310e4e56 in nsThread::ProcessNextEvent(bool, bool*) /gecko/xpcom/threads/nsThread.cpp:1200:14
    #9 0x7f97310ef5bc in NS_ProcessNextEvent(nsIThread*, bool) /gecko/xpcom/threads/nsThreadUtils.cpp:481:10
    #10 0x7f97323fa9af in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /gecko/ipc/glue/MessagePump.cpp:87:21
    #11 0x7f97322e79d7 in RunInternal /gecko/ipc/chromium/src/base/message_loop.cc:315:10
    #12 0x7f97322e79d7 in RunHandler /gecko/ipc/chromium/src/base/message_loop.cc:308:3
    #13 0x7f97322e79d7 in MessageLoop::Run() /gecko/ipc/chromium/src/base/message_loop.cc:290:3
    #14 0x7f9739582588 in nsBaseAppShell::Run() /gecko/widget/nsBaseAppShell.cpp:137:27
    #15 0x7f973cf2694b in nsAppStartup::Run() /gecko/toolkit/components/startup/nsAppStartup.cpp:271:30
    #16 0x7f973d13e1c1 in XREMain::XRE_mainRun() /gecko/toolkit/xre/nsAppRunner.cpp:4608:22
    #17 0x7f973d1400f1 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /gecko/toolkit/xre/nsAppRunner.cpp:4750:8
    #18 0x7f973d140de3 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /gecko/toolkit/xre/nsAppRunner.cpp:4804:21
    #19 0x55a71d40c1c7 in do_main /gecko/browser/app/nsBrowserApp.cpp:217:22
    #20 0x55a71d40c1c7 in main /gecko/browser/app/nsBrowserApp.cpp:331:16
    #21 0x7f97550aab96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310

A Pernosco session is available here: https://pernos.co/debug/k9HvA3ogEvGCpoxW7Af4XA/index.html

Component: Networking → DOM: Navigation
Keywords: sec-high
Group: network-core-security → dom-core-security
Flags: needinfo?(jyavenard)

This looks like a UAF in document channel code. jya, it looks like you touched this function last. Can you take a look? There's no test case, but there is a Pernosco session.

Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Attachment #9146986 - Attachment description: Bug 1634982 - Ensure the last reference to the ADocumentBridgeChannel isn't dropped too early. r?mattwoodrow → Bug 1634982 - Don't call DocumentChannelBridgeDisconnected when already being disconnected from the DocumentLoadListener. r?mattwoodrow
Flags: needinfo?(jyavenard)

oops, that was on beta already :(

status-firefox75: --- → unaffected
status-firefox76: --- → unaffected
status-firefox-esr68: --- → unaffected
status-thunderbird_esr68: --- → unaffected

Comment on attachment 9146986 [details]
Bug 1634982 - Don't call DocumentChannelBridgeDisconnected when already being disconnected from the DocumentLoadListener. r?mattwoodrow

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Not sure.
    Seems to me it should happen all the time.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
  • Which older supported branches are affected by this flaw?: 77
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: patch will apply as-is
  • How likely is this patch to cause regressions; how much testing does it need?: can't think of any.
Attachment #9146986 - Flags: sec-approval?

https://hg.mozilla.org/integration/autoland/rev/5a1d897553a67e778d7b63b5e237da2a939477ce
https://hg.mozilla.org/mozilla-central/rev/5a1d897553a6

Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox78: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla78

Please nominate this patch for beta-approval when you get a chance.

Comment on attachment 9146986 [details]
Bug 1634982 - Don't call DocumentChannelBridgeDisconnected when already being disconnected from the DocumentLoadListener. r?mattwoodrow

Beta/Release Uplift Approval Request

  • User impact if declined: UAF
  • Is this code covered by automated tests?: Unknown
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: That code is exercised all the time; it's surprising it didn't trigger any ASAN stuff,.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): We remove a call that will be made right after in the DocumentLoadListener callee . code flow is otherwise identical
  • String changes made/needed:
Attachment #9146986 - Flags: approval-mozilla-beta?

Comment on attachment 9146986 [details]
Bug 1634982 - Don't call DocumentChannelBridgeDisconnected when already being disconnected from the DocumentLoadListener. r?mattwoodrow

sec-approval+, a=dveditz for beta uplift

Attachment #9146986 - Flags: sec-approval?
Attachment #9146986 - Flags: sec-approval+
Attachment #9146986 - Flags: approval-mozilla-beta?
Attachment #9146986 - Flags: approval-mozilla-beta+
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
QA Whiteboard: [qa-triaged]

Hi,

I want to verify the fix but I'm not sure how to reproduce the issue in the first place, could I get some steps on how I could do this?

Thank you!

Flags: needinfo?(jyavenard)

Tyson, can you provide the step you used to produce the issue?

thanks

Flags: needinfo?(jyavenard) → needinfo?(twsmith)

(In reply to Jean-Yves Avenard [:jya] from comment #13)

Tyson, can you provide the step you used to produce the issue?

The test case for this issue was unreliable and failed to reduce so no good test case was available to attach. Our system deletes bucketed issues after the bug is marked as fixed so the original test case is no longer available. So as far as manual verification there is nothing that can be done AFAIK. On the plus side the fuzzers are no longer seeing this issue.

Flags: needinfo?(twsmith)

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Flags: needinfo?(jyavenard)
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][sec-survey]
Flags: needinfo?(jyavenard)

Based on comment 14, I will remove the qe-verify+ flag. Please add it back if new info is available for manual verification.

Flags: qe-verify+ → qe-verify-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: