Closed Bug 1742617 Opened 1 year ago Closed 1 year ago

Crash in [@ nsDocShell::MaybeFixBadCertDomainErrorURI]

Categories

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

Unspecified
Windows
defect

Tracking

()

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

People

(Reporter: aryx, Assigned: djackson)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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

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: https://searchfox.org/mozilla-central/rev/553bf8428885dbd1eab9b63f71ef647f799372c2/docshell/base/nsDocShell.cpp#5939

Flags: needinfo?(djackson)
Assignee: nobody → djackson
Status: NEW → ASSIGNED

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 djackson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c34a71432987
Add null pointer check for missing cert in MaybeFixBadCertDomainErrorURI r=keeler
Status: ASSIGNED → RESOLVED
Closed: 1 year 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.