1575217 - AddressSanitizer: heap-use-after-free [@ operator bool] with READ of size 8 though [@ mozilla::net::SpdyConnectTransaction::CreateShimError] or Crash [@ mozilla::net::nsHttpTransaction::IsDone]

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The attached testcase crashed on mozilla-central revision 29e9dde37bd2 (build with --enable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --enable-fuzzing --disable-debug).

For detailed crash information, see attachment.

The issue is not reproducible anymore after I rebased all of my patches to tip (this needs the websockets target patch, which will soon be in bug 1528951, but it also needs the changes from bug 1566342 for HTTP2 and those only landed on m-c recently).

Note that I was not able to reproduce the use-after-free, but running the test 1000 times in a row crashed with a SEGV instead (I will post the second crash stack after filing). These are likely connected as each test that the fuzzer saved for the use-after-free issue shows this behavior. It is unlikely that this is a bug in the fuzzing target itself, especially now that the bug vanished after rebasing. My guess is that either something changed about timings that made it less likely to reproduce or we fixed this in another bug meanwhile (but I can't find any candiates for that).

Filing after discussing this with :kershaw on IRC, so we can investigate if this is still an issue.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

The SEGV crash I got instead of the use-after-free looks like this:

==19996==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000001b6 (pc 0x7f033eba7e95 bp 0x7f02b51f39f0 sp 0x7f02b51f39e0 T13)
==19996==The signal is caused by a READ memory access.
==19996==Hint: address points to the zero page.
    #0 0x7f033eba7e94 in mozilla::net::nsHttpTransaction::IsDone() netwerk/protocol/http/nsHttpTransaction.cpp:717:43
    #1 0x7f033ecbf5d5 in mozilla::net::SpdyConnectTransaction::WebsocketWriteSegments(mozilla::net::nsAHttpSegmentWriter*, unsigned int, unsigned int*) netwerk/protocol/http/TunnelUtils.cpp:1337:50
    #2 0x7f033ea78fc6 in mozilla::net::Http2Stream::WriteSegments(mozilla::net::nsAHttpSegmentWriter*, unsigned int, unsigned int*) netwerk/protocol/http/Http2Stream.cpp:307:31
    #3 0x7f033ea5e8f7 in mozilla::net::Http2Session::WriteSegmentsAgain(mozilla::net::nsAHttpSegmentWriter*, unsigned int, unsigned int*, bool*) netwerk/protocol/http/Http2Session.cpp:3417:33
    #4 0x7f033eb63d1e in mozilla::net::nsHttpConnection::OnSocketReadable() netwerk/protocol/http/nsHttpConnection.cpp:2101:24
    #5 0x7f033eb66b71 in mozilla::net::nsHttpConnection::OnInputStreamReady(nsIAsyncInputStream*) netwerk/protocol/http/nsHttpConnection.cpp:2444:17
    #6 0x7f033eb66f74 in non-virtual thunk to mozilla::net::nsHttpConnection::OnInputStreamReady(nsIAsyncInputStream*) netwerk/protocol/http/nsHttpConnection.cpp
    #7 0x7f033e026811 in mozilla::net::nsSocketInputStream::OnSocketReady(nsresult) netwerk/base/nsSocketTransport2.cpp:285:27
    #8 0x7f033e038122 in mozilla::net::nsSocketTransport::OnSocketReady(PRFileDesc*, short) netwerk/base/nsSocketTransport2.cpp:2235:14
    #9 0x7f033e048c85 in mozilla::net::nsSocketTransportService::DoPollIteration(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>*) netwerk/base/nsSocketTransportService2.cpp
    #10 0x7f033e047482 in mozilla::net::nsSocketTransportService::Run() netwerk/base/nsSocketTransportService2.cpp:971:7
    #11 0x7f033e049aac in non-virtual thunk to mozilla::net::nsSocketTransportService::Run() netwerk/base/nsSocketTransportService2.cpp
    #12 0x7f033dd116eb in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1225:14
    #13 0x7f033dd17221 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:486:10
    #14 0x7f033f155838 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:303:20
    #15 0x7f033efef15f in RunInternal ipc/chromium/src/base/message_loop.cc:315:10
    #16 0x7f033efef15f in RunHandler ipc/chromium/src/base/message_loop.cc:308
    #17 0x7f033efef15f in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:290
    #18 0x7f033dd0a96b in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:459:11
    #19 0x7f0365b3fa28 in _pt_root nsprpub/pr/src/pthreads/ptthread.c:201:5
    #20 0x7f03657836da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
    #21 0x7f036476188e in clone /build/glibc-OTsEL5/glibc-2.27/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Comment 4

[Honza Bambas (:mayhemer)]

Regarding the UAF:
mTransaction is a refptr. Looks like SpdyConnectTransaction is Release()'d more times than AddRef()'ed.

The callstack between Http2Session::Shutdown and SpdyConnectTransaction::CreateShimError is likely going through:
https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/netwerk/protocol/http/Http2Session.cpp#154
https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/netwerk/protocol/http/Http2Session.cpp#1286
https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/netwerk/protocol/http/Http2Stream.cpp#1168
https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/netwerk/protocol/http/TunnelUtils.cpp#1425

I would be very interested in which recent change caused this crash to go away! It might help investigating.

Regarding the SEGV:
It's probably related. mDrivingTransaction is a refptr member of SpdyConnectTransaction. The crash has some similarities to the UAF, if the called-on object was freed, we should crash sooner already.

Comment 5

[Christian Holler (:decoder)]

Attachment: ASan log for mozilla-central revision e7e658ec1e98

I was able to reproduce this issue once more by running the fuzzer on mozilla-central tip (+ websockets target patch). Attached is a new ASan log for mozilla-central rev e7e658ec1e98. I will also attach the new testcase, but it requires the patch for repoducing.

Comment 6

[Christian Holler (:decoder)]

Attachment: Updated Testcase

Here is an updated testcase that locally reproduced the use-after-free if I run it 10000 times in a row on the WebSocket fuzzing target.

Comment 7

[Christian Holler (:decoder)]

The same testcase can also trigger a different variation with use-after-free [@ mozilla::net::DeleteHttpTransaction::Run], which looks like double-free'ing the HttpTransation.

Comment 8

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information for Comment 7

Comment 9

[Honza Bambas (:mayhemer)]

I think we can act on this. It would be good to figure if this is websockets isolated or have a larger impact. P1 for baseline diagnoses only and attention.

Comment 10

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #9)

I think we can act on this. It would be good to figure if this is websockets isolated or have a larger impact. P1 for baseline diagnoses only and attention.

From what I see, I think this is not only websocket releated. I think we can hit it with http2 proxies as well.

Honza, do you have time to take a look?

Comment 11

[Honza Bambas (:mayhemer)]

Taking this now.

Comment 12

[Honza Bambas (:mayhemer)]

I was able to locally reproduce "a crash" with:

• update to e7e658ec1e98
• apply D42816 on top of it
• build with comment 0 config (./mach build, ./mach gtest - just to build gtest libxul)
• added TestWebsocketFuzzing.cpp to netwerk/test/fuzz/moz.build
• export MOZ_LOG=sync,nsHttp:5 + file logging
• `FUZZER=NetworkWebsocket LIBFUZZER=1 MOZ_RUN_GTEST=1 obj-opt/dist/bin/firefox -runs=10000 ../crash-72188709c9f1dc0a790c5b7a10da3372c1cbda56

=>

==7831==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000001b6 (pc 0x7f80f1ea77c5 bp 0x7f80e1b679c0 sp 0x7f80e1b679b0 T13)
==7831==The signal is caused by a READ memory access.
==7831==Hint: address points to the zero page.
    #0 0x7f80f1ea77c4 in mozilla::net::nsHttpTransaction::IsDone() /home/cltbld/Mozilla/src/mozilla-unified/netwerk/protocol/http/nsHttpTransaction.cpp:698:43
    #1 0x7f80f1d3114e in mozilla::net::SpdyConnectTransaction::WebsocketWriteSegments(mozilla::net::nsAHttpSegmentWriter*, unsigned int, unsigned int*) /home/cltbld/Mozilla/src/mozilla-unified/netwerk/protocol/http/TunnelUtils.cpp:1393:50
    #2 0x7f80f1bcc796 in mozilla::net::Http2Stream::WriteSegments(mozilla::net::nsAHttpSegmentWriter*, unsigned int, unsigned int*) /home/cltbld/Mozilla/src/mozilla-unified/netwerk/protocol/http/Http2Stream.cpp:322:31

The log show we access SpdyConnectTransaction::mDrivingTransaction on multiple threads. This is 'illusion of atomic refcnt' problem.

Main thread does:

• https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/netwerk/protocol/http/nsHttpChannel.cpp#7980
• https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/netwerk/protocol/http/nsHttpTransaction.cpp#452
• https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/netwerk/protocol/http/TunnelUtils.cpp#1493

while the socket thread is between these two lines:

• https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/netwerk/protocol/http/TunnelUtils.cpp#1437-1438

This concurrent access on a single RefPtr can cause various, hard to predict issues, specifically UAFs or "use-while-being-destroyed" (UWBD?) or other heap breakage. This is the worst possible race bug that I don't see the first time...

Access to this members needs a lock. I will fix it and try if there are not more issues here (more members to lock...)

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: Bug 1575217, r=michal!,dragana!

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 9088716 [details]
Bug 1575217, r=michal!,dragana!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it's quite complicated. This is a tight race, hard to control, IMO.
• 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?: 65+
• If not all supported branches, which bug introduced the flaw?: 1434137, 1451293
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Easy, probably just about rebasing.
• How likely is this patch to cause regressions; how much testing does it need?: Not likely. It's just moving reassignment operations to a single (socket) thread and adds (debug only) assertions for it, no new locks added, just dispatch.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. We'll want beta and ESR68 patches as well.

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 9088716 [details]
Bug 1575217, r=michal!,dragana!

Beta/Release Uplift Approval Request

• User impact if declined: possible UAF
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): we are moving re-assignments/nullifications to a single thread (via dispatch). no new locks. logic single-thread racing (like nullified too soon/too late) should not occur as the nullification is made after the work has been done and (not) performing the branches specifically dependent on non-nullness are thus no longer necessary or have no effect.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: see above
• Fix Landed on Version: 71
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): see above
• String or UUID changes made by this patch:

Comment 17

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

https://hg.mozilla.org/integration/autoland/rev/e5ac6ba0a72f2bc804680df9574fc9fa5c65a529
https://hg.mozilla.org/mozilla-central/rev/e5ac6ba0a72f

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9088716 [details]
Bug 1575217, r=michal!,dragana!

Fix for potential UAF, let's land for beta 4.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9088716 [details]
Bug 1575217, r=michal!,dragana!

Approved for 68.2esr also.

Comment 20

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

https://hg.mozilla.org/releases/mozilla-beta/rev/2608778f7392

Comment 21

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

https://hg.mozilla.org/releases/mozilla-esr68/rev/49a043c443b9bfaa6aa118ad9479dc84150894a6

Comment 22

[Christian Holler (:decoder)]

Attachment: TSan Log for 3dc70a33491f+ with fix reverted

I remember originally reproducing this on TSan as well to confirm that the tool would have found this (and much easier) but apparently I never attached the log. So I reverted the fix locally and reproduced this again under TSan easily and it shows the data race on RefPtr as expected. It requires less than 10 runs to show up (compared to ASan having a 0.01% chance to find this). Adding the log for documentation purposes.