525092 - Support TLS false start in NSS

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Adam Langley]

Attachment: initial patch

TLS cut-through is where a client start sending application data before it has verified the Finished message from the server. This is a client-side only change which cuts a round-trip from the standard handshake.

If an attacker can persuade the client to use a weak cipher they may be able to read the initial records in a connection. Thus applications should only enable cut-through when weak ciphers have been removed from the list of acceptable ciphers.

I'm not confident that I understand the code well enough to make this patch, so I'm asking for help. The attached patch functions, but probably has unintended consequences. 

This mirrors the recent work on enabling this in OpenSSL:

http://bazaar.launchpad.net/~nagendra/openssl-patches/trunk/annotate/head:/handshake_cutthrough.patch

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

> TLS cut-through is where a client start sending application data before 
> it has verified the Finished message from the server 
  ... in a FULL handshake.

It must NEVER do that in an "abbreviated" (a.k.a. "resume") handshake.

Some TLS implementers interpret the language of RFC 5246 (pages 36 and 63) 
as prohibiting TLS cut-through.  With TLS cut-through, it is possible to 
send an entire http POST request without ever seeing the server's TLS 
finished message complete the handshake.  But I won't oppose this feature
provided that is is shown that the patch:
a) only affects full handshakes, not abbreviated ones
b) only affects clients, not servers
c) won't allow the client to receive application data before it has received
and validated a finished handshake message.
These things may be trivial to prove.  I just don't have time to do it now.

Comment 2

[Adam Langley]

Attachment: patch: not ready for prime time, just recording a work in progress.

Comment 3

[Adam Langley]

Attachment: Still not ready for prime time.

Comment 4

[Wan-Teh Chang]

Adam's patch is being reviewed in the Chromium code review site:
http://codereview.chromium.org/518065

We will attach a new patch when it has gone through my review,
and then ask Nelson to review it.

Comment 5

[Adam Langley]

Attachment: This is the patch that is currently running in Chromium

Comment 6

[Robert Relyea]

Comment on attachment 430722 [details] [diff] [review]
This is the patch that is currently running in Chromium

This patch may be correct, but I have some concerns. Nelson should really review this, but in order to facilitate his review, I'll comment on what I've seen.

I can easily confirm that this patch meets the following conditions Nelson outlined in comment 1:

a) only affects full handshakes.
b) only affects clients, not servers.

This is guarrenteed by the function ssl_CanFalseStart() which makes sure these and several other conditions are true any falseStart action is executed.

conidtion c is harder to verify. Most of this patch is set to drop out of loops 'early' (when wait_change_chiper or wait_new_session_ticket). This presumbably is to allow clients to start calling write functions. I suspect the state 'wait_change_cipher' is sufficient that if the client calls 'read', the underlying ssl engine will stall until it finished the SSL protocol.If that's true, then this condition is met as well.

ssl3_HandleServerHelloDone and ssl3_HandleFinished - 

The handshakeCallback is called if we are doing falseStart and the callback is set. This means if falseStart is set, we will call this callback twice: once when the client side of the handshakes are complete and once when the server side are complete (as opposed to the single time when both are complete).

Note that firstHsDone will always be false in both these cases. This makes me think that problem 3 described below is a bug.


ssl_GatherCompleteHandshake.

This is the first loop that drops out early. The code looks ok to my eyes (or course I could be missing a lot here).

ssl_Do_1stHandshake. 

This is the second loop that drops out early. I do have some questions, though...

1) Is it possible that rv can be set to some inconsitant value before we exit the loop? Without the break, we call (*ss->handshake)(ss) which will set rv. In the non-falseStart case, rv could be set to SECWouldBlock and the function will fail if it's has completed the handshake. Is it possible, however, for the handshake to return SECWouldBlock at the same time as changing the handshake state to wait_cipher_change. In the current code the handshake handler will get called again. In the new code the function will fail.

2) before we leave the loop in the normal case, we set ss->gs.readOffset and ss->gs.writeOffset? should one of those be initialized before the client starts to write?

3) what cause firstHSDone to get set in the false start case when the handshake really completes? As far as I can tell, this never happens.

---------------

It looks to me that we need to set up some state before we blow out here, and we probably should set firstHSDone in ssl3_HandleFinished if we are doing a falseStart, but I leave that to nelson.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Bob, your comment 6 above seems to suggest that you were going to ask me 
to SR this patch, but you actually marked the patch r-, not SR?.  

