1671479 - Crash in [@ nsCOMPtr_base::assign_from_qi | nsBinaryOutputStream::WriteCompoundObject]

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Fairly frequent e5e5 UAF crash:

Crash report: https://crash-stats.mozilla.org/report/index/fbd76a17-b2ba-44ff-91bb-dbc4f0201013

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll nsCOMPtr_base::assign_from_qi xpcom/base/nsCOMPtr.cpp:46
1 xul.dll nsBinaryOutputStream::WriteCompoundObject xpcom/io/nsBinaryStream.cpp:291
2 xul.dll mozilla::psm::TransportSecurityInfo::Write security/manager/ssl/TransportSecurityInfo.cpp:242
3 xul.dll nsBinaryOutputStream::WriteCompoundObject xpcom/io/nsBinaryStream.cpp:327
4 xul.dll NS_SerializeToString netwerk/base/nsSerializationHelper.cpp:23
5 xul.dll mozilla::net::HttpChannelParent::UpdateAndSerializeSecurityInfo netwerk/protocol/http/HttpChannelParent.cpp:1946
6 xul.dll mozilla::net::HttpChannelParent::OnStartRequest netwerk/protocol/http/HttpChannelParent.cpp:1241
7 xul.dll mozilla::net::ParentChannelListener::OnStartRequest netwerk/protocol/http/ParentChannelListener.cpp:90
8 xul.dll WindowlessBrowser::GetCanGoBack xpfe/appshell/nsAppShellService.cpp:357
9 xul.dll mozilla::extensions::ChannelWrapper::RequestListener::OnStartRequest toolkit/components/extensions/webrequest/ChannelWrapper.cpp:1074

Comment 1

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

No crashes past 82.0b9, so maybe fixed?

Comment 2

[Valentin Gosu [:valentin]]

It seems to me that the problem is we're holding the TransportSecurityInfo::mMutex in TransportSecurityInfo::Write but not in TransportSecurityInfo::SetSucceededCertChain

The crashes maybe went away if the timings of the callers changed, but I think the bug is still there.
Maybe we want to hold the mutex for TransportSecurityInfo::GetSucceededCertChain too?

Comment 3

[Valentin Gosu [:valentin]]

Attachment: Bug 1671479 - Hold mutex when calling TransportSecurityInfo::Get/SetSucceededCertChain r=keeler

Comment 4

[Dana Keeler (she/her) [:keeler]]

Yes. Historically this class has not been rigorous in its thread-safety and we should address this.

Comment 5

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

Nika/decoder - this is an example of a bug that (with the right GUARDED_BY/etc) might be caught during static analysis.

Comment 6

[Valentin Gosu [:valentin]]

Comment on attachment 9181957 [details]
Bug 1671479 - Hold mutex when calling TransportSecurityInfo::Get/SetSucceededCertChain r=keeler

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unclear if the race can be triggered by remote code.
• 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 successfully on esr78
• How likely is this patch to cause regressions; how much testing does it need?: Since it only involves holding the mutex while accessing mSucceededCertChain this is unlikely to cause regressions unless there is timing sensitive code/race that might be affected by the delay holding the mutex.

Comment 7

[Christian Holler (:decoder)]

Valentin, is this code covered by our tests and if so, which tests might apply? I was wondering why we wouldn't see this in TSan if there is a data race present. Maybe also an existing suppression is hiding it or we have it on file somewhere but didn't recognize it as a problem? Also, I assume this is not Windows-only?

Comment 8

[Valentin Gosu [:valentin]]

Recent reports say it's Windows and OSX only.
From what I can tell from the stack traces I've looked at, they all seem to have:
mozilla::extensions::ChannelWrapper::RequestListener::OnStartRequest(nsIRequest*)
So presumably this only happens with a specific extension installed?

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 9181957 [details]
Bug 1671479 - Hold mutex when calling TransportSecurityInfo::Get/SetSucceededCertChain r=keeler

Approved to land and uplift

Comment 10

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

https://hg.mozilla.org/integration/autoland/rev/89863e9e44a0e7c24d00da4e01602e9b3cac98f3
https://hg.mozilla.org/mozilla-central/rev/89863e9e44a0

Comment 11

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

https://hg.mozilla.org/releases/mozilla-beta/rev/26787a074f6e

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/ceee64adc3b7

Comment 13

[BugBot [:suhaib / :marco]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 14

[Valentin Gosu [:valentin]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #13)

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Done 👍

Comment 15

[Valentin Gosu [:valentin]]

I see some of the signatures have moved over to crashing on mFailedCertChain. I should have fixed that one too. 🤦‍‍♂️
Presumably we can fix those in bug 1672120.