1742617 - Crash in [@ nsDocShell::MaybeFixBadCertDomainErrorURI]

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

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

Crash signature which starts with Firefox 94, all except one crash on Windows (out of 337 crashes, <2 crashes/installation)

Crash report: https://crash-stats.mozilla.org/report/index/f72c54f8-5953-4e14-92c6-de9cb0211123

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll static nsDocShell::MaybeFixBadCertDomainErrorURI docshell/base/nsDocShell.cpp:5908
1 xul.dll static nsDocShell::AttemptURIFixup docshell/base/nsDocShell.cpp:6127
2 xul.dll mozilla::net::DocumentLoadListener::OnStartRequest netwerk/ipc/DocumentLoadListener.cpp:2184
3 xul.dll mozilla::net::ParentChannelListener::OnStartRequest netwerk/protocol/http/ParentChannelListener.cpp:88
4 xul.dll mozilla::net::nsHttpChannel::CallOnStartRequest netwerk/protocol/http/nsHttpChannel.cpp:1630
5 xul.dll mozilla::net::nsHttpChannel::ContinueOnStartRequest2 netwerk/protocol/http/nsHttpChannel.cpp:6947
6 xul.dll mozilla::net::nsHttpChannel::OnStartRequest netwerk/protocol/http/nsHttpChannel.cpp:6884
7 xul.dll nsInputStreamPump::OnInputStreamReady netwerk/base/nsInputStreamPump.cpp:374
8 xul.dll nsInputStreamReadyEvent::Run xpcom/io/nsStreamUtils.cpp:94
9 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:770

Comment 1

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

Almost no crashes on weekends.

Comment 2

[Andrew McCreight [:mccr8]]

It looks like the crash is a null deref on this line:
rv = cert->GetRawDER(certBytes);

I guess cert is null. This could happen if mServerCert is null in TransportSecurityInfo::GetServerCert, I guess?

This code lives in nsDocShell, but this particular chunk of code looks very PSM-y so I'll move it there. There's nothing that is obviously related to me that has changed recently in this chunk of code or in TransportSecurityInfo.

Comment 3

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

Dennis, do you have a moment to take care of this? Should just involve a null check on cert before calling GetRawDER here: https://searchfox.org/mozilla-central/rev/553bf8428885dbd1eab9b63f71ef647f799372c2/docshell/base/nsDocShell.cpp#5939

Comment 4

[Dennis Jackson]

Attachment: Bug 1742617 - Add null pointer check for missing cert in MaybeFixBadCertDomainErrorURI r=keeler

Comment 5

[Dennis Jackson]

Minimal patch attached. For this type of bug, is it worth me spending some time looking for a root cause or reproducible test case?

Comment 6

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

The root cause is the reviewer missed it (or maybe the API should return an error when the certificate isn't available, but that would require more changes elsewhere to handle potentially throwing an exception when it gets called from JS).

You could look for a reproducible test case, but it might be more effort than benefit. My guess is something to do with session resumption and the external TLS token cache being cleared?

Comment 7

[Andrew McCreight [:mccr8]]

For what it is worth, it looks like the other call sites null check the cert return value (some of them don't even check the rv...).

Comment 8

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

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:djackson, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c34a71432987
Add null pointer check for missing cert in MaybeFixBadCertDomainErrorURI r=keeler

Comment 10

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/c34a71432987

Comment 11

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

The patch landed in nightly and beta is affected.
:djackson, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.