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

RESOLVED FIXED in 3.15.2

Status

NSS
Libraries
P1
enhancement
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Adam Langley)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
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.
Attachment #759524 - Attachment is obsolete: true

Comment 2

4 years ago
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

4 years ago
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

4 years ago
> 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

4 years ago
(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

4 years ago
Applying this patch causes segfaults for me.
(Reporter)

Comment 7

4 years ago
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
Attachment #759546 - Attachment is obsolete: true
Attachment #790289 - Flags: review?(ryan.sleevi)
Attachment #790289 - Flags: checked-in+
(Reporter)

Comment 8

4 years ago
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.
Attachment #790347 - Flags: review?(kaie)

Updated

4 years ago
Attachment #790289 - Flags: review?(ryan.sleevi) → review+
(Reporter)

Updated

4 years ago
Blocks: 905383
(Reporter)

Comment 9

4 years ago
(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.
(Reporter)

Comment 10

4 years ago
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

4 years ago
(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?
(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
(Reporter)

Comment 13

4 years ago
(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.
(Reporter)

Comment 14

4 years ago
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
Attachment #793149 - Flags: review?(ryan.sleevi)
Attachment #793149 - Flags: checked-in+
(Reporter)

Comment 15

4 years ago
Created attachment 793166 [details] [diff] [review]
Remove two unused macros that were added by mistake

https://hg.mozilla.org/projects/nss/rev/873c4adff43c
Attachment #793166 - Flags: checked-in+
(Reporter)

Comment 16

4 years ago
(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).
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Attachment #790347 - Flags: review?(kaie) → review+
(Reporter)

Comment 17

4 years ago
Comment on attachment 790347 [details] [diff] [review]
ssltap patch

https://hg.mozilla.org/projects/nss/rev/fd7965562eb2
Attachment #790347 - Flags: checked-in+
(Reporter)

Comment 18

4 years ago
AES-GCM cipher suites support pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/096d62676298
https://hg.mozilla.org/mozilla-central/rev/096d62676298
Blocks: 916226

Updated

4 years ago
Attachment #793149 - Flags: review?(ryan.sleevi) → review+
(Reporter)

Comment 20

3 years ago
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.
Attachment #8349130 - Flags: review?(agl)
(Assignee)

Updated

3 years ago
Attachment #8349130 - Flags: review?(agl) → review+
(Reporter)

Comment 21

3 years ago
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
Attachment #8349130 - Flags: checked-in+
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

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

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