Make the AES-GCM cipher suites work in DTLS, by moving the

Make the AES-GCM cipher suites work in DTLS, by moving the
ssl3_BuildRecordPseudoHeader calls to where the isDTLS boolean is
available.

Make the three MAC-related fields of the SSLCipherSuiteInfo structure
report reasonable values for AEAD ciphers, which don't use a MAC.

R=rsleevi@chromium.org
BUG=255241
TEST=none

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

[wtc, 2013-08-17 00:21:15]

rsleevi: please review this CL.

agl: just FYI.

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/dtlsc...
File net/third_party/nss/ssl/dtlscon.c (left):

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/dtlsc...
net/third_party/nss/ssl/dtlscon.c:39: TLS_RSA_WITH_AES_128_GCM_SHA256,

This removes the temporary workaround.

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (left):

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:1903: version, PR_FALSE /* not DTLS */,

Note that we had to pass PR_FALSE as the |isDTLS| argument here because we
don't have that boolean available here.

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:2323: unsigned char      temp[MAX_MAC_LENGTH];

|isTLS| is only used once, so I just inline it.

I moved |tempLen| and |temp| to the block where they are used.

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:490: { mac_aead, mmech_null,       0,  0      
   },

This array element for mac_aead needs to exist but won't be used for AEAD
ciphers. So the fields of this array element have invalid/0 values.

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/sslim...
File net/third_party/nss/ssl/sslimpl.h (left):

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/sslim...
net/third_party/nss/ssl/sslimpl.h:538: SSL3SequenceNumber   seqnum);

The |type|, |version|, and |seqnum| arguments were only used to build
the record pseudo-header. For DTLS, the |version| is the logical version,
not the version used on the wire. This is why SSLAEADCipher didn't work
in DTLS.

To fix this, I have the callers of SSLAEADCipher call
ssl3_BuildRecordPseudoHeader and pass the pseudo-header as the
|additionalData| argument to SSLAEADCipher.

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/sslt.h
File net/third_party/nss/ssl/sslt.h (right):

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/sslt....
net/third_party/nss/ssl/sslt.h:108: ssl_hmac_sha512   = 7,

Question: to be future-proof, I added ssl_hmac_sha384 and ssl_hmac_sha512.
The sslt.h header is a public header, so the values of these enums cannot
change.

But, given our current opinion of HMAC-SHA256 ciphers, it seems unlikely
that we will implement HMAC-SHA384 ciphers. (I don't know of any
HMAC-SHA512 ciphers.) So perhaps I should not reserve the enums for
ssl_hmac_sha384 and ssl_hmac_sha512 now. What do you think?

Reserving these two enums resulted in two extra entries in the mac_defs
array in ssl3con.c, because that array is indexed by this enum type.

[Ryan Sleevi, 2013-08-17 02:00:23]

This mostly looks good, but I want to take another pass after a day to make sure
I didn't miss anything subtle. As always, reviewing changes in ssl3con is a pain
:)

