820887 - HandshakeCallback may stop before completing all necessary tasks

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

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.

Comment 1

[Honza Bambas (:mayhemer)]

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

Comment 2

[Al Billings [:abillings - ex-MoCo]]

Brian and Honza, any suggestions on who we should assign this to and a security rating?

Comment 3

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

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.

Comment 4

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

Attachment: 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.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Brian - how recent is this patch? It doesn't apply on central anymore...

Comment 6

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

(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.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 8

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

Attachment: Clarify and simplify code flow in HandshakeCallback [v2]

David, this version has been rebased on top of mozilla-inbound as of revision c401b8894929.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 10

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

Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5009e8f7601

Comment 11

[Ed Morley [:emorley]]

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

Comment 12

[Sergey Vlasov]

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.

Comment 13

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

(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

[Niklas Hambüchen]

(In reply to Brian Smith (:briansmith), was [e-mail] (: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.

Comment 15

[BoerenkoolMetWorst]

(In reply to mail from comment #14)
> (In reply to Brian Smith (:briansmith), was [e-mail] (: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

[calomelorg]

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.