Implement SSL certificate error handling on the Mac. If the user gives...

Implement SSL certificate error handling on the Mac.  If the user gives
us bad certs to allow, we tell SecureTransport to not verify the server
cert, and only allow the cert to be one of the bad certs the user allows.

In the future we should figure out how to verify the server cert ourselves.

R=avi,eroman
BUG=http://crbug.com/11983
TEST=Visit https://www.ssl247.com/ and https://alioth.debian.org/.  Clicking
the "Proceed anyway" button should bring you to the site with a red
"https" in the location bar. 

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

[Avi (use Gerrit), 2009-08-11 14:35:58]

With function name change and scoped pointer change, LGTM.

http://codereview.chromium.org/165191/diff/1008/1010
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/165191/diff/1008/1010#newcode246
Line 246: SecCertificateRef GetServerCert(SSLContextRef ssl_context) {
This name is confusing to users of Core Foundation. Any function that returns an
object that must be released should have "Copy" in its name. Please rename this
function to "CopyServerCert" which implies the need to CFRelease (keep the
comment about CFReleasing, though).

http://codereview.chromium.org/165191/diff/1008/1010#newcode524
Line 524: SecCertificateRef server_cert = GetServerCert(ssl_context_);
The socket code does not use base/scoped_cftyperef because it predated it. Feel
free to use scoped_cftyperef for situations like this.

[eroman, 2009-08-11 18:25:08]

http://codereview.chromium.org/165191/diff/1008/1011
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/165191/diff/1008/1011#newcode6
Line 6: #define NET_BASE_SSL_CONFIG_SERVICE_H__
If you are bored, could be fun to remove one of the trailing "_" to match this
with the new style.

http://codereview.chromium.org/165191/diff/1008/1011#newcode50
Line 50: // allowed_bad_certs that should not trigger an ERR_CERT_* error when
calling
style nit: |allowed_bad_certs|

http://codereview.chromium.org/165191/diff/1008/1010
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/165191/diff/1008/1010#newcode254
Line 254: const_cast<void*>(CFArrayGetValueAtIndex(certs, 0)));
why does this need to be cast to void* before being cast to SecCertificateRef ?

