880543 - Implement the AES GCM cipher suites in RFC 5288 and RFC 5289

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Wan-Teh Chang]

Attachment: Patch by Adam Langley

Adam Langley sent me a patch to add AES GCM cipher suites to NSS.
The attached patch is Adam's patch, adapted to the current NSS
trunk.

Comment 1

[Wan-Teh Chang]

Attachment: [Corrected] Patch by Adam Langley

I included my unrelated change to lib/pk11wrap/pk11cert.c by mistake.
I also forgot that the patch Adam sent to me is on top of his SSL
policy-removal patch, which is why he did not need to update the
static cipherPolicy ssl_ciphers[] array in lib/ssl/sslsock.c.

This patch fixes both of those problems. With this patch, Firefox
can use AES GCM cipher suites with Facebook and Google websites.

Comment 2

[Kurt Roeckx]

What's the status of this?  Now that TLS 1.2 seems to be working, I think this is important thing to have.  I think it's best that we all move to GCM when available.

Comment 3

[Kurt Roeckx]

Comment on attachment 759546 [details] [diff] [review]
[Corrected] Patch by Adam Langley

Review of attachment 759546 [details] [diff] [review]:
-----------------------------------------------------------------

Looking at the GCM cipher suites defined in the RFCs I see:
   TLS_RSA_WITH_AES_128_GCM_SHA256 = {0x00,0x9C}
   TLS_RSA_WITH_AES_256_GCM_SHA384 = {0x00,0x9D}
   TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 = {0x00,0x9E}
   TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 = {0x00,0x9F}
   TLS_DH_RSA_WITH_AES_128_GCM_SHA256 = {0x00,0xA0}
   TLS_DH_RSA_WITH_AES_256_GCM_SHA384 = {0x00,0xA1}
   TLS_DHE_DSS_WITH_AES_128_GCM_SHA256 = {0x00,0xA2}
   TLS_DHE_DSS_WITH_AES_256_GCM_SHA384 = {0x00,0xA3}
   TLS_DH_DSS_WITH_AES_128_GCM_SHA256 = {0x00,0xA4}
   TLS_DH_DSS_WITH_AES_256_GCM_SHA384 = {0x00,0xA5}
   TLS_DH_anon_WITH_AES_128_GCM_SHA256 = {0x00,0xA6}
   TLS_DH_anon_WITH_AES_256_GCM_SHA384 = {0x00,0xA7}
   TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256  = {0xC0,0x2B};
   TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384  = {0xC0,0x2C};
   TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256   = {0xC0,0x2D};
   TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384   = {0xC0,0x2E};
   TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256    = {0xC0,0x2F};
   TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384    = {0xC0,0x30};
   TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256     = {0xC0,0x31};
   TLS_ECDH_RSA_WITH_AES_256_GCM_SHA384     = {0xC0,0x32};

I only see 128 bit implementations.
It's my understanding that nss implements:
- RSA
- DHE_RSA (client side only)
- DHE_DSS (client side only)
- ECDH_RSA
- ECDH_ECDSA
- ECDHE_RSA
- ECDHE_ECDSA

And so no support for DH_*

When looking at various sites, I see google implements 128 and 256 bit of:
- RSA
- ECDHE_RSA

Facebook only does RSA

I don't know of any other sites doing GCM.

I assume that either ECDHE_RSA or DHE_RSA is going to be popular, with ECDHE_RSA probably preferred.  I'm just wondering why DHE_RSA also isn't implemented, and maybe even the others.

I also wonder why the 256 bit versions aren't supported.

::: lib/ssl/ssl3con.c
@@ +83,3 @@
>  #ifdef NSS_ENABLE_ECC
> + { TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,SSL_NOT_ALLOWED, PR_TRUE,PR_FALSE},
> + { TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,  SSL_NOT_ALLOWED, PR_TRUE,PR_FALSE},

Why aren't the ECDHE GCM cipher suited on top of this list?

What I currently see as prefered order is:
7 suites providing PFS, 256 bit
2 not providing PFS, 256 bit
1 providing PFS, 256 bit
12 providing PFS, 128 bit
[...]

Now you add the GCM one without PFS on top of that

@@ +1368,1 @@
>  	mac += 2;

I guess this if could use some comment.

::: lib/ssl/sslenum.c
@@ +33,4 @@
>      /* 256-bit */
>  #ifdef NSS_ENABLE_ECC
> +    TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
> +    TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,

The comment about says to update ssl3CipherSuite in ssl3ecc.c.  Either that's not done or the comment is wrong.

