Closed Bug 1329360 Opened 6 years ago Closed 6 years ago

avoid some NSS functions that internally use PK11_GetInternalKeySlot

Categories

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

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: keeler, Assigned: keeler)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

bug 1329360 - avoid some NSS functions that internally use PK11_GetInternalKeySlot
59 bytes, text/x-review-board-request
Cykesiopka
: review+
Details 
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 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()`.
Attachment #8826660 - Flags: review?(cykesiopka.bmo) → review+
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.
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6d89beea427
avoid some NSS functions that internally use PK11_GetInternalKeySlot r=Cykesiopka
https://hg.mozilla.org/mozilla-central/rev/e6d89beea427
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.