Closed
Bug 1281569
Opened 9 years ago
Closed 9 years ago
Remove unnecessary step of converting nsIX509Certs to Raw DER just to create a CERTCertificate in nsNSSCertificateDB
Categories
(Core :: Security: PSM, defect, P1)
Core
Security: PSM
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.
![]() |
||
Comment 1•9 years ago
|
||
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?
![]() |
Assignee | |
Comment 2•9 years ago
|
||
(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
![]() |
Assignee | |
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
https://reviewboard.mozilla.org/r/62276/#review59244
Great - r=me
![]() |
||
Comment 5•9 years ago
|
||
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+
![]() |
Assignee | |
Comment 6•9 years ago
|
||
Keywords: checkin-needed
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
Comment 8•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•