::: lib/ssl/sslimpl.h
@@ +285,2 @@
>  #else
> +#define ssl_V3_SUITES_IMPLEMENTED 38

Shouldn't this be 36?  You only add 1 non-ECC cipher, 3 in total

Comment 4

[cloos]

> I don't know of any other sites doing GCM.

All of my nginx installs default to GCM when I connect with gnutls-cli:

- Status: The certificate is trusted. 
- Description: (TLS1.2-PKIX)-(ECDHE-RSA-SECP256R1)-(AES-256-GCM)-(AEAD)
- Session ID: B8:B2:37:92:26:8E:88:65:F3:7D:82:DA:D4:42:06:9B:27:66:E0:05:CD:2B:04:72:89:18:CA:DC:8E:A8:D0:70
- Ephemeral EC Diffie-Hellman parameters
 - Using curve: SECP256R1
 - Curve size: 256 bits
- Version: TLS1.2
- Key Exchange: ECDHE-RSA
- Server Signature: RSA-SHA256
- Cipher: AES-256-GCM
- MAC: AEAD
- Compression: NULL
- Handshake was completed

- Simple Client Mode:

nginx is configured on those boxen with:

        ssl_protocols TLSv1.2 TLSv1.1 TLSv1;
        ssl_ciphers ECDH:DH:RSA:!EXP:!NULL:+HIGH:-MEDIUM:-LOW;
        ssl_prefer_server_ciphers on;

So any box with nginx 1.4 or later (at least) with that config can serve as a testbed.

Apache with that ciphers string does not do GCM when I use gnutls-cli.

However, when I use openssl s_client to apache (configured as above) it does pick GCM:

SSL-Session:
    Protocol  : TLSv1.2
    Cipher    : ECDHE-RSA-AES256-GCM-SHA384

so it looks like Apache 2.4 also should be a viable test server for GCM.

Comment 5

[Kurt Roeckx]

