Allow signing EC certs and creating EC origin-bound certs.

ReReland: Allow signing EC certs and creating EC origin-bound certs.

BUG=88782
TEST=X509UtilNSSTest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112780

[mattm, 2011-11-11 23:35:04]

(wtc: fyi, the problem I was seeing with verifying the RSA cert was because I
forgot the /8 on the signature length.  I guess the EC verify just ignored the
excess length.  I did verify that munging the signature data causes the test to
fail.)

[wtc, 2011-11-15 03:10:36]

On 2011/11/11 23:35:04, mattm wrote:
> (wtc: fyi, the problem I was seeing with verifying the RSA cert was because I
> forgot the /8 on the signature length.  I guess the EC verify just ignored the
> excess length.  I did verify that munging the signature data causes the test
to
> fail.)

Could you please track down why EC signature verification succeeds
when the signature length is too long?  We should make it as strict
as RSA signature verification.  Thanks.

[wtc, 2011-11-15 03:10:57]

Patch Set 1 LGTM.

http://codereview.chromium.org/8537025/diff/1/crypto/third_party/nss/secsign.cc
File crypto/third_party/nss/secsign.cc (right):

http://codereview.chromium.org/8537025/diff/1/crypto/third_party/nss/secsign....
crypto/third_party/nss/secsign.cc:61: if (key->keyType != ecKey)

Add curly braces around the return statement because it is
two lines.

http://codereview.chromium.org/8537025/diff/1/crypto/third_party/nss/secsign....
crypto/third_party/nss/secsign.cc:87: return SECFailure;

Please call PORT_SetError() before returning SECFailure.
Please find an appropriate error code from
http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslerr.html

I think you can use SEC_ERROR_INVALID_ALGORITHM

http://codereview.chromium.org/8537025/diff/1/crypto/third_party/nss/secsign....
crypto/third_party/nss/secsign.cc:102: rv = PK11_Sign(key, &sig, &hash);

I suggest we use this code for all signature algorithms
(not just ECDSA) and on all platforms (including Windows).
Can you do this?

http://codereview.chromium.org/8537025/diff/1/net/base/x509_util_nss.cc
File net/base/x509_util_nss.cc (right):

