Bug 1287267 - Flush ServerHello before sending an alert

Bug 1287267 - Flush ServerHello before sending an alert

[ekr-rietveld, 2016-08-08 20:09:56]

I'm not happy about losing all these FATAL_ERRORs because one of the side
effects is that they print out the line of code that failed which is useful for
debugging. I can understand the refactor but if so, I think we need to add some
sort of other macro

https://codereview.appspot.com/305130043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_auth_unittest.cc (right):

https://codereview.appspot.com/305130043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_auth_unittest.cc:127:
client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
This seems like it's still a bug in the DTLS code, right? So maybe a separate
bug?

https://codereview.appspot.com/305130043/diff/1/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/305130043/diff/1/lib/ssl/tls13con.c#newcode1376
lib/ssl/tls13con.c:1376: tls13_SendEncryptedServerHandshake(sslSocket *ss)
SendEncryptedServerSequence

[mt, 2016-08-08 23:20:05]

Hmm, I don't know what to do about the FATAL_ERROR stuff.  I know that it's
useful to get line numbers, but I find myself in the debugger in most of the
cases where I need these.

Would you be OK if I opened a separate bug for this?  I would really like to fix
more than just the few places where I removed FATAL_ERROR from this patch
(ideally every place that SECFailure first arises).

https://codereview.appspot.com/305130043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_auth_unittest.cc (right):

https://codereview.appspot.com/305130043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_auth_unittest.cc:127:
client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
On 2016/08/08 20:09:56, ekr-webrtc wrote:
> This seems like it's still a bug in the DTLS code, right? So maybe a separate
> bug?

It's not a bug inasmuch as we are still producing valid output.  The problem is
that the server sends an alert prior to sending the ServerHello, that's all. 
The reason we have a different error code is that we have this:

http://searchfox.org/mozilla-central/rev/389a3e0817b873a64376ec2b65f6807afbba...

I don't know if this is worth a bug.  I could replicate the fix that I've used
in TLS 1.3, but I sort of decided that it wasn't worth the effort.

https://codereview.appspot.com/305130043/diff/1/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/305130043/diff/1/lib/ssl/tls13con.c#newcode1376
lib/ssl/tls13con.c:1376: tls13_SendEncryptedServerHandshake(sslSocket *ss)
On 2016/08/08 20:09:56, ekr-webrtc wrote:
> SendEncryptedServerSequence

Done.

[ekr-rietveld, 2016-08-08 23:57:58]

On 2016/08/08 23:20:05, mt wrote:
> Hmm, I don't know what to do about the FATAL_ERROR stuff.  I know that it's
> useful to get line numbers, but I find myself in the debugger in most of the
> cases where I need these.

Different strokes, I guess. I use this all the time.


> Would you be OK if I opened a separate bug for this?  I would really like to
fix
> more than just the few places where I removed FATAL_ERROR from this patch
> (ideally every place that SECFailure first arises).

I'd rather decide on a fix now and fix these locations, and then file a bug to
fix the
rest of these, unless you think figuring out the fix is complicated.

> 
>
https://codereview.appspot.com/305130043/diff/1/external_tests/ssl_gtest/ssl_...
> File external_tests/ssl_gtest/ssl_auth_unittest.cc (right):
> 
>
https://codereview.appspot.com/305130043/diff/1/external_tests/ssl_gtest/ssl_...
> external_tests/ssl_gtest/ssl_auth_unittest.cc:127:
> client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
> On 2016/08/08 20:09:56, ekr-webrtc wrote:
> > This seems like it's still a bug in the DTLS code, right? So maybe a
separate
> > bug?
> 
> It's not a bug inasmuch as we are still producing valid output.  The problem
is
> that the server sends an alert prior to sending the ServerHello, that's all. 
> The reason we have a different error code is that we have this:
> 
>
http://searchfox.org/mozilla-central/rev/389a3e0817b873a64376ec2b65f6807afbba...
> 
> I don't know if this is worth a bug.  I could replicate the fix that I've used
> in TLS 1.3, but I sort of decided that it wasn't worth the effort.
> 
> https://codereview.appspot.com/305130043/diff/1/lib/ssl/tls13con.c
> File lib/ssl/tls13con.c (right):
> 
> https://codereview.appspot.com/305130043/diff/1/lib/ssl/tls13con.c#newcode1376
> lib/ssl/tls13con.c:1376: tls13_SendEncryptedServerHandshake(sslSocket *ss)
> On 2016/08/08 20:09:56, ekr-webrtc wrote:
> > SendEncryptedServerSequence
> 
> Done.

