Closed Bug 1113974 Opened 9 years ago Closed 8 years ago

Disable AES-256-CBC modes by default

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: emk, Assigned: emk)

References

Details

(Whiteboard: [psm-deprecation])

Attachments

(1 file, 2 obsolete files)

Chromium folks consider this.
What exactly is being proposed here? AFAICT, the proposal is to remove these:

    * TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
    * TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
    * TLS_DHE_RSA_WITH_AES_256_CBC_SHA
    * TLS_RSA_WITH_AES_256_CBC_SHA

AFAICT, the goal is to get AEAD cipher suites to be used more, in cases where servers prefer 256-bit CBC-mode cipher suites over 128-bit AEAD cipher suites because they think 256 bits is more important than AEAD mode. I do think that is a problem worth attempting to solve.

But, I'm less convinced that removing AES-256 support due to its performance implications is the right solution. Note that it isn't necessary to completely remove support for the above cipher suites to get the intended effect. Instead, you can (1) treat these cipher suites like RC4, hiding them only from the initial handshake, and (2) you can add the AES-256-GCM cipher suites.

I think we should see some numbers regarding AES-256-GCM performance before we accept Google's argument that AES-256's performance is so poor that it is better to drop it. I think it's likely that the most performance-sensitive sites are already prefering AES-128 and many/most will eventually prefer ChaCha20-Poly1305 once there is a standard for it. Note that only 5.3% of Firefox full handshakes select AES-256 cipher suites. Indeed, only about 10% of full handshakes use AES-256.

AES-256 does have advantages as far as protection from future quantum attacks are concerned. Also, removing AES-256 support seems like an unnecessary PR problem to create, because it does have security advantages.

Anyway, at the very least, we'd have to measure how making a change like this decreases or increases the use of forward secrecy, particularly ECDHE key exchange. Forward secrecy is more important than the choice of symmetric cipher suite.
> Note that only 5.3% of Firefox full handshakes select AES-256 cipher suites. Indeed, only about 10% of full handshakes use AES-256.

Sorry, the 10% figure is the correct one, not the 5.3% figure.
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #2)
> What exactly is being proposed here? AFAICT, the proposal is to remove these:
> 
>     * TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
>     * TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
>     * TLS_DHE_RSA_WITH_AES_256_CBC_SHA
>     * TLS_RSA_WITH_AES_256_CBC_SHA

Yes.

> AFAICT, the goal is to get AEAD cipher suites to be used more, in cases
> where servers prefer 256-bit CBC-mode cipher suites over 128-bit AEAD cipher
> suites because they think 256 bits is more important than AEAD mode. I do
> think that is a problem worth attempting to solve.
> 
> But, I'm less convinced that removing AES-256 support due to its performance
> implications is the right solution. Note that it isn't necessary to
> completely remove support for the above cipher suites to get the intended
> effect. Instead, you can (1) treat these cipher suites like RC4, hiding them
> only from the initial handshake,

This is an interesting idea.

> and (2) you can add the AES-256-GCM cipher suites.

My primary motivation is to prefer 128-bit AEAD cipher suites more. I'm fine with re-enabling AES-256-CBC when AES-256-GCM support is added (bug 975832).
see also Bug 949564
I hope aes256 gcm is added and I am ok with removing aes 256 cbc.

I was one of those admins who previously put all aes256 first but now I do set on all servers I admin aes 128gcm ahead of aes 256cbc.

Of course I also have aes 256gcm ahead of aes 128gcm.

I think this is an issue when looking at cpu cost for server admins to decide on, not browsers, a individual browsing a site their cpu is unlikely to be overloaded by a single user browsing the internet, it only really becomes important on a server thats dealing with many users connecting at once.
Whiteboard: [psm-deprecation]
Attached patch patch (obsolete) — Splinter Review
Apply this patch on top of a patch from bug 975832.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8759618 - Flags: review?(dkeeler)
Comment on attachment 8759618 [details] [diff] [review]
patch

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

I'm concerned about compatibility here. Can we re-use the RC4 fallback mechanism to not offer these cipher suites on the first handshake and only enable them if we get a cipher overlap error? In the meantime, we can make a try build with this patch so Matt can run the TLS canary with it.
Attachment #8759618 - Flags: review?(dkeeler)
Matt, could you execute a TLS canary run?
Flags: needinfo?(mwobensmith)
I also hid DHE-AeS-128. Chrome is already doing this. It will also fix bug 1184104.
Attachment #8759618 - Attachment is obsolete: true
Attachment #8759967 - Flags: review?(dkeeler)
Comment on attachment 8759967 [details] [diff] [review]
Hide some deprecated cipher suites behind the TLS insecure fallback

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

I hadn't counted on RC4 - let's handle bug 1268728 first.

