net: detect and error out with ESET/NetNanny HTTPS interception + False Start

net: detect and error out with ESET HTTPS interception + False Start

ESET products (anti-virus) and NetNanny can intercept HTTPS connections. However,
they're False Start intolerant. Rather than have connections hang
forever, we detect them by the leaf certificate's issuer common name
and give a helpful error message.

BUG=58152
TEST=Install ESET anti-virus. Configure to intercept HTTPS connections. Goto https://mail.google.com. Confirm that a helpful error page appears.

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

[agl, 2010-10-11 20:48:52]

I'm checking Kaspersky and NetNanny at the moment.

[wtc, 2010-10-11 21:01:26]

LGTM.

http://codereview.chromium.org/3723001/diff/1/2
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3723001/diff/1/2#newcode5567
chrome/app/generated_resources.grd:5567: Server has a weak Diffie-Hellman public
key
Nit: Diffie-Hellman => ephemeral Diffie-Hellman ?
Or is that too technical?

http://codereview.chromium.org/3723001/diff/1/4
File net/base/net_error_list.h (right):

http://codereview.chromium.org/3723001/diff/1/4#newcode189
net/base/net_error_list.h:189: // This means that we detected an ESET product
intercepting our HTTPS
Nit: please remove "This means that" from this error code and
the SSL_SNAP_START_NPN_MISPREDICTION error above.

http://codereview.chromium.org/3723001/diff/1/5
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/3723001/diff/1/5#newcode998
net/socket/ssl_client_socket_nss.cc:998: pseudo_connected_      = false;
eset_mitm_detected_ should also be reset to false here.

http://codereview.chromium.org/3723001/diff/1/5#newcode1751
net/socket/ssl_client_socket_nss.cc:1751: char* common_name =
CERT_GetCommonName(&cert->issuer);
Nit: char* => const char*

BUG: CERT_GetCommonName may return NULL, so the if statement
below should read:
    if (common_name) {
      if (strcmp(common_name, "ESET_RootSslCert") == 0)
        that->eset_mitm_detected_ = true;
      PORT_Free(common_name);
    }

If the issuer name of the CA certificate is the same for
all installations of ESET products, you can also just
compare cert->issuerName with the fixed issuer name.

http://codereview.chromium.org/3723001/diff/1/5#newcode1754
net/socket/ssl_client_socket_nss.cc:1754: PORT_Free(common_name);
BUG (memory leak): call CERT_DestroyCertificate(cert).

[agl, 2010-10-11 22:04:10]

Have addressed comments (thanks for catching the leak!)

After spending a couple of hours installing and uninstalling 'anti-virus'
products I found that NetNanny also has a problem. Therefore I've duplicated the
same pattern. (Note: due to time pressure I won't wait for an affirmative ok
from you before landing.)

http://codereview.chromium.org/3723001/diff/1/2
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3723001/diff/1/2#newcode5567
chrome/app/generated_resources.grd:5567: Server has a weak Diffie-Hellman public
key
On 2010/10/11 21:01:26, wtc wrote:
> Nit: Diffie-Hellman => ephemeral Diffie-Hellman ?
> Or is that too technical?

Done.

http://codereview.chromium.org/3723001/diff/1/4
File net/base/net_error_list.h (right):

http://codereview.chromium.org/3723001/diff/1/4#newcode189
net/base/net_error_list.h:189: // This means that we detected an ESET product
intercepting our HTTPS
On 2010/10/11 21:01:26, wtc wrote:
> Nit: please remove "This means that" from this error code and
> the SSL_SNAP_START_NPN_MISPREDICTION error above.

Done.

http://codereview.chromium.org/3723001/diff/1/5
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/3723001/diff/1/5#newcode998
net/socket/ssl_client_socket_nss.cc:998: pseudo_connected_      = false;
On 2010/10/11 21:01:26, wtc wrote:
> eset_mitm_detected_ should also be reset to false here.

Done.

http://codereview.chromium.org/3723001/diff/1/5#newcode1751
net/socket/ssl_client_socket_nss.cc:1751: char* common_name =
CERT_GetCommonName(&cert->issuer);
On 2010/10/11 21:01:26, wtc wrote:
> Nit: char* => const char*

One cannot pass a const char* to PORT_Free.

> BUG: CERT_GetCommonName may return NULL, so the if statement
> below should read:

Done.

> If the issuer name of the CA certificate is the same for
> all installations of ESET products, you can also just
> compare cert->issuerName with the fixed issuer name.

I thought about this, but it seems that there's some scope for alternations here
(i.e. if the order of the RDN entries in the certificate changes). I like
matching the CN better.

http://codereview.chromium.org/3723001/diff/1/5#newcode1754
net/socket/ssl_client_socket_nss.cc:1754: PORT_Free(common_name);
On 2010/10/11 21:01:26, wtc wrote:
> BUG (memory leak): call CERT_DestroyCertificate(cert).

Done.

[wtc, 2010-10-11 22:48:34]

On 2010/10/11 22:04:10, agl wrote:
>
> After spending a couple of hours installing and uninstalling 'anti-virus'
> products I found that NetNanny also has a problem. Therefore I've duplicated
the
> same pattern.

Did you rename the error code to be not specific to ESET
(ERR_ANTI_VIRUS_SSL_INTERCEPTION), or did you add a new
error code for NetNanny (ERR_NET_NANNY_ANTI_VIRUS_SSL_INTERCEPTION)?

> (Note: due to time pressure I won't wait for an affirmative ok
> from you before landing.)

This is fine, but please upload a new Patch Set so I can review the
changes you made after the fact.