1281569 - Remove unnecessary step of converting nsIX509Certs to Raw DER just to create a CERTCertificate in nsNSSCertificateDB

Status: RESOLVED FIXED

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

Description

[:Cykesiopka]

There are a few instances in the code base where this pattern appears:
> SECItem der;
> rv = cert->GetRawDER(&der.len, (uint8_t **)&der.data);
> [...]
> free(der.data);

While this code is correct (GetRawDER() is an XPCOM function, so callers should be using free()), it is confusing to see free() used on SECItems.

We can introduce a new method, getRawDERAsSECItem(), that allows this confusing pattern to be eliminated.

Comment 1

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

From what I've found, the particularly problematic instances are in nsNSSCertificateDB.cpp where the code seems to use the raw DER to create a new CERTCertificate. Presumably this predates nsIX509Cert::GetCert, which I think is what we should be using in these cases. Thoughts?

Comment 2

[:Cykesiopka]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #1)
> From what I've found, the particularly problematic instances are in
> nsNSSCertificateDB.cpp where the code seems to use the raw DER to create a
> new CERTCertificate. Presumably this predates nsIX509Cert::GetCert, which I
> think is what we should be using in these cases. Thoughts?

Nice catch - I agree.

Comment 3

[:Cykesiopka]

Attachment: Bug 1281569 - Remove unnecessary step of converting nsIX509Certs to Raw DER just to create a CERTCertificate in nsNSSCertificateDB.

There are a few places in nsNSSCertificateDB.cpp where the following is done:
1. GetRawDER() is called on a nsIX509Cert to obtain the DER representation of
   the cert.
2. The DER is used to construct a CERTCertificate for use with NSS functions.

This step of converting to the DER is unnecessary, since GetCert() will provide
an already constructed CERTCertificate.

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

Comment 4

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

https://reviewboard.mozilla.org/r/62276/#review59244

Great - r=me

Comment 5

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

Comment on attachment 8767912 [details]
Bug 1281569 - Remove unnecessary step of converting nsIX509Certs to Raw DER just to create a CERTCertificate in nsNSSCertificateDB.

https://reviewboard.mozilla.org/r/62278/#review59246

Comment 6

[:Cykesiopka]

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16d6844de673

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ae66227417f
Remove unnecessary step of converting nsIX509Certs to Raw DER just to create a CERTCertificate in nsNSSCertificateDB. r=keeler

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ae66227417f