1389664 - have PSM always initialize the user's pin to the empty string if necessary at startup

Status: RESOLVED FIXED

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

Description

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

Currently in NSS_DISABLE_DBM builds, we have a method attemptToLogInWithDefaultPassword() that we call as-needed that sets the database password to the empty string if NSS thinks this is necessary (and this is necessary to get tests to pass on Android - see bug 978120). We were never entirely confident in this solution, so it remained rather ad-hoc. After some investigation, it appears that this does do the correct thing with respect to behavior compatibility with the old database. However, it would be best to move this to a more logical, centralized location (with the added benefit that we only attempt this initialization once per run). Also, we can take away the #ifdefs and unify the code a bit.

Comment 1

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

Attachment: bug 1389664 - centralize on-demand empty pin initialization of the user's NSS database

The sqlite-backed NSS database implementation requires explicitly setting some
kind of pin (password, really). To maintain behavior compatibility with the old
database implementation, we set the pin to the empty string as necessary.
Previously this would only happen on Android (NSS_DISABLE_DBM builds), but
because we're moving towards using the sqlite-backed implementation on all
platforms, we should enable this code everywhere and move it to a more central
location.

This also fixes some now-unnecessary test behavior.

Review commit: https://reviewboard.mozilla.org/r/167742/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167742/

Comment 2

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8896493 [details]
bug 1389664 - centralize on-demand empty pin initialization of the user's NSS database

https://reviewboard.mozilla.org/r/167742/#review173300

::: security/certverifier/NSSCertDBTrustDomain.cpp:1263
(Diff revision 1)
> +    // true. For the SQL DB, we need to set a password or we won't be able to
> +    // import any certificates or change trust settings.
> +    // In gtests, NSS may have already been initialized in no-DB mode. Oddly,
> +    // PK11_NeedUserInit will return true, but PK11_InitPin will fail. Work
> +    // around this by confirming that we don't have a read-only internal slot.
> +    // This is bug 1389570.

We should probably get bug 1389570 fixed before landing this and not do the workaround. I'll look at bug 1389570 later and see what I can do.

Comment 3

[J.C. Jones [:jcj] (he/they)]

Hmmm... Keeler: Since so much of this is changing pk11tokendb.initPassword to pk11tokendb.changePassword, should initPassword get changed in some way, too?

Comment 4

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8896493 [details]
bug 1389664 - centralize on-demand empty pin initialization of the user's NSS database

https://reviewboard.mozilla.org/r/167742/#review174550

Other than my comment, it looks pretty good. I agree with Franziskus about the change at NSS landing first though.

::: security/manager/ssl/tests/unit/test_certDB_import_with_master_password.js:111
(Diff revision 1)
>  
>    // Set a master password.
>    let tokenDB = Cc["@mozilla.org/security/pk11tokendb;1"]
>                    .getService(Ci.nsIPK11TokenDB);
>    let token = tokenDB.getInternalKeyToken();
> -  token.initPassword("password");
> +  token.changePassword("", "password");

It seems to me that we might want to fix whatever prompted us to have to not use `initPassword` here, because it's non-obvious that on a fresh profile we should need to `changePassword` from the empty string.

Perhaps we can just rework `initPassword` in a follow-up to be syntactic sugar for calling `changePassword('', aNewPassword)` ?

Comment 5

[Franziskus Kiefer [:franziskus]]

NSS tip with the fix for bug 1389570 landed on inbound.

Comment 6

[Franziskus Kiefer [:franziskus]]

Or maybe not. I had to back out again. I have to fix all those tests first :(

Comment 7

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

(clearing reviews for now)

Comment 8

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

Comment on attachment 8896493 [details]
bug 1389664 - centralize on-demand empty pin initialization of the user's NSS database

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167742/diff/1-2/

Comment 9

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8896493 [details]
bug 1389664 - centralize on-demand empty pin initialization of the user's NSS database

https://reviewboard.mozilla.org/r/167742/#review177022

This looks good to me now.

Comment 10

[:Cykesiopka]

Comment on attachment 8896493 [details]
bug 1389664 - centralize on-demand empty pin initialization of the user's NSS database

https://reviewboard.mozilla.org/r/167742/#review177498

Looks good.

Comment 11

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

Thanks!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de65f4720642d20af1036df4e3c44043ce60907a

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/192a101ff358
centralize on-demand empty pin initialization of the user's NSS database r=Cykesiopka,jcj

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/192a101ff358