1720594 - Crash in [@ mozilla::net::nsSocketTransportService::SocketContext::TimeoutIn]

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[u608768]

Crash report: https://crash-stats.mozilla.org/report/index/a56688b4-f42c-4be3-a0ca-725cd0210713

Reason: SIGSEGV /SEGV_MAPERR

Top 10 frames of crashing thread:

0 libxul.so mozilla::net::nsSocketTransportService::SocketContext::TimeoutIn const netwerk/base/nsSocketTransportService2.cpp:91
1 libxul.so mozilla::net::nsSocketTransportService::Run netwerk/base/nsSocketTransportService2.cpp:1127
2 libxul.so {virtual override thunk} 
3 libxul.so nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1146
4 libxul.so mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:300
5 libxul.so MessageLoop::Run ipc/chromium/src/base/message_loop.cc:306
6 libxul.so nsThread::ThreadFunc xpcom/threads/nsThread.cpp:392
7 libnspr4.so _pt_root nsprpub/pr/src/pthreads/ptthread.c:201
8 libpthread.so.0 start_thread 
9 libc.so.6 __GI___clone 

Null mHandler maybe?

Comment 1

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

Since the crash volume is low (less than 5 per week), the severity is downgraded to S3. Feel free to change it back if you think the bug is still critical.

For more information, please visit auto_nag documentation.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Wildptr crashes

Comment 3

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:valentin, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

We'll revisit in the necko triage meetings next week.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

https://crash-stats.mozilla.org/report/index/db23844f-48ae-44a4-ae76-24f260221023

0 	xul.dll 	mozilla::net::nsSocketTransportService::SocketContext::TimeoutIn(unsigned int) const 	netwerk/base/nsSocketTransportService2.cpp:93 	context
1 	xul.dll 	mozilla::net::nsSocketTransportService::SocketContext::IsTimedOut(unsigned int) const 	netwerk/base/nsSocketTransportService2.cpp:71 	inlined
1 	xul.dll 	mozilla::net::nsSocketTransportService::DoPollIteration(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>*) 	netwerk/base/nsSocketTransportService2.cpp:1407 	inlined
1 	xul.dll 	mozilla::net::nsSocketTransportService::Run() 	netwerk/base/nsSocketTransportService2.cpp:1180 	cfi
2 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1199 	cfi
3 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/threads/nsThreadUtils.cpp:465 	inlined
3 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:300 	cfi
4 	xul.dll 	MessageLoop::RunInternal() 	ipc/chromium/src/base/message_loop.cc:381 	inlined
4 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:374 	cfi
5 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:356 	cfi
6 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp:384 	cfi
7 	nss3.dll 	_PR_NativeRunThread(void*) 	nsprpub/pr/src/threads/combined/pruthr.c:399 	cfi
8 	nss3.dll 	pr_root(void*) 	nsprpub/pr/src/md/windows/w95thred.c:139 	cfi
9 	ucrtbase.dll 	thread_start<unsigned int (__cdecl*)(void*), 1> 		cfi
10 	kernel32.dll 	BaseThreadInitThunk 		cfi
--> 11 	mozglue.dll 	mozilla::interceptor::FuncHook<mozilla::interceptor::WindowsDllInterceptor<mozilla::interceptor::VMSharingPolicyShared>, void (*)(int, void*, void*)>::operator()(int&, void*&, void*&) const 	toolkit/xre/dllservices/mozglue/nsWindowsDllInterceptor.h:150 	inlined**
11 	mozglue.dll 	patched_BaseThreadInitThunk(int, void*, void*) 	toolkit/xre/dllservices/mozglue/WindowsDllBlocklist.cpp:586 	cfi
12 	ntdll.dll 	RtlUserThreadStart 	

A bunch of these have WindowsDllInterceptor on the stack. Makes me think there's some other software interaction with this.

Comment 6

[Greg Hess]

Waiting for additional crash reports to identify actionable investigations.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Review for actionability

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