So, I'll wait for the next version of this patch which addresses your concerns
before reviewing it.

Comment 8

[Robert Relyea]

Comment on attachment 430722 [details] [diff] [review]
This is the patch that is currently running in Chromium

We can do it that way. I've identified some areas in the patch which worries me (marked in my previous comment). 

It may be my concerns are unfounded. If the do have basis, then this patch is probably a r- (which is why I marked it that way).

If the have no basis, then things are probably ok. So I'll go ahead and cancel my r- and put a sr? for you nelson.

Thanks.

bob

Comment 9

[Adam Langley]

Thank you for your detailed review. I'll give it the time it needs and respond, hopefully tomorrow.


Cheers

Comment 10

[Wan-Teh Chang]

The Internet-Draft specifying TLS False Start has been announced:
http://www.ietf.org/mail-archive/web/tls/current/msg06321.html

I'd also like to add that I've reviewed AGL's patch that's
currently running in Chromium, but it should still get an
authoritative review by Nelson.

Comment 11

[Robert Sayre]

Can we get this in our copy of NSS, please? It's been awaiting review for several months now.

Comment 12

[Wan-Teh Chang]

Attachment: Patch v5

I made the following changes to Adam's patch.
1. Change some TABs to spaces in ssl.h.
2. Add "rv = SECSuccess" before dropping out of the loop
in ssl_Do1stHandshake in sslsecur.c.  This fixes issue 1
noted by Bob Relyea in comment 6.
3. In sslstress.txt, use -g instead of -h in the new cases
because the new command-line option for strsclnt and tstclnt
is -g.

Bob, thank you for the detailed review in comment 6.  I
agree with you that the ssl3_CanFalseStart function guarantees
conditions a and b Nelson outlined in comment 1.  I am afraid
that the patch does not meet condition c because the
ssl_Do1stHandshake change that allows ssl_SecureSend to write
early will also allow ssl_SecureRecv to read early.

One possible solution is to not change ssl_Do1stHandshake.
Instead, change ssl_SecureSend so that it won't call
ssl_Do1stHandshake if ssl3_CanFalseStart returns true.

There are a total of three callers of ssl_Do1stHandshake:
http://mxr.mozilla.org/security/ident?i=ssl_Do1stHandshake&filter=

SSL_ForceHandshake also calls ssl_Do1stHandshake, but
only for SSL2, so we can ignore that ssl_Do1stHandshake
call:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsecur.c&rev=1.43&mark=375,393,406-407#374

Adam, what do you think my proposed solution?

Bob, in comment 6 you said that we will call the handshakeCallback
twice.  This won't happen because one is called if ssl3_CanFalseStart
returns true, and the other is called if ssl3_CanFalseStart returns
false.

The patch does not change when we set firstHsDone to true.
firstHsDone is still set to true only when the first handshake
really completes, in ssl3_HandleFinished:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.142&mark=8423#8423

Comment 13

[Wan-Teh Chang]

This patch interdiff shows that we will only call the handshakeCallback once:
https://bugzilla.mozilla.org/attachment.cgi?oldid=430722&action=interdiff&newid=461101&headers=1

Comment 14

[Wan-Teh Chang]

Attachment: Patch v6

I found that I already addressed Nelson's condition c
unknowingly when I worked on a Chrome bug:
http://codereview.chromium.org/850008

Here is an updated patch, which implements the
solution I proposed in comment 12.  I need to start
taking ginkgo biloba supplements now.

Comment 15

[Adam Langley]

(In reply to comment #14)
> One possible solution is to not change ssl_Do1stHandshake.
> Instead, change ssl_SecureSend so that it won't call
> ssl_Do1stHandshake if ssl3_CanFalseStart returns true.

As ssl3_CanFalseStart tests that we have a current write cipherspec, I believe that would be a good fix. Thanks!


AGL

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, Let me know if Ginko helps you any. :)

> Instead, change ssl_SecureSend so that it won't call
> ssl_Do1stHandshake if ssl3_CanFalseStart returns true.

... and ssl2 is disabled.

(I really wish we could get rid of the SSL2 code, finally.)

Comment 17

[Wan-Teh Chang]

