Handle unexpected connection close during SSL handshake on Mac

Handle unexpected connection close during SSL handshake on Mac

SSLHandlshake may return errSSLClosedGraceful error code which is
interpreted as net::OK by NetErrorFromOSStatus, but the caller code
sometimes do not check status!=noErr cases if net_error==OK,
and this can lead unexpected status in the handshake sequence
(and causes ExtensionApiTest.CrossOriginXHR failure on Mac).

BUG=29711
TEST=ExtensionApiTest.CrossOriginXHR

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

[kinuko, 2009-12-15 09:56:33]

Hi, can you take a look at the patch?   Thanks,

[Avi (use Gerrit), 2009-12-15 15:05:42]

Nice catch. LGTM

[wtc, 2009-12-16 00:48:52]

Kinuko,

Thank you for fixing this bug.  Does the test server
really close the SSL connection gracefully with an
SSL close_notify alert message during handshake?  It's
hard to believe because usually the server sends some
other SSL alert message when the handshake fails.

I have two suggested changes below.

http://codereview.chromium.org/500018/diff/1/3
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/500018/diff/1/3#newcode715
net/socket/ssl_client_socket_mac.cc:715: return ERR_UNEXPECTED;
Please add curly braces {} to this if statement because
with the comment the if body has two lines.

Please use ERR_SSL_PROTOCOL_ERROR instead of ERR_UNEXPECTED.
ERR_UNEXPECTED means the unexpected error is our own fault
(i.e., a programming error).  So, if a server behaves
unexpectedly, it is the server's fault, and we have to
handle any unexpected external input properly.

http://codereview.chromium.org/500018/diff/1/3#newcode773
net/socket/ssl_client_socket_mac.cc:773: return ERR_UNEXPECTED;
Same here: please use ERR_SSL_PROTOCOL_ERROR instead.

[wtc, 2009-12-16 01:18:21]

hawk, avi:

I found the SSLHandshake source code at
http://www.opensource.apple.com/source/libsecurity_ssl/libsecurity_ssl-36800/...

It seems to show that SSLHandshake can return
errSSLClosedGraceful only if SSLClose has been called.
SecureTransport calls SSLClose internally when it receives
the close_notify alert message:
http://www.opensource.apple.com/source/libsecurity_ssl/libsecurity_ssl-36800/...

So it all makes sense if the server sends us a close_notify
during handshake.  I am still surprised that our test
server does that.  I'm wondering if we're calling
SSLHandshake more times than we should.  We don't handle
the receipt of close_notify during handshake in our
Windows code.  Perhaps the Windows Schannel hides that
from us.

[hawk, 2009-12-16 01:43:45]

On 2009/12/16 01:18:21, wtc wrote:
> So it all makes sense if the server sends us a close_notify
> during handshake.  I am still surprised that our test
> server does that.

I find that curious too. I seem to recall a broken SSL stack from years ago that
would follow up fatal alerts with a close_notify (in violation of the spec). I
wonder if that's what's happening here. I'd like to see a packet trace to find
out.

> I'm wondering if we're calling
> SSLHandshake more times than we should.

Do you mean that we may be inadvertently calling it after the handshake has
completed?

[wtc, 2009-12-16 01:55:23]

On 2009/12/16 01:43:45, hawk wrote:
>
> > I'm wondering if we're calling
> > SSLHandshake more times than we should.
> 
> Do you mean that we may be inadvertently calling it after the handshake has
> completed?

Yes.  I wonder if this is what happens:

  The handshake finishes successfully
  The server sends close_notify for some reason
  SecureTransport calls SSLClose upon receiving close_notify
  We call SSLHandshake again

This seems unlikely, and doesn't match what our code does...

Kinuko, I don't want you to collect the SSL packet trace for us.
Could you just tell us everything you found while debugging this?
Thanks!

[kinuko, 2009-12-16 05:33:44]

On 2009/12/16 01:55:23, wtc wrote:
> Kinuko, I don't want you to collect the SSL packet trace for us.
> Could you just tell us everything you found while debugging this?
> Thanks!

Sure, what I did was basically watching on DoHandshakeLoop on the client side
during the test.
The typical sequence observed was as following:

  enters DoHandshakeLoop with state=STATE_HANDSHAKE_START
  enters DoHandshakeStart; SSLHandshake returns errSSLWouldBlock
    (... this may repeat a few times)
  enters DoHandshakeLoop with state=STATE_HANDSHAKE_START
  enters DoHandshakeStart; SSLHandshake returns errSSLClosedGraceful

I haven't looked into server-side, but seems like first SSLHandshake
successful(?) call returns errSSLClosedGraceful.
Please let me know what else I can do.

btw I've already hastily submitted this fix - if you guys wanted to keep this
test failing until we find out the root cause I can revert my change.  Please
let me know if that's the case.   (Meanwhile I'm going to update the bug)
Thanks,

[wtc, 2009-12-16 19:04:22]

Kinuko,

Thanks for the info.  You just need to create a new CL
to change ERR_UNEXPECTED to ERR_SSL_PROTOCOL_ERROR.
hawk and I may not have time to get to the bottom of this.
We're just curious if the server actually behaves this way.
But like I said earlier, Chrome needs to handle any
unexpected server behavior properly, so the error handling
code you added is good.

[kinuko, 2009-12-17 06:04:03]

Thanks for clarifying, I'm going to make a new CL.

On 2009/12/16 19:04:22, wtc wrote:
> Kinuko,
> 
> Thanks for the info.  You just need to create a new CL
> to change ERR_UNEXPECTED to ERR_SSL_PROTOCOL_ERROR.
> hawk and I may not have time to get to the bottom of this.
> We're just curious if the server actually behaves this way.
> But like I said earlier, Chrome needs to handle any
> unexpected server behavior properly, so the error handling
> code you added is good.