774547 - Sets the record layer version number of the initial ClientHello to at most { 3, 1 } (TLS 1.0)

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

This bug was originally reported in Chromium issue 136666:
http://code.google.com/p/chromium/issues/detail?id=136666

Some websites ignore ClientHello.client_version and only use the record
layer version number of ClientHello when negotiating protocol versions.
In addition, these websites reset the TCP connection if the record layer
version number is { 3, 2 } (TLS 1.1).

We know of three such websites:
  https://www.billpaysite.com/
  https://cybozulive.com/
  https://members.mh-frontier.jp/mypage/

(The last two websites seem to use F5 BIG-IP.)

The TLS 1.2 RFC (RFC 5246), Appendix E.1 talks about this issue:

   Earlier versions of the TLS specification were not fully clear on
   what the record layer version number (TLSPlaintext.version) should
   contain when sending ClientHello (i.e., before it is known which
   version of the protocol will be employed).  Thus, TLS servers
   compliant with this specification MUST accept any value {03,XX} as
   the record layer version number for ClientHello.

   TLS clients that wish to negotiate with older servers MAY send any
   value {03,XX} as the record layer version number.  Typical values
   would be {03,00}, the lowest version number supported by the client,
   and the value of ClientHello.client_version.  No single value will
   guarantee interoperability with all old servers, but this is a
   complex topic beyond the scope of this document.

The OpenSSL CHANGES file describes a change that seems related to this
issue:

   *) Workarounds for some broken servers that "hang" if a client hello
     record length exceeds 255 bytes.

     1. Do not use record version number > TLS 1.0 in initial client
        hello: some (but not all) hanging servers will now work.
     2. If we set OPENSSL_MAX_TLS1_2_CIPHER_LENGTH this will truncate
        the number of ciphers sent in the client hello. This should be
        set to an even number, such as 50, for example by passing:
        -DOPENSSL_MAX_TLS1_2_CIPHER_LENGTH=50 to config or Configure.
        Most broken servers should now work.
     3. If all else fails setting OPENSSL_NO_TLS1_2_CLIENT will disable
        TLS 1.2 client support entirely.
     [Steve Henson]

Attached is a patch that sets the record layer version number of ClientHello
to at most { 3, 1 } (TLS 1.0) if we don't know what version the server
supports.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch v2

It turns out this workaround should only be made to the initial ClientHello.
The record layer version number of the ClientHello in a renegotiation should
use the currently negotiated version number, otherwise at least OpenSSL
servers will report an error.

OpenSSL CVS trunk had this bug, described in OpenSSL ticket #2811:
TLSv1.1+ renegotiation broken
http://rt.openssl.org/Ticket/Display.html?id=2811
(Enter user:guest password:guest if prompted.)
OpenSSL patch: http://cvs.openssl.org/chngview?cn=22565

Comment 2

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

Comment on attachment 652651 [details] [diff] [review]
Proposed patch v2

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

r+. I suggested some changes. If you think those changes are wrong then just explain why. If you think my suggestions need more investigation then we can make my suggested changes in a separate bug after doing that investigation.

::: mozilla/security/nss/lib/ssl/ssl3con.c
@@ +2222,5 @@
>      } else {
> +	SSL3ProtocolVersion version = cwSpec->version;
> +
> +	if (capRecordVersion) {
> +	    version = PR_MIN(SSL_LIBRARY_VERSION_TLS_1_0, version);

What if SSL 3.0 and TLS 1.0 are disabled?

It seems like it would be better if this were:

version = PR_MIN(PR_MAX(SSL_LIBRARY_VERSION_TLS_1_0, ss->vrange.min),
                 version);

So that we'd avoid, as much as possible, potential problems with servers that have the opposite problem problem (rejecting handshakes when the record version number is too low).

@@ +2257,5 @@
>   * ssl_SEND_FLAG_USE_EPOCH (for DTLS)
>   *    Forces the use of the provided epoch
> + * ssl_SEND_FLAG_CAP_RECORD_VERSION
> + *    Caps the record layer version number of TLS ClientHello to { 3, 1 }
> + *    (TLS 1.0). Some TLS 1.0 servers (which seem to use F5 BIG-IP) ignore 

If you make the above change:
"Caps the record layer version number to the minimum enabled version of TLS (not including SSL 3.0)."

Nit: Maybe it is not a good idea to mention other venders and products by name in the source code, but it is good to do mention them in Bugzilla.

Comment 3

[Wan-Teh Chang]

Brian: thank you for the review and the thoughtful suggestions.

(In reply to Brian Smith (:bsmith) from comment #2)
>
> What if SSL 3.0 and TLS 1.0 are disabled?
> 
> It seems like it would be better if this were:
> 
> version = PR_MIN(PR_MAX(SSL_LIBRARY_VERSION_TLS_1_0, ss->vrange.min),
>                  version);
> 
> So that we'd avoid, as much as possible, potential problems with servers
> that have the opposite problem problem (rejecting handshakes when the record
> version number is too low).

The change you suggested seems reasonable. However, it will cause a different
behavior. Suppose only TLS 1.1 is enabled on our side and we talk to an F5 BIG-IP
server.

With my patch:
- We send a ClientHello with client_version={ 3, 2 } and record layer version={ 3, 1 }
- F5 BIG-IP sends a ServerHello with server_version={ 3, 1 }
- We abort the handshake with a protocol_version alert
- We report local NSS error code SSL_ERROR_NO_CYPHER_OVERLAP

With your suggested change:
- We send a ClientHello with client_version={ 3, 2 } and record layer version={ 3, 2 }
- F5 BIG-IP resets the TCP connection
- We report local NSS error code PR_CONNECT_RESET_ERROR

The first outcome seems better because the error code is more informative.

The following is the guidance from TLS 1.2 (RFC 5246) Appendix E.1:

   Earlier versions of the TLS specification were not fully clear on
   what the record layer version number (TLSPlaintext.version) should
   contain when sending ClientHello (i.e., before it is known which
   version of the protocol will be employed).  Thus, TLS servers
   compliant with this specification MUST accept any value {03,XX} as
   the record layer version number for ClientHello.

   TLS clients that wish to negotiate with older servers MAY send any
   value {03,XX} as the record layer version number.  Typical values
   would be {03,00}, the lowest version number supported by the client,
   and the value of ClientHello.client_version.  No single value will
   guarantee interoperability with all old servers, but this is a
   complex topic beyond the scope of this document.

So hopefully new servers won't reject handshakes when the record layer
version number is { 3, 1 }.

> Nit: Maybe it is not a good idea to mention other venders and products by
> name in the source code, but it is good to do mention them in Bugzilla.

It is important for future maintainers of NSS to know when this workaround can
be removed. Only a single product seems to exhibit this behavior. I think that's
useful info.

Comment 4

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

Thanks Wan-Teh, that makes sense. The patch is already r+d.

Comment 5

[Wan-Teh Chang]

Patch checked in on the NSS trunk (NSS 3.14).

Checking in dtlscon.c;
/cvsroot/mozilla/security/nss/lib/ssl/dtlscon.c,v  <--  dtlscon.c
new revision: 1.4; previous revision: 1.3
done
Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.187; previous revision: 1.186
done
Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.107; previous revision: 1.106
done