New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment
link

Issue 315828: Workaround F5 ClientHello length bug.

Reported by agl@chromium.org, Nov 6 2013 Project Member

Issue description

F5 tell us that we need only ensure that a ClientHello record is not between 256 and 511 bytes long (inclusive). We can do that by adding an extension to pad the ClientHello when it would otherwise fall into that range.

This is a tracking bug for landing that change.
 

Comment 1 by wtc@chromium.org, Nov 6 2013

Cc: wtc@chromium.org
I've used the cipher_suites list and the elliptic_curves extension to
test padding before. The cipher_suites list gives us more flexibility.

A new extension specifically for padding is a good idea, but I'm a
little worried that some firewall devices might block it as an unknown
extension.

Comment 2 by agl@chromium.org, Nov 6 2013

> some firewall devices might block it as an unknown extension.

But we've not hit this for NPN, ALPN or ChannelID extensions.

Comment 4 by cbentzel@chromium.org, Nov 11 2013

Labels: -Pri-2 Pri-1 M-31 Merge-Requested
I realize this is late in the game for M31 merge, but this fixes an issue which causes Chrome to time out when connecting to some sites fronted by F5 Big IP devices.

Comment 5 by bugdroid1@chromium.org, Nov 12 2013

Project Member
------------------------------------------------------------------------
r234619 | agl@chromium.org | 2013-11-12T20:16:18.926542Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/ssl/sslimpl.h?r1=234619&r2=234618&pathrev=234619
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/ssl/ssl3con.c?r1=234619&r2=234618&pathrev=234619
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/ssl/ssl3ext.c?r1=234619&r2=234618&pathrev=234619
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/patches/paddingextension.patch?r1=234619&r2=234618&pathrev=234619

net: don't add padding extension for SSLv3.

This is a no-op change because our SSLv3 handshakes aren't long enough to
trigger the padding extension. But, if they were, the padding extension would
break them.

BUG= 315828 

Review URL: https://codereview.chromium.org/66553007
------------------------------------------------------------------------

Comment 6 by bugdroid1@chromium.org, Nov 13 2013

Project Member
------------------------------------------------------------------------
r234850 | agl@chromium.org | 2013-11-13T18:08:41.974304Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/ssl/ssl3ext.c?r1=234850&r2=234849&pathrev=234850
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/patches/applypatches.sh?r1=234850&r2=234849&pathrev=234850
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/README.chromium?r1=234850&r2=234849&pathrev=234850
   A http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/patches/paddingextensionall.patch?r1=234850&r2=234849&pathrev=234850

net: add padding extension to all handshakes.

This, temporary, change adds the padding extension to all handshakes to check
whether we encounter any servers with problems.

BUG= 315828 

Review URL: https://codereview.chromium.org/62443004
------------------------------------------------------------------------

Comment 7 by laforge@google.com, Nov 13 2013

Did we regress over M-30?

Comment 8 by cbentzel@chromium.org, Nov 18 2013

I'm guessing this is way to late to go into M-31 - and I don't believe this is a regression relative to M-30.

agl+wtc: Do you think we should merge this to M32 or should we just wait for this to go to M33? Is there UMA to get an estimate of how many connections this would impact?

Comment 9 by wtc@chromium.org, Nov 18 2013

In Chrome 31, the TLS ClientHello size should decrease by four bytes
(we gained four AES-GCM cipher suites but lost six HMAC-SHA256 cipher
suites). So this problem could only get slightly better in Chrome 31
than Chrome 30. I think we should not merge this fix to Chrome 31
because the fix may have its own side effect. Are we getting a lot of
reports of this problem?

I think the fix should be merged to Chrome 32. We also need the
OpenSSL version of the fix, which Dr. Steve Henson has implemented.

Comment 10 by cbentzel@chromium.org, Nov 19 2013

Labels: -M-31 M-32

Comment 11 by kareng@google.com, Nov 19 2013

m32 is really far down in its path, is there a reason this can't wait to m33?

Comment 12 by kareng@google.com, Nov 21 2013

wtc and i talked about this and we agreed we'll let it bake a bit.

Comment 13 by agl@chromium.org, Nov 25 2013

Labels: -M-32 -Merge-Requested
Pulling Merge-Request label after talking to Karen. M32 is a weird release and I don't think we will be breaking anymore more with M32 than M31 so I guess this has to wait until M33.

Comment 14 by bugdroid1@chromium.org, Dec 3 2013

Project Member
------------------------------------------------------------------------
r238460 | wtc@chromium.org | 2013-12-03T21:32:21.979825Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/patches/paddingextension.patch?r1=238460&r2=238459&pathrev=238460
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/ssl/ssl3ext.c?r1=238460&r2=238459&pathrev=238460

Fill the TLS padding extension with zero bytes instead of spaces.

R=agl@chromium.org
BUG= 315828 
TEST=none

Review URL: https://codereview.chromium.org/99253006
------------------------------------------------------------------------

Comment 15 by agl@chromium.org, Feb 20 2014

Status: Fixed

Comment 16 by wtc@chromium.org, Feb 20 2014

Labels: M-33

Comment 17 by bugdroid1@chromium.org, Feb 20 2014

Project Member
------------------------------------------------------------------------
r252421 | agl@chromium.org | 2014-02-20T23:52:01.744737Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/ssl/ssl3ext.c?r1=252421&r2=252420&pathrev=252421
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/patches/applypatches.sh?r1=252421&r2=252420&pathrev=252421
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/README.chromium?r1=252421&r2=252420&pathrev=252421
   D http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/patches/paddingextensionall.patch?r1=252421&r2=252420&pathrev=252421

net: don't always add TLS padding.

In order to flush out any problems with padding, Chrome has always been
adding it, even when the ClientHello was small enough not to need it.

Since that change is in Chrome 33 (and the Chrome 34 branch), it's time
to remove it.

BUG= 315828 

Review URL: https://codereview.chromium.org/171713011
------------------------------------------------------------------------

Comment 18 by bugdroid1@chromium.org, Feb 27 2014

Project Member
Labels: merge-merged-1847
------------------------------------------------------------------------
r253838 | agl@chromium.org | 2014-02-27T16:17:00.743845Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1847/src/net/third_party/nss/patches/applypatches.sh?r1=253838&r2=253837&pathrev=253838
   M http://src.chromium.org/viewvc/chrome/branches/1847/src/net/third_party/nss/README.chromium?r1=253838&r2=253837&pathrev=253838
   D http://src.chromium.org/viewvc/chrome/branches/1847/src/net/third_party/nss/patches/paddingextensionall.patch?r1=253838&r2=253837&pathrev=253838
   M http://src.chromium.org/viewvc/chrome/branches/1847/src/net/third_party/nss/ssl/ssl3ext.c?r1=253838&r2=253837&pathrev=253838

Merge 252421 "net: don't always add TLS padding."

> net: don't always add TLS padding.
> 
> In order to flush out any problems with padding, Chrome has always been
> adding it, even when the ClientHello was small enough not to need it.
> 
> Since that change is in Chrome 33 (and the Chrome 34 branch), it's time
> to remove it.
> 
> BUG= 315828 
> 
> Review URL: https://codereview.chromium.org/171713011

TBR=agl@chromium.org

Review URL: https://codereview.chromium.org/180973006
------------------------------------------------------------------------

Comment 19 by srsridhar@chromium.org, Mar 5 2014

Cc: srsridhar@chromium.org
Labels: Needs-Feedback
Could you please let us know if QA can verify the fix. If so please provide us with the steps to verify the same.

Sign in to add a comment