net: boost AES-GCM ciphers if the machine has AES-NI.

net: boost AES-GCM ciphers if the machine has AES-NI.

We want to use ChaCha20 based cipher suites on machines that don't have AES-NI
and AES-GCM on those that do.

BUG=310768

[wtc, 2013-11-19 23:00:44]

Patch set 3 LGTM.

https://codereview.chromium.org/75663004/diff/30002/base/cpu.h
File base/cpu.h (right):

https://codereview.chromium.org/75663004/diff/30002/base/cpu.h#newcode28
base/cpu.h:28: AVX,

Do we need an enum for AES-NI here?

https://codereview.chromium.org/75663004/diff/30002/base/cpu.h#newcode52
base/cpu.h:52: bool has_aesni() const { return has_aesni_; }

It seems that has_aesni() should follow has_avx() because it is also an
instruction set feature. Similarly for the has_aesni_ member.

https://codereview.chromium.org/75663004/diff/30002/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/75663004/diff/30002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:3197: if (base::CPU().has_aesni()) {

Since this won't change over time, we should ideally do this only once. We have
a NSSSSLInitSingleton in net/socket/nss_ssl_util.cc that might be a good place
to do this.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl.h (right):

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl.h:270: /* SSL_CipherOrderSet sets the cipher suite
preference order from |ciphers|,

Nit: Add a blank line before this line.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl.h:275: size_t len);

Nit: use unsigned int, which is the commonly used type for lengths in NSS. (Note
that SSL_GetNumImplementedCiphers on line 38 even returns a PRUint64.)

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:12371: size_t i, done;

Nit: use unsigned int.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:12373: for (i = done = 0; i < len; i++) {

It seems that |done| is always equal to |i|, so we can replace |done| by |i|.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:12376: char found = 0;

Nit: Use PRBool and PR_FALSE, PR_TRUE instead of char, 0, 1.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:12378: for (j = done; j <
ssl_V3_SUITES_IMPLEMENTED; j++) {

This valid cipher suite check is quadratic. We should look into using a "model
socket" in Chromium. The model socket is the first argument to SSL_ImportFD.
Properties of the model socket are copied to the new SSL socket.

Update: I looked into this. Since the |ciphers| array depends on SSL config
settings such as TLS/SSL version range and disabled cipher suite list, the model
socket doesn't seem easy to do. We would need to update the model socket
whenever SSL config settings change.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslimpl.h:1675: extern SECStatus
ssl3_CipherOrderSet(sslSocket *ss, const PRUint16 *cipher, size_t len);

Nit: use "unsigned int" for |len|.

Nit: it seems that we can use ssl3CipherSuite instead of PRUint16 in this
function.

[agl, 2013-11-20 18:21:06]

https://codereview.chromium.org/75663004/diff/30002/base/cpu.h
File base/cpu.h (right):

https://codereview.chromium.org/75663004/diff/30002/base/cpu.h#newcode28
base/cpu.h:28: AVX,
On 2013/11/19 23:00:44, wtc wrote:
> Do we need an enum for AES-NI here?

I don't believe so. AES-NI support is orthogonal to this, linear progression of
Intel features.

https://codereview.chromium.org/75663004/diff/30002/base/cpu.h#newcode52
base/cpu.h:52: bool has_aesni() const { return has_aesni_; }
On 2013/11/19 23:00:44, wtc wrote:
> 
> It seems that has_aesni() should follow has_avx() because it is also an
> instruction set feature. Similarly for the has_aesni_ member.

Done.

https://codereview.chromium.org/75663004/diff/30002/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/75663004/diff/30002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:3197: if (base::CPU().has_aesni()) {
On 2013/11/19 23:00:44, wtc wrote:
> 
> Since this won't change over time, we should ideally do this only once. We
have
> a NSSSSLInitSingleton in net/socket/nss_ssl_util.cc that might be a good place
> to do this.

Done.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl.h (right):

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl.h:270: /* SSL_CipherOrderSet sets the cipher suite
preference order from |ciphers|,
On 2013/11/19 23:00:44, wtc wrote:
> 
> Nit: Add a blank line before this line.

Done.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl.h:275: size_t len);
On 2013/11/19 23:00:44, wtc wrote:
> 
> Nit: use unsigned int, which is the commonly used type for lengths in NSS.
(Note
> that SSL_GetNumImplementedCiphers on line 38 even returns a PRUint64.)

Done.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:12371: size_t i, done;
On 2013/11/19 23:00:44, wtc wrote:
> 
> Nit: use unsigned int.

Done.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:12373: for (i = done = 0; i < len; i++) {
On 2013/11/19 23:00:44, wtc wrote:
> 
> It seems that |done| is always equal to |i|, so we can replace |done| by |i|.

Done.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:12376: char found = 0;
On 2013/11/19 23:00:44, wtc wrote:
> 
> Nit: Use PRBool and PR_FALSE, PR_TRUE instead of char, 0, 1.

Done.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:12378: for (j = done; j <
ssl_V3_SUITES_IMPLEMENTED; j++) {
On 2013/11/19 23:00:44, wtc wrote:
> Update: I looked into this. Since the |ciphers| array depends on SSL config
> settings such as TLS/SSL version range and disabled cipher suite list, the
model
> socket doesn't seem easy to do. We would need to update the model socket
> whenever SSL config settings change.

It might be possible to use a model socket because this ordering does not change
which ciphers are enabled etc, it just reorders them. Let me know if I've missed
something, otherwise it seems to be that the NSS singleton could configure a
model socket with this information.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslimpl.h:1675: extern SECStatus
ssl3_CipherOrderSet(sslSocket *ss, const PRUint16 *cipher, size_t len);
On 2013/11/19 23:00:44, wtc wrote:
> 
> Nit: use "unsigned int" for |len|.
> 
> Nit: it seems that we can use ssl3CipherSuite instead of PRUint16 in this
> function.

Done.

[wtc, 2013-11-22 01:14:15]

Patch set 4 LGTM.

Please make the model socket change in a separate CL, to make code review
easier.

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/75663004/diff/30002/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:12378: for (j = done; j <
ssl_V3_SUITES_IMPLEMENTED; j++) {

On 2013/11/20 18:21:07, agl wrote:
>
> It might be possible to use a model socket because this ordering does not
change
> which ciphers are enabled etc, it just reorders them.

I think you're right, although if the |ciphers| input array doesn't include all
ciphers, the missing ciphers will be disabled by the for loop at the end of this
function.

> Let me know if I've missed
> something, otherwise it seems to be that the NSS singleton could configure a
> model socket with this information.

Yes, I agree. We just need to make sure to include all the ciphers.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:53: }

I think it is less work to reverse the nested for loops because the ciphers we
want to remove are likely to be in the top half of the |ciphers| array.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:64: for (size_t i = num - 1; i < num; i--) {

The i < num check looks wrong at first glance. You are relying on wrapping of an
unsigned integer type, right?

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:69: return j+1;

Nit: the Style Guide recommends spaces around '+' and '-'.

Since the only caller of this function ignores the return value, this function
can return void.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:72: // CiphersCopy copies the zero-terminated array
|in| to |out|.

Document the return value.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:83: 

Nit: delete one blank line

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:88: NSSSSLInitSingleton() {

Initialize the new num_ciphers_ member to 0.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:145: // included if it is disabled.)

"it won't be included if it is disabled" sounds wrong. Did you mean "it won't be
used if it is disabled"?

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:148: // forward secure ChaCha20-Poly1305. If the
local machine has AES-NI then

Nit: forward secure => forward secret (two occurrences)

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:152: 0xcc14  /*
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305 */,