::: security/manager/ssl/nsNSSComponent.cpp
@@ +1351,5 @@
>  static_assert(MOZ_ARRAY_LENGTH(sCipherPrefs) - 1 <= sizeof(uint32_t) * CHAR_BIT,
>                "too many cipher suites");
>  
>  /*static*/ bool
> +nsNSSComponent::AreAnyFallbackCiphersEnabled(bool allowRC4)

Hmmm - I hadn't anticipated that rc4 would complicate this. Let's first land bug 1268728 after the branch (which I think is today or tomorrow) and then rebase this patch and see how things look.

::: security/manager/ssl/tests/unit/test_weak_crypto.js
@@ +183,5 @@
>    let cert = yield getCert();
>    ok(!!cert, "Got self-signed cert");
>    let port = startServer(cert, false);
>    storeCertOverride(port, cert);
> +  yield startClient("0-1", port, Cr.NS_OK);

It would be helpful if these names were a bit more descriptive.

@@ +186,5 @@
>    storeCertOverride(port, cert);
> +  yield startClient("0-1", port, Cr.NS_OK);
> +  yield startClient("0-2", port, Cr.NS_OK, {isPrivate: true});
> +  // disable fallback cipher suites other than RC4
> +  Services.prefs.setBoolPref("security.ssl3.ecdhe_rsa_aes_256_sha", false);

When we remove RC4 entirely we'll still want a test for cipher fallback, so I imagine this test can be repurposed into that.
Attachment #8759967 - Flags: review?(dkeeler)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #13)
> Hmmm - I hadn't anticipated that rc4 would complicate this. Let's first land
> bug 1268728 after the branch (which I think is today or tomorrow) and then
> rebase this patch and see how things look.

OK, hopefully we don't have to backout it. FYI Chrome has removed the last vestiges of RC4:
https://bugs.chromium.org/p/chromium/issues/detail?id=375342#c87
I removed the SSL_WEAK_CIPHERS_FALLBACK telemetry. I don't think we still need this.
Attachment #8759967 - Attachment is obsolete: true
Attachment #8760786 - Flags: review?(dkeeler)
Comment on attachment 8760786 [details] [diff] [review]
Hide some deprecated cipher suites behind the TLS insecure fallback, v2

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

Ok - this looks good to me. I do have the one question (see below). Also, I'm about to leave for PTO for the rest of the week and then I'll be at a conference all next week. I would prefer to be available if any compatibility issues arise, so let's wait until the week of the 20th to land this.

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ -1095,5 @@
>  
>      return false;
>    }
>  
> -  // Disallow PR_CONNECT_RESET_ERROR if fallback limit reached.

Why remove this part?
Attachment #8760786 - Flags: review?(dkeeler)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #16)
> ::: security/manager/ssl/nsNSSIOLayer.cpp
> @@ -1095,5 @@
> >  
> >      return false;
> >    }
> >  
> > -  // Disallow PR_CONNECT_RESET_ERROR if fallback limit reached.
> 
> Why remove this part?

The current fallback limit is TLS 1.2. So this part effectively removes PR_CONNECT_RESET_ERROR from fallback reasons.
Ok, so just to make sure I'm on the same page, the idea is to reintroduce PR_CONNECT_RESET_ERROR into the possible fallback reasons?
What is the point of doing this? Did somebody do measurements that show that even when Firefox offers AES-GCM-256 cipher suites, AES-256-CBC cipher suites are getting chosen instead of AES-GCM ciphersuites? What were the numbers? Otherwise, isn't this a bad idea?
Also, the argument above is that "Chrome is considering doing this" but Chrome is still offering (as of today):
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
TLS_RSA_WITH_AES_256_CBC_SHA
(In reply to David Keeler [:keeler] (use needinfo?) from comment #18)
> Ok, so just to make sure I'm on the same page, the idea is to reintroduce
> PR_CONNECT_RESET_ERROR into the possible fallback reasons?

Yes.

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #20)
> Also, the argument above is that "Chrome is considering doing this" but
> Chrome is still offering (as of today):
> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
> TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
> TLS_RSA_WITH_AES_256_CBC_SHA

Hm, apparently Chrome folks changed their mind. According to Chromium issue 442572[1], they intentionally didn't implement AES-256-GCM when the issue was filed. But Chrome has added support for AES-256-GCM since then[2].

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=442572
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=591516
I separated the DHE part into bug 1279479.
Hi there, sorry for taking so long. Do you still need a canary run? And is it against the same try build in comment 11?
Flags: needinfo?(mwobensmith) → needinfo?(VYV03354)
(In reply to Matt Wobensmith [:mwobensmith][:matt:] from comment #23)
> Hi there, sorry for taking so long. Do you still need a canary run?

No, I will not land this patch anymore.
Flags: needinfo?(VYV03354)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: