Closed Bug 1281569 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

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.
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?
(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.
Summary: Add getRawDERAsSECItem() to nsIX509Cert to avoid confusing mix of using free() with SECItems → 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/
Attachment #8767912 - Flags: review?(dkeeler)
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
Attachment #8767912 - Flags: review?(dkeeler) → review+
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8ae66227417f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.