You should be able to use TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, etc. These
macros are defined in the NSS header sslproto.h, which we include.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:162: const PRUint16 *all_ciphers =
SSL_GetImplementedCiphers();

Nit: put '*' next to the type. Also fix lines 171-172.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:168: CiphersRemove(aes_gcm_ciphers, ciphers_.get(),
num_ciphers_)) {

Nit: We can pass the sizes of chacha_ciphers and aes_gcm_ciphers (excluding the
zero terminators) to CiphersRemove so that CiphersRemove doesn't need to
calculate the size of its |to_remove| input.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util.h
File net/socket/nss_ssl_util.h (right):

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.h:30: // GetNSSCipherOrder either returns NULL, to
indicate the no special cipher

indicate the => "indicate that" or remove "the"

[agl, 2013-11-22 17:53:48]

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:53: }
On 2013/11/22 01:14:16, wtc wrote:
> 
> I think it is less work to reverse the nested for loops because the ciphers we
> want to remove are likely to be in the top half of the |ciphers| array.

Done.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:64: for (size_t i = num - 1; i < num; i--) {
On 2013/11/22 01:14:16, wtc wrote:
> 
> The i < num check looks wrong at first glance. You are relying on wrapping of
an
> unsigned integer type, right?

Yes.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:69: return j+1;
On 2013/11/22 01:14:16, wtc wrote:
> 
> Nit: the Style Guide recommends spaces around '+' and '-'.
> 
> Since the only caller of this function ignores the return value, this function
> can return void.

Done.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:72: // CiphersCopy copies the zero-terminated array
|in| to |out|.
On 2013/11/22 01:14:16, wtc wrote:
> 
> Document the return value.

Done.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:83: 
On 2013/11/22 01:14:16, wtc wrote:
> 
> Nit: delete one blank line

Done.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:88: NSSSSLInitSingleton() {
On 2013/11/22 01:14:16, wtc wrote:
> 
> Initialize the new num_ciphers_ member to 0.

Done.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:145: // included if it is disabled.)
On 2013/11/22 01:14:16, wtc wrote:
> 
> "it won't be included if it is disabled" sounds wrong. Did you mean "it won't
be
> used if it is disabled"?

I really did mean that, but it's clearly confusing. I've changed it to "Note
that, even if a cipher is specified in the ordering, it must still be enabled in
order to be included in a ClientHello."

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:148: // forward secure ChaCha20-Poly1305. If the
local machine has AES-NI then
On 2013/11/22 01:14:16, wtc wrote:
> 
> Nit: forward secure => forward secret (two occurrences)

Done.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:152: 0xcc14  /*
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305 */,
On 2013/11/22 01:14:16, wtc wrote:
> 
> You should be able to use TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, etc. These
> macros are defined in the NSS header sslproto.h, which we include.

Done.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:162: const PRUint16 *all_ciphers =
SSL_GetImplementedCiphers();
On 2013/11/22 01:14:16, wtc wrote:
> 
> Nit: put '*' next to the type. Also fix lines 171-172.

Done.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:168: CiphersRemove(aes_gcm_ciphers, ciphers_.get(),
num_ciphers_)) {
On 2013/11/22 01:14:16, wtc wrote:
> 
> Nit: We can pass the sizes of chacha_ciphers and aes_gcm_ciphers (excluding
the
> zero terminators) to CiphersRemove so that CiphersRemove doesn't need to
> calculate the size of its |to_remove| input.

By reordering the for loops in CiphersRemove, as you suggested, the function no
longer needs to calculate the length beforehand.

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util.h
File net/socket/nss_ssl_util.h (right):

https://codereview.chromium.org/75663004/diff/280001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.h:30: // GetNSSCipherOrder either returns NULL, to
indicate the no special cipher
On 2013/11/22 01:14:16, wtc wrote:
> 
> indicate the => "indicate that" or remove "the"

Done.

[agl, 2013-11-22 19:30:51]

+ajwong for base/ OWNERS.

[wtc, 2013-11-23 01:02:07]

Patch set 6 LGTM.

https://codereview.chromium.org/75663004/diff/520001/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

https://codereview.chromium.org/75663004/diff/520001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.cc:86: : num_ciphers_(0) {

Nit: the Style Guide allows a short initializer on the previous line.

[agl, 2013-11-25 18:37:12]

+mark for base/ OWNERS.

[Mark Mentovai, 2013-11-25 18:53:11]

LGTM for base OWNERS approval.

[commit-bot: I haz the power, 2013-11-25 19:07:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/75663004/520001

[commit-bot: I haz the power, 2013-11-25 19:23:50]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[wtc, 2013-11-27 14:50:12]

https://codereview.chromium.org/75663004/diff/520001/net/socket/nss_ssl_util.h
File net/socket/nss_ssl_util.h (right):

https://codereview.chromium.org/75663004/diff/520001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.h:37: const uint16* GetNSSCipherOrder(size_t*
out_length);

Compilation failed because uint16 is not defined.

We need to either switch to PRUint16 or include "base/basictypes.h".

[agl, 2013-11-27 16:08:32]

https://codereview.chromium.org/75663004/diff/520001/net/socket/nss_ssl_util.h
File net/socket/nss_ssl_util.h (right):

https://codereview.chromium.org/75663004/diff/520001/net/socket/nss_ssl_util....
net/socket/nss_ssl_util.h:37: const uint16* GetNSSCipherOrder(size_t*
out_length);
On 2013/11/27 14:50:13, wtc wrote:
> 
> Compilation failed because uint16 is not defined.
> 
> We need to either switch to PRUint16 or include "base/basictypes.h".

Thanks. I had missed this CQ failure.

codereview broke and I cannot upload changes to this issue any longer so I've
moved it to https://codereview.chromium.org/91913002/