NSS: add `balloon' extension to when we might hit the F5 bug.

NSS: add `padding' extension when we might hit the F5 bug.

In the case that a ClientHello record is between 256 and 511 bytes long, add an
extension to make it at least 512 bytes. This works around a bug in F5 terminators.

BUG=315828
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234040

[wtc, 2013-11-06 21:51:58]

Patch set 1 LGTM.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/README.ch...
File net/third_party/nss/README.chromium (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/README.ch...
net/third_party/nss/README.chromium:142: * In the case that a ClientHello record
is between 256 and 511 bytes long,

Nit: is this the record length of the handshake message length?

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5246: length += balloonExtensionLen;

This workaround should be disabled if IS_DTLS(ss) is true.

We should add a comment here to explain the purpose of the workaround.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2304: int recordLength = clientHelloLength +

Nit: is this the record length or the handshake message length? I think it's the
latter.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2306: 3 /* handshake message length */;

Nit: list them in the order of 1 + 3 + clientHelloLength to match the Handshake
message definition:

      struct {
          HandshakeType msg_type;    /* handshake type */
          uint24 length;             /* bytes in message */
          select (HandshakeType) {
              case hello_request:       HelloRequest;
              case client_hello:        ClientHello;
              ...
              case finished:            Finished;
          } body;
      } Handshake;

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2316: * ClientHello record is either < 256
bytes or is >= 512 bytes. This ensures

Nit: record or handshake message?

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2340: padding = PORT_Alloc(paddingLen);

Since paddingLen is at most 256, we can use a stack buffer or even a static
buffer to avoid heap allocation.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/sslim...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/sslim...
net/third_party/nss/ssl/sslimpl.h:241: ssl3_CalculateBalloonExtensionLength(int
clientHelloLength);

Should the argument be an unsigned int?

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/sslt.h
File net/third_party/nss/ssl/sslt.h (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/sslt....
net/third_party/nss/ssl/sslt.h:208: ssl_balloon_xtn                  = 35655,

:-) Why don't we just call this extension "padding"?

[wtc, 2013-11-06 22:17:30]

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/README.ch...
File net/third_party/nss/README.chromium (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/README.ch...
net/third_party/nss/README.chromium:142: * In the case that a ClientHello record
is between 256 and 511 bytes long,
On 2013/11/06 21:51:58, wtc wrote:
> 
> Nit: is this the record length of the handshake message length?

Please ignore this comment.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2304: int recordLength = clientHelloLength +

On 2013/11/06 21:51:58, wtc wrote:
> 
> Nit: is this the record length or the handshake message length? I think it's
the
> latter.

You are right. This is the record length. (The handshake message length is on
line 2306.)

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2316: * ClientHello record is either < 256
bytes or is >= 512 bytes. This ensures
On 2013/11/06 21:51:58, wtc wrote:
> 
> Nit: record or handshake message?

Please ignore this comment.

[wtc, 2013-11-06 22:25:07]

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5246: length += balloonExtensionLen;

This workaround can also be turned off for renegotiation, which doesn't exist in
SSLv2.

[wtc, 2013-11-06 23:33:20]

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2312: return 512 - recordLength;

IMPORTANT: we also need to make sure this is at least 4, because an extension
with a zero-length extension_data still takes four bytes to encode:

      struct {
          ExtensionType extension_type;
          opaque extension_data<0..2^16-1>;
      } Extension;

      enum {
          signature_algorithms(13), (65535)
      } ExtensionType;

See also line 2322 in ssl3_AppendBalloonExtension.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2319: ssl3_AppendBalloonExtension(sslSocket
*ss, int extensionLen, PRUint32 maxBytes)

Nit: extensionLen probably should be an unsigned int.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2322: PRInt32 paddingLen = extensionLen - 4;

Should assert extensionLen == 0 || extensionLen >= 4.

[agl, 2013-11-08 19:39:17]

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5246: length += balloonExtensionLen;
On 2013/11/06 21:51:58, wtc wrote:
> 
> This workaround should be disabled if IS_DTLS(ss) is true.
> 
> We should add a comment here to explain the purpose of the workaround.

Done.

I've tested various sizes of renegotiation ClientHello on a known bad server
(wellsfargo.com) and all I get is a handshake_failure alert for all of them.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2306: 3 /* handshake message length */;
On 2013/11/06 21:51:58, wtc wrote:
> 
> Nit: list them in the order of 1 + 3 + clientHelloLength to match the
Handshake
> message definition:
> 
>       struct {
>           HandshakeType msg_type;    /* handshake type */
>           uint24 length;             /* bytes in message */
>           select (HandshakeType) {
>               case hello_request:       HelloRequest;
>               case client_hello:        ClientHello;
>               ...
>               case finished:            Finished;
>           } body;
>       } Handshake;

Done.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2312: return 512 - recordLength;
On 2013/11/06 23:33:20, wtc wrote:
> IMPORTANT: we also need to make sure this is at least 4, because an extension
> with a zero-length extension_data still takes four bytes to encode:

Good catch, thanks!

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2319: ssl3_AppendBalloonExtension(sslSocket
*ss, int extensionLen, PRUint32 maxBytes)
On 2013/11/06 23:33:20, wtc wrote:
> 
> Nit: extensionLen probably should be an unsigned int.

Done.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2322: PRInt32 paddingLen = extensionLen - 4;
On 2013/11/06 23:33:20, wtc wrote:
> 
> Should assert extensionLen == 0 || extensionLen >= 4.

Done.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:2340: padding = PORT_Alloc(paddingLen);
On 2013/11/06 21:51:58, wtc wrote:
> 
> Since paddingLen is at most 256, we can use a stack buffer or even a static
> buffer to avoid heap allocation.

Done.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/sslim...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/sslim...
net/third_party/nss/ssl/sslimpl.h:241: ssl3_CalculateBalloonExtensionLength(int
clientHelloLength);
On 2013/11/06 21:51:58, wtc wrote:
> 
> Should the argument be an unsigned int?

Done.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/sslt.h
File net/third_party/nss/ssl/sslt.h (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/sslt....
net/third_party/nss/ssl/sslt.h:208: ssl_balloon_xtn                  = 35655,
On 2013/11/06 21:51:58, wtc wrote:
> 
> :-) Why don't we just call this extension "padding"?

Meh, ok :)

[wtc, 2013-11-08 20:10:05]

Patch set 2 LGTM.

In the Canary channel, it may be a good idea to write a follow-up CL to always
pad ClientHello to >= 512 bytes to discover if there is any side effect.

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5246: length += balloonExtensionLen;

On 2013/11/08 19:39:17, agl wrote:
>
> I've tested various sizes of renegotiation ClientHello on a known bad server
> (http://wellsfargo.com) and all I get is a handshake_failure alert for all of
them.

Thank you for the experiments. I guess they don't respond to client-initiated
renegotiations, perhaps for security reasons.

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/ssl3con.c:5247: * (inclusive). Records with such lengths
trigger bugs in F5 devices.

Nit: Records => Initial ClientHello records

Based on the info from F5 Networks, the code in question is trying to guess
whether the record is SSL 2.0 or SSL 3.0/TLS, so it should only apply to the
initial records.

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/ssl3con.c:5256: length += paddingExtensionLen;

Nit: these two lines can be moved inside the "if" block.

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/ssl3ext.c:2344: if (SECSuccess !=
ssl3_AppendHandshakeNumber(ss, ssl_padding_xtn, 2))

Nit: just curious why you use the SECSuccess != Foo() form. This style isn't
required in NSS.

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
File net/third_party/nss/ssl/sslt.h (right):

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/sslt.h:208: ssl_padding_xtn                  = 35655,

Please update the CL's description, which still says "balloon".

[agl, 2013-11-08 20:33:22]

Have added a TODO to land a patch for canary to always send larger ClientHellos.

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/ssl3con.c:5247: * (inclusive). Records with such lengths
trigger bugs in F5 devices.
On 2013/11/08 20:10:06, wtc wrote:
> 
> Nit: Records => Initial ClientHello records
> 
> Based on the info from F5 Networks, the code in question is trying to guess
> whether the record is SSL 2.0 or SSL 3.0/TLS, so it should only apply to the
> initial records.

Done.

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/ssl3con.c:5256: length += paddingExtensionLen;
On 2013/11/08 20:10:06, wtc wrote:
> 
> Nit: these two lines can be moved inside the "if" block.

Done.

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/ssl3ext.c:2344: if (SECSuccess !=
ssl3_AppendHandshakeNumber(ss, ssl_padding_xtn, 2))
On 2013/11/08 20:10:06, wtc wrote:
> 
> Nit: just curious why you use the SECSuccess != Foo() form. This style isn't
> required in NSS.

I've been reviewing code from thaidn this morning and he uses it a lot :)

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
File net/third_party/nss/ssl/sslt.h (right):

https://codereview.chromium.org/62103003/diff/130001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/sslt.h:208: ssl_padding_xtn                  = 35655,
On 2013/11/08 20:10:06, wtc wrote:
> 
> Please update the CL's description, which still says "balloon".

Done.

[commit-bot: I haz the power, 2013-11-08 20:35:07]

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

[commit-bot: I haz the power, 2013-11-08 20:57:40]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-11-08 21:14:10]

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

[commit-bot: I haz the power, 2013-11-08 23:38:14]

Change committed as 234040