http://codereview.chromium.org/165191/diff/1008/1010#newcode328
Line 328: status = SSLSetPeerDomainName(ssl_context_, hostname_.c_str(),
nit: can you use hostname_.data() instead?

http://codereview.chromium.org/165191/diff/1008/1010#newcode337
Line 337: // ssl_config_.allowed_bad_certs_.
typo: there is no trailing "_" on allowed_bad_certs (anymore).

http://codereview.chromium.org/165191/diff/1008/1010#newcode514
Line 514: } else if (status == noErr) {
This is inconsistent with the many other places in the file where you do "if
(!status)" to test for the noErr case.

That said, I MUCH prefer the style of testing "status == noErr", since it is
clear to me what is going on, without having to know that under the hood noErr
is 0.

http://codereview.chromium.org/165191/diff/1008/1010#newcode517
Line 517: if (!ssl_config_.allowed_bad_certs.empty()) {
I recommend breaking this body out into a function, since this is a fairly large
if block.

http://codereview.chromium.org/165191/diff/1008/1010#newcode519
Line 519: DCHECK_EQ(net_error, OK);
Flip the order -- expectation first, then actual.

http://codereview.chromium.org/165191/diff/1008/1010#newcode523
Line 523: // Check cert.
minor nit: I would say either "Check the certificate" or "Check server_cert".

http://codereview.chromium.org/165191/diff/1008/1010#newcode528
Line 528: DCHECK(!status);
Why do we expect that SecCertificateGetData() can't return an error?

http://codereview.chromium.org/165191/diff/1008/1010#newcode536
Line 536: DCHECK(!status);
Same question as above.

http://codereview.chromium.org/165191/diff/1008/1010#newcode552
Line 552: DCHECK(net_error);
DCHECK_NE(OK, net_error);

[eroman, 2009-08-11 18:27:16]

Also, I am concerned that we are not adding any unit tests with HTTPS bug fixes.

This is a crucial feature, but seems to have weaker testing than the rest of our
network stack... I guess the platform-specificness of the implementations make
it annoying to write tests.

Do we have a bug on file about beefing up the SSL test suite, or making it
easier to write such tests?

[wtc, 2009-08-12 22:02:10]

Please review the new Patch Set.

avi: I changed GetServerCert to return X509Certificate*
instead of SecCertificateRef.  I still renamed the function
CopyServerCert.  Do you think I should use "Copy" here even
though the function doesn't return a CF type?

eroman: I made all the suggested changes.  Thanks.

Please remember to note in my perf review that I don't
write unit tests for the SSL code.  All the SSL unit tests
were written by others.

The code in question is already covered by two unit tests
in http_network_transaction_unittest.cc, so I can get off
the hook this time.  But I am surprised to find out that
those two unit tests aren't disabled for Mac, so I need
to find out why they're passing on Mac right now.

http://codereview.chromium.org/165191/diff/1008/1010
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/165191/diff/1008/1010#newcode254
Line 254: const_cast<void*>(CFArrayGetValueAtIndex(certs, 0)));
On 2009/08/11 18:25:08, eroman wrote:
> why does this need to be cast to void* before being cast to SecCertificateRef
?

CFArrayGetValueAtIndex returns const void *, but SecCertificateRef is
struct OpaqueSecCertificateRef *.  So we need a const_cast to cast
away the const first.

http://codereview.chromium.org/165191/diff/1008/1010#newcode328
Line 328: status = SSLSetPeerDomainName(ssl_context_, hostname_.c_str(),
On 2009/08/11 18:25:08, eroman wrote:
> nit: can you use hostname_.data() instead?

The SSLSetPeerDomainName man page says:
http://developer.apple.com/documentation/security/Reference/secureTransportRe...

  peerName
    The fully qualified domain name of the peer—for example,
    store.apple.com. The name is in the form of a C string,
    except that NULL termination is optional.

  peerNameLen
    The number of bytes passed in the peerName parameter.

Since NULL termination is optional, I will use
hostname_.data() instead.  Thanks.

http://codereview.chromium.org/165191/diff/1008/1010#newcode517
Line 517: if (!ssl_config_.allowed_bad_certs.empty()) {
On 2009/08/11 18:25:08, eroman wrote:
> I recommend breaking this body out into a function, since this is a fairly
large
> if block.

I rewrote this code.  The if block is shorter now.

Do you still want me to break it out into a function?

http://codereview.chromium.org/165191/diff/1008/1010#newcode528
Line 528: DCHECK(!status);
On 2009/08/11 18:25:08, eroman wrote:
> Why do we expect that SecCertificateGetData() can't return an error?

Because we just got a good SecCertificateRef from
SecureTransport, and we're just asking for some members
of the opaque certificate structure.  So I don't
expect SecCertificateGetData to fail here.

Note that this code is gone after the rewrite.

[Avi (use Gerrit), 2009-08-12 22:06:53]

On 2009/08/12 22:02:10, wtc wrote:
> Do you think I should use "Copy" here even
> though the function doesn't return a CF type?

That's fine with me, as I don't think we have any standard naming conventions
for our code to indicate ownership. It was just that if we're using CF types, we
should use their naming conventions for the semantics.

Still LG.

[eroman, 2009-08-13 02:55:23]

lgtm

http://codereview.chromium.org/165191/diff/41/1027
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/165191/diff/41/1027#newcode252
Line 252: DCHECK(CFArrayGetCount(certs) > 0);
DCHECK_GT ?

[wtc, 2009-08-14 22:36:12]

On 2009/08/11 18:27:16, eroman wrote:
> Also, I am concerned that we are not adding any unit tests with HTTPS bug
fixes.

As promised, I looked into why the two unit tests for handling bad certificates
in
http_network_transaction_unittest.cc passed on Mac before I checked in this CL.
It's because they're using mock SSL sockets!

I am going to add unit tests for handling bad certificates that use real SSL
sockets.
I have a CL at http://codereview.chromium.org/170016.  Thank you for keeping
me honest.