Closed Bug 950858 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox26 --- wontfix
firefox27 --- fixed
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.2 ? affected
b2g-v1.3 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

(Keywords: csectype-dos, regression, Whiteboard: [qa-])

Attachments

(2 files)

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.
Special thanks to David Balažic for bringing this to my attention.
Duplicate of this bug: 950777
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.
[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
Attachment #8348522 - Flags: review?(dkeeler)
Attachment #8348522 - Flags: approval-mozilla-release?
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?
Attachment #8348522 - Flags: review?(dkeeler) → review-
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)
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.
Thanks for catching my oversight, David and David!
Attachment #8351076 - Flags: review?(dkeeler)
Comment on attachment 8351076 [details] [diff] [review]
AccumulateCipherSuite-crash.patch

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

LGTM.
Attachment #8351076 - Flags: review?(dkeeler) → review+
Attachment #8348522 - Flags: approval-mozilla-release?
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.
Attachment #8351076 - Flags: approval-mozilla-beta?
Attachment #8351076 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f21e31b7f336
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 961645
Attachment #8351076 - Flags: approval-mozilla-beta?
Attachment #8351076 - Flags: approval-mozilla-beta+
Attachment #8351076 - Flags: approval-mozilla-aurora?
Attachment #8351076 - Flags: approval-mozilla-aurora+
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?
status-b2g-v1.2: --- → ?
Flags: needinfo?(brian)
If telemetry is enabled on B2G (and as far as I can tell it is), then yes, it would affect it.
Flags: needinfo?(brian)
Is this something important enough to merit backporting to v1.2?
Brian, do you already have a server set up to test this?
Flags: needinfo?(brian)
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.
Clearing NEEDINFO since David answered the question.
Flags: needinfo?(brian)
Marking this as [qa-] for verification on desktop. If there's something we can do to verify this is fixed, please advise.
Whiteboard: [qa-]
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)
(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.
You need to log in before you can comment on or make changes to this bug.