(In reply to comment #16)
>
> > Instead, change ssl_SecureSend so that it won't call
> > ssl_Do1stHandshake if ssl3_CanFalseStart returns true.
> 
> ... and ssl2 is disabled.

Yes.  The patch checks both ss->version >= SSL_LIBRARY_VERSION_3_0
and ssl3_CanFalseStart() in ssl_SecureSend.

Comment 18

[Wan-Teh Chang]

I now believe the patch does not guarantee Nelson's condition c.

Although ssl_SecureRecv does not skip the ssl_Do1stHandshake
call,  ssl_Do1stHandshake calls ss->handshake, which I believe
is ssl_GatherRecord1stHandshake:
http://mxr.mozilla.org/security/search?string=ss-%3Ehandshake

ssl_GatherRecord1stHandshake calls ssl3_GatherCompleteHandshake.
But ssl3_GatherCompleteHandshake is changed to drop out early
for false start.

A possible solution is to pass an argument to these handshake
functions that indicates whether we're about to do a Read or Write
immediately after the handshake completes.  Given this info,
ssl3_GatherCompleteHandshake will drop out early only if the
upcoming IO operation is Write.

To avoid changing the handshake function's prototype, we can
add this "upcoming IO operation" info to the 'ss' structure.

Comment 19

[Wan-Teh Chang]

Attachment: Patch v6.1 (checked in)

I've now studied the relevant code in depth, and convinced
myself that the patch also guarantees Nelson's condition c.

I added a comment to the patch to record a new understanding.
Other than that, this patch is the same as patch v6.

The reason condition c is guaranteed is the do-while loop
in ssl3_GatherAppDataRecord:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3gthr.c&rev=1.9&mark=187,193,195,206,210,229,234-236#186

Although the do-while loop in ssl3_GatherCompleteHandshake
drops out early because of false start, the do-while loop
in ssl3_GatherAppDataRecord causes us to go back to reading
and handling handshake messages (if ss->gs.buf.len == 0,
it's a handshake/change cipher spec message).

A premature application data message (before ss->firstHsDone
becomes true) will result in a SECFailure because of a recent
bug fix in ssl3_HandleRecord:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.142&mark=9017-9018,9022#9014

Re: Bob's question about ss->gs.readOffset and ss->gs.writeOffset:
I verified that the patch deals with ss->gs (only used for reading)
properly.

Finally, after we send the change cipher spec message, the
SSL socket can send application data because ss->ssl3.cwSpec
is ready.

So I am now satisfied with this patch.

Comment 20

[Wan-Teh Chang]

Comment on attachment 461436 [details] [diff] [review]
Patch v6.1 (checked in)

I checked in the patch on the NSS trunk (NSS 3.13).

Checking in cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.68; previous revision: 1.67
done
Checking in cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.63; previous revision: 1.62
done
Checking in lib/ssl/ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.39; previous revision: 1.38
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.143; previous revision: 1.142
done
Checking in lib/ssl/ssl3gthr.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3gthr.c,v  <--  ssl3gthr.c
new revision: 1.10; previous revision: 1.9
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.78; previous revision: 1.77
done
Checking in lib/ssl/sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.44; previous revision: 1.43
done
Checking in lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.68; previous revision: 1.67
done
Checking in tests/ssl/sslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslstress.txt,v  <--  sslstress.txt
new revision: 1.19; previous revision: 1.18
done

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, You are the most thorough reviewer I know.  After reading your 
comment 19, I have little or no doubt that the code presently does guarantee
my condition 'c'.  But I am concerned that it is a fragile guarantee.  To 
maintain that guarantee, there are many parts of the code that must work 
just right.  If any one of them is changed, it may break this guarantee,
and yet that fact may be totally inobvious at that time.  

I wonder if we can make it a more robust guarantee by using a state flag.
If the SSLSocket had a flag that meant "Client has received and validated a finished handshake message", then we could test that flag at various places
and refuse to accumulate any application data for the client's use while it
remains false.

Comment 22

[Wan-Teh Chang]

Nelson, thanks for your review comment.

I believe the existing  firstHsDone flag is
exactly the state flag you asked for.  This MXR
query shows where libSSL sets firstHsDone to PR_TRUE:
http://mxr.mozilla.org/security/ident?i=firstHsDone&filter=

Right now libSSL sets firstHsDone to PR_TRUE
only at the real end of a handshake, and this
recent bug fix should do the spirit of your
suggestion (it still accumulates application
data, but won't give it to the caller):
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.142&mark=9017-9018,9022#9014

I can look into changing ssl3_GatherData to
check firstHsDone.

Comment 23

[Wan-Teh Chang]

Comment on attachment 461436 [details] [diff] [review]
Patch v6.1 (checked in)

I checked in the patch on the NSS_3_12_BRANCH (NSS 3.12.8).

Checking in cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.67.2.1; previous revision: 1.67
done
Checking in cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.62.2.1; previous revision: 1.62
done
Checking in lib/ssl/ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.38.2.1; previous revision: 1.38
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.142.2.1; previous revision: 1.142
done
Checking in lib/ssl/ssl3gthr.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3gthr.c,v  <--  ssl3gthr.c
new revision: 1.9.20.1; previous revision: 1.9
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.77.2.1; previous revision: 1.77
done
Checking in lib/ssl/sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.43.2.1; previous revision: 1.43
done
Checking in lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.67.2.1; previous revision: 1.67
done
Checking in tests/ssl/sslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslstress.txt,v  <--  sslstress.txt
new revision: 1.18.2.1; previous revision: 1.18
done

Comment 24

[Wan-Teh Chang]

I'm leaving the bug open until NSS 3.12.8 is released so that Nelson
can see the superreview request.

sayrer: I don't know how hard it is to get this patch into Firefox 4 at
this stage.  A simple patch for mozilla/security/manager is needed
to enable the SSL_ENABLE_FALSE_START option, and a Firefox
preference to turn it off.

Comment 25

[Wan-Teh Chang]

Comment on attachment 461436 [details] [diff] [review]
Patch v6.1 (checked in)

I'm requesting approval2.0 on this patch.

The benefit of this patch is that it will enable Firefox
to use TLS False Start, a client-side change to the TLS/SSL
protocol that can reduce the latency due to TLS/SSL full
handshake.  Note that a mozilla/security/manager patch is
still needed to enable TLS False Start.

The compatibility/regression risk of this patch is near
zero because it's easy to convince yourself that if the
ss->opt.enableFalseStart option is PR_FALSE (the default),
this patch is effectively a no-op.

Comment 26

[Wan-Teh Chang]

Comment on attachment 461436 [details] [diff] [review]
Patch v6.1 (checked in)

I pushed this patch to mozilla-central in patchset 8638116b698f:
http://hg.mozilla.org/mozilla-central/rev/8638116b698f

Comment 27

[Wan-Teh Chang]

Attachment: Allow two more SSL functions to be called early (checked in)

Mozilla's HandshakeCallback (in mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp)
calls two NSS SSL functions that Chromium doesn't
call, so for Mozilla to enable TLS false start,
these two SSL functions need to work when the handshake
callback is called early due to false start.

Comment 28

[Adam Langley]

Comment on attachment 462446 [details] [diff] [review]
Allow two more SSL functions to be called early (checked in)

I suggested rather than call it "enoughFirstHsDone", I would name it differently in each function. Maybe "haveSecurityState" and "haveExtensionInformation".

However, wtc gave a good reason for keeping it as it is (grepability), so LGTM.

Comment 29

[Wan-Teh Chang]

Comment on attachment 462446 [details] [diff] [review]
Allow two more SSL functions to be called early (checked in)

I checked in the patch on the NSS trunk (NSS 3.13)
and the NSS_3_12_BRANCH (NSS 3.12.8).

Checking in sslauth.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslauth.c,v  <--  sslauth.c
new revision: 1.17; previous revision: 1.16
done
Checking in sslreveal.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslreveal.c,v  <--  sslreveal.c
new revision: 1.8; previous revision: 1.7
done

Checking in sslauth.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslauth.c,v  <--  sslauth.c
new revision: 1.16.66.1; previous revision: 1.16
done
Checking in sslreveal.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslreveal.c,v  <--  sslreveal.c
new revision: 1.7.2.1; previous revision: 1.7
done

Comment 30

[Wan-Teh Chang]

Attachment: Allow one more SSL function to be called early (checked in)

SSL_GetChannelInfo needs the same fix.

I searched for the "ss->opt.useSecurity && ss->firstHsDone"
pattern in the NSS source tree.  SSL_GetSessionID and
ssl_Poll may also need changing to accommodate false start.
SSL_GetSessionID doesn't seem to be used, and ssl_Poll is
more involved.  So I didn't address them in this patch.

Comment 31

[Wan-Teh Chang]

Comment on attachment 469741 [details] [diff] [review]
Allow one more SSL function to be called early (checked in)

Patch checked in on the NSS trunk (NSS 3.13) and NSS_3_12_BRANCH
(NSS 3.12.8).

Checking in sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.24; previous revision: 1.23
done

Checking in sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.23.2.1; previous revision: 1.23
done