1334694 - remove token arguments from nsIX509CertDB.importPKCS12File and exportPKCS12File

Status: RESOLVED FIXED

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

Description

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

As far as I can tell, the token arguments to nsIX509CertDB.importPKCS12File and exportPKCS12File are always null (even in add-ons[0]). Furthermore, exportPKCS12File only uses the argument to make sure the user is logged in to the token that the given certificates are presumably on. However, there is no guarantee that the certificates are on that token. More to the point, if the user is not logged in to the token the certificates are on, the platform will ask the user to log in anyway, so the argument is unnecessary in that case.

[0] There is one add-on where this argument isn't null, but it appears to be using ctypes to call PK11_GetInternalKeySlot directly. It's not clear that this works.

Comment 1

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

Attachment: bug 1334694 - remove token arguments from nsIX509CertDB.importPKCS12File and exportPKCS12File

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

Comment 2

[:Cykesiopka]

Comment on attachment 8831354 [details]
bug 1334694 - remove token arguments from nsIX509CertDB.importPKCS12File and exportPKCS12File

https://reviewboard.mozilla.org/r/107912/#review109166

Ripping out nsPKCS12Blob related code always makes me happy.

::: security/manager/ssl/nsNSSCertificateDB.cpp:35
(Diff revision 1)
>  #include "nsNSSCertTrust.h"
>  #include "nsNSSCertificate.h"
>  #include "nsNSSComponent.h"
>  #include "nsNSSHelper.h"
>  #include "nsNSSShutDown.h"
>  #include "nsPK11TokenDB.h"

Nit: Looks like we can get rid of this include as well.

::: security/manager/ssl/nsPKCS12Blob.h:45
(Diff revision 1)
>    nsCOMPtr<nsIInterfaceRequestor> mUIContext;
>  
>    // local helper functions
>    nsresult getPKCS12FilePassword(SECItem *);
>    nsresult newPKCS12FilePassword(SECItem *);
>    nsresult inputToDecoder(SEC_PKCS12DecoderContext *, nsIFile *);

Looks like we should include p12.h for `SEC_PKCS12DecoderContext` and other related types in nsPKCS12Blob.cpp.

::: security/manager/ssl/nsPKCS12Blob.cpp
(Diff revision 1)
> -#include "nsIDirectoryService.h"
>  #include "nsIFile.h"
>  #include "nsIInputStream.h"
> -#include "nsKeygenHandler.h" // For GetSlotWithMechanism
>  #include "nsNSSCertificate.h"
> -#include "nsNSSComponent.h"

I believe we need to keep this because `nickname_collision()` references `NS_NSSCOMPONENT_CID`.

::: security/manager/ssl/nsPKCS12Blob.cpp
(Diff revision 1)
>  #include "nsIFile.h"
>  #include "nsIInputStream.h"
> -#include "nsKeygenHandler.h" // For GetSlotWithMechanism
>  #include "nsNSSCertificate.h"
> -#include "nsNSSComponent.h"
> -#include "nsNSSHelper.h"

Looks like this file still uses `PipUIContext`, so we should keep the include.

Comment 3

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

Comment on attachment 8831354 [details]
bug 1334694 - remove token arguments from nsIX509CertDB.importPKCS12File and exportPKCS12File

https://reviewboard.mozilla.org/r/107912/#review109166

Thanks!

> Nit: Looks like we can get rid of this include as well.

Cool.

> Looks like we should include p12.h for `SEC_PKCS12DecoderContext` and other related types in nsPKCS12Blob.cpp.

Ok - fixed.

> I believe we need to keep this because `nickname_collision()` references `NS_NSSCOMPONENT_CID`.

Fixed.

> Looks like this file still uses `PipUIContext`, so we should keep the include.

Fixed.

Comment 4

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

Comment on attachment 8831354 [details]
bug 1334694 - remove token arguments from nsIX509CertDB.importPKCS12File and exportPKCS12File

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

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02d4b78c189e

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da9c1e0ddf0f
remove token arguments from nsIX509CertDB.importPKCS12File and exportPKCS12File r=Cykesiopka

Comment 7

[Carsten Book [:Tomcat]]

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