http://codereview.chromium.org/8537025/diff/1/net/base/x509_util_nss.cc#newco...
net/base/x509_util_nss.cc:174: bool CreateOriginBoundCert(
It may be better to name this function CreateOriginBoundCertCommon
or CreateOriginBoundCertInternal to suggest its relation with
CreateOriginBoundCertRSA and CreateOriginBoundCertEC.

[mattm, 2011-11-15 04:13:20]

On 2011/11/15 03:10:36, wtc wrote:
> On 2011/11/11 23:35:04, mattm wrote:
> > (wtc: fyi, the problem I was seeing with verifying the RSA cert was because
I
> > forgot the /8 on the signature length.  I guess the EC verify just ignored
the
> > excess length.  I did verify that munging the signature data causes the test
> to
> > fail.)
> 
> Could you please track down why EC signature verification succeeds
> when the signature length is too long?  We should make it as strict
> as RSA signature verification.  Thanks.

It looks like this is because the signature is variable length encoding, so the
code just ends up eventually calling SEC_ASN1DecodeItem on it and thus will fail
if it is too short, but not too long.  So, I guess to verify this we would need
a version of SEC_ASN1DecodeItem that returned how many bytes it actually
consumed.

The call stack goes like:

SEC_ASN1DecodeItem
common_DecodeDerSig
DSAU_DecodeDerSigToLen
decodeECorDSASignature
vfy_CreateContext
VFY_CreateContextWithAlgorithmID
crypto::SignatureVerifier::VerifyInit

[mattm, 2011-11-15 05:42:31]

http://codereview.chromium.org/8537025/diff/1/crypto/third_party/nss/secsign.cc
File crypto/third_party/nss/secsign.cc (right):

http://codereview.chromium.org/8537025/diff/1/crypto/third_party/nss/secsign....
crypto/third_party/nss/secsign.cc:61: if (key->keyType != ecKey)
On 2011/11/15 03:10:57, wtc wrote:
> 
> Add curly braces around the return statement because it is
> two lines.

Done.

http://codereview.chromium.org/8537025/diff/1/crypto/third_party/nss/secsign....
crypto/third_party/nss/secsign.cc:87: return SECFailure;
On 2011/11/15 03:10:57, wtc wrote:
> 
> Please call PORT_SetError() before returning SECFailure.
> Please find an appropriate error code from
> http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslerr.html
> 
> I think you can use SEC_ERROR_INVALID_ALGORITHM

Done.

http://codereview.chromium.org/8537025/diff/1/crypto/third_party/nss/secsign....
crypto/third_party/nss/secsign.cc:102: rv = PK11_Sign(key, &sig, &hash);
On 2011/11/15 03:10:57, wtc wrote:
> 
> I suggest we use this code for all signature algorithms
> (not just ECDSA) 

Hm, I could, but I think handling them all would make things a bit messier.   
The switch for the algo_id would get bigger, and there are differences in how
the signature is encoded between RSA and DSA keys.  Well, it wouldn't be too
horrible but I'm not sure it's  worthwhile.

>and on all platforms (including Windows).
> Can you do this?

Updated to use on all platforms.  Let me know if you still want me to do the
other part.

http://codereview.chromium.org/8537025/diff/1/net/base/x509_util_nss.cc
File net/base/x509_util_nss.cc (right):

http://codereview.chromium.org/8537025/diff/1/net/base/x509_util_nss.cc#newco...
net/base/x509_util_nss.cc:174: bool CreateOriginBoundCert(
On 2011/11/15 03:10:57, wtc wrote:
> It may be better to name this function CreateOriginBoundCertCommon
> or CreateOriginBoundCertInternal to suggest its relation with
> CreateOriginBoundCertRSA and CreateOriginBoundCertEC.

Done.

[wtc, 2011-11-15 06:15:01]

Patch Set 4 LGTM.

http://codereview.chromium.org/8537025/diff/1/crypto/third_party/nss/secsign.cc
File crypto/third_party/nss/secsign.cc (right):

http://codereview.chromium.org/8537025/diff/1/crypto/third_party/nss/secsign....
crypto/third_party/nss/secsign.cc:102: rv = PK11_Sign(key, &sig, &hash);
On 2011/11/15 05:42:31, mattm wrote:
>
> Updated to use on all platforms.  Let me know if you still want me to do the
> other part.

Not doing the other part (use PK11_Sign for all algorithms)
is fine.

This means we don't need to patch secsign.c in our copy of
NSS, right?

[wtc, 2011-11-15 06:37:53]

On 2011/11/15 04:13:20, mattm wrote:
>
> It looks like this is because the signature is variable length encoding, so
the
> code just ends up eventually calling SEC_ASN1DecodeItem on it and thus will
fail
> if it is too short, but not too long.  So, I guess to verify this we would
need
> a version of SEC_ASN1DecodeItem that returned how many bytes it actually
> consumed.
> 
> The call stack goes like:
> 
> SEC_ASN1DecodeItem
> common_DecodeDerSig
> DSAU_DecodeDerSigToLen
> decodeECorDSASignature
> vfy_CreateContext
> VFY_CreateContextWithAlgorithmID
> crypto::SignatureVerifier::VerifyInit

Thank you for tracking this down.

We should fail with the SEC_ERROR_EXTRA_INPUT error.
This MXR query shows that only the "QuickDER" decoder
has been updated to detect this condition:
http://mxr.mozilla.org/security/ident?i=SEC_ERROR_EXTRA_INPUT

Could you please file an NSS bug report about this problem
of SEC_ASN1DecodeItem?

We should at least change the common_DecodeDerSig function
to call SEC_QuickDERDecodeItem instead of SEC_ASN1DecodeItem.

I suspect this is where the real fix should be made:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/sec...

I guess that if len != 0 && cx->status == allDone, we should
fail with SEC_ERROR_EXTRA_INPUT.

[mattm, 2011-11-16 01:41:12]

http://codereview.chromium.org/8537025/diff/1/crypto/third_party/nss/secsign.cc
File crypto/third_party/nss/secsign.cc (right):

http://codereview.chromium.org/8537025/diff/1/crypto/third_party/nss/secsign....
crypto/third_party/nss/secsign.cc:102: rv = PK11_Sign(key, &sig, &hash);
On 2011/11/15 06:15:01, wtc wrote:
> On 2011/11/15 05:42:31, mattm wrote:
> >
> > Updated to use on all platforms.  Let me know if you still want me to do the
> > other part.
> 
> Not doing the other part (use PK11_Sign for all algorithms)
> is fine.
> 
> This means we don't need to patch secsign.c in our copy of
> NSS, right?
> 

right.  Should I revert that?

[wtc, 2011-11-16 05:29:15]

On 2011/11/16 01:41:12, mattm wrote:
>
> On 2011/11/15 06:15:01, wtc wrote:
> > This means we don't need to patch secsign.c in our copy of
> > NSS, right?
> 
> right.  Should I revert that?

Yes, please.

[commit-bot: I haz the power, 2011-11-16 21:37:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/8537025/3004

[mattm, 2011-11-16 22:35:44]

On 2011/11/15 06:37:53, wtc wrote:
> On 2011/11/15 04:13:20, mattm wrote:
> >
> > It looks like this is because the signature is variable length encoding, so
> the
> > code just ends up eventually calling SEC_ASN1DecodeItem on it and thus will
> fail
> > if it is too short, but not too long.  So, I guess to verify this we would
> need
> > a version of SEC_ASN1DecodeItem that returned how many bytes it actually
> > consumed.
> > 
> > The call stack goes like:
> > 
> > SEC_ASN1DecodeItem
> > common_DecodeDerSig
> > DSAU_DecodeDerSigToLen
> > decodeECorDSASignature
> > vfy_CreateContext
> > VFY_CreateContextWithAlgorithmID
> > crypto::SignatureVerifier::VerifyInit
> 
> Thank you for tracking this down.
> 
> We should fail with the SEC_ERROR_EXTRA_INPUT error.
> This MXR query shows that only the "QuickDER" decoder
> has been updated to detect this condition:
> http://mxr.mozilla.org/security/ident?i=SEC_ERROR_EXTRA_INPUT
> 
> Could you please file an NSS bug report about this problem
> of SEC_ASN1DecodeItem?

filed https://bugzilla.mozilla.org/show_bug.cgi?id=703103

> We should at least change the common_DecodeDerSig function
> to call SEC_QuickDERDecodeItem instead of SEC_ASN1DecodeItem.

I found https://bugzilla.mozilla.org/show_bug.cgi?id=352932 had already been
filed for that.
 
> I suspect this is where the real fix should be made:
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/sec...
> 
> I guess that if len != 0 && cx->status == allDone, we should
> fail with SEC_ERROR_EXTRA_INPUT.

[commit-bot: I haz the power, 2011-11-17 00:34:35]

Change committed as 110393

[Ryan Sleevi, 2011-11-17 23:25:00]

http://codereview.chromium.org/8537025/diff/16001/net/base/x509_util_nss_unit...
File net/base/x509_util_nss_unittest.cc (right):

http://codereview.chromium.org/8537025/diff/16001/net/base/x509_util_nss_unit...
net/base/x509_util_nss_unittest.cc:190: // so skipping this part of the test
should be fine.
nit: Add a reference to http://crbug.com/101231 ?

Or does this fail independent of that bug (ie: IsValidOSCertHandle in
x509_certificate_mac.cc returns false for ECC certs)

[mattm, 2011-11-17 23:57:15]

hey, I hadn't even hit send yet!  Updated again :)

http://codereview.chromium.org/8537025/diff/16001/net/base/x509_util_nss_unit...
File net/base/x509_util_nss_unittest.cc (right):

http://codereview.chromium.org/8537025/diff/16001/net/base/x509_util_nss_unit...
net/base/x509_util_nss_unittest.cc:190: // so skipping this part of the test
should be fine.
On 2011/11/17 23:25:00, Ryan Sleevi wrote:
> nit: Add a reference to http://crbug.com/101231 ?

done

> Or does this fail independent of that bug (ie: IsValidOSCertHandle in
> x509_certificate_mac.cc returns false for ECC certs)

no, the X509Certificate::CreateFromBytes would be returning NULL if that were
the case.

[wtc, 2011-11-30 20:30:00]

Patch Set 6 LGTM.

The function name VerifyX509CertificateParsing is not ideal
because it doesn't point out the function is OBC specific.
But I can't come up with a good name that's not too long.

I also proposed an alternative solution that does all the
verification using NSS CERTCertificate.  Perhaps that's a
better solution?

http://codereview.chromium.org/8537025/diff/3004/net/base/x509_util_nss_unitt...
File net/base/x509_util_nss_unittest.cc (right):

http://codereview.chromium.org/8537025/diff/3004/net/base/x509_util_nss_unitt...
net/base/x509_util_nss_unittest.cc:122: der_cert.data(), der_cert.size());
An alternative solution is to not parse into an
X509Certificate object at all, and just do the
subject common name checking on the NSS CERTCertificate
object.

  EXPECT_EQ("anonymous.invalid", CERT_GetCommonName(&nss_cert->subject));
  EXPECT_EQ(SECSuccess, CERT_CertTimesValid(nss_cert));

should work.

You can add a comment to note that EC certs can't be
parsed on Mac OS X 10.5, which is one reason we operate
on NSS CERTCertificate only.

http://codereview.chromium.org/8537025/diff/20001/net/base/x509_util_nss_unit...
File net/base/x509_util_nss_unittest.cc (right):

http://codereview.chromium.org/8537025/diff/20001/net/base/x509_util_nss_unit...
net/base/x509_util_nss_unittest.cc:88: EXPECT_EQ("anonymous.invalid",
cert->subject().GetDisplayName());

This expected certificate subject name is specific to
origin-bound certificates.  This should be reflected in
the name of the function, VerifyOriginBoundCertParsing?

http://codereview.chromium.org/8537025/diff/20001/net/base/x509_util_nss_unit...
net/base/x509_util_nss_unittest.cc:191: // TODO(mattm): Try removing this check
when http://crbug.com/101231 is fixed.

and also merging VerifyX509CertificateParsing back into
VerifyOriginBoundCert.

[mattm, 2011-12-01 00:18:30]

http://codereview.chromium.org/8537025/diff/3004/net/base/x509_util_nss_unitt...
File net/base/x509_util_nss_unittest.cc (right):

http://codereview.chromium.org/8537025/diff/3004/net/base/x509_util_nss_unitt...
net/base/x509_util_nss_unittest.cc:122: der_cert.data(), der_cert.size());
On 2011/11/30 20:30:02, wtc wrote:
> An alternative solution is to not parse into an
> X509Certificate object at all, and just do the
> subject common name checking on the NSS CERTCertificate
> object.
> 
>   EXPECT_EQ("anonymous.invalid", CERT_GetCommonName(&nss_cert->subject));
>   EXPECT_EQ(SECSuccess, CERT_CertTimesValid(nss_cert));
> 
> should work.
> 
> You can add a comment to note that EC certs can't be
> parsed on Mac OS X 10.5, which is one reason we operate
> on NSS CERTCertificate only.

done.

[commit-bot: I haz the power, 2011-12-01 00:19:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/8537025/24001

[commit-bot: I haz the power, 2011-12-01 02:59:32]

Change committed as 112385

[mattm, 2011-12-01 20:17:08]

Third time's a charm?  (fix unittest leak, and sending changes separately for
the win_shared exports.)

[wtc, 2011-12-01 21:58:27]

Patch Set 9 LGTM.