1136301 - if a nsIX509Cert / nsNSSCertificate is temporary, mCert->slot will be null, and this should be checked

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Code in nsNSSCertificate will pass mCert->slot to NSS functions (e.g. PK11_NeedLogin, etc.) that do not validate their input (i.e. check if slot is null). It appears that temporary CERTCertificates have a null slot (which makes sense). PSM needs to check for and handle these cases.

Comment 1

[Atul Kumar [:AtKr2]]

Can someone assign this bug to me? This will be my first bug.

Comment 2

[:Cykesiopka]

Hi Atul,

I've assigned the bug to you. If you need any help with the bug, feel free to ask and choose the "mentor" option from the "Need more information from" drop down below the comment box.

Have fun!

Comment 3

[Atul Kumar [:AtKr2]]

Attachment: null check for mCert->slot added in destructorSafeDestroyNSSReference & MarkForPermDeletion

As discussed with dkeeler on mail, I ran xpcshell tests for this patch from /secuity/manager/ssl/tests/unit after making changes. Summary of test run on 'Windows 8.1 64 bit' is as follows
Ran 72 tests
Expected results: 70
Unexpected results: 0
Skipped: 2 

Two functions were touched - 
private void 
nsNSSCertificate::destructorSafeDestroyNSSReference()
public nsresult
nsNSSCertificate::MarkForPermDeletion()

Below function is already checking for it
NS_IMETHODIMP
nsNSSCertificate::GetTokenName(nsAString& aTokenName)

Comment 4

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

Comment on attachment 8616045 [details] [diff] [review]
null check for mCert->slot added in destructorSafeDestroyNSSReference & MarkForPermDeletion

Review of attachment 8616045 [details] [diff] [review]:
-----------------------------------------------------------------

This is great! Functionally, the patch is perfect. However, this file sometimes uses an outdated code style that we should update in the places we're already modifying. r=me with those changes made.

::: security/manager/ssl/nsNSSCertificate.cpp
@@ +224,5 @@
>  
>    // make sure user is logged in to the token
>    nsCOMPtr<nsIInterfaceRequestor> ctx = new PipUIContext();
>  
> +  if (mCert->slot 

nit: there's a trailing space on this line we should delete. Also, while you did a perfect job of matching the existing style, the current style guidelines say to have && at the end of lines rather than the beginning. Additionally, I bet we can squeeze some of these statements onto the same line and still be under 80 characters, so this should probably look more like this:

if (mCert->slot && PK11_NeedLogin(mCert->slot) &&
    !PK11_NeedUserInit(mCert->slot) && !PK11_IsInternal(mCert->slot)) {

(note the opening { being on the same line as well)

Comment 5

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

Also, remember to set the user field in your commit (hg commit --user "name <email>" or specify a default in your .hgrc).

Comment 6

[Atul Kumar [:AtKr2]]

Attachment: 1136301-mCertSlotNullCheck.patch

Improved formatting as asked in review. Additionally updated formatting for the next line in function MarkForPermDeletion by bringing '{' to same line of the if statement. 
Ran the tests again and same result.

Comment 7

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

Comment on attachment 8616463 [details] [diff] [review]
1136301-mCertSlotNullCheck.patch

Review of attachment 8616463 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great - just a minor alignment nit. Also, for the r=whoever in the commit message, we usually use IRC nicknames, so go ahead and use r=keeler for that.

::: security/manager/ssl/nsNSSCertificate.cpp
@@ +225,5 @@
>    // make sure user is logged in to the token
>    nsCOMPtr<nsIInterfaceRequestor> ctx = new PipUIContext();
>  
> +  if (mCert->slot && PK11_NeedLogin(mCert->slot) &&
> +    !PK11_NeedUserInit(mCert->slot) && !PK11_IsInternal(mCert->slot)) {

nit: the first '!' in this line should line up with the first 'm' in the previous line.

Comment 8

[Atul Kumar [:AtKr2]]

Attachment: 1136301-mCertSlotNullCheck.patch

As requested, changed r=reviewer to reflect IRC nick and aligned the multi-line if statement.

Comment 9

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

Comment on attachment 8616839 [details] [diff] [review]
1136301-mCertSlotNullCheck.patch

Review of attachment 8616839 [details] [diff] [review]:
-----------------------------------------------------------------

One minor clarification, but otherwise looks good.

::: security/manager/ssl/nsNSSCertificate.cpp
@@ +226,5 @@
>    nsCOMPtr<nsIInterfaceRequestor> ctx = new PipUIContext();
>  
> +  if (mCert->slot && PK11_NeedLogin(mCert->slot) &&
> +      !PK11_NeedUserInit(mCert->slot) && !PK11_IsInternal(mCert->slot)) {
> +        if (SECSuccess != PK11_Authenticate(mCert->slot, true, ctx)) {

Sorry - I wasn't clear. This if statement should still only be indented two spaces from the level of the previous if.

Comment 10

[Atul Kumar [:AtKr2]]

Attachment: 1136301-mCertSlotNullCheck.patch

Made changes as suggested.

Comment 11

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

Comment on attachment 8617325 [details] [diff] [review]
1136301-mCertSlotNullCheck.patch

Review of attachment 8617325 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcfdee109765

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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