Under some circumstances, certain TLS connections are dropped by certain

Under some circumstances, certain TLS connections are dropped by certain
remote servers when the TLS ClientHello record exceeds 256 bytes.

This patch changes the number of ECC formats advertised in the ClientHello
to exactly match the same formats advertised by the desktop version of
Chromium during TLS negotiation, netting a savings of approximately 50
bytes in the ClientHello record. This effectively eliminates the occurrence
of the issue.

Patch is named with a 'z' to ensure it is applied after the other patches
in the folder when import_from_android.sh is run, since that script processes
patches in alphabetical order.

R=digit@chromium.org,wtc@chromium.org
BUG:chromium:245500
TEST: 
1. With V25, Visit http://campusstatebank.com 
2. Enter a fictitious username and click "Submit"
3. The "processing login" page appears.
4. Nothing happens. In some cases, the logo will fail to show.
5. With the proposed patch applied, visit http://campusstatebank.com 
6. Enter a fictitious username and click "Submit"
7. The "processing login" page appears.
8. The browser is redirected to a page where the password can be entered.

Contributed by mckev@amazon.com

[wtc, 2013-06-21 01:24:03]

digit: Could you please review this CL and advise how to
generate a Chromium patch for OpenSSL?

See also my attempt at generating an OpenSSL patch at
https://codereview.chromium.org/17408011

mckev: Please add the following at the end of the CL's
description:

R=digit@chromium.org,wtc@chromium.org
BUG=245500
TEST=<description of how a QA engineer should verify your
bug fix>

[mckev, 2013-06-21 01:42:16]

On 2013/06/21 01:24:03, wtc wrote:
> digit: Could you please review this CL and advise how to
> generate a Chromium patch for OpenSSL?
> 
> See also my attempt at generating an OpenSSL patch at
> https://codereview.chromium.org/17408011
> 
> mckev: Please add the following at the end of the CL's
> description:
> 
> mailto:R=digit@chromium.org,wtc@chromium.org
> BUG=245500
> TEST=<description of how a QA engineer should verify your
> bug fix>

I believe this CL is actually incomplete, since it should also include changes
to the README and the patched version of t1_lib.c.  Please advise.

[wtc, 2013-06-21 17:52:18]

On 2013/06/21 01:42:16, mckev wrote:
>
> I believe this CL is actually incomplete, since it should also include changes
> to the README and the patched version of t1_lib.c.  Please advise.

I found that there are instructions for adding new Chromium patches
at the end of third_party/openssl/README.chromium.

I followed the procedure but ended up with a result similar to your
patch set 1. So I am afraid that we will need to wait for digit's
advice.

[wtc, 2013-06-21 17:57:53]

Comments on patch set 1:

https://codereview.chromium.org/17425002/diff/1/patches.chromium/z_reduce_cli...
File patches.chromium/z_reduce_client_hello-size.patch (right):

https://codereview.chromium.org/17425002/diff/1/patches.chromium/z_reduce_cli...
patches.chromium/z_reduce_client_hello-size.patch:64: ++		if
((s->tlsext_ecpointformatlist = OPENSSL_malloc(1)) == NULL)

Good. I missed the size argument passed to this OPENSSL_malloc() call
in my CL. It should match the value assigned to
s->tlsext_ecpointformatlist_length
below.

https://codereview.chromium.org/17425002/diff/1/patches.chromium/z_reduce_cli...
patches.chromium/z_reduce_client_hello-size.patch:76: ++		/* we only support
elliptic curves in NSA Suite B */

support => advertise support for

Nit: strictly speaking, only P-256 and P-384 are in the NSA Suite B.
But in this NSS source code "Suite B" is often loosely used to also
include P-521.

[mckev, 2013-06-21 18:00:44]

