Make SSL False Start work with asynchronous certificate validation

Make SSL False Start work with asynchronous certificate validation
(SSL_AuthCertificateComplete).

Patch by Brian Smith <brian@briansmith.org>;.

NSS bug https://bugzilla.mozilla.org/show_bug.cgi?id=713933

R=agl@chromium.org
BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227704

[wtc, 2013-09-13 00:46:55]

Here is an NSS patch for the SSL False Start code from
Brian Smith. (I will also review this patch.)

I also changed ssl_client_socket_nss.cc to take advantage of
the new CanFalseStartCallback design.

I will create the patch file after the review is done.

[agl, 2013-09-13 15:03:28]

LGTM

I don't know the async verification flow well enough to say much about this code
in relation to that, but I assume that Brian has tested that.

My only overall comment would be that I would remove the support for
applications that did False Start prior to this. I suspect that there aren't any
other than Chromium, it adds complexity code in the code, and the penalty (False
Start doesn't trigger) is minor.

https://codereview.chromium.org/23621040/diff/22001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/23621040/diff/22001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:717: // Called by NSS to determine if we can
false start.
In other comments, "false start" is written as "False Start".

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2818: PRINT_BUF(50, (ss, "Send record (plain
text)", pIn, nIn));
This seems to be an unrelated change.

[Ryan Sleevi, 2013-09-14 02:13:31]

This scares me for its changes to the state machine. I remember discussing this
with Brian.

That's not to say these aren't solvable, but I really want to make sure close
attention is paid to the points below. Perhaps I've missed something.

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl.h (right):

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl.h:768: ** before the peer's Finished message has
been verified.
This concerns me.

The old behaviour matches the "and there is not a custom CanFalseStartCallback"
- namely, the HS callback was called before the peer's Finished message was
verified AND before any app data was sent.

This caused us to update all of the connection info & state BEFORE we "lied"
about whether or not we were actually connected to the higher layer (the lying
is necessary to get the higher layers to write into the socket, so that we can
false start)

With this change, the HS callback is responsible for updating the state - and
thus will not happen until AFTER we've lied.

Am I misreading something? This seems subtle and likely a potential cause of
bugs - in particular, the socket state not reflecting the peer it's actually
connected to.

I seem to recall Brian's patch notes indicating that the expectation was the
async cert verification interface would instead be used to update the server
state - but that's not how we implement things today.

My biggest concern is this will skew the UMAs we have, because now they'll also
include the time for the higher layers to call Write, which is not presently
included.

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:7498: * called.
This was Brian's concession to Chromium, but as you've implemented it, we can't
take advantage of it.

[wtc, 2013-09-16 16:14:48]

Thank you for the review. I responded to rsleevi's comment below.

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:7498: * called.

On 2013/09/14 02:13:31, Ryan Sleevi wrote:
> This was Brian's concession to Chromium, but as you've implemented it, we
can't
> take advantage of it.

I should have pointed out that Brian's patch also works in our source
tree without my changes to ssl_client_socket_nss.cc. In that "backward
compatible" mode, we are indeed taking advantage of Brian's concession
here.

After I verified the "backward compatible" mode works in Chromium, I
proceeded to try the new CanFalseStartCallback function. I had to
change ssl_client_socket_nss.cc to call our original HandshakeCallback
manually, after SSL_ForceHandshake returns SECSuccess. Does this change
address your concern?

[Ryan Sleevi, 2013-09-17 19:06:02]

On 2013/09/16 16:14:48, wtc wrote:
> Thank you for the review. I responded to rsleevi's comment below.
> 
>
https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
> File net/third_party/nss/ssl/ssl3con.c (right):
> 
>
https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
> net/third_party/nss/ssl/ssl3con.c:7498: * called.
> 
> On 2013/09/14 02:13:31, Ryan Sleevi wrote:
> > This was Brian's concession to Chromium, but as you've implemented it, we
> can't
> > take advantage of it.
> 
> I should have pointed out that Brian's patch also works in our source
> tree without my changes to ssl_client_socket_nss.cc. In that "backward
> compatible" mode, we are indeed taking advantage of Brian's concession
> here.
> 
> After I verified the "backward compatible" mode works in Chromium, I
> proceeded to try the new CanFalseStartCallback function. I had to
> change ssl_client_socket_nss.cc to call our original HandshakeCallback
> manually, after SSL_ForceHandshake returns SECSuccess. Does this change
> address your concern?

Thanks. I think that subtlety was missed when I reviewed.

So +1 to agl's comments about the transition code. Since we package libssl
hermetically, it was never strictly needed.

The only other question I would ask - do we have tests to actually make sure
we're false starting? I didn't see any in our ssl_client_socket_unittests that
specifically test that false start is "working".

We should be able to test this by interposing in the socket, like I was doing
for the other tests, correct?

[wtc, 2013-09-18 22:41:18]

On 2013/09/13 15:03:28, agl wrote:
>
> My only overall comment would be that I would remove the support for
> applications that did False Start prior to this. I suspect that there aren't
any
> other than Chromium, it adds complexity code in the code, and the penalty
(False
> Start doesn't trigger) is minor.

If NSS does a False Start but calls the handshake callback at the true
completion
of the handshake, an unpatched Chrome will fail because Chrome needs the
handshake
callback to be called before SSL_ForceHandshake returns SECsuccess. Chrome's
handshake callback sets the boolean member handshake_callback_called_ to true,
which Chrome checks after SSL_ForceHandshake returns SECSuccess.

However, I can fix this by making False Start require not only enabling the
SSL_ENABLE_FALSE_START option but also registering a canFalseStartCallback.
This is not ideal, but seems like a good trade-off. If you are OK with this,
I will run this idea by Brian. Please let me know.

Another idea is to make SSL_ENABLE_FALSE_START obsolete, and add a new
SSL_ENABLE_FALSE_START_V2 option, which does not require a custom
canFalseStartCallback.

[wtc, 2013-09-18 22:43:21]

On 2013/09/17 19:06:02, Ryan Sleevi wrote:
>
> The only other question I would ask - do we have tests to actually make sure
> we're false starting?

I don't know of any. Seems unlikely, especially if our TestServer doesn't
support the NPN or ALPN extension.

[wtc, 2013-09-18 22:57:23]

agl,rsleevi: Please review the diffs between patch sets 5 and 6.

I reviewed Brian's patch and made some minor changes.

https://codereview.chromium.org/23621040/diff/22001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/23621040/diff/22001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:717: // Called by NSS to determine if we can
false start.

On 2013/09/13 15:03:29, agl wrote:
> In other comments, "false start" is written as "False Start".

Done.

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2818: PRINT_BUF(50, (ss, "Send record (plain
text)", pIn, nIn));

On 2013/09/13 15:03:29, agl wrote:
> This seems to be an unrelated change.

Done.

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:7334: ss->enoughFirstHsDone = PR_TRUE;

I think we need to wait until we have called ssl3_SendChangeCipherSpecs to set
ss->enoughFirstHsDone to PR_TRUE because SSL_GetChannelInfo requires
ss->ssl3.cwSpec to have been updated.

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:10286: }

It seems that we should still set rv to SECSuccess even if
ssl3_CheckFalseStart(ss) returned SECFailure.

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:11012: ss->ssl3.hs.ws = idle_handshake;

I will ask Brian if it's important to set ss->ssl3.hs.ws to idle_handshake
after calling ss->handshakeCallback.

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3gthr.c (left):

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3gthr.c:378: ss->ssl3.hs.ws ==
wait_new_session_ticket) &&

agl: do you remember why ss->ssl3.hs.ws == wait_finished isn't in this
conditional expression?

Compare this with the conditional expression in sslsecur.c (the original
code).

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslauth.c (right):

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslauth.c:141: } else if (ss->sec.secretKeyBits < 90) {

This doesn't really matter because we don't have any cipher suite that
uses 80 or 90-bit keys, but it seems that "90" should be changed to "80"
here.

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslimpl.h:908: PRBool                canFalseStart;   /*
Can/did we False Start */

I moved this new member elsewhere because this section is
under the comment "This group of values is used for DTLS".

[agl, 2013-09-19 16:54:33]

On Wed, Sep 18, 2013 at 6:41 PM,  <wtc@chromium.org> wrote:
> an unpatched Chrome will fail because Chrome needs the
> handshake callback to be called before SSL_ForceHandshake returns SECsuccess.

But we ship libssl with Chrome so there's no version skew, right?

I guess some distros might be slicing and dicing the code in different
ways, but I'm not sure whether we should be leaving scar tissue in
upstream NSS to support that.


Cheers

AGL

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[agl, 2013-09-19 16:59:32]

(leaving LGTM to sleevi.)

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslauth.c (right):

https://codereview.chromium.org/23621040/diff/22001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslauth.c:141: } else if (ss->sec.secretKeyBits < 90) {
On 2013/09/18 22:57:23, wtc wrote:
> 
> This doesn't really matter because we don't have any cipher suite that
> uses 80 or 90-bit keys, but it seems that "90" should be changed to "80"
> here.

That seems reasonable, although then I think it should be "<= 80", not "< 80",
no?

[wtc, 2013-09-19 17:08:41]

On 2013/09/19 16:54:33, agl wrote:
> On Wed, Sep 18, 2013 at 6:41 PM,  <mailto:wtc@chromium.org> wrote:
> > an unpatched Chrome will fail because Chrome needs the
> > handshake callback to be called before SSL_ForceHandshake returns
SECsuccess.
> 
> But we ship libssl with Chrome so there's no version skew, right?

Ah, you are right. What I meant is a Chrome with a patched bundled libSSL
and unpatched ssl_client_socket_nss.cc will fail. Clearly we won't ship
Chrome that way. Bad example.

A better example would be an application that uses the system libSSL
and requires the handshake callback to be called early, at False Start
time. Unless we post a query in the NSS mailing list, we have to assume
such an application exists and keep it working.

[wtc, 2013-09-19 17:15:48]

On 2013/09/19 16:59:32, agl wrote:
> 
> That seems reasonable, although then I think it should be "<= 80", not "< 80",
> no?

Our criterion for False Start is ss->ssl3.cwSpec->cipher_def->secret_key_size >=
10
(key size in bytes), so we consider the 80-bit level (2^80 work) as secure. So I
think it should be "< 80".

[agl, 2013-10-08 20:17:05]

LGTM.

I don't have the state on this that yourself and bsmith do, but I can't see
anything wrong with it.

[commit-bot: I haz the power, 2013-10-09 03:31:14]

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

[commit-bot: I haz the power, 2013-10-09 09:40:52]

Change committed as 227704