implement certificate verification state machine

implement certificate verification state machine


BUG=None
TEST=None

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

[joth, 2010-10-01 19:42:09]

This patch builds upon pending CL http://codereview.chromium.org/3518004/show

Once these are in, it should just be needing the X509Certificate implementation
to get to a reasonable working state.

[wtc, 2010-10-02 14:06:01]

LGTM.

http://codereview.chromium.org/3571011/diff/5001/6001
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/3571011/diff/5001/6001#newcode346
net/socket/ssl_client_socket_openssl.cc:346: net_error = ERR_SSL_PROTOCOL_ERROR;
This is unlikely to be the right error code.

http://codereview.chromium.org/3571011/diff/5001/6001#newcode395
net/socket/ssl_client_socket_openssl.cc:395: completed_handshake_ = true;
You probably should set completed_handshake_ to true only
if result is OK at this point.

http://codereview.chromium.org/3571011/diff/5001/6001#newcode398
net/socket/ssl_client_socket_openssl.cc:398: // This may or may not apply for
use, so lets invalidate it anyway.
Typo: for use => for us
      lets => let's

http://codereview.chromium.org/3571011/diff/5001/6001#newcode409
net/socket/ssl_client_socket_openssl.cc:409: // TODO(joth): This should be a
no-op until we enable caching,
Nit: enable caching => enable session caching
We cache many things, so it's good to be clear what kind of
caching this is.

http://codereview.chromium.org/3571011/diff/5001/6001#newcode422
net/socket/ssl_client_socket_openssl.cc:422: long verify_result =
SSL_get_verify_result(ssl_);
BUG(?): I think you should remove this SSL_get_verify_result
call.  The job of UpdateServerCert is to obtain the server's
certificate.  It should not care about any certificate
verification error.

[joth, 2010-10-04 13:30:05]

On 2 October 2010 15:06, <wtc@chromium.org> wrote:

> LGTM.
>
>
> http://codereview.chromium.org/3571011/diff/5001/6001
> File net/socket/ssl_client_socket_openssl.cc (right):
>
> http://codereview.chromium.org/3571011/diff/5001/6001#newcode346
> net/socket/ssl_client_socket_openssl.cc:346: net_error =
> ERR_SSL_PROTOCOL_ERROR;
> This is unlikely to be the right error code.
>
> My understanding is this is effectively the equivalent to line 1929 of the
NSS version:
      // SSL_ForceHandshake returned SECSuccess prematurely.
...
      net_error = ERR_SSL_PROTOCOL_ERROR;

..But this does depend on not doing the server validity check
within UpdateServerCert, as per your final comment below.





> http://codereview.chromium.org/3571011/diff/5001/6001#newcode395
> net/socket/ssl_client_socket_openssl.cc:395: completed_handshake_ =
> true;
> You probably should set completed_handshake_ to true only
> if result is OK at this point.
>
>
AFAICT completed_handshake_ indicates we're got to the end of the handshake
sequence, not that the certificate was verified. For example, the user may
override an invalid certificate warning an load the page anyway, which
requires completed_handshake_ to go true to proceed to data transfer.
This is consistent with the NSS socket's DoVerifyCertComplete.



> http://codereview.chromium.org/3571011/diff/5001/6001#newcode398
> net/socket/ssl_client_socket_openssl.cc:398: // This may or may not
> apply for use, so lets invalidate it anyway.
> Typo: for use => for us
>      lets => let's
>
> Done


> http://codereview.chromium.org/3571011/diff/5001/6001#newcode409
> net/socket/ssl_client_socket_openssl.cc:409: // TODO(joth): This should
> be a no-op until we enable caching,
> Nit: enable caching => enable session caching
> We cache many things, so it's good to be clear what kind of
> caching this is.
>
> Done.


> http://codereview.chromium.org/3571011/diff/5001/6001#newcode422
> net/socket/ssl_client_socket_openssl.cc:422: long verify_result =
> SSL_get_verify_result(ssl_);
> BUG(?): I think you should remove this SSL_get_verify_result
> call.  The job of UpdateServerCert is to obtain the server's
> certificate.  It should not care about any certificate
> verification error.
>
>
> Good point. The existence of this code is really only a crutch as it's not
correctly wired up to X509Certificate::Verify() yet. It's redundant though,
due to the way SSLVerifyCallback is currently written to just
return preverify_ok.
I moved this to some logging code in DoHandshake() with a TODO to remove
once real verification is complete.

