Crash in [@ mozilla::net::nsSocketTransportService::SocketContext::TimeoutIn]
Categories
(Core :: Networking, defect, P2)
Tracking
()
People
(Reporter: u608768, Assigned: jesup)
Details
(Keywords: crash, csectype-wildptr, sec-high, Whiteboard: [necko-triaged] [necko-priority-queue][post-critsmash-triage][adv-main112+r][adv-esr102.10+r])
Crash Data
Attachments
(1 file)
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr102+
tjr
:
sec-approval+
|
Details | Review |
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?
Updated•3 years ago
|
Comment 1•2 years ago
|
||
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.
Assignee | ||
Comment 2•2 years ago
|
||
Wildptr crashes
Updated•2 years ago
|
Updated•2 years ago
|
Comment 3•2 years ago
|
||
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•2 years ago
|
||
We'll revisit in the necko triage meetings next week.
Comment 5•2 years ago
|
||
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.
Waiting for additional crash reports to identify actionable investigations.
Assignee | ||
Comment 8•1 year ago
|
||
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.)
This bug has been added to Necko priority queue and we will be working on it shortly.
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 10•1 year ago
|
||
I plan to look into this bug this week
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 11•1 year ago
|
||
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.
Assignee | ||
Comment 12•1 year ago
|
||
Assignee | ||
Comment 13•1 year ago
|
||
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
Assignee | ||
Comment 14•1 year ago
•
|
||
AttachSocket() does a manual addref if it succeeds, and DetachSocket() does a manual deref.... not good.
Assignee | ||
Comment 15•1 year ago
|
||
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•1 year ago
|
||
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•1 year ago
|
||
Comment on attachment 9319812 [details]
Bug 1720594: Add an Assertion r=#necko-reviewers
Approved to land and uplift
Updated•1 year ago
|
Comment 18•1 year ago
|
||
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•1 year ago
|
||
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
towontfix
.
For more information, please visit auto_nag documentation.
Comment 20•1 year ago
|
||
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, ...)
Updated•1 year ago
|
Updated•1 year ago
|
Comment 22•1 year ago
|
||
Comment 23•1 year ago
|
||
Please nominate this for Beta and ESR102 approval when you get a chance.
Updated•1 year ago
|
Assignee | ||
Comment 24•1 year ago
|
||
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
Assignee | ||
Comment 25•1 year ago
|
||
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•1 year ago
|
||
Comment on attachment 9319812 [details]
Bug 1720594: Add an Assertion r=#necko-reviewers
Approved for 112.0b8
Comment 27•1 year ago
|
||
uplift |
Comment 28•1 year ago
|
||
Comment on attachment 9319812 [details]
Bug 1720594: Add an Assertion r=#necko-reviewers
Approved for 102.10esr.
Comment 29•1 year ago
|
||
uplift |
Updated•1 year ago
|
Updated•1 year ago
|
Updated•6 months ago
|
Description
•