1601712 - AddressSanitizer: heap-use-after-free [@ RequestBlockedOnRead] with READ of size 2 with HTTP2 Proxy

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20191205-3dc70a33491f.

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 ASan, also build gtests using ./mach gtest dontruntests).
   2b. Alternatively you can download builds from TC using python -mfuzzfetch -a --fuzzing --tests gtest (see https://github.com/MozillaSecurity/fuzzfetch).
2. Run MOZ_RUN_GTEST=1 LIBFUZZER=1 FUZZER=NetworkHttp2ProxyHttp2 objdir/dist/bin/firefox test.bin

This is a recent regression, the test reproduces deterministically and it requires HTTP2 proxied via HTTP2.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

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

:decoder, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 4

[Kershaw Chang [:kershaw]]

Dragana, could you take a look please?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Nhi, can you please help find an assignee for this?

Comment 6

[Michal Novotny [:michal]]

Http2Session is closed and deleted at frame 1 when nsHttpConnection::OnInputStreamReady() at frame 9 fails at https://searchfox.org/mozilla-central/rev/a92ed79b0bc746159fc31af1586adbfa9e45e264/netwerk/protocol/http/nsHttpConnection.cpp#2619. TLSFilterTransaction is deleted as well, so both are used after they are freed at frames 15-18. I'm not sure what's the best way to fix this. Always dispatching an event in CheckAvailData::Dispatch() fixes this particular problem. Another option would be to keep reference to Http2Session in TLSFilterTransaction::ReadSegments() and to TLSFilterTransaction in nsAHttpTransaction::ReadSegmentsAgain() to make sure they won't go away.

#0 0x00007f228f7dc118 in mozilla::net::Http2Session::~Http2Session() (this=0x7f228f7dc110 <mozilla::net::Http2Session::~Http2Session()>)
at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/Http2Session.cpp:179
#1 0x00007f228f7dc989 in mozilla::net::Http2Session::~Http2Session() (this=0x7f226f4e8800) at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/Http2Session.cpp:179
#2 0x00007f228f7d991b in mozilla::net::Http2Session::Release() (this=0x7f226f4e8800) at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/Http2Session.cpp:50
#3 0x00007f228f837d04 in mozilla::RefPtrTraits<mozilla::net::nsAHttpTransaction>::Release(mozilla::net::nsAHttpTransaction*) (aPtr=0x7f226f4e8800)
at /mnt/work/opt/moz/hg-central-2/_obj-browser-release-tb-fp-dbg/dist/include/mozilla/RefPtr.h:50
#4 0x00007f228f837c85 in RefPtr<mozilla::net::nsAHttpTransaction>::ConstRemovingRefPtrTraits<mozilla::net::nsAHttpTransaction>::Release(mozilla::net::nsAHttpTransaction*)
(aPtr=0x7f226f4e8800) at /mnt/work/opt/moz/hg-central-2/_obj-browser-release-tb-fp-dbg/dist/include/mozilla/RefPtr.h:379
#5 0x00007f228fa7d95d in RefPtr<mozilla::net::nsAHttpTransaction>::assign_assuming_AddRef(mozilla::net::nsAHttpTransaction*) (this=0x7f226f4d2e30, aNewPtr=0x0)
at /mnt/work/opt/moz/hg-central-2/_obj-browser-release-tb-fp-dbg/dist/include/mozilla/RefPtr.h:69
#6 0x00007f228f96b399 in RefPtr<mozilla::net::nsAHttpTransaction>::operator=(decltype(nullptr)) (this=0x7f226f4d2e30)
at /mnt/work/opt/moz/hg-central-2/_obj-browser-release-tb-fp-dbg/dist/include/mozilla/RefPtr.h:168
#7 0x00007f228f96b5bd in mozilla::net::TLSFilterTransaction::Close(nsresult) (this=0x7f226f4d2df0, aReason=-2142568376)
at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/TunnelUtils.cpp:165
#8 0x00007f228fa18090 in mozilla::net::nsHttpConnection::CloseTransaction(mozilla::net::nsAHttpTransaction*, nsresult, bool)
(this=0x7f226f335400, trans=0x7f226f4d2df0, reason=-2142568376, aIsShutdown=false) at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/nsHttpConnection.cpp:1972
#9 0x00007f228fa23631 in mozilla::net::nsHttpConnection::OnInputStreamReady(nsIAsyncInputStream*) (this=0x7f226f335400, in=0x7f226f48b880)
at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/nsHttpConnection.cpp:2620
#10 0x00007f228fa6bea7 in mozilla::net::CheckAvailData::Run() (this=0x7f226f4c8df0) at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/TunnelUtils.cpp:1761
#11 0x00007f228f97c01b in mozilla::net::CheckAvailData::Dispatch() (this=0x7f226f4c8df0) at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/TunnelUtils.cpp:1769
#12 0x00007f228f97bcf0 in mozilla::net::InputStreamShim::AsyncWait(nsIInputStreamCallback*, unsigned int, unsigned int, nsIEventTarget*)
(this=0x7f226f48b880, callback=0x7f226f335410, flags=0, requestedCount=0, target=0x0) at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/TunnelUtils.cpp:1813
#13 0x00007f228fa2018f in mozilla::net::nsHttpConnection::ResumeRecv() (this=0x7f226f335400) at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/nsHttpConnection.cpp:1821
#14 0x00007f228fa69a96 in mozilla::net::ConnectionHandle::ResumeRecv() (this=0x7f227497cba0) at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/nsHttpConnectionMgr.cpp:1817
#15 0x00007f228f7de5ff in mozilla::net::Http2Session::ResumeRecv() (this=0x7f226f4e8800) at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/Http2Session.h:44
#16 0x00007f228f7f8113 in mozilla::net::Http2Session::ReadSegmentsAgain(mozilla::net::nsAHttpSegmentReader*, unsigned int, unsigned int*, bool*)
(this=0x7f226f4e8800, reader=0x7f226f4d2e00, count=32768, countRead=0x7f228275b0f0, again=0x7f228275ad67)
at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/Http2Session.cpp:2991
#17 0x00007f228f7e2c2e in mozilla::net::Http2Session::ReadSegments(mozilla::net::nsAHttpSegmentReader*, unsigned int, unsigned int*)
(this=0x7f226f4e8800, reader=0x7f226f4d2e00, count=32768, countRead=0x7f228275b0f0) at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/Http2Session.cpp:3055
#18 0x00007f228f96debd in mozilla::net::TLSFilterTransaction::ReadSegments(mozilla::net::nsAHttpSegmentReader*, unsigned int, unsigned int*)
(this=0x7f226f4d2df0, aReader=0x7f226f335400, aCount=32768, outCountRead=0x7f228275b0f0) at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/TunnelUtils.cpp:428
#19 0x00007f228f81e892 in mozilla::net::nsAHttpTransaction::ReadSegmentsAgain(mozilla::net::nsAHttpSegmentReader*, unsigned int, unsigned int*, bool*)
(this=0x7f226f4d2df0, reader=0x7f226f335400, count=32768, countRead=0x7f228275b0f0, again=0x7f228275b0ef)
at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/nsAHttpTransaction.h:99
#20 0x00007f228fa15592 in mozilla::net::nsHttpConnection::OnSocketWritable() (this=0x7f226f335400) at /mnt/work/opt/moz/hg-central-2/netwerk/protocol/http/nsHttpConnection.cpp:2097
#21 0x00007f228fa195cf in mozilla::net::nsHttpConnection::OnOutputStreamReady(nsIAsyncOutputStream*) (this=0x7f226f335400, out=0x7f226f48b900)

Comment 7

[Michal Novotny [:michal]]

Attachment: Bug 1601712 - Avoid notifying callback directly from InputStreamShim::AsyncWait()

Comment 8

[Michal Novotny [:michal]]

Comment on attachment 9119285 [details]
Bug 1601712 - Avoid notifying callback directly from InputStreamShim::AsyncWait()

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I don't know.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: 72
• If not all supported branches, which bug introduced the flaw?: Bug 1528850
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: This patch should apply cleanly to 72.
• How likely is this patch to cause regressions; how much testing does it need?: Risk of causing a regression is probably low. The patch is simple and was tested locally using the fuzzing testcase.

Comment 9

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

Comment on attachment 9119285 [details]
Bug 1601712 - Avoid notifying callback directly from InputStreamShim::AsyncWait()

I'm going to say approved primarily based on low affected users of requiring HTTP2 proxied via HTTP2.

Comment 10

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

https://hg.mozilla.org/integration/autoland/rev/cc9af538369ef99f39340612b19ff98695ad0f03
https://hg.mozilla.org/mozilla-central/rev/cc9af538369e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 12

[Michal Novotny [:michal]]

Comment on attachment 9119285 [details]
Bug 1601712 - Avoid notifying callback directly from InputStreamShim::AsyncWait()

Beta/Release Uplift Approval Request

• User impact if declined: Possible UAF when HTTP2 is tunneled over HTTP2
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: It's not easy to reproduce. The fix was tested using the provided fuzzing testcase.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The risk is probably low because the change is quite simple. But the code is a bit messy so it's really hard to say.
• String changes made/needed: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9119285 [details]
Bug 1601712 - Avoid notifying callback directly from InputStreamShim::AsyncWait()

Fixes a Necko sec bug. Approved for 73.0b5.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9947a9139a88