We appear to be crashing on mHandler as a WildPtr. SocketContext::mHandler is a rawptr, so there's a non-zero chance we racing against the poll, or there's some odd error path that leaves it in the poll after it should be removed. Analysis of the ownership relationship and timing of mHandler should be done, and if this is safe (and needs to be a raw ptr) it should be documented. (In that case the crash would likely be due to someone UAF-writing to the SocketContext mHandler pointer.)

Comment 9

[Greg Hess]

This bug has been added to Necko priority queue and we will be working on it shortly.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

I plan to look into this bug this week

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

So as best I can tell, the poll (with SocketContext->mHandler) should be turned off via DetachSocket() before the nsSocketTransport dies, and my assumption is that somehow it isn't. We could set up an assertion to this affect to check it; it appears that mAttached tracks whether we have an active AttachSocket.

I still hate that it's using a raw ptr; we could also make it a refptr probably, and see if we see any leaks.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1720594: Add an Assertion r=#necko-reviewers

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

If this is the cause (and that seems possible), then this should take a controlled crash instead of UAFing.
I'll try a patch that makes it a RefPtr, but that might be more tricky to do without leaks

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

AttachSocket() does a manual addref if it succeeds, and DetachSocket() does a manual deref.... not good.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9319812 [details]
Bug 1720594: Add an Assertion r=#necko-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very hard
• 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?: trivial
• How likely is this patch to cause regressions; how much testing does it need?: None. It will catch things that would have crashed or done bad stuff. Base bug appears to be in the 1-10 crashes/week.
• Is Android affected?: Unknown

Comment 16

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

Today is the last day for uplifts, and given the details of this bug, we'll land it next cycle. Leaving the sec-approval flag for 12 days from now.

Comment 17

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

Comment on attachment 9319812 [details]
Bug 1720594: Add an Assertion r=#necko-reviewers

Approved to land and uplift

Comment 18

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

Add an Assertion r=necko-reviewers,kershaw,valentin
https://hg.mozilla.org/integration/autoland/rev/be11e4da5db397aeb7a3e909fb5192d007e16fa0
https://hg.mozilla.org/mozilla-central/rev/be11e4da5db3

Comment 19

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

The patch landed in nightly and beta is affected.
:jesup, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Comment 20

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

Landed: https://hg.mozilla.org/integration/autoland/rev/be11e4da5db397aeb7a3e909fb5192d007e16fa0

Backed out together with bug 1818998 for causing leakcheck failures:
https://hg.mozilla.org/integration/autoland/rev/11e6330ee0087beaec68da00bc61c30dbd4d16df

Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=RHIpi1zcTHCHZeTd0AdLsw.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=9a66f82632b9654596487b4c9d327b6141dcf685
Failure log: https://treeherder.mozilla.org/logviewer?job_id=409136019&repo=autoland

TEST-UNEXPECTED-FAIL | leakcheck | default 4432 bytes leaked (Mutex, PollableEvent, ReentrantMonitor, nsAStreamCopier, nsPipe, ...)

Comment 21

[Valentin Gosu [:valentin] (he/him)]

Accidentally cleared the flag.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/978db5a292c3

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9319812 [details]
Bug 1720594: Add an Assertion r=#necko-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Possible UAFs/wildptrs
• Is this code covered by automated tests?: Yes
• 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): Just adds an assertion
• String changes made/needed:
• Is Android affected?: Yes

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9319812 [details]
Bug 1720594: Add an Assertion r=#necko-reviewers

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Possible UAFs (low incidence rate)
• Fix Landed on Version: 113
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just an assertion

Comment 26

[Dianna Smith [:diannaS]]

Comment on attachment 9319812 [details]
Bug 1720594: Add an Assertion r=#necko-reviewers

Approved for 112.0b8

Comment 27

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/57890be5a24a

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9319812 [details]
Bug 1720594: Add an Assertion r=#necko-reviewers

Approved for 102.10esr.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/3fd3a867c47d