A few responses that don't really affect the substance of things.

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:2627: pseudoHeaderLen =
ssl3_BuildRecordPseudoHeader(pseudoHeader,
This is really weird indentation of that first param. Just $.02

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:11519: /* XXX Is this true for all AEAD
ciphers? */
No.

This is covered in http://tools.ietf.org/html/rfc5116#section-4 pp12

   Each AEAD algorithm MUST provide a description relating the length of
   the plaintext to that of the ciphertext.  This relation MUST NOT
   depend on external parameters, such as an authentication strength
   parameter (e.g., authentication tag length).  That sort of dependence
   would complicate the use of the algorithm by creating a situation in
   which the information from the AEAD registry was not sufficient to
   ensure interoperability.


So the existing ciphers:
http://tools.ietf.org/html/rfc5116#section-5.1
   An AEAD_AES_128_GCM ciphertext is exactly 16 octets longer than its
   corresponding plaintext.

http://tools.ietf.org/html/rfc5116#section-5.2
   This algorithm is identical to AEAD_AES_128_GCM, but with the
   following differences:

http://tools.ietf.org/html/rfc5116#section-5.3
   An AEAD_AES_128_CCM ciphertext is exactly 16 octets longer than its
   corresponding plaintext.

http://tools.ietf.org/html/rfc5116#section-5.4
   This algorithm is identical to AEAD_AES_128_CCM, but with the
   following differences:


Also considering the proposed
http://tools.ietf.org/html/draft-mcgrew-aead-aes-cbc-hmac-sha2-02

see
http://tools.ietf.org/html/draft-mcgrew-aead-aes-cbc-hmac-sha2-02#section-2.3

Note that because CBC is used, there is padding.

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/sslt.h
File net/third_party/nss/ssl/sslt.h (right):

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/sslt....
net/third_party/nss/ssl/sslt.h:108: ssl_hmac_sha512   = 7,
On 2013/08/17 00:21:15, wtc wrote:
> 
> Question: to be future-proof, I added ssl_hmac_sha384 and ssl_hmac_sha512.
> The sslt.h header is a public header, so the values of these enums cannot
> change.
> 
> But, given our current opinion of HMAC-SHA256 ciphers, it seems unlikely
> that we will implement HMAC-SHA384 ciphers. (I don't know of any
> HMAC-SHA512 ciphers.) So perhaps I should not reserve the enums for
> ssl_hmac_sha384 and ssl_hmac_sha512 now. What do you think?
> 
> Reserving these two enums resulted in two extra entries in the mac_defs
> array in ssl3con.c, because that array is indexed by this enum type.

My preference would be not to introduce them until they're actually used, and to
deal with the asymmetry then. Given all the performance concerns, and Keccak, it
does seem unlikely to see these used or to provide significant value.

[wtc, 2013-08-19 23:31:56]

Thank you for your review. Patch set 2 has the changes you
suggested.

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:2627: pseudoHeaderLen =
ssl3_BuildRecordPseudoHeader(pseudoHeader,

On 2013/08/17 02:00:23, Ryan Sleevi wrote:
> This is really weird indentation of that first param. Just $.02

Done.

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:11519: /* XXX Is this true for all AEAD
ciphers? */

On 2013/08/17 02:00:23, Ryan Sleevi wrote:
> No.
> 
> This is covered in http://tools.ietf.org/html/rfc5116#section-4 pp12
> 
>    Each AEAD algorithm MUST provide a description relating the length of
>    the plaintext to that of the ciphertext.  This relation MUST NOT
>    depend on external parameters, such as an authentication strength
>    parameter (e.g., authentication tag length).  That sort of dependence
>    would complicate the use of the algorithm by creating a situation in
>    which the information from the AEAD registry was not sufficient to
>    ensure interoperability.

Done. Thank you for the research. I updated the comment.

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/sslt.h
File net/third_party/nss/ssl/sslt.h (right):

https://codereview.chromium.org/23299002/diff/1/net/third_party/nss/ssl/sslt....
net/third_party/nss/ssl/sslt.h:108: ssl_hmac_sha512   = 7,

On 2013/08/17 02:00:23, Ryan Sleevi wrote:
>
> My preference would be not to introduce them until they're actually used, and
to
> deal with the asymmetry then.

Done.

https://codereview.chromium.org/23299002/diff/23001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23299002/diff/23001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:470: #define mmech_invalid 
(CK_MECHANISM_TYPE)0x80000000L

0x80000000 is the CKM_VENDOR_DEFINED. I think we are using
this value as an invalid mechanism, which is why I renamed
the macro mmech_null to mmech_invalid.

[Ryan Sleevi, 2013-08-20 01:38:50]

This LGTM for all supported AEAD ciphers, but please see below on why I think
our implementation is incorrect if/when mcrew-aead-cbc-hmac-sha2 is approved.

https://codereview.chromium.org/23299002/diff/23001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23299002/diff/23001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:1778: * the protocol that was negotiated when
the spec became current, NOT based on
s/spec/cipher spec/?

My concern is the ambiguity between the "TLS spec" and the "cipher spec", and
the fact that you're not using the ssl3cipherSpec here.

https://codereview.chromium.org/23299002/diff/23001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:11520: * plaintext length for a given
ciphertext. */
How *would* this work for the AES-CBC+SHA case?

In order to know how many bytes of padding there were, you'd need to do the CBC
decrypt, so that you could see the padding bytes. But in order to generate the
AAD needed to decrypt, you'd need to first verify the MAC of A and C.

Am I insane, or does that algorithm not work for TLS?

See
http://tools.ietf.org/html/draft-mcgrew-aead-aes-cbc-hmac-sha2-01#section-2.2

Alternatively, it means in the AEAD case for CBC, the value used in the header
should be the *padded* size, *not* the plaintext size. If this is true, then the
decrypt code is correct, but the actual size of the plaintext (plaintext->len)
will != decryptedLen.

That is, AEAD ciphers should provide a function that returns the *ciphertext*
length for a given *plaintext*, not the inverse as you've specified. This seems
to match my understanding of the definition of AEAD algorithms of RFC5116.

That means that line 2628 is incorrect, since contentLen should be
AEADCiphertextLen

[wtc, 2013-08-20 19:13:11]

Thank you for your review. Patch set 3 is ready for checkin.

https://codereview.chromium.org/23299002/diff/23001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23299002/diff/23001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:1778: * the protocol that was negotiated when
the spec became current, NOT based on

On 2013/08/20 01:38:50, Ryan Sleevi wrote:
> s/spec/cipher spec/?
> 
> My concern is the ambiguity between the "TLS spec" and the "cipher spec", and
> the fact that you're not using the ssl3cipherSpec here.

Done.

https://codereview.chromium.org/23299002/diff/23001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:11520: * plaintext length for a given
ciphertext. */

On 2013/08/20 01:38:50, Ryan Sleevi wrote:
> How *would* this work for the AES-CBC+SHA case?
> 
> In order to know how many bytes of padding there were, you'd need to do the
CBC
> decrypt, so that you could see the padding bytes. But in order to generate the
> AAD needed to decrypt, you'd need to first verify the MAC of A and C.
> 
> Am I insane, or does that algorithm not work for TLS?
> 
> See
> http://tools.ietf.org/html/draft-mcgrew-aead-aes-cbc-hmac-sha2-01#section-2.2

You are right. I believe AEAD_AES_128_CBC_HMAC_SHA_256, etc. won't work for
TLS, because TLS specifies the additional authenticated data for AEAD ciphers
to include the length of the plaintext (the plaintext is the
TLSCompressed.fragment):

      additional_data = seq_num + TLSCompressed.type +
                        TLSCompressed.version + TLSCompressed.length;
 
> Alternatively, it means in the AEAD case for CBC, the value used in the header
> should be the *padded* size, *not* the plaintext size.

This will require a change to the TLS 1.2 specification because
TLSCompressed.length is the unpadded size.

> That is, AEAD ciphers should provide a function that returns the *ciphertext*
> length for a given *plaintext*, not the inverse as you've specified.

RFC 5116 Section 4 requires the former function, as you pointed out before. The
latter (inverse) function is necessary for TLS 1.2 AEAD ciphers because of how
additional_data is specified.

[commit-bot: I haz the power, 2013-08-20 21:57:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/23299002/6004

[commit-bot: I haz the power, 2013-08-21 02:46:12]

Change committed as 218606