950858 - MOZ_CRASH("impossible cipher suite") hit in Firefox 26 release

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

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

See:
http://hg.mozilla.org/releases/mozilla-release/annotate/39faf812aaec/security/manager/ssl/src/nsNSSCallbacks.cpp#l1106
http://forums.mozillazine.org/viewtopic.php?f=9&t=2783195
http://forums.mozillazine.org/viewtopic.php?f=9&t=2783195

This patch got uplifted to Firefox 26. The patch only accounts for the cipher suites that are enabled in Firefox 27 and later. However, this is a different set of cipher suites than is enabled in Firefox 26.

Also, MOZ_CRASH is too extreme. MOZ_ASSERT is better for this type of thing.

Comment 1

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

Special thanks to David Balažic for bringing this to my attention.

Comment 3

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

The problem is slightly different than I originally expected. The real problem is that we accidentally omitted the case for TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA in the switch statement. I cross-checked the switch cases against the list of implemented cipher suites in Firefox 26 and TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA is the only one that is missing.

STR:

1. Create a test server using OpenSSL (I used version OpenSSL 0.9.8k from MozillaBuild, so almost any version of OpenSSL should be acceptable):

openssl ecparam -out ec_key.pem -name prime256v1 -genkey
openssl req -new -key ec_key.pem -x509 -nodes -days 365 -out cert.pem -subj "/CN=localhost"
openssl s_server  -accept 443 -www -cert cert.pem -key ec_key.pem -cipher "ECDHE-ECDSA-DES-CBC3-SHA"

(NOTE: On Windows inside a MinGW/MSYS shell, replace "/CN=localhost" with "//CN=localhost".)

This server will only negotiate the TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA cipher suite.

2. In Firefox, navigate to https://localhost using the address bar.
3. Override the certificate error.
4. <CRASH>

Note that this will only reproduce in Firefox 26. Firefox 27+ disable the TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA cipher suite and it cannot be enabled in those versions.

Comment 4

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

Attachment: AccumulateCipherSuite-crash.patch

[Approval Request Comment]
Regression caused by (bug #): bug 942728

User impact if declined: crash on a very small number of sites

Testing completed (on m-c, etc.): manually tested; see above. This bug only affects Firefox 26.

Risk to taking this patch (and alternatives if risky): Very, very low risk

String or IDL/UUID changes made by this patch: none

Comment 5

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8348522 [details] [diff] [review]
AccumulateCipherSuite-crash.patch

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

If I understand correctly, the issue is that the CipherPrefs array in nsNSSComponent.cpp has TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA but the telemetry doesn't handle the case that the server chooses it. If that's the case, it looks like CipherPrefs also has SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA, which isn't handled by the telemetry either. Am I understanding correctly that this will have the same problem?

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +1120,4 @@
>        value = 0;
>        break;
>    }
> +  MOZ_ASSERT(value != 0);

Is this a change we want to make on the other branches/central as well?

Comment 6

[David Balažic]

Confirming, SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA is the one used by the server in the original report.
(sent email to Brian about this, but no reply yet)

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

We're unthrottling FF26 today, will keep an eye on this in the next 24 hours to see if there's much user impact - ideally we can put this in known issues and find a workaround for the duration of FF26.

Comment 8

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

Attachment: AccumulateCipherSuite-crash.patch

Thanks for catching my oversight, David and David!

Comment 9

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8351076 [details] [diff] [review]
AccumulateCipherSuite-crash.patch

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

LGTM.

Comment 10

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

Comment on attachment 8351076 [details] [diff] [review]
AccumulateCipherSuite-crash.patch

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

This code should be dead code. However, we thought that it was dead code in Firefox 26 too, and it wasn't. This patch makes the code less likely to crash in the event that the code isn't dead code. Very low risk (negative risk?), no string changes. Regression caused by bug 942728.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f21e31b7f336

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/62d21984ab23
https://hg.mozilla.org/releases/mozilla-beta/rev/6d4df8edfaaa

Is this something that affects B2G as well? If so, should we consider nominating it for v1.2?

Comment 14

[Dana Keeler (she/her) [:keeler]]

If telemetry is enabled on B2G (and as far as I can tell it is), then yes, it would affect it.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is this something important enough to merit backporting to v1.2?

Comment 16

[u279076]

Brian, do you already have a server set up to test this?

Comment 17

[Dana Keeler (she/her) [:keeler]]

I was able to reproduce this on b2g26 by disabling every cipher suite but TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA (either set all of the security.ssl3.* prefs to false but "security.ssl3.ecdhe_ecdsa_des_ede3_sha" in some pref file (I never actually got this working) or just remove every entry but TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA in the CipherPrefs array nsNSSComponent.cpp and recompile) and visiting https://google.com. In most cases, I think the client/server will negotiate a better cipher, but as we've seen, there are definitely some servers that will only negotiate this. Since this crashes the phone, it's probably worth taking.

Comment 18

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

Clearing NEEDINFO since David answered the question.

Comment 19

[u279076]

Marking this as [qa-] for verification on desktop. If there's something we can do to verify this is fixed, please advise.

Comment 20

[David Balažic]

Status with FF 27.0 on Windows 7 64bit:

Instead of crash now I get:

Secure Connection Failed

An error occurred during a connection to server:8443. Cannot communicate securely with peer: no common encryption algorithm(s). (Error code: ssl_error_no_cypher_overlap)

Comment 21

[Dana Keeler (she/her) [:keeler]]

(In reply to David Balažic from comment #20)
> Status with FF 27.0 on Windows 7 64bit:
> 
> Instead of crash now I get:
> 
> Secure Connection Failed
> 
> An error occurred during a connection to server:8443. Cannot communicate
> securely with peer: no common encryption algorithm(s). (Error code:
> ssl_error_no_cypher_overlap)

Looks like a different bug - please file a new one with steps to reproduce.