On 2013/06/21 17:57:53, wtc wrote:
> Comments on patch set 1:
> 
>
https://codereview.chromium.org/17425002/diff/1/patches.chromium/z_reduce_cli...
> File patches.chromium/z_reduce_client_hello-size.patch (right):
> 
>
https://codereview.chromium.org/17425002/diff/1/patches.chromium/z_reduce_cli...
> patches.chromium/z_reduce_client_hello-size.patch:64: ++		if
> ((s->tlsext_ecpointformatlist = OPENSSL_malloc(1)) == NULL)
> 
> Good. I missed the size argument passed to this OPENSSL_malloc() call
> in my CL. It should match the value assigned to
> s->tlsext_ecpointformatlist_length
> below.
> 
>
https://codereview.chromium.org/17425002/diff/1/patches.chromium/z_reduce_cli...
> patches.chromium/z_reduce_client_hello-size.patch:76: ++		/* we only support
> elliptic curves in NSA Suite B */
> 
> support => advertise support for
> 
> Nit: strictly speaking, only P-256 and P-384 are in the NSA Suite B.
> But in this NSS source code "Suite B" is often loosely used to also
> include P-521.

wtc: Thanks for the comments.  I think waiting for digit is a good idea, too. 
My best guess is that both the patches as well as the patched files need to be
included in the CL.  I think the patches are only used when the next patch is
created in the future, so that the import_from_android script works as expected.
 However, because of the order in which they are applied (alpha), the script
will probably need to be modified down the road.

[wtc, 2013-06-21 22:12:04]

Review comments on patch set 2:

mckev: I just noticed your comment about Chromium patches
being applied in alphabetical order. Not doing that is why
my earlier attempts did not work!

I suggest some changes below. To save your time, I made
those changes to a copy of your CL and uploaded them to
my CL. You can just download the files from patch set 3
of my CL:
https://codereview.chromium.org/17408011/

You have a typo in the CL's description: occurence => occurrence

I fixed that typo in my version of your CL.

If you don't have time to make the change today, we can
commit my CL, which indicates you as the contributor.
Hopefully you can copy the changes from my CL to your CL,
and we can just commit your CL.

Thanks!

https://codereview.chromium.org/17425002/diff/9001/openssl/openssl.config
File openssl/openssl.config (right):

https://codereview.chromium.org/17425002/diff/9001/openssl/openssl.config#new...
openssl/openssl.config:1057: "

We should add:

OPENSSL_PATCHES_reduce_client_hello_size_SOURCES="\
ssl/t1_lib.c \
"

here.

https://codereview.chromium.org/17425002/diff/9001/openssl/ssl/t1_lib.c
File openssl/ssl/t1_lib.c (right):

https://codereview.chromium.org/17425002/diff/9001/openssl/ssl/t1_lib.c#newco...
openssl/ssl/t1_lib.c:1698: /* we only support elliptic curves in NSA Suite B */

I suggest we change
    we only support ...
to
    we only advertise support for ...

because OpenSSL actually supports all the curves in RFC 4492.

[wtc, 2013-06-21 22:15:59]

https://codereview.chromium.org/17425002/diff/9001/README.chromium
File README.chromium (right):

https://codereview.chromium.org/17425002/diff/9001/README.chromium#newcode178
README.chromium:178: 

I noticed some white spaces on this blank line. I removed them in patch set 4
of my CL.

[mckev, 2013-06-21 23:15:25]

On 2013/06/21 22:15:59, wtc wrote:
> https://codereview.chromium.org/17425002/diff/9001/README.chromium
> File README.chromium (right):
> 
> https://codereview.chromium.org/17425002/diff/9001/README.chromium#newcode178
> README.chromium:178: 
> 
> I noticed some white spaces on this blank line. I removed them in patch set 4
> of my CL.

wtc: Thanks for all of your help and for saving me a little time with the
changes.  I applied your changes to this CL.  The typo in the CL has been fixed,
thanks for noticing it. :-)

[wtc, 2013-06-22 00:19:42]

Patch set 3 LGTM. Thanks!

[commit-bot: I haz the power, 2013-06-22 00:19:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mckev@amazon.com/17425002/20001

[commit-bot: I haz the power, 2013-06-22 00:20:07]

Change committed as 207965

[wtc, 2013-06-22 00:43:36]

mckev: I wrote another CL at https://codereview.chromium.org/16879015/
to bring your CL into Chromium and add your name to the
AUTHORS file.