Closed Bug 992972 Opened 6 years ago Closed 6 years ago

Add sha256PKPin attribute to nsIX509Cert3 or nsIX509Cert

Categories

(Core :: DOM: Security, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: cviecco, Assigned: cviecco)

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch upping-idl (obsolete) — Splinter Review
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: nobody → cviecco
Attached patch upping-idl (obsolete) — Splinter Review
now with an actual implementation
Attachment #8402717 - Attachment is obsolete: true
Attachment #8402717 - Flags: superreview?(brian)
Attachment #8402774 - Flags: superreview?(brian)
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-
(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.
(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)
(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)
Attached patch upping-idl (obsolete) — Splinter Review
Attachment #8402774 - Attachment is obsolete: true
Attachment #8405502 - Flags: review?(brian)
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+
Attached patch upping-idl (v2)Splinter Review
Keeping r+ from bsmith.
Attachment #8405502 - Attachment is obsolete: true
Attachment #8408313 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2604efa5ae36
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.