783448 - When renegotiating, continue to use the client_version used in the initial ClientHello

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

When renegotiating, we need to continue to use the client_version
used in the initial ClientHello to work around a Windows SChannel
bug.

Windows SChannel compares the client_version inside the RSA
EncryptedPreMasterSecret of a renegotiation with the client_version
of the initial ClientHello rather than the ClientHello in the
renegotiation. This bug was described by Martin Rex in a message
to the IETF TLS working group on 22 Sep 2011:
http://www.ietf.org/mail-archive/web/tls/current/msg08079.html

It is also described in OpenSSL ticket #2702:
TLS bad_mac_record with IIS 7 and client authentication
http://rt.openssl.org/Ticket/Display.html?id=2702
(Enter user:guest password:guest if prompted.)
OpenSSL patch: http://cvs.openssl.org/chngview?cn=22087

Comment 1

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

Comment on attachment 652648 [details] [diff] [review]
Proposed patch

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

[This is probably easier to read within the Splinter tool.]

::: mozilla/security/nss/lib/ssl/ssl3con.c
@@ +4032,5 @@
> +     * During a renegotiation, ss->clientHelloVersion will be used again to
> +     * work around a Windows SChannel bug. Ensure that it is still enabled.
> +     */
> +    if (ss->firstHsDone) {
> +	if (SSL3_ALL_VERSIONS_DISABLED(&ss->vrange)) {

Is this defensive programming? Is it supported to call SSL_OptionSet and/or change the enabled version range after a connection has been established?

@@ +4038,5 @@
> +	    return SECFailure;
> +	}
> +
> +	if (ss->clientHelloVersion < ss->vrange.min ||
> +	    ss->clientHelloVersion > ss->vrange.max) {

Similarly, it seems like these checks must be redundant, because when we sent the initial handshake, we only chose a client hello version that was within range.

@@ +4275,1 @@
>      ss->clientHelloVersion = ss->version;

Why not this instead?:

/* We set the client hello version during the initial
 * handshake, and then never change it, to work arond
 * the Windows SChannel bug described above.
 */
if (!ss->firstHsDone) {
    ss->clientHelloVersion = ss->version;
}

This would be clearer than asserting that the assignment will have no effect.

Comment 2

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

In the meeting today, we decided we will check in Wan-Teh's original patch and leave the bug open to address the review comments, because addressing the review comments are just cleanup.

Comment 3

[Wan-Teh Chang]

Comment on attachment 652648 [details] [diff] [review]
Proposed patch

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

Brian, thank you for your review comments.

::: mozilla/security/nss/lib/ssl/ssl3con.c
@@ +4038,5 @@
> +	    return SECFailure;
> +	}
> +
> +	if (ss->clientHelloVersion < ss->vrange.min ||
> +	    ss->clientHelloVersion > ss->vrange.max) {

You are right. Originally my patch didn't have these two
checks, as this patch interdiff shows:

https://chromiumcodereview.appspot.com/10828269/diff2/5:8/net/third_party/nss/ssl/ssl3con.c#newcode4044

Ryan suggested that I add these checks in his code review.

The conditions detected by these checks can only happen
if the enabled version range of the socket is changed
after the initial handshake.

@@ +4275,1 @@
>      ss->clientHelloVersion = ss->version;

I wanted to assert that for renegotiation, we must have
set ss->version to ss->clientHelloVersion. I agree that
my comment is not clear.

The statement
  ss->clientHelloVersion = ss->version;
is an important invariant of the code, so I think it
should be done unconditionally. If this invariant does not
hold, NSS may be confused.

Comment 4

[Wan-Teh Chang]

Attachment: Proposed patch v1.1

I improved the comment a little. Patch checked in
on the NSS trunk (NSS 3.14).

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.191; previous revision: 1.190
done