1328456 - disallow empty token names as the argument to nsIPK11TokenDB.findTokenByName

Status: RESOLVED FIXED

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

Description

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

nsIPK11TokenDB.findTokenByName("") is shorthand for nsIPK11TokenDB.getInternalKeyToken() which is a little silly as it's not all that much shorter and it's a bit unexpected due to not being documented at all. We should just enforce that findTokenByName take a non-empty string and change existing code to call getInternalKeyToken.

Comment 1

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

Attachment: bug 1328456 - disallow empty token names for nsIPK11TokenDB.findTokenByName

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

Comment 2

[:Cykesiopka]

Comment on attachment 8824218 [details]
bug 1328456 - disallow empty token names for nsIPK11TokenDB.findTokenByName

https://reviewboard.mozilla.org/r/102726/#review103486

Looks good.

::: security/manager/pki/resources/content/resetpassword.xul:11
(Diff revision 1)
>  <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
>  
>  <!DOCTYPE dialog SYSTEM "chrome://pippki/locale/pippki.dtd">
>  
>  <dialog id="reset_password" title="&resetPassword.title;"
>    xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"      

Would be nice to get rid of the trailing whitespace while you're here.

::: security/manager/ssl/nsIPK11TokenDB.idl:38
(Diff revision 1)
>    /*
> -   * Find a token by name
> +   * Find a token by name. Throws NS_ERROR_FAILURE if no token with the given
> +   * name can be found.
> +   * @param tokenName a string identifying the name of the token. Must be
> +   *                  non-empty.
> +   * @return nsIPK11Token a token with the given name

No harm in doing so, but we probably don't need to repeat the return type.

Comment 3

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

Comment on attachment 8824218 [details]
bug 1328456 - disallow empty token names for nsIPK11TokenDB.findTokenByName

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

Comment 4

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

Thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1b501863c27
disallow empty token names for nsIPK11TokenDB.findTokenByName r=Cykesiopka

Comment 6

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

Try looked good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c0145e52eb6
Also, this could have an add-on compatibility impact. The fix should be fairly obvious, however (just use getInternalKeyToken).

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f1b501863c27