Closed
Bug 992972
Opened 10 years ago
Closed 10 years ago
Add sha256PKPin attribute to nsIX509Cert3 or nsIX509Cert
Categories
(Core :: DOM: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: cviecco, Assigned: cviecco)
Details
Attachments
(1 file, 3 obsolete files)
4.06 KB,
patch
|
cviecco
:
review+
|
Details | Diff | Splinter Review |
Public key pinning requires computing sha256PKPin of a certificate, and creating the build in list would be nice to do in JS so that we process all things with one engine. This is a requirement to write the built-in generator in JS.
Assignee | ||
Comment 1•10 years ago
|
||
Can you check only the interface (assuming I changed the uuid, and the implementation does not depend on nsPublicKeyPinningService)
Attachment #8402717 -
Flags: superreview?(brian)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → cviecco
Assignee | ||
Comment 2•10 years ago
|
||
now with an actual implementation
Attachment #8402717 -
Attachment is obsolete: true
Attachment #8402717 -
Flags: superreview?(brian)
Assignee | ||
Updated•10 years ago
|
Attachment #8402774 -
Flags: superreview?(brian)
Comment 3•10 years ago
|
||
Comment on attachment 8402774 [details] [diff] [review] upping-idl Review of attachment 8402774 [details] [diff] [review]: ----------------------------------------------------------------- Seems reasonable. Some nits below. I'd like to re-review because there are some significant changes needed though. ::: security/manager/ssl/public/nsIX509Cert3.idl @@ +57,5 @@ > + > + /** > + * The base64 encoding of the sha256 of the DER encoded public key > + */ > + readonly attribute AString sha256PKPin; It is better to just change nsIX509Cert instead of adding this to nsIX509Cert3.idl. Putting it in nsIX509Cert3 just causes us to have to do unnecessary QueryInterface calls. Also, you must update the UUID. ::: security/manager/ssl/src/nsNSSCertificate.cpp @@ +36,5 @@ > #include "nsCertVerificationThread.h" > #include "nsIObjectOutputStream.h" > #include "nsIObjectInputStream.h" > #include "nsIProgrammingLanguage.h" > +#include "nsPublicKeyPinningService.h" this include is unnecessary, right? @@ +1080,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +nsNSSCertificate::GetSha256PKPin(nsAString& _sha256PKPin) please use either no prefix or the "a" prefix. @@ +1085,5 @@ > +{ > + nsNSSShutDownPreventionLock locker; > + if (isAlreadyShutDown()) > + return NS_ERROR_NOT_AVAILABLE; > + _sha256PKPin.Truncate(); Please move the Truncate to just before the base64 conversion below. @@ +1096,5 @@ > + srv = PK11_HashBuf(SEC_OID_SHA256, temp_dest, > + derPublicKey->data, derPublicKey->len); > + if (srv != SECSuccess) { > + return NS_ERROR_FAILURE; > + } The above should be simplified and made safer: #include "ScopedNSSTypes.h" Digest digest; nsresult rv = digest.DigestBuf(SEC_OID_SHA256, mCert->derPublicKey.data, mCert->derPublicKey.len); NS_ENSURE_SUCCESS(rv, rv); @@ +1107,5 @@ > + memset(temp_dest, 0x00, SHA256_LENGTH*2); > + if (!NSSBase64_EncodeItem(NULL, base64Out, HASH_LENGTH_MAX*2, &toConvert)) { > + return NS_ERROR_FAILURE; > + } > + _sha256PKPin = NS_ConvertASCIItoUTF16(base64Out); Similarly: #include "mozilla/Base64.h" aSHA256PKPin.Truncate(); return Base64Encode(nsDependentCSubstring( digest.get().data, digest.get().data + digest.get().len), aShA256PKPin);
Attachment #8402774 -
Flags: superreview?(brian) → superreview-
Comment 4•10 years ago
|
||
(In reply to Camilo Viecco (:cviecco) from comment #1) > Created attachment 8402717 [details] [diff] [review] > upping-idl > > Can you check only the interface (assuming I changed the uuid, and the > implementation does not depend on nsPublicKeyPinningService) Sorry, I only looked at the SR request, not all the comments. I think the interface is OK. I am not thrilled about the capitalization of "GetSha256PKPin" and I'm not sure we should abbreviate "PublicKey" to "PK". Also, when I originally saw the name I expected the function to return the pin(s) in our public key pinning database, instead of just the hash value that would be used for pinning. Perhaps a name like SHA256SPKIHashForPinning would be better. I suspect that in the future we'll have to make this a function that takes the ID of the hash algorithm (SHA-256, SHA-384, etc.) as a parameter. In order to avoid churn in this public interface going forward, perhaps we should just do that now? It should be pretty easy to do that if you switch to using the mozilla::psm::Digest-based implementation that I suggested.
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #4) > (In reply to Camilo Viecco (:cviecco) from comment #1) > > Created attachment 8402717 [details] [diff] [review] > > upping-idl > > > > Can you check only the interface (assuming I changed the uuid, and the > > implementation does not depend on nsPublicKeyPinningService) > > Sorry, I only looked at the SR request, not all the comments. > > I think the interface is OK. I am not thrilled about the capitalization of > "GetSha256PKPin" and I'm not sure we should abbreviate "PublicKey" to "PK". > Also, when I originally saw the name I expected the function to return the > pin(s) in our public key pinning database, instead of just the hash value > that would be used for pinning. Perhaps a name like SHA256SPKIHashForPinning > would be better. > > I suspect that in the future we'll have to make this a function that takes > the ID of the hash algorithm (SHA-256, SHA-384, etc.) as a parameter. In > order to avoid churn in this public interface going forward, perhaps we > should just do that now? It should be pretty easy to do that if you switch > to using the mozilla::psm::Digest-based implementation that I suggested. I liked your implementation suffestion, however I think this should be an attribute: readonly attribute string sha256SubjectPublicKeyInfoDigest; instead of a void getSubjectPublicKeyInfoDigest(in int digestID, out Astring digest); because we Will either 1. add a bunch of digestIDs in the idl hoping that we dont need any more, and requiring us to test and matain code that we dont really need. 2. Only write a bunch of certIDs and in that case why not just update the idl to contain all those attributes? I am going forward using your implementation idea but changing the attribute name (which I agree was really bad). Can you comment before I ask for sr again?
Flags: needinfo?(brian)
Comment 6•10 years ago
|
||
(In reply to Camilo Viecco (:cviecco) from comment #5) > (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) > from comment #4) > I liked your implementation suffestion, however I think this should be an > attribute: > > readonly attribute string sha256SubjectPublicKeyInfoDigest; > > instead of a > void getSubjectPublicKeyInfoDigest(in int digestID, out Astring digest); Note that we could have the functoin-stlye interface but only define a single digest ID to start. It really doesn't matter at this point which approach is taken. The reason I suggested the more involved way is to avoid revising the interface in the future, since it is a commonly-used interface. So, up to you.
Flags: needinfo?(brian)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8402774 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8405502 -
Flags: review?(brian)
Comment 8•10 years ago
|
||
Comment on attachment 8405502 [details] [diff] [review] upping-idl Review of attachment 8405502 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/public/nsIX509Cert.idl @@ +234,5 @@ > + /** > + * The base64 encoding of the DER encoded public key info using the specified > + * digest. > + */ > + readonly attribute AString sha256SubjectPublicKeyInfoDigest; Why not make this an ACString so that you don't need to do the char -> PRUnichar conversion yourself? ::: security/manager/ssl/src/nsNSSCertificate.cpp @@ +1080,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +nsNSSCertificate::GetSha256SubjectPublicKeyInfoDigest(nsAString& aSha256PKDigest) Nit: please use the name aSha256SPKIDigest. @@ +1083,5 @@ > NS_IMETHODIMP > +nsNSSCertificate::GetSha256SubjectPublicKeyInfoDigest(nsAString& aSha256PKDigest) > +{ > + nsNSSShutDownPreventionLock locker; > + if (isAlreadyShutDown()) Always use braces around conditional statement bodies. @@ +1090,5 @@ > + aSha256PKDigest.Truncate(); > + Digest digest; > + nsresult rv = digest.DigestBuf(SEC_OID_SHA256, mCert->derPublicKey.data, > + mCert->derPublicKey.len); > + NS_ENSURE_SUCCESS(rv, rv); Nit: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros NS_ENSURE_* is deprecated; use this instead: if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } (I don't like it either.) @@ +1093,5 @@ > + mCert->derPublicKey.len); > + NS_ENSURE_SUCCESS(rv, rv); > + nsAutoCString charBuf; > + rv = Base64Encode(nsDependentCSubstring((const char*) digest.get().data, > + digest.get().len), Nit: no C-style casts in PSM; use reinterpret_cast<const char*>(digest.get().data) instead. Nit: this line is indented too much by too characters. @@ +1095,5 @@ > + nsAutoCString charBuf; > + rv = Base64Encode(nsDependentCSubstring((const char*) digest.get().data, > + digest.get().len), > + charBuf); > + NS_ENSURE_SUCCESS(rv, rv); Ditto: if (NS_WARN_IF(... @@ +1096,5 @@ > + rv = Base64Encode(nsDependentCSubstring((const char*) digest.get().data, > + digest.get().len), > + charBuf); > + NS_ENSURE_SUCCESS(rv, rv); > + aSha256PKDigest = NS_ConvertASCIItoUTF16(charBuf); return NS_CStringToUTF16(charBuf, NS_CSTRING_ENCODING_ASCII, aSha256SPKIDigest); Using NS_CStringToUTF16, we copy the characters once. The way we wrote it, IIUC, we copy the characters twice. If you change the type of the variable in the IDL to ACString then you can avoid doing this conversion at all: return Base64Encode(nsDependencCSubstring(reinterpret_cast<const char*>(digest.get().data), digest.get().len), aSHA256SPKIDigest);
Attachment #8405502 -
Flags: review?(brian) → review+
Assignee | ||
Comment 9•10 years ago
|
||
Keeping r+ from bsmith.
Attachment #8405502 -
Attachment is obsolete: true
Attachment #8408313 -
Flags: review+
Assignee | ||
Comment 10•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=907f60f493ce
Assignee | ||
Comment 11•10 years ago
|
||
The correct tbpl: https://tbpl.mozilla.org/?tree=Try&rev=ed26bcda8b75
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2604efa5ae36
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•