(In reply to Kurt Roeckx from comment #3)
> I don't know of any other sites doing GCM.

I don't know why I completly forgot about this, but Debian stable (apache 2.2 with openssl 1.0.1) does:
- RSA
- DHE_RSA

So I think there clearly is a need to have DHE_RSA.

ECC isn't supported in apache 2.2 yet, it was added somewhere in 2.3 and should now be available in 2.4

Comment 6

[Kurt Roeckx]

Applying this patch causes segfaults for me.

Comment 7

[Wan-Teh Chang]

Attachment: Patch v2, by Adam Langley

This patch is equivalent to patch set 8 at
https://codereview.chromium.org/21696002/. Ryan and I reviewed this
patch there.

Ryan, you just need to review the three tests/ssl files in this
patch. I modeled their changes after the HMAC-SHA256 cipher suite
patch: https://hg.mozilla.org/projects/nss/rev/67471ffe04fb

I checked in this patch:
https://hg.mozilla.org/projects/nss/rev/e698c2409762

Comment 8

[Wan-Teh Chang]

Attachment: ssltap patch

Add the following AES-GCM cipher suites:

      CipherSuite TLS_RSA_WITH_AES_128_GCM_SHA256 = {0x00,0x9C}
      CipherSuite TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 = {0x00,0x9E}
      CipherSuite TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 = {0xC0,0x2F}

CipherSuite TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 = {0xC0,0x2B} is already
in ssltap.c.

Comment 9

[Wan-Teh Chang]

(In reply to Kurt Roeckx from comment #3)

Thank you for your review comments. Most of the problems you pointed out were also
fixed in our review of the patch. I will add a comment to explain the
    mac += 2;
code.

Here are the answers to your questions.

1. NSS can only support the AES_128_GCM cipher suites because NSS only supports
the TLS 1.2 PRF with SHA-256 as the hash function. The AES_256_GCM cipher suites
all use the TLS 1.2 PRF with SHA-384 as the hash function.

2. I added TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 as you suggested.

3. The current cipher suite list has the four AES-GCM cipher suites at the top,
above the 256-bit CBC block ciphers.

Comment 10

[Wan-Teh Chang]

Remaining work:

1. Add DTLS support.

2. Make SSL_GetChannelInfo() return reasonable values in the three MAC related fields
of the SSLChannelInfo structure for AEAD ciphers:
    /* MAC info */
    const char *         macAlgorithmName;
    SSLMACAlgorithm      macAlgorithm;
    PRUint16             macBits;

3. Investigate whether we really need to add the explicit_nonce_size or tag_size fields
to the ssl3BulkCipherDef structure.

Comment 11

[Kurt Roeckx]

(In reply to Wan-Teh Chang from comment #9)
> 
> 1. NSS can only support the AES_128_GCM cipher suites because NSS only
> supports
> the TLS 1.2 PRF with SHA-256 as the hash function. The AES_256_GCM cipher
> suites
> all use the TLS 1.2 PRF with SHA-384 as the hash function.

Is that just lack of SHA-384 in general?

> 3. The current cipher suite list has the four AES-GCM cipher suites at the
> top,
> above the 256-bit CBC block ciphers.

At least the 3 first have PFS now.  Maybe we should have a discussion about the default order in some other bug?

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Kurt Roeckx from comment #11)
> At least the 3 first have PFS now.  Maybe we should have a discussion about
> the default order in some other bug?

There is a discussion about the default order (amongst other things) on the mailing list:
https://groups.google.com/forum/#!topic/mozilla.dev.tech.crypto/gFfKw3EOffo
https://groups.google.com/forum/#!topic/mozilla.dev.tech.crypto/36na1B2brGU

You can subscribe to the list at https://lists.mozilla.org/listinfo/dev-tech-crypto

Comment 13

[Wan-Teh Chang]

(In reply to Kurt Roeckx from comment #11)
> (In reply to Wan-Teh Chang from comment #9)
> > 
> > 1. NSS can only support the AES_128_GCM cipher suites because NSS only
> > supports
> > the TLS 1.2 PRF with SHA-256 as the hash function. The AES_256_GCM cipher
> > suites
> > all use the TLS 1.2 PRF with SHA-384 as the hash function.
> 
> Is that just lack of SHA-384 in general?

No. NSS supports SHA-384. NSS only supports the TLS 1.2 PRF with SHA-256 as the hash function.

Comment 14

[Wan-Teh Chang]

Attachment: Support DTLS, report reasonable MAC algorithm info for AEAD ciphers

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.

This patch was reviewed by Ryan Sleevi at https://codereview.chromium.org/23299002/.

https://hg.mozilla.org/projects/nss/rev/e98f18e7a3d5

Comment 15

[Wan-Teh Chang]

Attachment: Remove two unused macros that were added by mistake

https://hg.mozilla.org/projects/nss/rev/873c4adff43c

Comment 16

[Wan-Teh Chang]

(In reply to Wan-Teh Chang from comment #10)
> Remaining work:
> ...
> 3. Investigate whether we really need to add the explicit_nonce_size or
> tag_size fields to the ssl3BulkCipherDef structure.

The explicit_nonce_size and tag_size fields of the ssl3BulkCipherDef structure
are always used together. Only their sum is used, to calculate the overhead (in
bytes) that an AEAD cipher adds to a plaintext.

In general this overhead may not be constant-size, so each AEAD cipher will need
to provide two callback functions:
1. A function that returns the ciphertext length, given a plaintext length. RFC 5116
Section 4 requires that each AEAD algorithm's specification provides this:

   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.

2. A function that returns the plaintext length, given a ciphertext. This is
required for TLS 1.2 AEAD ciphers because the additional_data is specified to
include the plaintext length (TLSCompressed.length).

Comment 17

[Wan-Teh Chang]

Comment on attachment 790347 [details] [diff] [review]
ssltap patch

https://hg.mozilla.org/projects/nss/rev/fd7965562eb2

Comment 18

[Wan-Teh Chang]

AES-GCM cipher suites support pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/096d62676298

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/096d62676298

Comment 20

[Wan-Teh Chang]

Attachment: Fix compiler warning about unhandled enum value in a switch statement

The compiler warns that ssl_calg_aes_gcm is not handled in
that switch statement. I also changed calg_aes_gcm to
ssl_calg_aes_gcm. They are synonyms. The latter, with the
ssl_ prefix, is preferred.

Comment 21

[Wan-Teh Chang]

Comment on attachment 8349130 [details] [diff] [review]
Fix compiler warning about unhandled enum value in a switch statement

https://hg.mozilla.org/projects/nss/rev/383d285c3197

Comment 22

[guigs [:guigs] PST (please needinfo me!)]

Will GCM's be applied? This was marked as fixed before this last patch was added? Also am I in the wrong place? https://support.mozilla.org/en-US/questions/1019597

Comment 23

[Robert Relyea]

This bug only add GCM 256. The rest are waiting on the PKCS #11 workgroup.

bob