[wtc, 2010-10-04 23:13:53]

http://codereview.chromium.org/3571011/diff/5001/6001
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/3571011/diff/5001/6001#newcode346
net/socket/ssl_client_socket_openssl.cc:346: net_error = ERR_SSL_PROTOCOL_ERROR;
You said:
> My understanding is this is effectively the equivalent to
> line 1929 of the NSS version:
>       // SSL_ForceHandshake returned SECSuccess prematurely.
> ...
>       net_error = ERR_SSL_PROTOCOL_ERROR;

That is a workaround for an NSS bug.  OpenSSL doesn't
have the bug, so this file does not need this workaround.

Note: ERR_SSL_PROTOCOL_ERROR is appropriate for
ssl_client_socket_nss.cc because the peer needs to violate
the SSL protocol to cause SSL_ForceHandshake to return
SECSuccess prematurely.

You should CHECK/assert that the UpdateServerCert() call
does not return NULL.

http://codereview.chromium.org/3571011/diff/10001/11001#newcode51
net/socket/ssl_client_socket_openssl.cc:51: // TODO(joth): Implement full
mapping.
Typo: SYSVCALL => SYSCALL

Should this really be mapped to ERR_SSL_PROTOCOL_ERROR?
A system call failure is not an SSL protocol error.

[joth, 2010-10-05 12:04:15]

On 5 October 2010 00:13, <wtc@chromium.org> wrote:

>
> http://codereview.chromium.org/3571011/diff/5001/6001
> File net/socket/ssl_client_socket_openssl.cc (right):
>
> http://codereview.chromium.org/3571011/diff/5001/6001#newcode346
> net/socket/ssl_client_socket_openssl.cc:346: net_error =
> ERR_SSL_PROTOCOL_ERROR;
> You said:
>
>> My understanding is this is effectively the equivalent to
>> line 1929 of the NSS version:
>>       // SSL_ForceHandshake returned SECSuccess prematurely.
>> ...
>>       net_error = ERR_SSL_PROTOCOL_ERROR;
>>
>
> That is a workaround for an NSS bug.  OpenSSL doesn't
> have the bug, so this file does not need this workaround.
>
> Note: ERR_SSL_PROTOCOL_ERROR is appropriate for
> ssl_client_socket_nss.cc because the peer needs to violate
> the SSL protocol to cause SSL_ForceHandshake to return
> SECSuccess prematurely.
>
> You should CHECK/assert that the UpdateServerCert() call
> does not return NULL.
>
> Thanks for the explanation. Replaced with a CHECK.
May I expand the comment in the NSS version to mention this is a workaround?



> http://codereview.chromium.org/3571011/diff/10001/11001#newcode51
> net/socket/ssl_client_socket_openssl.cc:51: // TODO(joth): Implement
> full mapping.
> Typo: SYSVCALL => SYSCALL
>
> Thanks! I fixed that once but I obviously failed to save it.


> Should this really be mapped to ERR_SSL_PROTOCOL_ERROR?
> A system call failure is not an SSL protocol error.
>
> Good question. I need to hit the following case in  order for the SSLv3
fall-back to work:
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/http/http...
Of the error codes present there, protocol error sounds the closest.

AFAICT, this occurs the result of the underlying socket read returning 0
(end of file); when that happens I shutdown the transport side of the BIO,
and then the SSL layer gets this error:
- SSL_do_handshake() returns 0.
- SSL_get_error() returns SSL_ERROR_SYSCALL
- errno holds 0
- ERR_get_error() returns 0 (i.e. nothing on the error stack).

None of these seem useful enough to make a more focused error mapping.

Guess I could make a note that the underlying transport was closed in some
bool, detect that when the handshake fails, and so map it to
ERR_CONNECTION_CLOSED or ERR_CONNECTION_RESET (need to work out which), and
then add either/both of those to the switch statement in
http_stream_request.cc, but that seems overkill (especially given the other
implementations are presumably already mapping an unexpected socket shutdown
in handshake to protocol error?)

Let me know what you think

Cheers