1329360 - avoid some NSS functions that internally use PK11_GetInternalKeySlot

Status: RESOLVED FIXED

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

Description

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

CERT_AddTempCertToPerm and CERT_ImportCerts (when called with keepCerts=true)
internally use PK11_GetInternalKeySlot. The current plan for making NSS always
available involves initializing it in memory-only mode and later opening the
user's certificate and key databases. Doing so means that
PK11_GetInternalKeySlot will not return the right token, so we can't rely on
functions that make use of it internally. For now we'll simply use equivalent
functions that take an explicit PK11SlotInfo argument and pass in the current
internal token. A later patch will change all places where PSM and Gecko use the
internal token to use the correct token.

(See https://wiki.mozilla.org/Security/CryptoEngineering/Platform_Use_of_NSS )

Comment 1

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

Attachment: bug 1329360 - avoid some NSS functions that internally use PK11_GetInternalKeySlot

CERT_AddTempCertToPerm and CERT_ImportCerts (when called with keepCerts=true)
internally use PK11_GetInternalKeySlot. The current plan for making NSS always
available involves initializing it in memory-only mode and later opening the
user's certificate and key databases. Doing so means that
PK11_GetInternalKeySlot will not return the right token, so we can't rely on
functions that make use of it internally. For now we'll simply use equivalent
functions that take an explicit PK11SlotInfo argument and pass in the current
internal token. A later patch will change all places where PSM and Gecko use the
internal token to use the correct token.

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

Comment 2

[:Cykesiopka]

Comment on attachment 8826660 [details]
bug 1329360 - avoid some NSS functions that internally use PK11_GetInternalKeySlot

https://reviewboard.mozilla.org/r/104566/#review105426

Cool - glad to see us moving towards something simpler.

::: security/manager/ssl/nsNSSCertificateDB.cpp:537
(Diff revision 1)
> -  int i = 0;
> -  for (CERTCertListNode *chainNode = CERT_LIST_HEAD(certChain);
> -       !CERT_LIST_END(chainNode, certChain);
> +  if (encounteredFailure) {
> +    PR_SetError(savedErrorCode, 0);
> +    return SECFailure;
> -       chainNode = CERT_LIST_NEXT(chainNode), i++) {
> -    rawArray[i] = &chainNode->cert->derCert;
>    }
> -  SECStatus srv = CERT_ImportCerts(CERT_GetDefaultCertDB(), usage, chainLen,
> -                                   rawArray, nullptr, true, caOnly, nullptr);
>  
> -  PORT_Free(rawArray);
> +  return SECSuccess;

Nit: I think we can just return `savedErrorCode` instead, and maybe update the callers to propagate the return value via `GetXPCOMFromNSSError()`.

Comment 3

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

Comment on attachment 8826660 [details]
bug 1329360 - avoid some NSS functions that internally use PK11_GetInternalKeySlot

https://reviewboard.mozilla.org/r/104566/#review105426

Thanks!

> Nit: I think we can just return `savedErrorCode` instead, and maybe update the callers to propagate the return value via `GetXPCOMFromNSSError()`.

I don't know if this is what you meant, but I ended up having ImportCertsIntoPermanentStorage return an nsresult (obtained via GetXPCOMFromNSSError) since its callers all return nsresult anyway.

Comment 4

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

Comment on attachment 8826660 [details]
bug 1329360 - avoid some NSS functions that internally use PK11_GetInternalKeySlot

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6d89beea427
avoid some NSS functions that internally use PK11_GetInternalKeySlot r=Cykesiopka

Comment 6

[Carsten Book [:Tomcat]]

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