Closed Bug 1742617 Opened 2 years ago Closed 2 years ago

Crash in [@ nsDocShell::MaybeFixBadCertDomainErrorURI]


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




97 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox94 --- wontfix
firefox95 --- wontfix
firefox96 --- wontfix
firefox97 --- fixed


(Reporter: aryx, Assigned: djackson)


(Keywords: crash, regression)

Crash Data


(1 file)

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

Crash report:


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

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.

Component: DOM: Navigation → Security: PSM

Dennis, do you have a moment to take care of this? Should just involve a null check on cert before calling GetRawDER here:

Flags: needinfo?(djackson)
Assignee: nobody → djackson

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

Flags: needinfo?(djackson) → needinfo?(dkeeler)

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?

Flags: needinfo?(dkeeler)

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...).

Priority: -- → P1

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.

Flags: needinfo?(dkeeler)
Flags: needinfo?(djackson)
Pushed by
Add null pointer check for missing cert in MaybeFixBadCertDomainErrorURI r=keeler
Closed: 2 years ago
Flags: needinfo?(dkeeler)
Flags: needinfo?(djackson)
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch

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.

Flags: needinfo?(djackson)
Flags: needinfo?(djackson)
You need to log in before you can comment on or make changes to this bug.