[ekr-rietveld, 2016-08-08 23:58:47]

https://codereview.appspot.com/305130043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_auth_unittest.cc (right):

https://codereview.appspot.com/305130043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_auth_unittest.cc:127:
client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
On 2016/08/08 23:20:04, mt wrote:
> On 2016/08/08 20:09:56, ekr-webrtc wrote:
> > This seems like it's still a bug in the DTLS code, right? So maybe a
separate
> > bug?
> 
> It's not a bug inasmuch as we are still producing valid output.  The problem
is
> that the server sends an alert prior to sending the ServerHello, that's all. 
> The reason we have a different error code is that we have this:
> 
>
http://searchfox.org/mozilla-central/rev/389a3e0817b873a64376ec2b65f6807afbba...
> 
> I don't know if this is worth a bug.  I could replicate the fix that I've used
> in TLS 1.3, but I sort of decided that it wasn't worth the effort.

OK

[mt, 2016-08-09 01:35:57]

Updated to include a macro for places that can't use an alert.

Strategy here was to identify a few places (not all of them) where we wanted the
macro originally.  Then, go to the earliest point that the failure might occur
and use the macro there.

tls13_ComputeFinished, tls13_ComputeHandshakeHashes, tls13_HkdfExtract,
tls13_DeriveSecret, and tls13_SetCipherSpec are all called from places where
alerts are possible sometimes and other times not, so these don't log their
failures directly, but instead rely on the caller to log for them.  Otherwise,
all of the tls13_* functions I touched log the failures for themselves.

[ekr-rietveld, 2016-08-09 15:46:27]

This seems like we are going to be sad pandas if we ever want to send a non
internal_error alert.

https://codereview.appspot.com/305130043/diff/20001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/305130043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:139: } while (0)
Let's have FATAL_ERROR() call LOG_ERROR()

https://codereview.appspot.com/305130043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1474: PORT_SetError(code);
Maybe I'm missing something, but where does this send the alert?

It seems like
 HAndleClientHelloPart2 ->
 SendServerHelloSequence ->
 SendServerEncryptedHelloSeequence

[mt, 2016-08-10 00:35:38]

On 2016/08/09 15:46:27, ekr-webrtc wrote:
> This seems like we are going to be sad pandas if we ever want to send a non
> internal_error alert.

Yeah, it will suck.

However, I believe that the theory is that we are supposed to do all the
checking when we read things in.  Then, the only thing that can fail when
sending is nasty internal stuff that shouldn't fail unless something is really
wrong.

https://codereview.appspot.com/305130043/diff/20001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/305130043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:139: } while (0)
On 2016/08/09 15:46:27, ekr-webrtc wrote:
> Let's have FATAL_ERROR() call LOG_ERROR()

Done.

https://codereview.appspot.com/305130043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1474: PORT_SetError(code);
On 2016/08/09 15:46:27, ekr-webrtc wrote:
> Maybe I'm missing something, but where does this send the alert?
> 
> It seems like
>  HAndleClientHelloPart2 ->
>  SendServerHelloSequence ->
>  SendServerEncryptedHelloSeequence

In HandleClientHelloPart2, after we call SendServerHelloSequence, a
handshake_failure alert is sent.

[ekr-rietveld, 2016-08-14 22:45:55]

LGTM

https://codereview.appspot.com/305130043/diff/20001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/305130043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1474: PORT_SetError(code);
On 2016/08/10 00:35:37, mt wrote:
> On 2016/08/09 15:46:27, ekr-webrtc wrote:
> > Maybe I'm missing something, but where does this send the alert?
> > 
> > It seems like
> >  HAndleClientHelloPart2 ->
> >  SendServerHelloSequence ->
> >  SendServerEncryptedHelloSeequence
> 
> In HandleClientHelloPart2, after we call SendServerHelloSequence, a
> handshake_failure alert is sent.

I see it.