Last Comment Bug 783448 - When renegotiating, continue to use the client_version used in the initial ClientHello
: When renegotiating, continue to use the client_version used in the initial Cl...
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P1 normal (vote)
: 3.14
Assigned To: Wan-Teh Chang
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-16 19:00 PDT by Wan-Teh Chang
Modified: 2012-09-27 22:14 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (5.24 KB, patch)
2012-08-16 19:00 PDT, Wan-Teh Chang
brian: review+
Details | Diff | Review
Proposed patch v1.1 (5.29 KB, patch)
2012-09-27 21:52 PDT, Wan-Teh Chang
no flags Details | Diff | Review

Description Wan-Teh Chang 2012-08-16 19:00:36 PDT
Created attachment 652648 [details] [diff] [review]
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?) 2012-09-11 15:05:46 PDT
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?) 2012-09-27 10:13:08 PDT
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 2012-09-27 21:32:15 PDT
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 2012-09-27 21:52:33 PDT
Created attachment 665770 [details] [diff] [review]
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

Note You need to log in before you can comment on or make changes to this bug.