1656697 - ThreadSanitizer: data race [@ CharAt] vs. [@ nsTSubstring<char>::StartBulkWriteImpl] with use-after-free

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20200801-750bc4c5c4ad (tsan build).

For detailed crash information, see attachment.

To reproduce the issue, perform the following steps:

1. Download the attached testcase, save as "test.bin".
   2a. Build with --enable-fuzzing (requires Clang and TSan, also build gtests using ./mach gtest dontruntests).
   2b. Alternatively you can download builds from TC using python -mfuzzfetch --tsan --fuzzing --gtest (see https://github.com/MozillaSecurity/fuzzfetch).
2. Run FUZZER=NetworkHttp2ProxyHttp2 objdir/dist/bin/firefox -runs=2 test.bin

(See also the new documentation on the fuzzing interface for further pointers about builds and reproducing: https://firefox-source-docs.mozilla.org/tools/fuzzing/fuzzing_interface.html#fuzzing-interface)

Initially this was reported by AddressSanitizer as a heap-use-after-free, I will add the corresponding trace in the follow-up comment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: AddressSanitizer: heap-use-after-free [@ CharAt] with READ of size 1
Build version: mozilla-central revision 20200731-161920b70ae4
Build type: ASan optimized


Backtrace:

    ==11923==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c000036acb at pc 0x7f3bf1f87840 bp 0x7ffece41a9f0 sp 0x7ffece41a9e8
    READ of size 1 at 0x60c000036acb thread T0 (MainThread)
    SCARINESS: 40 (1-byte-read-heap-use-after-free)
        #0 0x7f3bf1f8783f in CharAt dist/include/nsTString.h:174:12
        #1 0x7f3bf1f8783f in GetPrivate netwerk/protocol/http/nsHttpConnectionInfo.h:129:45
        #2 0x7f3bf1f8783f in mozilla::net::UpdateAltSvcEvent::Run() netwerk/protocol/http/Http2Session.cpp:2381:41
        #3 0x7f3bf12dde39 in mozilla::RunnableTask::Run() xpcom/threads/TaskController.cpp:242:16
        #4 0x7f3bf12da325 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) xpcom/threads/TaskController.cpp:512:26
        #5 0x7f3bf12d81e2 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) xpcom/threads/TaskController.cpp:371:15
        #6 0x7f3bf12d861f in mozilla::TaskController::ProcessPendingMTTask(bool) xpcom/threads/TaskController.cpp:168:36
        #7 0x7f3bf12e9c74 in operator() xpcom/threads/TaskController.cpp:86:37
        #8 0x7f3bf12e9c74 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_5>::Run() dist/include/nsThreadUtils.h:577:5
        #9 0x7f3bf130e88c in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1234:14
        #10 0x7f3bf131977c in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:513:10
        #11 0x7f3befd20523 in bool mozilla::SpinEventLoopUntil<(mozilla::ProcessFailureBehavior)1, mozilla::net::FuzzingStreamListener::waitUntilDone()::'lambda'()>(mozilla::net::FuzzingStreamListener::waitUntilDone()::'lambda'()&&, nsIThread*) dist/include/nsThreadUtils.h:362:25
        #12 0x7f3befd17589 in waitUntilDone netwerk/test/fuzz/FuzzingStreamListener.h:23:5
        #13 0x7f3befd17589 in mozilla::net::FuzzingRunNetworkHttp(unsigned char const*, unsigned long) netwerk/test/fuzz/TestHttpFuzzing.cpp:222:22
        [...]
        #26 0x562bb66088b9 in _start (/home/worker/firefox/firefox+0xa48b9)
    
    DEDUP_TOKEN: CharAt
    0x60c000036acb is located 11 bytes inside of 128-byte region [0x60c000036ac0,0x60c000036b40)
    freed by thread T4 (Socket Thread) here:
        #0 0x562bb6680fed in free llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:123:3
        #1 0x7f3bf1e7c2d6 in ~nsTSubstring dist/include/nsTSubstring.h:328:21
        #2 0x7f3bf1e7c2d6 in mozilla::net::Http2Stream::~Http2Stream() netwerk/protocol/http/Http2Stream.cpp:117:1
        #3 0x7f3bf1e7cab8 in mozilla::net::Http2Stream::~Http2Stream() netwerk/protocol/http/Http2Stream.cpp:111:29
        #4 0x7f3bf11aba4b in PLDHashTable::RawRemove(PLDHashTable::Slot&) xpcom/ds/PLDHashTable.cpp:633:3
        #5 0x7f3bf11ab71a in PLDHashTable::RawRemove(PLDHashEntryHdr*) xpcom/ds/PLDHashTable.cpp:617:3
        #6 0x7f3bf11ab5b6 in PLDHashTable::RemoveEntry(PLDHashEntryHdr*) xpcom/ds/PLDHashTable.cpp:611:3
        #7 0x7f3bf1e592ab in RemoveEntry dist/include/nsTHashtable.h:288:48
        #8 0x7f3bf1e592ab in Remove dist/include/nsBaseHashtable.h:231:13
        #9 0x7f3bf1e592ab in mozilla::net::Http2Session::RecvGoAway(mozilla::net::Http2Session*) netwerk/protocol/http/Http2Session.cpp:2174:34
        #10 0x7f3bf1e69f4f in mozilla::net::Http2Session::WriteSegmentsAgain(mozilla::net::nsAHttpSegmentWriter*, unsigned int, unsigned int*, bool*) netwerk/protocol/http/Http2Session.cpp:3416:10
        #11 0x7f3bf21746b6 in mozilla::net::nsHttpConnection::OnSocketReadable() netwerk/protocol/http/nsHttpConnection.cpp:2153:24
        #12 0x7f3bf2177ff8 in mozilla::net::nsHttpConnection::OnInputStreamReady(nsIAsyncInputStream*) netwerk/protocol/http/nsHttpConnection.cpp:2505:17
        #13 0x7f3bf2178d07 in non-virtual thunk to mozilla::net::nsHttpConnection::OnInputStreamReady(nsIAsyncInputStream*) netwerk/protocol/http/nsHttpConnection.cpp
        #14 0x7f3bf1637a29 in mozilla::net::nsSocketInputStream::OnSocketReady(nsresult) netwerk/base/nsSocketTransport2.cpp:286:27
        #15 0x7f3bf1649a0c in mozilla::net::nsSocketTransport::OnSocketReady(PRFileDesc*, short) netwerk/base/nsSocketTransport2.cpp:2287:14
        #16 0x7f3bf165d3c2 in mozilla::net::nsSocketTransportService::DoPollIteration(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>*) netwerk/base/nsSocketTransportService2.cpp
        #17 0x7f3bf165bb4e in mozilla::net::nsSocketTransportService::Run() netwerk/base/nsSocketTransportService2.cpp:1155:7
        #18 0x7f3bf165dcfc in non-virtual thunk to mozilla::net::nsSocketTransportService::Run() netwerk/base/nsSocketTransportService2.cpp
        #19 0x7f3bf130e88c in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1234:14
        #20 0x7f3bf131977c in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:513:10
        #21 0x7f3bf26ae1e2 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:302:20
        #22 0x7f3bf259a607 in RunInternal ipc/chromium/src/base/message_loop.cc:334:10
        #23 0x7f3bf259a607 in RunHandler ipc/chromium/src/base/message_loop.cc:327:3
        #24 0x7f3bf259a607 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:309:3
        #25 0x7f3bf1307237 in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:447:10
        #26 0x7f3c16f85d3e in _pt_root nsprpub/pr/src/pthreads/ptthread.c:201:5
        #27 0x7f3c16bc66da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
    
    DEDUP_TOKEN: free
    previously allocated by thread T0 (MainThread) here:
        #0 0x562bb668126d in malloc llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:145:3
        #1 0x7f3bf10cb64b in Alloc xpcom/string/nsSubstring.cpp:206:42
        #2 0x7f3bf10cb64b in nsTSubstring<char>::StartBulkWriteImpl(unsigned int, unsigned int, bool, unsigned int, unsigned int, unsigned int) xpcom/string/nsTSubstring.cpp:203:32
        #3 0x7f3bf10dec78 in nsTSubstring<char>::Append(char const*, unsigned int, std::nothrow_t const&) xpcom/string/nsTSubstring.cpp:771:12
        #4 0x7f3bf10de9be in nsTSubstring<char>::Append(char const*, unsigned int) xpcom/string/nsTSubstring.cpp:742:7
        #5 0x7f3bf217e207 in mozilla::net::nsHttpConnectionInfo::BuildHashKey() netwerk/protocol/http/nsHttpConnectionInfo.cpp:207:14
        #6 0x7f3bf217f830 in mozilla::net::nsHttpConnectionInfo::RebuildHashKey() netwerk/protocol/http/nsHttpConnectionInfo.cpp:290:3
        #7 0x7f3bf20b625f in SetIsolated netwerk/protocol/http/nsHttpConnectionInfo.h:145:5
        #8 0x7f3bf20b625f in mozilla::net::nsHttpChannel::ContinueOnBeforeConnect(bool, nsresult) netwerk/protocol/http/nsHttpChannel.cpp:691:20
        #9 0x7f3bf20b254f in mozilla::net::nsHttpChannel::OnBeforeConnect() netwerk/protocol/http/nsHttpChannel.cpp:644:10
        #10 0x7f3bf20b1666 in mozilla::net::nsHttpChannel::PrepareToConnect() netwerk/protocol/http/nsHttpChannel.cpp:507:10
        #11 0x7f3bf2123564 in mozilla::net::nsHttpChannel::ContinueBeginConnectWithResult() netwerk/protocol/http/nsHttpChannel.cpp:7159:10
        #12 0x7f3bf211c597 in mozilla::net::nsHttpChannel::BeginConnect() netwerk/protocol/http/nsHttpChannel.cpp:6940:8
        #13 0x7f3bf2119f08 in mozilla::net::nsHttpChannel::MaybeResolveProxyAndBeginConnect() netwerk/protocol/http/nsHttpChannel.cpp:6663:8
        #14 0x7f3bf2119b02 in mozilla::net::nsHttpChannel::AsyncOpenFinal(mozilla::TimeStamp) netwerk/protocol/http/nsHttpChannel.cpp:6619:12
        #15 0x7f3bf2118dce in mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*) netwerk/protocol/http/nsHttpChannel.cpp:6596:5
        #16 0x7f3befd1751e in mozilla::net::FuzzingRunNetworkHttp(unsigned char const*, unsigned long) netwerk/test/fuzz/TestHttpFuzzing.cpp:219:19
        [...]
        #28 0x7f3c15aa4b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    
    DEDUP_TOKEN: malloc
    Thread T4 (Socket Thread) created by T0 (MainThread) here:
        #0 0x562bb666ba1a in pthread_create llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:209:3
        #1 0x7f3c16f761e5 in _PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:458:14
        #2 0x7f3c16f6715e in PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:533:12
        #3 0x7f3bf1309f17 in nsThread::Init(nsTSubstring<char> const&) xpcom/threads/nsThread.cpp:659:8
        #4 0x7f3bf13183da in nsThreadManager::NewNamedThread(nsTSubstring<char> const&, unsigned int, nsIThread**) xpcom/threads/nsThreadManager.cpp:629:12
        #5 0x7f3bf132343a in NS_NewNamedThread(nsTSubstring<char> const&, nsIThread**, already_AddRefed<nsIRunnable>, unsigned int) xpcom/threads/nsThreadUtils.cpp:161:57
        #6 0x7f3bef906da9 in nsresult NS_NewNamedThread<14ul>(char const (&) [14ul], nsIThread**, nsIRunnable*, unsigned int) dist/include/nsThreadUtils.h:85:10
        #7 0x7f3bf1658e5a in mozilla::net::nsSocketTransportService::Init() netwerk/base/nsSocketTransportService2.cpp:775:7
        #8 0x7f3bf1288650 in mozilla::xpcom::CreateInstanceImpl(mozilla::xpcom::ModuleID, nsISupports*, nsID const&, void**) xpcom/components/StaticComponents.cpp:10591:7
        #9 0x7f3bf12b6aa0 in CreateInstance xpcom/components/nsComponentManager.cpp:219:46
        #10 0x7f3bf12b6aa0 in nsComponentManagerImpl::GetServiceLocked((anonymous namespace)::MutexLock&, (anonymous namespace)::EntryWrapper&, nsID const&, void**) xpcom/components/nsComponentManager.cpp:1370:17
        #11 0x7f3bf12ad30d in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) xpcom/components/nsComponentManager.cpp:1555:10
        #12 0x7f3bf12be272 in CallGetService xpcom/components/nsComponentManagerUtils.cpp:61:43
        #13 0x7f3bf12be272 in nsGetServiceByContractIDWithError::operator()(nsID const&, void**) const xpcom/components/nsComponentManagerUtils.cpp:253:21
        #14 0x7f3bf113f146 in nsCOMPtr_base::assign_from_gs_contractid_with_error(nsGetServiceByContractIDWithError const&, nsID const&) xpcom/base/nsCOMPtr.cpp:91:7
        #15 0x7f3bf15a7c53 in operator= dist/include/nsCOMPtr.h:762:5
        #16 0x7f3bf15a7c53 in mozilla::net::nsIOService::InitializeSocketTransportService() netwerk/base/nsIOService.cpp:424:29
        [...]
        #40 0x7f3c15aa4b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    
    DEDUP_TOKEN: pthread_create
    Shadow bytes around the buggy address:
      0x0c187fffed30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c187fffed40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    =>0x0c187fffed50: fa fa fa fa fa fa fa fa fd[fd]fd fd fd fd fd fd
      0x0c187fffed60: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
      0x0c187fffed70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    Shadow byte legend (one shadow byte represents 8 application bytes):
      Addressable:           00
      Partially addressable: 01 02 03 04 05 06 07
      Heap left redzone:       fa
      Freed heap region:       fd

Comment 4

[Honza Bambas (:mayhemer)]

Yes, we know we do this :( I filed bug 1564120 to fix this once and for all. But we can use a simpler solution here - do a proper clone of the CI structure at the right place.

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: Bug 1656697, r=valentin

Comment 6

[Kershaw Chang [:kershaw]]

Comment on attachment 9174987 [details]
Bug 1656697, r=valentin

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I'd say it's not easy, since this is a race about accessing the connection info at the same 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?: All
• 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?: The risk is low, since it's not easy to trigger this issue.
• How likely is this patch to cause regressions; how much testing does it need?: Low. The patch is quite straightforward.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 9174987 [details]
Bug 1656697, r=valentin

sec-approval+

Comment 8

[Ryan VanderMeulen [:RyanVM]]

We've already built RC builds this week. Are you saying we need to respin everything and take this in 81/78.3esr still this cycle, Dan?

Comment 9

[Daniel Veditz [:dveditz]]

I was thinking we had another week for some reason. As long as there's no testcase checked in until later this race is pretty obscure from the patch.

Comment 10

[Kershaw Chang [:kershaw]]

I think we don't need to uplift this patch, since this issue is already existed for a long time and not easy to hit.

Comment 11

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

Sorry for the delayed landing:
https://hg.mozilla.org/integration/autoland/rev/bb5d1f8f84de4c8cc862db14ab5e4ba13227ce81

Comment 12

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

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

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 14

[Kershaw Chang [:kershaw]]

Done.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR78 approval when you get a chance. It grafts cleanly as-landed.

Comment 16

[Kershaw Chang [:kershaw]]

Comment on attachment 9174987 [details]
Bug 1656697, r=valentin

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec-high bug.
• User impact if declined: Firefox could crash in some cases.
• Fix Landed on Version: 82
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch is quite simple.
• String or UUID changes made by this patch: None

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9174987 [details]
Bug 1656697, r=valentin

Approved for 78.4esr.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/005391f69f10

Comment 19

[Kershaw Chang [:kershaw]]

I think we should back out this bug for causing the regression.
I believe the crash rate of this bug should be low, so we could live with this bug until we figure out a proper solution.

Comment 20

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

Backed out from mozilla-central as requested by kershaw for the mentioned regressions:

https://hg.mozilla.org/integration/autoland/rev/6cb4194e29ec9933e6de0ebbb7d20e457d34116b

Comment 21

[Julien Cristau [:jcristau]]

beta backout:
https://hg.mozilla.org/releases/mozilla-beta/rev/ab51048968d9fea10a8b38b9732cc89fbd538de9

Comment 22

[Christian Holler (:decoder)]

Signature does not appear to be showing up in the wild, very likely because the result here is a use-after-free that rarely would result in a plain crash if things are happening in a short timeframe.

Comment 23

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

Backout merged to central
https://hg.mozilla.org/mozilla-central/rev/6cb4194e29ec

Comment 24

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

Sorry, wanted to say the backout got merged to central.

Comment 25

[Kershaw Chang [:kershaw]]

With checking the logs in bug 1668168, the regression is caused by cloning the http connection in nsHttpTransaction::DisableSpdy.
The reason is that nsHttpTransaction and nsHttpChannel share the same connection info. In nsHttpTransaction::Init, we only assign the connection info, not clone it.
If we clone the connection info in DisableSpdy() without disabling spdy of the connection info in nsHttpChannel, the channel would still use the old connection info and still tries to connect to a h2 server. This happens when connection based authentication is used.

To fix the crash in this bug, only cloning the connection info before creating SpdyConnectTransaction should be enough.

Comment 26

[Julien Cristau [:jcristau]]

esr78 backout:
https://hg.mozilla.org/releases/mozilla-esr78/rev/c07fd996818e

Comment 27

[Kershaw Chang [:kershaw]]

:dveditz, should I request for sec-approval again before landing this patch?
Not sure what to do, since I can't clear the previous sec-approval+ flag.

Comment 28

[Jens Stutte [:jstutte]]

Hi Tom, can you answer this?

Comment 29

[Julien Cristau [:jcristau]]

Comment on attachment 9174987 [details]
Bug 1656697, r=valentin

Resetting uplift flag until this re-lands.

Comment 30

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9174987 [details]
Bug 1656697, r=valentin

Re-granting sec-approval

Comment 31

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

https://hg.mozilla.org/integration/autoland/rev/669d1c2dfa24eff4002dc242705e7ed2960e2d5c

Comment 32

[Julien Cristau [:jcristau]]

Are we going to want this in 82, or let it ride to 83? (We're building the last 82 beta tomorrow so it's getting tight if the fix is somehow risky)

Comment 33

[Kershaw Chang [:kershaw]]

(In reply to Julien Cristau [:jcristau] from comment #32)

Are we going to want this in 82, or let it ride to 83? (We're building the last 82 beta tomorrow so it's getting tight if the fix is somehow risky)

Given that my patch caused a regression before, I'd like to let this ride to 83.
Thanks.

Comment 34

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

https://hg.mozilla.org/integration/autoland/rev/669d1c2dfa24eff4002dc242705e7ed2960e2d5c
https://hg.mozilla.org/mozilla-central/rev/669d1c2dfa24

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Is this ready for an ESR78 approval request? It grafts cleanly.

Comment 36

[Kershaw Chang [:kershaw]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)

Is this ready for an ESR78 approval request? It grafts cleanly.

Ok, I think this should be safe enough for ESR78 approval.

Comment 37

[Kershaw Chang [:kershaw]]

Comment on attachment 9174987 [details]
Bug 1656697, r=valentin

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec-high bug.
• User impact if declined: Firefox could be crash when using h2 proxy, but I think it's not easy to hit this.
• Fix Landed on Version: 83
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The way that SpdyConnectTransaction uses connection info object is simply reading some data. Using a cloned one should not cause any regression.
• String or UUID changes made by this patch: N/A

Comment 38

[Julien Cristau [:jcristau]]

Comment on attachment 9174987 [details]
Bug 1656697, r=valentin

approved for 78.5

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/38ecbf5f1b5b