1813618 - nsNSSComponent::HasActiveSmartCards() incorrectly returning true on machine with no 3rd party PKCS11 modules installed

Status: RESOLVED FIXED

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

Description

[Andrew Creskey [:acreskey]]

This is the behaviour that I'm seeing:

1. Launch Firefox Nightly, new profile
2. Observe the output of "nsNSSComponent::HasActiveSmartCards()"
3. On my newish MacBook Pro, this function returns true, which is disabling the performance feature preconnect / speculative connect.

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1579023

Notes: I have not (knowingly) installed any PKCS#11 modules, although it's certainly possible that one was installed with tools?

Comment 1

[Andrew Creskey [:acreskey]]

Attachment: SECModModule.png

In stepping into HasActiveSmartCards, this is the SECMODModule
https://searchfox.org/mozilla-central/rev/861fb9abfcaff123aab45f6ac56a0106b116dc14/security/manager/ssl/nsNSSComponent.cpp#864

Comment 2

[Andrew Creskey [:acreskey]]

Attachment: Slot.png

And this is the slot for which PK11_IsFriendly() returns false.

Comment 3

[Andrew Creskey [:acreskey]]

Please let me know if there's anything I can debug locally to pin point the problem.

Comment 4

[John Schanck [:jschanck]]

I'm also seeing PK11_IsFriendly return false on the builtin roots module, which we always load.

I don't see any patches that would have changed that return value recently. Is it possible that this has been broken since Bug 1579023 landed?

Comment 5

[John Schanck [:jschanck]]

Attachment: Bug 1813618 - add 'public certificates token' profile to builtins module. r=keeler

Comment 6

[Andrew Creskey [:acreskey]]

(In reply to John Schanck [:jschanck] from comment #4)

I'm also seeing PK11_IsFriendly return false on the builtin roots module, which we always load.

I don't see any patches that would have changed that return value recently. Is it possible that this has been broken since Bug 1579023 landed?

That seems very possible.
I'm going to spend a bit of time looking at the scope of this, and I'll update if I find anything.

Comment 7

[Andrew Creskey [:acreskey]]

Seeing this behaviour on Windows and Linux.
Will check Android. [yes - same behaviour]

Comment 8

[Andrew Creskey [:acreskey]]

Actually, this seems to be working fine on android, I only see desktop platforms being affected.

Comment 9

[John Schanck [:jschanck]]

Thanks for looking into it. It turns out that, while my patch is sufficient on Linux, there are problems with other automatically loaded modules on Windows and OSX. So we're working on a different approach.

Comment 10

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

Set release status flags based on info from the regressing bug 1579023

Comment 11

[Donal Meehan [:dmeehan]]

:jschanck do you plan on landing this patch anytime soon?
Will it land in for 112?

Comment 12

[John Schanck [:jschanck]]

We determined that my patch would only fix the issue for a fraction of users, so we're going with a different approach. It might not be done for 112.

Comment 13

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

Attachment: Bug 1813618 - buffer client auth certificate selection UI for speculative connections r?valentin,jschanck!

When necko makes a speculative connection, the peer may ask for a client
authentication certificate. This patch makes it so that when this happens,
no certificate selection UI will be shown until the connection is claimed (as
in, it is no longer merely speculative).

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3155d983a779
buffer client auth certificate selection UI for speculative connections r=valentin,jschanck,necko-reviewers,kershaw

Comment 15

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/3155d983a779

Comment 16

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

The patch landed in nightly and beta is affected.
:keeler, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox113 to wontfix.

For more information, please visit auto_nag documentation.

Comment 17

[Andrew Creskey [:acreskey]]

Because we'd like to measure the performance of this change and since it's been a longstanding issue I would strongly prefer that we not uplift this right now.

Comment 18

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

Agreed - we shouldn't uplift this yet.

Comment 19

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #14)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3155d983a779
buffer client auth certificate selection UI for speculative connections
r=valentin,jschanck,necko-reviewers,kershaw

== Change summary for alert #38158 (as of Fri, 21 Apr 2023 11:35:26 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
38%	tp5n main_startup_fileio	windows11-64-2009-shippable-qr	e10s fission stylo webrender-sw	511,556.21 -> 316,187.92
33%	tp5n main_startup_fileio	windows11-64-2009-shippable-qr	e10s fission stylo webrender-sw	522,650.67 -> 348,953.92
32%	tp5n main_startup_fileio	windows11-64-2009-shippable-qr	e10s fission stylo webrender	519,490.00 -> 351,258.58

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=38158

Comment 20

[Andrew Creskey [:acreskey]]

We were not expecting significant improvements to be picked up by the performance tests in CI since they run against an https proxy (and also since speculative connects are explicitly disabled on proxied connects in some cases).
The above alert shows an improvement in file io counts on an older startup pageload test.

However we are seeing measurable improvements by looking at the pageload_event before and after this fix landed.
Discussed here.

Comment 21

[Pascal Chevrel:pascalc]

Backed out in mozillla-release for causing bug 1835103
I am marking 114 as wontfix as this was backed out and 115 as fixed as this is still in central

https://hg.mozilla.org/releases/mozilla-release/rev/defb5e07935c