Last Comment Bug 880543 - Implement the AES GCM cipher suites in RFC 5288 and RFC 5289
: Implement the AES GCM cipher suites in RFC 5288 and RFC 5289
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P1 enhancement with 13 votes (vote)
: 3.15.2
Assigned To: Adam Langley
:
:
Mentors:
Depends on:
Blocks: 905383 916226
  Show dependency treegraph
 
Reported: 2013-06-06 17:58 PDT by Wan-Teh Chang
Modified: 2014-09-22 14:51 PDT (History)
26 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch by Adam Langley (47.25 KB, patch)
2013-06-06 17:58 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
[Corrected] Patch by Adam Langley (49.55 KB, patch)
2013-06-06 19:03 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Patch v2, by Adam Langley (60.18 KB, patch)
2013-08-14 09:53 PDT, Wan-Teh Chang
ryan.sleevi: review+
wtc: checked‑in+
Details | Diff | Splinter Review
ssltap patch (2.39 KB, patch)
2013-08-14 11:18 PDT, Wan-Teh Chang
kaie: review+
wtc: checked‑in+
Details | Diff | Splinter Review
Support DTLS, report reasonable MAC algorithm info for AEAD ciphers (32.82 KB, patch)
2013-08-20 15:31 PDT, Wan-Teh Chang
ryan.sleevi: review+
wtc: checked‑in+
Details | Diff | Splinter Review
Remove two unused macros that were added by mistake (949 bytes, patch)
2013-08-20 15:44 PDT, Wan-Teh Chang
wtc: checked‑in+
Details | Diff | Splinter Review
Fix compiler warning about unhandled enum value in a switch statement (504 bytes, patch)
2013-12-17 17:46 PST, Wan-Teh Chang
agl: review+
wtc: checked‑in+
Details | Diff | Splinter Review

Description Wan-Teh Chang 2013-06-06 17:58:24 PDT
Created attachment 759524 [details] [diff] [review]
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 2013-06-06 19:03:01 PDT
Created attachment 759546 [details] [diff] [review]
[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 2013-07-11 14:29:15 PDT
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 2013-07-23 12:39:12 PDT
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 2013-07-23 14:50:37 PDT
> 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 2013-07-23 14:57:23 PDT
(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 2013-08-06 15:23:15 PDT
Applying this patch causes segfaults for me.
Comment 7 Wan-Teh Chang 2013-08-14 09:53:07 PDT
Created attachment 790289 [details] [diff] [review]
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 2013-08-14 11:18:31 PDT
Created attachment 790347 [details] [diff] [review]
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 2013-08-14 15:23:32 PDT
(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 2013-08-14 15:38:06 PDT
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 2013-08-15 03:26:33 PDT
(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?) 2013-08-18 19:59:22 PDT
(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 2013-08-19 10:48:54 PDT
(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 2013-08-20 15:31:15 PDT
Created attachment 793149 [details] [diff] [review]
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 2013-08-20 15:44:42 PDT
Created attachment 793166 [details] [diff] [review]
Remove two unused macros that were added by mistake

https://hg.mozilla.org/projects/nss/rev/873c4adff43c
Comment 16 Wan-Teh Chang 2013-08-20 16:54:38 PDT
(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 18 Wan-Teh Chang 2013-08-23 16:20:35 PDT
AES-GCM cipher suites support pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/096d62676298
Comment 19 Phil Ringnalda (:philor) 2013-08-25 08:27:39 PDT
https://hg.mozilla.org/mozilla-central/rev/096d62676298
Comment 20 Wan-Teh Chang 2013-12-17 17:46:03 PST
Created attachment 8349130 [details] [diff] [review]
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 2013-12-18 16:25:32 PST
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 rmcguigan 2014-09-22 11:31:47 PDT
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 2014-09-22 14:51:46 PDT
This bug only add GCM 256. The rest are waiting on the PKCS #11 workgroup.

bob

Note You need to log in before you can comment on or make changes to this bug.