HandshakeCallback may stop before completing all necessary tasks

RESOLVED FIXED in mozilla25

Status

()

Core
Security: PSM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

In HandshakeCallback:

>   if (SECSuccess != SSL_SecurityStatus(fd, &sslStatus, &cipherName, &keyLength,
>                                        &encryptBits, &signer, nullptr)) {
>     return;
>   }

This early return means that we do not any of the rest of the work in HandshakeCallback(). However, the handshake (and thus the connect) will continue as though everything is normal. This seems wrong. We should be stopping the handshake here in some way.

I haven't analyzed this to see whether it causes any security issues, so I am hiding this bug for now.
It may fail only when nss ssl socket is not found in the layers [1].  Then I believe nothing can work.  First sight only thought...


[1] http://hg.mozilla.org/mozilla-central/annotate/352bd17710c8/security/nss/lib/ssl/sslauth.c#l69
Brian and Honza, any suggestions on who we should assign this to and a security rating?
Thanks Honza. I verified what Honza said in comment 1.

This is lower than sec-low and not exploitable unless we're already doing the wrong thing, as Honza noted in comment 1.
Group: core-security
Depends on: 832848
Created attachment 747748 [details] [diff] [review]
Clarify and simplify code flow in HandshakeCallback

Note that we don't support "low" or "null" cipher suites anymore; only "high" ones.

If you r+ this, please sr?honzab at the same time.
Assignee: nobody → bsmith
Status: NEW → ASSIGNED
Attachment #747748 - Flags: review?(dkeeler)
Depends on: 799007
Brian - how recent is this patch? It doesn't apply on central anymore...
Flags: needinfo?(bsmith)
(In reply to David Keeler (:keeler) from comment #5)
> Brian - how recent is this patch? It doesn't apply on central anymore...

It should apply on top of the patch for bug 832848. I just made this patch yesterday.
Flags: needinfo?(bsmith)
Comment on attachment 747748 [details] [diff] [review]
Clarify and simplify code flow in HandshakeCallback

I spoke to Brian on irc about this not applying (even the patches in the dependent bug don't apply), so I'm going to clear the review until he gets back to me.
Attachment #747748 - Flags: review?(dkeeler)
Flags: needinfo?(bsmith)
Created attachment 763391 [details] [diff] [review]
Clarify and simplify code flow in HandshakeCallback [v2]

David, this version has been rebased on top of mozilla-inbound as of revision c401b8894929.
Attachment #747748 - Attachment is obsolete: true
Attachment #763391 - Flags: review?(dkeeler)
Flags: needinfo?(bsmith)
Comment on attachment 763391 [details] [diff] [review]
Clarify and simplify code flow in HandshakeCallback [v2]

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

Looks good to me.
Attachment #763391 - Flags: review?(dkeeler) → review+
Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5009e8f7601
Target Milestone: --- → mozilla25
https://hg.mozilla.org/mozilla-central/rev/c5009e8f7601
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 12

5 years ago
This commit also changed strings for status->mCipherName (visible in JS through nsISSLStatus.cipherName). Before the change this field contained values like "RC4", "3DES-EDE-CBC" or "AES-128" (from security/nss/lib/ssl/ssl3con.c:ssl3_cipherName[]). After the change this field contains the full ciphersuite name like "TLS_ECDHE_RSA_WITH_RC4_128_SHA".

This is visible in the "Page Info" window. E.g., the nightly build before this change (2013-07-14-03-02-02) shows this for https://www.google.com:

  Connection Encrypted: High-grade Encryption (RC4, 128 bit keys)

Builds since 2013-07-14-03-02-02, however, show:

  Connection Encrypted: High-grade Encryption (TLS_ECDHE_RSA_WITH_RC4_128_SHA, 128 bit keys)

Was this change intended? Note that getting the full TLS ciphersuite name is useful (e.g., to find out whether it provides perfect forward secrecy - TLS_ECDHE_RSA_WITH_RC4_128_SHA provides PFS, SSL_RSA_WITH_RC4_128_MD5 does not, but older Firefox versions display just "RC4, 128 bit keys" for both of them). However, this change may break some code (e.g., the CipherFox extension tries to parse nsISSLStatus.cipherName, but it does not work in nightly at all, most likely due to UI changes). If the breakage is not too big, getting the full ciphersuite name in JS would probably be a good thing.
(In reply to Sergey Vlasov from comment #12)
> This is visible in the "Page Info" window. E.g., the nightly build before
> this change (2013-07-14-03-02-02) shows this for https://www.google.com:
> 
>   Connection Encrypted: High-grade Encryption (RC4, 128 bit keys)
> 
> Builds since 2013-07-14-03-02-02, however, show:
> 
>   Connection Encrypted: High-grade Encryption
> (TLS_ECDHE_RSA_WITH_RC4_128_SHA, 128 bit keys)
> 
> Was this change intended? Note that getting the full TLS ciphersuite name is
> useful (e.g., to find out whether it provides perfect forward secrecy -
> TLS_ECDHE_RSA_WITH_RC4_128_SHA provides PFS, SSL_RSA_WITH_RC4_128_MD5 does
> not, but older Firefox versions display just "RC4, 128 bit keys" for both of
> them). However, this change may break some code (e.g., the CipherFox
> extension tries to parse nsISSLStatus.cipherName, but it does not work in
> nightly at all, most likely due to UI changes). If the breakage is not too
> big, getting the full ciphersuite name in JS would probably be a good thing.

Changing cipherName was not an intended outcome of this bug. It was simply a typo. However, I agree with you that it seems like a good idea to expose all of the information about the ciphersuite in the page info dialog box, if we're going to expose any. So, I am not going to file a follow-up bug to revert the behavior, especially since we already have bug 886752 on file to improve these strings further.

Gavin & Sergey: are either/both of you interested in working on bug 886752? I think that evilpie may now too busy to work on it, and he cares less about it because of this accident.

Comment 14

5 years ago
(In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (:bsmith) from comment #13)
> Changing cipherName was not an intended outcome of this bug. It was simply a
> typo. However, I agree with you that it seems like a good idea to expose all
> of the information about the ciphersuite in the page info dialog box, if
> we're going to expose any. So, I am not going to file a follow-up bug to
> revert the behavior, especially since we already have bug 886752 on file to
> improve these strings further.

Please keep the ciphersuite info exposed. It is useful.
Blocks: 897615

Comment 15

5 years ago
(In reply to mail from comment #14)
> (In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (:bsmith)
> from comment #13)
> > Changing cipherName was not an intended outcome of this bug. It was simply a
> > typo. However, I agree with you that it seems like a good idea to expose all
> > of the information about the ciphersuite in the page info dialog box, if
> > we're going to expose any. So, I am not going to file a follow-up bug to
> > revert the behavior, especially since we already have bug 886752 on file to
> > improve these strings further.
> 
> Please keep the ciphersuite info exposed. It is useful.

I agree with this.

Comment 16

5 years ago
PLEASE, please keep the full cipher suite listed. Showing more information to an informed user is better then filtering the cipher and making the user assume the encryption method. 

As an add on developer we can now query the full cipher suite and grade the encrypted connection. This is functionality our users have asked for since 2010 which we can now offer in Firefox 25.
You need to log in before you can comment on or make changes to this bug.