944157 - Implement the TLS padding extension

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Wan-Teh Chang]

Attachment: Patch by Adam Langley

A TLS padding extension has been proposed to work around a bug in
a server-side SSL/TLS implementation that incorrectly handles a
ClientHello record of 256 - 511 bytes as an SSL 2.0 ClientHello.

The Internet-Draft is: http://tools.ietf.org/html/draft-agl-tls-padding-02

Brian: I have reviewed the attached patch by Adam Langley at
https://codereview.chromium.org/62103003
https://codereview.chromium.org/66553007

So your review is optional.

The patch uses a temparary extension number 35655.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8339587 [details] [diff] [review]
Patch by Adam Langley

Review of attachment 8339587 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3ext.c
@@ +2142,5 @@
> +    unsigned int extensionLength;
> +
> +    if (recordLength < 256 || recordLength >= 512) {
> +	return 0;
> +    }

IMO, at least in the short term, it is a good idea to do this all the time, including when recordLength < 256. We will learn more about interoperability problems this way.

In the short term, I would actually like to make the padded record larger, like 1024 bytes, to make sure we find all the interop issues related to large ClientHellos, not just the F5 issue.

@@ +2150,5 @@
> +     if (extensionLength < 4) {
> +	 extensionLength = 4;
> +     }
> +
> +     return extensionLength;

The 7 lines above are indented one space too far.

@@ +2161,5 @@
> +ssl3_AppendPaddingExtension(sslSocket *ss, unsigned int extensionLen,
> +			    PRUint32 maxBytes)
> +{
> +    unsigned int paddingLen = extensionLen - 4;
> +    unsigned char padding[256];

Should we just make padding static and avoid the memset call?

Comment 2

[Wan-Teh Chang]

Attachment: Patch by Adam Langley, v2

Brian:

Thank you for the review. I made the changes you suggested.

I also considered making the |padding| buffer static. I didn't
do that because I thought the code to do memset may be smaller
than the size taken by a 256-byte static buffer. But since you
also suggested it, I made the change.

Comment 3

[Wan-Teh Chang]

Attachment: Supplemental patch: always pad ClientHello to >= 512 bytes [NOT FOR CHECKIN]

Brian: here is a patch that we can apply on top of the main
patch to always pad ClientHello to 512 bytes, as you suggested.
Chrome's Canary build has this patch.

One issue I considered is whether we should add an SSL_PAD_CLIENT_HELLO
option. I think that if the ClientHello padding doesn't break
other servers, we should just enable it by default, to avoid
adding another option that client applications that may talk to
F5 BIG-IP servers need to enable.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8341976 [details] [diff] [review]
Patch by Adam Langley, v2

http://hg.mozilla.org/projects/nss/rev/3e8ef40afd8f

Comment 5

[Wan-Teh Chang]

Attachment: Unlock sid->u.ssl3.lock on ssl3_AppendPaddingExtension failure

Brian: I didn't check in Adam's patch because I wanted to wait for an official
assignment of the TLS padding extension number by IANA. Please confirm that
you don't want to wait for that.

We need this additional change for the current code in ssl3_SendClientHello.
By the way, can you review my patch in bug 947681? Thanks.

Comment 6

[Wan-Teh Chang]

Comment on attachment 8359941 [details] [diff] [review]
Unlock sid->u.ssl3.lock on ssl3_AppendPaddingExtension failure

Patch checked in: https://hg.mozilla.org/projects/nss/rev/dfc0a9f054de

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Wan-Teh Chang from comment #5)
> Brian: I didn't check in Adam's patch because I wanted to wait for an
> official
> assignment of the TLS padding extension number by IANA. Please confirm that
> you don't want to wait for that.

Thanks for explaining that. We are adding this and the option to do ALPN to Firefox to help with interop testing during the W3C interop event next week. It is easiest to keep the patch checked into NSS and I don't see any issue with keeping it in the NSS release even if we don't get the codepoint assignment. In particular, servers are supposed to (pretty much required to) ignore this extension and I don't think we have to worry about collisions with the unusual value that was chosen.

> We need this additional change for the current code in ssl3_SendClientHello.
> By the way, can you review my patch in bug 947681? Thanks.

Yes, I will get to it as soon as I can. Thanks for the reminder.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Clearing NEEDINFO.