The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 53

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

({addon-compat})

Trunk
mozilla53
addon-compat
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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

Updated

3 months ago
Whiteboard: [psm-assigned]
Comment hidden (mozreview-request)
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.
Attachment #8819460 - Flags: review?(MattN+bmo) → review+

Comment 3

3 months ago
mozreview-review
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.
Attachment #8819460 - Flags: review?(cykesiopka.bmo)

Comment 4

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

https://reviewboard.mozilla.org/r/99222/#review99880
Attachment #8819460 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 5

3 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 7

3 months ago
mozreview-review
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.
Attachment #8819460 - Flags: review?(gps) → review+

Comment 8

3 months ago
mozreview-review
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.
Attachment #8819460 - Flags: review?(cykesiopka.bmo) → review+
(Assignee)

Comment 9

3 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 11

3 months ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f732b38a5a2
add nsIPK11Token.hasPassword to replace unnecessary uses of nsIPKCS11Slot.status r=Cykesiopka,gps,MattN,sebastian
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.
Keywords: addon-compat

Comment 13

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f732b38a5a2
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.