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 ClientHello to work around a Windows SChannel bug.

Cap the record layer version number to TLS 1.0 only for the initial
ClientHello. The record layer version number of the ClientHello in
a renegotiation should use the currently negotiated version number.

R=agl@chromium.org,rsleevi@chromium.org
BUG=141629
TEST=Visit https://solutionscenter.naradana.net/, an IIS server that
requests (but doesn't require) client certificates over renegotiation.
The page should be laid out correctly.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152116

[wtc, 2012-08-15 03:20:46]

Please review.

I will add the patch file and update README.chromium after
both of you are happy with the CL.  Thanks.

[Ryan Sleevi, 2012-08-15 03:39:07]

I'm having a hard time parsing the (if (sidOK)) section, so I'll take a look
again tomorrow and hope I can grok it, but comments below.

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:789:
PORT_Assert(ssl3_VersionIsSupported(ss->protocolVariant, ss->version));
This is related to your change to ekr's patch, right?

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:4106: if (ss->firstHsDone) {
if (sidOK), isn't a guarantee that firstHsDone is true, since it means we have
previously negotiated a session?

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:4108: ss->version = ss->clientHelloVersion;
It seems implicit in this is the assumption that sid->version cannot be greater
than ss->clientHelloVersion, right? Is that a safe assumption [it would seem so,
I just want to confirm]

ssl3_NegotiateVersion ensures that sid->version is bounded within (min, max), so
I wanted to make sure we couldn't accidentally blow over max here.

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:4114: PR_FALSE) != SECSuccess) {
nit: Shouldn't it be two tabs, then 4 spaces (matching indent of the if), then
spaces to align the PR_FALSE with the opening paren on 4113?

Same with line 4156

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:4381: ss->ssl3.hs.ws = wait_server_hello;
Did you mean to move this? It doesn't seem related.

Previously, the wait state would only be updated if rv == SECSuccess, where rv
was set on 4386

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
File net/third_party/nss/ssl/sslsock.c (right):

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
net/third_party/nss/ssl/sslsock.c:1924: if
(!ssl3_VersionRangeIsValid(ss->protocolVariant, vrange)) {
same here?

[agl, 2012-08-15 04:02:23]

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:2289: * TLS initial ClientHello. */
(nit: I think there should be a 'the' in this comment.)

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:4106: if (ss->firstHsDone) {
On 2012/08/15 03:39:07, Ryan Sleevi wrote:
> if (sidOK), isn't a guarantee that firstHsDone is true, since it means we have
> previously negotiated a session?

I'm not quite sure that I understand your question, but I read sidOK to be that
we have a session to attempt a resumption with. But that's doesn't mean that
we've already done a handshake.

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:4110: sidOK = PR_FALSE;
Is this case ok? If we reject sid because it's version is greater than
clientHelloVersion, might we still end up sending a version other than
clientHelloVersion in the renegotiation?

I'm not sure why this section isn't just:

if (sidOK && ssl3_NegotiateVersion(ss, sid->version, PR_FALSE) != SECSuccess) {
    sidOK = PR_FALSE;
}

if (ss->firstHsDone)
  ss->version = ss->clientHelloVersion;

[Ryan Sleevi, 2012-08-15 04:09:44]

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/10828269/diff/5/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:4106: if (ss->firstHsDone) {
On 2012/08/15 04:02:23, agl wrote:
> On 2012/08/15 03:39:07, Ryan Sleevi wrote:
> > if (sidOK), isn't a guarantee that firstHsDone is true, since it means we
have
> > previously negotiated a session?
> 
> I'm not quite sure that I understand your question, but I read sidOK to be
that
> we have a session to attempt a resumption with. But that's doesn't mean that
> we've already done a handshake.

If we have a session to attempt a resumption with, doesn't that mean we've
exchanged Finished messages at least once, hence we've done a handshake?

Where I'm trying to understand MSFTs bug and this implementation is what the
expected result is when
1) Resuming a previous session on a new connection
2) Resuming a previous session on an existing connection (since the session ID
is not invalidated until the server says it is, and it could request
renegotiation but not invalidate the session)

The intermingling of session variables (sidOK, sid->version) with connection
variables (ss->firstHsDone, ss->clientHelloVersion) is what is giving me trouble
in reasoning.

[wtc, 2012-08-15 16:16:59]

Response to review comments on patch set 2:

Thank you for the review!  I anticipated some of your
questions and thought about annotated the CL with some
explanations.  I should have done that.  Given the large
number of questions, I will try to make the logic clearer
and add more comments.

Let me first answer your questions.

http://codereview.chromium.org/10828269/diff/5/net/third_party/nss/ssl/ssl3con.c
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/10828269/diff/5/net/third_party/nss/ssl/ssl3co...
net/third_party/nss/ssl/ssl3con.c:789:
PORT_Assert(ssl3_VersionIsSupported(ss->protocolVariant, ss->version));

Yes, this fixes an unrelated bug that I discovered while
studying the code that modifies ss->version.  I can remove
this bug fix from the CL.

http://codereview.chromium.org/10828269/diff/5/net/third_party/nss/ssl/ssl3co...
net/third_party/nss/ssl/ssl3con.c:4106: if (ss->firstHsDone) {

On 2012/08/15 04:09:44, Ryan Sleevi wrote:
>
> If we have a session to attempt a resumption with, doesn't that mean we've
> exchanged Finished messages at least once, hence we've done a handshake?

If ss->firstHsDone is true, it means the first handshake
on this SSL socket ('ss') is done.  The session we're
requesting to resume ('sid') was negotiated on some other
SSL socket.

> 1) Resuming a previous session on a new connection

This is the sidOK=true ss->firstHsDone=false case.  The
expected result is to set client_version of ClientHello
to the version in 'sid'.  NSS has always been doing this.
I just added a comment (lines 4091-4096) to explain the
rationale.

> 2) Resuming a previous session on an existing connection 

This is the sidOK=true ss->firstHsDone=true case.  This
CL changes the behavior of NSS in this case.  NSS used to
set client_version of the renegotiation ClientHello to
sid->version.  Because of the SChannel bug, NSS will need
to set client_version of the renegotiation ClientHello
to the client_version of the initial ClientHello, which
is still available in ss->clientHelloVersion at this point.
(That value will be overwritten on line 4263 below, where
I added an assertion.)

http://codereview.chromium.org/10828269/diff/5/net/third_party/nss/ssl/ssl3co...
net/third_party/nss/ssl/ssl3con.c:4108: ss->version = ss->clientHelloVersion;

In this case, the SChannel bug requires us to set
ss->version to ss->clientHelloVersion.  So if we want to
resume the session 'sid', we need to make sure sid->version
is enabled on this SSL socket.

Since the session 'sid' may have been negotiated on a
different SSL socket with a different version range,
sid->version may be greater than ss->clientHelloVersion.
While answering this question, I noticed that we should
also ensure sid->version >= ss->vrange.min.

http://codereview.chromium.org/10828269/diff/5/net/third_party/nss/ssl/ssl3co...
net/third_party/nss/ssl/ssl3con.c:4110: sidOK = PR_FALSE;

On 2012/08/15 04:02:23, agl wrote:
> Is this case ok? If we reject sid because it's version is greater than
> clientHelloVersion, might we still end up sending a version other than
> clientHelloVersion in the renegotiation?

This case will be handled correctly on lines 4152-4153
below.

> I'm not sure why this section isn't just:
> 
> if (sidOK && ssl3_NegotiateVersion(ss, sid->version, PR_FALSE) != SECSuccess)
{
>     sidOK = PR_FALSE;
> }
> 
> if (ss->firstHsDone)
>   ss->version = ss->clientHelloVersion;

I considered writing the code that way before, but
ssl3_NegotiateVersion is quite right for the renegotiation
case, where the version upper bound is ss->clientHelloVersion
instead of ss->vrange.max.

I also considered moving the SChannel bug workaround logic
into the ssl3_NegotiateVersion function.  Since that
function is also used by server-side code, I didn't want
to make it more complicated.  I will look into that again.

http://codereview.chromium.org/10828269/diff/5/net/third_party/nss/ssl/ssl3co...
net/third_party/nss/ssl/ssl3con.c:4114: PR_FALSE) != SECSuccess) {

On 2012/08/15 03:39:07, Ryan Sleevi wrote:
> nit: Shouldn't it be two tabs, then 4 spaces (matching indent of the if), then
> spaces to align the PR_FALSE with the opening paren on 4113?

The original code also uses the maximum number of tabs,
followed by just enough spaces to align.  I think as long
as the line starts with a tab, it'll look aligned in a
diff.

http://codereview.chromium.org/10828269/diff/5/net/third_party/nss/ssl/ssl3co...
net/third_party/nss/ssl/ssl3con.c:4381: ss->ssl3.hs.ws = wait_server_hello;

On 2012/08/15 03:39:07, Ryan Sleevi wrote:
> Did you mean to move this? It doesn't seem related.

Yes, this line needs to be moved here to support the
assertion on line 2293.

The NSS code that sets ss->ssl3.hs.ws when sending
ClientHello is not very clean.

For the initial ClientHello, ss->ssl3.hs.w3 is initialized
to wait_server_hello early in this function, inside the
ssl3_InitState call on line 4025:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3...

It then sets ss->ssl3.hs.ws to wait_server_hello again
here.

For a renegotiation ClientHello, the ssl3_InitState call
on line 4025 has no effect (because ss->ssl3.initialized
is true), so ss->ssl3.hs.ws is idle_handshake at this
point.  Therefore, if I don't move this assignment before
the ssl3_FlushHandshake call, the assertion on line 2293
will fail in a renegotiation.

Another solution would be to remove all the assertions
on line 2287-2294.  It does seem better to transition to
a new ss->ssl3.hs.ws only when everything succeeds.

http://codereview.chromium.org/10828269/diff/5/net/third_party/nss/ssl/sslsock.c
File net/third_party/nss/ssl/sslsock.c (right):

http://codereview.chromium.org/10828269/diff/5/net/third_party/nss/ssl/sslsoc...
net/third_party/nss/ssl/sslsock.c:1924: if
(!ssl3_VersionRangeIsValid(ss->protocolVariant, vrange)) {

On 2012/08/15 03:39:07, Ryan Sleevi wrote:
> same here?

Yes, this is part of the unrelated bug fix for EKR's
DTLS code.

[Ryan Sleevi, 2012-08-16 00:33:25]

I think I'm on board, I'm still just trying to reason about the ss->vrange code
and make sure I'm not missing any edge cases here.

http://codereview.chromium.org/10828269/diff/12001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/10828269/diff/12001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:4115: sid->version <= ss->clientHelloVersion)
{
I'm still having trouble parsing this, in part because it's ignoring
ss->vrange.max (unlike line 4121)

Should there also be a check that sid->version <= ss->vrange.max ? I'm not fully
sure when such a situation could arise, which is why I'm having trouble with
this block, trying to contrive one :)

http://codereview.chromium.org/10828269/diff/12001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:4161: ss->version = ss->clientHelloVersion;
Is it possible for the socket to be re-configured to negotiate a different set
of versions? This is caught by line 4163, but is not caught here, AFAICT.

[wtc, 2012-08-16 01:40:05]

Please review patch set 4.

Patch set 4 retained the structure of patch set 2, which
you reviewed.  I just added comments and made minor fixes.

I will now attempt a more drastic cleanup.  If I get something
I like, I will attach it for you to compare with patch set 4.

http://codereview.chromium.org/10828269/diff/5/net/third_party/nss/ssl/ssl3con.c
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/10828269/diff/5/net/third_party/nss/ssl/ssl3co...
net/third_party/nss/ssl/ssl3con.c:4106: if (ss->firstHsDone) {

On 2012/08/15 16:16:59, wtc wrote:
>
> If ss->firstHsDone is true, it means the first handshake
> on this SSL socket ('ss') is done.  The session we're
> requesting to resume ('sid') was negotiated on some other
> SSL socket.

I found that my reply has a typo. It should read (with the
change highlighted in CAPS):

If ss->firstHsDone is true, it means the first handshake on this SSL socket
('ss') is done. The session we're requesting to resume ('sid') MAY HAVE BEEN
negotiated on
some other SSL socket.

http://codereview.chromium.org/10828269/diff/12001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/10828269/diff/12001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:4115: sid->version <= ss->clientHelloVersion)
{

This is an important question.  Here is the reasoning.

1. Line 788 in ssl3_NegotiateVersion ensures that
ss->version <= ss->vrange.max:

  788 ss->version = PR_MIN(peerVersion, ss->vrange.max);

2. If ss->firstHsDone is false, ss->version is set by
a ssl3_NegotiateVersion call (on either line 4121 or
line 4163).

3. ss->version is copied to ss->clientHelloVersion on
line 4271:

    4271 ss->clientHelloVersion = ss->version;

4. Therefore, as long as ss->vrange.max is not changed
in between, we know ss->clientHelloVersion <= ss->vrange.max
here.  So sid->version <= ss->clientHelloVersion implies
sid->version <= ss->vrange.max.

http://codereview.chromium.org/10828269/diff/12001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:4161: ss->version = ss->clientHelloVersion;

On 2012/08/16 00:33:25, Ryan Sleevi wrote:
> Is it possible for the socket to be re-configured to negotiate a different set
> of versions?

An application can do that, but a lot of NSS code will break.
Ideally we should change SSL_VersionRangeSet to fail if
the SSL socket has already used the current ss->vrange.

http://codereview.chromium.org/10828269/diff/12001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:4169: sid = ssl3_NewSessionID(ss, PR_FALSE);

I considered handling the SChannel bug only once, at line
4174, for both the sid and no-sid cases.  But it is this
ssl3_NewSessionID call that prevented that, because
ss->version must be correct when we pass 'ss' to the
ssl3_NewSessionID call.

[wtc, 2012-08-16 02:27:31]

rsleevi asked me to replace assertions with real error
checking. I did that in patch set 5. Please review. Thanks.

[Ryan Sleevi, 2012-08-16 22:25:54]

lgtm

[commit-bot: I haz the power, 2012-08-17 13:00:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/10828269/8

[commit-bot: I haz the power, 2012-08-17 18:35:59]

Change committed as 152116