net: don't add padding extension for SSLv3.

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234619

[wtc, 2013-11-11 19:49:53]

Patch set 1 LGTM. Could you also fix the problems with the return value of
ssl3_AppendPaddingExtension that I reported in
https://codereview.chromium.org/62443004/? Thanks.

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

https://codereview.chromium.org/66553007/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5252: ss->version > SSL_LIBRARY_VERSION_3_0 &&

We can just test the local variable 'isTLS'.

(We may be able to test 'total_exten_len', which would match the test on line
5366, but testing 'isTLS' seems better.)

[agl, 2013-11-11 20:23:49]

I originally had all the arguments / return values match the types of the
variables in ssl3con, but you asked that I change one of them[1] and so I made
them all unsigned to be consistent.

I've only changed the return value in this patch set, is that what you had in
mind?

(p.s. returning 0 for "no extension" makes the rest of the code easier. I think
it's better than -1.)

[1]
https://codereview.chromium.org/62103003/diff/1/net/third_party/nss/ssl/sslim...

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

https://codereview.chromium.org/66553007/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:5252: ss->version > SSL_LIBRARY_VERSION_3_0 &&
On 2013/11/11 19:49:53, wtc wrote:
> 
> We can just test the local variable 'isTLS'.
> 
> (We may be able to test 'total_exten_len', which would match the test on line
> 5366, but testing 'isTLS' seems better.)

Done.

[wtc, 2013-11-11 21:50:33]

Review comments on patch set 2:

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

https://codereview.chromium.org/66553007/diff/50001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:4978: PRInt32          paddingExtensionLen;

This variable receives the return value of ssl3_CalculatePaddingExtensionLength.
Since ssl3_CalculatePaddingExtensionLength cannot fail, it is better for this
variable to have the unsigned type.

https://codereview.chromium.org/66553007/diff/50001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/66553007/diff/50001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:2340: return 0;

It is this return statement that I think should be changed to -1 because we have
detected an internal error. The return statement on line 2333 should still be
"return 0".

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

https://codereview.chromium.org/66553007/diff/50001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslimpl.h:245: PRUint32 maxBytes);

I may have made a mistake in my previous comment.

It is this function that should return a signed type. The
ssl3_CalculatePaddingExtensionLength function above should continue to return
unsigned int.

[agl, 2013-11-12 16:21:57]

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

https://codereview.chromium.org/66553007/diff/50001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:4978: PRInt32          paddingExtensionLen;
On 2013/11/11 21:50:33, wtc wrote:
> 
> This variable receives the return value of
ssl3_CalculatePaddingExtensionLength.
> Since ssl3_CalculatePaddingExtensionLength cannot fail, it is better for this
> variable to have the unsigned type.

Done.

https://codereview.chromium.org/66553007/diff/50001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/66553007/diff/50001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:2340: return 0;
On 2013/11/11 21:50:33, wtc wrote:
> 
> It is this return statement that I think should be changed to -1 because we
have
> detected an internal error. The return statement on line 2333 should still be
> "return 0".

Gah, sorry. I thought you were talking about
ssl3_CalculatePaddingExtensionLength. Yes, this one should be signed because it
returns -1! Thanks.

[commit-bot: I haz the power, 2013-11-12 16:26:24]

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

[wtc, 2013-11-12 17:56:28]

Patch set 3 LGTM.

[commit-bot: I haz the power, 2013-11-12 20:16:23]

Change committed as 234619