1810536 - Crashes in Http/3 code

Status: RESOLVED FIXED

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

Description

[Kershaw Chang [:kershaw]]

According to this search, we have some UAF crashes in our Http/3 code.

Comment 1

[Valentin Gosu [:valentin]]

Crash report: https://crash-stats.mozilla.org/report/index/bdd3b5ee-9b94-40ae-b7cc-8998f0230110

Reason: EXCEPTION_ACCESS_VIOLATION_WRITE

Top 10 frames of crashing thread:

9 	xul.dll 	core::mem::drop(neqo_glue::NeqoHttp3Conn*) 	/rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/mem/mod.rs:988 	inlined
9 	xul.dll 	neqo_glue::neqo_http3conn_release(neqo_glue::NeqoHttp3Conn*) 	netwerk/socket/neqo_glue/src/lib.rs:212 	cfi
10 	xul.dll 	mozilla::net::NeqoHttp3Conn::Release() 	netwerk/socket/neqo_glue/NeqoHttp3Conn.h:112 	inlined
10 	xul.dll 	mozilla::RefPtrTraits<mozilla::net::NeqoHttp3Conn>::Release(mozilla::net::NeqoHttp3Conn*) 	mfbt/RefPtr.h:50 	inlined
10 	xul.dll 	RefPtr<mozilla::net::NeqoHttp3Conn>::ConstRemovingRefPtrTraits<mozilla::net::NeqoHttp3Conn>::Release(mozilla::net::NeqoHttp3Conn*) 	mfbt/RefPtr.h:381 	inlined
10 	xul.dll 	RefPtr<mozilla::net::NeqoHttp3Conn>::~RefPtr() 	mfbt/RefPtr.h:81 	inlined
10 	xul.dll 	mozilla::net::Http3Session::~Http3Session() 	netwerk/protocol/http/Http3Session.cpp:299 	cfi
11 	xul.dll 	mozilla::net::Http3Session::Release() 	netwerk/protocol/http/Http3Session.cpp:58 	cfi
12 	xul.dll 	mozilla::RefPtrTraits<mozilla::net::Http3Session>::Release(mozilla::net::Http3Session*) 	mfbt/RefPtr.h:50 	inlined
12 	xul.dll 	RefPtr<mozilla::net::Http3Session>::ConstRemovingRefPtrTraits<mozilla::net::Http3Session>::Release(mozilla::net::Http3Session*) 	mfbt/RefPtr.h:381 	inlined
12 	xul.dll 	RefPtr<mozilla::net::Http3Session>::assign_assuming_AddRef(mozilla::net::Http3Session*) 	mfbt/RefPtr.h:69 	inlined
12 	xul.dll 	RefPtr<mozilla::net::Http3Session>::operator=(void*) 	mfbt/RefPtr.h:168 	inlined
12 	xul.dll 	mozilla::net::HttpConnectionUDP::CloseTransaction(mozilla::net::nsAHttpTransaction*, nsresult, bool) 	netwerk/protocol/http/HttpConnectionUDP.cpp:496 	cfi
13 	xul.dll 	mozilla::net::HttpConnectionUDP::OnQuicTimeoutExpired() 	netwerk/protocol/http/HttpConnectionUDP.cpp:527 	cfi

I think we might have a problem here:

https://searchfox.org/mozilla-central/rev/a8187e40b492dff78e3d3225e652cc06f447484b/netwerk/protocol/http/Http3Session.cpp#886-893

if (!mTimer ||
    NS_FAILED(mTimer->InitWithNamedFuncCallback(
        &HttpConnectionUDP::OnQuicTimeout, mUdpConn, aTimeout,
        nsITimer::TYPE_ONE_SHOT, "net::HttpConnectionUDP::OnQuicTimeout"))) {
  NS_DispatchToCurrentThread(
      NewRunnableMethod("net::HttpConnectionUDP::OnQuicTimeoutExpired",
                        mUdpConn, &HttpConnectionUDP::OnQuicTimeoutExpired));
}

InitWithNamedFuncCallback takes a pointer to mUdpConn.
By the time we call the timer, mUdpConn might have been released.

Comment 2

[Valentin Gosu [:valentin]]

Attachment: Bug 1810536 - Use NS_NewTimerWithCallback to create timer r=#necko

Comment 3

[Valentin Gosu [:valentin]]

Comment on attachment 9312479 [details]
Bug 1810536 - Use NS_NewTimerWithCallback to create timer r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not trivial but I think this is exploitable.
• 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?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Grafts cleanly to esr102
• How likely is this patch to cause regressions; how much testing does it need?: There is a small risk of regressions: while the patch is relatively simple, it does keep a ref to the HttpConnectionUDP for longer than before which can change the order in which objects get released.
• Is Android affected?: Yes

Comment 4

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 9312479 [details]
Bug 1810536 - Use NS_NewTimerWithCallback to create timer r=#necko

Approved to land and request uplift

Comment 5

[Valentin Gosu [:valentin]]

Comment on attachment 9312479 [details]
Bug 1810536 - Use NS_NewTimerWithCallback to create timer r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: Potential UAF crash with HTTP/3 when a timeout occurs
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): While the patch is relatively simple, it does keep a ref to the HttpConnectionUDP for longer than before which can change the order in which objects get released.
• String changes made/needed:
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: Potential UAF crash with HTTP/3 when a timeout occurs
• Fix Landed on Version: 111
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): While the patch is relatively simple, it does keep a ref to the HttpConnectionUDP for longer than before which can change the order in which objects get released.

Comment 6

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

Use NS_NewTimerWithCallback to create timer r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/417a22f77ccaf71d33ffd2f6f54c387667808c9f
https://hg.mozilla.org/mozilla-central/rev/417a22f77cca

Comment 7

[Pascal Chevrel:pascalc]

Comment on attachment 9312479 [details]
Bug 1810536 - Use NS_NewTimerWithCallback to create timer r=#necko

Approved for 110 beta 5, thanks.

Comment 8

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/87ee446e3c28

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9312479 [details]
Bug 1810536 - Use NS_NewTimerWithCallback to create timer r=#necko

Approved for 102.8esr.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/678b4113ea15