crypto: change ECSignatureCreator defaults to match SPDY.

crypto: add DecodeSignature and use SHA-256 with ECDSA.

This changes ECSignatureCreator to use the hash function that SPDY
expects (SHA-256). There are no other users of ECSignatureCreator in
the tree so I'm going to defer making these choices parameters until there's
a benefit to be had.

It also adds DecodeSignature to convert from ASN.1 signatures to the `raw'
form that SPDY needs.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157551

[Ryan Hamilton, 2012-09-12 16:14:18]

I guess this explains why the CREDENTIAL frames were not being verified by the
GFE?  I'm a bit confused about how we managed to get this wrong.  IIRC, wtc and
I spent a good deal of time looking at various RFC before I wrote this code. 
Perhaps TLS channel ID uses different EC signatures than a normal TLS handshake?
 (And our code was written for the former)

In any case, thanks for finding this.

[agl, 2012-09-12 16:23:56]

On 2012/09/12 16:14:18, Ryan Hamilton wrote:
> I guess this explains why the CREDENTIAL frames were not being verified by the
> GFE?  I'm a bit confused about how we managed to get this wrong.  IIRC, wtc
and
> I spent a good deal of time looking at various RFC before I wrote this code. 
> Perhaps TLS channel ID uses different EC signatures than a normal TLS
handshake?
>  (And our code was written for the former)

Yes, they aren't DER encoded which differs from TLS.

[Ryan Sleevi, 2012-09-12 17:55:22]

Is there any particular reason that TLS Channel IDs chose to go with "raw" ECDSA
signatures as opposed to DER-encoded (as used by DSA and ECDSA within TLS
signatures - RFC 4492 Section 5.4)

All other things being equal, it would seem like consistency with TLS and X.509
(RFC 3279) would be no worse than the current implementation, but it would make
it 'easier' for other implementations to use existing signing infrastructure.

https://chromiumcodereview.appspot.com/10910226/diff/3008/crypto/ec_signature...
File crypto/ec_signature_creator_nss.cc (right):

https://chromiumcodereview.appspot.com/10910226/diff/3008/crypto/ec_signature...
crypto/ec_signature_creator_nss.cc:46: return PK11_Sign(key, result, &hash);
BUG: You return here, but then have code that runs afterwards.

https://chromiumcodereview.appspot.com/10910226/diff/3008/crypto/ec_signature...
File crypto/ec_signature_creator_unittest.cc (right):

https://chromiumcodereview.appspot.com/10910226/diff/3008/crypto/ec_signature...
crypto/ec_signature_creator_unittest.cc:69: // SignatureVerifier expects the
signatures to be DER encoded.
Shouldn't we also be fixing this to match?

[agl, 2012-09-12 18:51:48]

TLS uses 'raw' ECC keys, but encoded signatures. ChannelIDs are at least
self-consistent :)

But basically it's because ChannelIDs just contain four numbers: x, y, r and s
and it seemed silly not to encode them all the same.

https://chromiumcodereview.appspot.com/10910226/diff/3008/crypto/ec_signature...
File crypto/ec_signature_creator_nss.cc (right):

https://chromiumcodereview.appspot.com/10910226/diff/3008/crypto/ec_signature...
crypto/ec_signature_creator_nss.cc:46: return PK11_Sign(key, result, &hash);
On 2012/09/12 17:55:22, Ryan Sleevi wrote:
> BUG: You return here, but then have code that runs afterwards.

Thanks, I screwed up changing this code. Fixed.

https://chromiumcodereview.appspot.com/10910226/diff/3008/crypto/ec_signature...
File crypto/ec_signature_creator_unittest.cc (right):

https://chromiumcodereview.appspot.com/10910226/diff/3008/crypto/ec_signature...
crypto/ec_signature_creator_unittest.cc:69: // SignatureVerifier expects the
signatures to be DER encoded.
On 2012/09/12 17:55:22, Ryan Sleevi wrote:
> Shouldn't we also be fixing this to match?

The SignatureVerifier is used elsewhere too, and it takes a generic "signature"
(RSA, ECDSA etc). I think it's more reasonable to use ASN.1 at a generic
interface.

[wtc, 2012-09-12 19:47:45]

Review comments on patch set 3:

The most serious problem is the different formats of ECDSA
signatures used by the signature creator and verifier classes.

I suspect that the Channel ID code is the only thing that
uses SignatureVerifier to verify ECDSA signatures. So we
should be able to change SignatureVerifier to accept raw
ECDSA signatures. If we want to avoid that risk, we should
change the SPDY code to decode DER-encoded signatures.

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
File crypto/ec_signature_creator.h (right):

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
crypto/ec_signature_creator.h:45: // |signature| as a pair of big-endian field
elements.

The r and s of a signature are integers. (The x,y coordinates
of a point are field elements.)

This comments needs to say the r and s integers are each
big-endian, zero-padded to the right length, and then
concatenated. That right length is hard to describe
concisely (the length in bytes of the order of the curve's
base point). For P-256, that's 32 bytes.

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
File crypto/ec_signature_creator_nss.cc (right):

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
crypto/ec_signature_creator_nss.cc:48: SECITEM_FreeItem(result, false /* don't
free *result itself */);

Change "false" to "PR_FALSE".

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
File crypto/ec_signature_creator_unittest.cc (right):

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
crypto/ec_signature_creator_unittest.cc:13: #endif

The Chromium Coding Style page recommends that these headers
be included last:
http://dev.chromium.org/developers/coding-style#TOC-Platform-defines

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
crypto/ec_signature_creator_unittest.cc:69: // SignatureVerifier expects the
signatures to be DER encoded.

It would be bad if ECSignatureCreator and SignatureVerifier
used different formats of ECDSA signatures.

If we can't change the format of ECDSA signatures accepted
by SignatureVerifier, we should change the SPDY code to
call DSAU_DecodeDerSigToLen to decode the DER-encoded signature
generated by ECSignatureCreator.

[agl, 2012-09-12 21:41:46]

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
File crypto/ec_signature_creator.h (right):

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
crypto/ec_signature_creator.h:45: // |signature| as a pair of big-endian field
elements.
On 2012/09/12 19:47:45, wtc wrote:
> 
> The r and s of a signature are integers. (The x,y coordinates
> of a point are field elements.)

This is a NIST curve, so x and y are field elements too (of
the scalar field). Although I agree that it might confuse
things to call them that, so I've removed that wording.

> This comments needs to say the r and s integers are each
> big-endian, zero-padded to the right length, and then
> concatenated. That right length is hard to describe
> concisely (the length in bytes of the order of the curve's
> base point). For P-256, that's 32 bytes.

Done.

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
File crypto/ec_signature_creator_nss.cc (right):

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
crypto/ec_signature_creator_nss.cc:48: SECITEM_FreeItem(result, false /* don't
free *result itself */);
On 2012/09/12 19:47:45, wtc wrote:
> 
> Change "false" to "PR_FALSE".

(moot)

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
File crypto/ec_signature_creator_unittest.cc (right):

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
crypto/ec_signature_creator_unittest.cc:13: #endif
On 2012/09/12 19:47:45, wtc wrote:
> 
> The Chromium Coding Style page recommends that these headers
> be included last:
> http://dev.chromium.org/developers/coding-style#TOC-Platform-defines

(moot)

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
crypto/ec_signature_creator_unittest.cc:69: // SignatureVerifier expects the
signatures to be DER encoded.
On 2012/09/12 19:47:45, wtc wrote:
> If we can't change the format of ECDSA signatures accepted
> by SignatureVerifier, we should change the SPDY code to
> call DSAU_DecodeDerSigToLen to decode the DER-encoded signature
> generated by ECSignatureCreator.

I've changed the SPDY code to decode the DER signatures.

[wtc, 2012-09-12 22:06:27]

Patch set 4 LGTM.

It seems that DecodeSignature should be a member function of
ECPrivateKey, so that it can have access to the size of |n|
(base point order).

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
File crypto/ec_signature_creator.h (right):

https://chromiumcodereview.appspot.com/10910226/diff/10001/crypto/ec_signatur...
crypto/ec_signature_creator.h:45: // |signature| as a pair of big-endian field
elements.

On 2012/09/12 21:41:46, agl wrote:
>
> This is a NIST curve, so x and y are field elements too (of
> the scalar field).

For P-256, r and s are integers mod n, whereas x and y are
integers mod p. n and p are both 256-bit primes but are
different. This is why I think it's confusing to say r and
s are field elements, even though they are technically because
n < p.

https://chromiumcodereview.appspot.com/10910226/diff/7002/crypto/ec_signature...
File crypto/ec_signature_creator.h (right):

https://chromiumcodereview.appspot.com/10910226/diff/7002/crypto/ec_signature...
crypto/ec_signature_creator.h:57: // big-endian, zero-padded, 256-bit integers,
r and s. On success it returns

The size of r and s depends on the ECPrivateKey object.
It is fine to say "256-bit" for now because I think we can
only create P-256 ECPrivateKeys.

It seems bad to use different types for input and output.
Is std::string more convenient for the SPDY code?

https://chromiumcodereview.appspot.com/10910226/diff/7002/crypto/ec_signature...
File crypto/ec_signature_creator_nss.cc (right):

https://chromiumcodereview.appspot.com/10910226/diff/7002/crypto/ec_signature...
crypto/ec_signature_creator_nss.cc:53: return DSAU_EncodeDerSigWithLen(result,
&sig, sig.len);

To be future-proof, we should store sig.len in a member
variable for use by DecodeSignature(). This avoids the
constant kP256ScalarBytes.

[agl, 2012-09-13 20:17:59]

(Will wait to land until I can bring up a test server and do and end-to-end test
again.)

https://chromiumcodereview.appspot.com/10910226/diff/7002/crypto/ec_signature...
File crypto/ec_signature_creator.h (right):

https://chromiumcodereview.appspot.com/10910226/diff/7002/crypto/ec_signature...
crypto/ec_signature_creator.h:57: // big-endian, zero-padded, 256-bit integers,
r and s. On success it returns
On 2012/09/12 22:06:27, wtc wrote:
> 
> The size of r and s depends on the ECPrivateKey object.
> It is fine to say "256-bit" for now because I think we can
> only create P-256 ECPrivateKeys.
> 
> It seems bad to use different types for input and output.
> Is std::string more convenient for the SPDY code?

I've changed the output to also be a std::vector<uint8>

https://chromiumcodereview.appspot.com/10910226/diff/7002/crypto/ec_signature...
File crypto/ec_signature_creator_nss.cc (right):

https://chromiumcodereview.appspot.com/10910226/diff/7002/crypto/ec_signature...
crypto/ec_signature_creator_nss.cc:53: return DSAU_EncodeDerSigWithLen(result,
&sig, sig.len);
On 2012/09/12 22:06:27, wtc wrote:
> 
> To be future-proof, we should store sig.len in a member
> variable for use by DecodeSignature(). This avoids the
> constant kP256ScalarBytes.

Done.

[wtc, 2012-09-14 19:37:16]

Patch set 5 LGTM!

[wtc, 2012-09-14 19:49:42]

agl,rsleevi: ideally DecodeSignature should be a free function.
The only state DecodeSignature needs from ECSignatureCreator
is the raw signature length, and this is really a property of
the ECPrivateKey.

Raw ECDSA signatures also seem common. For example, raw
ECDSA signatures are used in XML DSIG and JSON Web Signatures
(see http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-05#page-10
).

In the future we may need direct support of raw ECDSA
signautres to avoid the unnecessary DER encoding and
decoding.

[agl, 2012-09-14 19:55:06]

On Fri, Sep 14, 2012 at 3:49 PM,  <wtc@chromium.org> wrote:
> agl,rsleevi: ideally DecodeSignature should be a free function.
> The only state DecodeSignature needs from ECSignatureCreator
> is the raw signature length, and this is really a property of
> the ECPrivateKey.

Due to the way that the SPDY unittests are written, DecodeSignature
needed to be a member function so that the mock ECSignatureCreator can
provide its own implementation. (The mock signatures are invalid DER
and so the generic DecodeSignature would fail.)


Cheers

AGL

[commit-bot: I haz the power, 2012-09-18 17:41:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/10910226/10003

[commit-bot: I haz the power, 2012-09-18 21:34:05]

Retried try job too often for step(s) browser_tests