1324071 - add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status

Status: RESOLVED FIXED

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

Description

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

This is a common pattern to determine if the user has set a master password:

  let secmodDB = Cc["@mozilla.org/security/pkcs11moduledb;1"]
                   .getService(Ci.nsIPKCS11ModuleDB);
  let slot = secmodDB.findSlotByName("");
  let mpEnabled = slot &&
                  slot.status != Ci.nsIPKCS11Slot.SLOT_UNINITIALIZED &&
                  slot.status != Ci.nsIPKCS11Slot.SLOT_READY;

There are a few reasons why this is not optmial:

1. It's not apparent from reading this that findSlotByName("") returns the internal key slot. This is bad from a maintenance perspective in that if you have to modify code that involves the internal key slot, it's not clear you have to investigate code like this.
2. We're really interested in the token itself, rather than the slot it's on, so using nsIPKCS11Slot is unintuitive.
3. nsIPKCS11Slot.status is an unclear API, particularly in this case.

We should just expose an attribute nsIPK11Token.hasPassword and forbid findSlotByName from taking an empty string as its parameter.

Comment 1

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

Attachment: bug 1324071 - add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status

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

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8819460 [details]
bug 1324071 - add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status

https://reviewboard.mozilla.org/r/99222/#review99574

browser/ and toolkit/ and services/ changes look fine to me though I'm no expert in the slot properties.

Comment 3

[:Cykesiopka]

Comment on attachment 8819460 [details]
bug 1324071 - add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status

https://reviewboard.mozilla.org/r/99222/#review99588

I agree with the changes in general, but it looks like this needs a bit more work.

::: security/manager/pki/resources/content/changepassword.js:91
(Diff revision 1)
> -   var slot = secmoddb.findSlotByName(tokenName);
> -   if (slot) {
> +  let tokenDB = Cc["@mozilla.org/security/pk11tokendb;1"]
> +                  .getService(Ci.nsIPK11TokenDB);
> +  let token = tokenDB.getInternalKeyToken();
> +  if (token) {

Hmm, IINM this code is meant to handle any token, so this change will break setting PWs on other tokens.

::: security/manager/ssl/nsIPK11Token.idl:73
(Diff revision 1)
> +   * True if a password has been configured for this token, and false otherwise.
> +   * (Whether or not the user is currently logged in makes no difference.)
> +   * In particular, this can be used to determine if a user has set a master
> +   * password (if this is the internal key token).
> +   */
> +  readonly attribute boolean hasPassword;

We should probably have a PSM level test to make sure this works.

::: security/manager/ssl/nsPKCS11Slot.cpp:456
(Diff revision 1)
> +  if (name.IsEmpty()) {
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }

Looks like there's still a `findSlotByName("")` call in toolkit/components/passwordmgr/test/LoginTestUtils.jsm, which will make at least toolkit/components/passwordmgr/test/browser/browser_notifications_2.js fail.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8819460 [details]
bug 1324071 - add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status

https://reviewboard.mozilla.org/r/99222/#review99880

Comment 5

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

Comment on attachment 8819460 [details]
bug 1324071 - add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status

https://reviewboard.mozilla.org/r/99222/#review99588

Thanks!

> Hmm, IINM this code is meant to handle any token, so this change will break setting PWs on other tokens.

D'oh - I knew I had forgotten something.

> We should probably have a PSM level test to make sure this works.

Ok - I added some to test_pkcs11_token.js.

> Looks like there's still a `findSlotByName("")` call in toolkit/components/passwordmgr/test/LoginTestUtils.jsm, which will make at least toolkit/components/passwordmgr/test/browser/browser_notifications_2.js fail.

Good catch. Fixed.

Comment 6

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

Comment on attachment 8819460 [details]
bug 1324071 - add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status

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

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8819460 [details]
bug 1324071 - add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status

https://reviewboard.mozilla.org/r/99222/#review100752

I only looked at the services/ code and verified it is similar to what is being done in browser.js.

Comment 8

[:Cykesiopka]

Comment on attachment 8819460 [details]
bug 1324071 - add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status

https://reviewboard.mozilla.org/r/99222/#review101080

Looks good with the issue below addressed.

::: security/manager/pki/resources/content/changepassword.js:95
(Diff revision 2)
> +    token = pk11db.findTokenByName(tokenName);
> +  } else {
> +    token = pk11db.getInternalKeyToken();

Both `pk11db` should be `tokenDB` instead.

Comment 9

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

Comment on attachment 8819460 [details]
bug 1324071 - add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status

https://reviewboard.mozilla.org/r/99222/#review101080

Thanks for everyone's reviews!

> Both `pk11db` should be `tokenDB` instead.

Whoops - fixed.

Comment 10

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

Comment on attachment 8819460 [details]
bug 1324071 - add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f732b38a5a2
add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status r=Cykesiopka,gps,MattN,sebastian

Comment 12

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

I forgot to mention this could have an add-on compatibility impact: nsIPKCS11ModuleDB.findSlotByName no longer accepts an empty string. Implementation should use nsIPK11TokenDB.getInternalKeyToken in those cases.

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6f732b38a5a2

Comment 14

[Axel Grude [:realRaven]]

I wish I had know this (for my Addon QuickPasswords) now I have to deal with the fallout. Thanks for raising the bug though!