890994 - Add ALPN support to Gecko

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

This has some compatibility risk due to causing the TLS client hello message to grow significantly larger, which may often push it past 256 bytes. Apparently, some BIG IP load balancers have a bug with client hello messages larger than (or equal to?) 256 bytes. There is already a patch available that can be applied to the load balancer to fix the interop issue. Apparently this issue affects https://welllsfargo.com though I haven't verified it yet; if so, that's a good indication that there are likely to be many sites affected.

Chrome/Chromium has reduced the number of cipher suites it sends so that the client hello will typically be under 256 bytes, to work around this issue. We can likely do similar. There seem to be quite a few cipher suites (Camellia, SEED, some DSA cipher suites, some 3DES cipher suites) that could likely be disabled with little compatibility impact. But, disabling any cipher suite, in theory, could reduce our compatibility with deployed sites on the internet, so we'd have to be careful in doing so.

Another possibility would be to remove the ec_point_formats extension. The IETF spec. says that if you don't send the ec_point_formats extension then the server is free to assume you support every EC point format. However, I wonder in practice if there are any servers that will send compressed points to a client that doesn't explicitly advertise support for compressed points; it would seem like a bad idea to do so, despite what the spec says. More alarming is the possibility that sites would just refuse to use ECC at all if the extension isn't present.

The server name indication field in the client_hello is a variable-length field that can easily grow large. I looked at the Alexa top 1 million list and I found that amazon EC2 hostnames can easily be 55 bytes long; there are also several (porn spam) hostnames in the list that are 73 bytes long.

A quick estimation shows that, not counting the SNI extension, we should expect to have a client hello of about 215 bytes. That leaves only 40 bytes for the SNI extension before we hit 256 bytes. So, even before we turn on ALPN we are likely going to run into compatibility issues with sites with long hostnames that have this bug.

Is there any reason to implement ALPN other than the political issue where the IETF TLS working group would prefer us to use ALPN to NPN? If so, perhaps it is now less work to just argue that ALPN is not worth the trouble. (AFAICT, it wasn't worth the trouble even before we discovered these issues.)

	228 bytes client hello
		  72 cipher suites (36)
		  94 bytes fixed overhead
			32 Session ID
			32 random
			 9 status_request
			 4 bytes record header
 			 4 Session ticket extension
			 3 Length
			 2 Version
			 2 Compression methods
			 2 cipher suites length
			 1 Handshake type
			 1 session id length
			 2 extensions length

		 49 Other Extensions:
		        22 signature_algorithms
			12 elliptic curves (3)
			 6 EC point formats
			 5 renegotiation extension
			 4 NPN

Comment 1

[Patrick McManus [:mcmanus]]

for agl's padding code to work around the 256 issue see also

https://codereview.chromium.org/62103003/

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: http/2 wip nss alpn patch

Subject: [PATCH 1/7] nss alpn support

from agl originally: http://src.chromium.org/viewvc/chrome?view=revision&revision=216503
also includes a pad certain client hellos to avoid ALPN hangs
from agl https://codereview.chromium.org/download/issue62103003_190003.diff

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

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: 0002-use-alpn-in-gecko-psm.patch

psm bits

Comment 4

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

Comment on attachment 8348849 [details] [diff] [review]
0002-use-alpn-in-gecko-psm.patch

Patch will need to be extended after addressing review comments for the other patch.

Comment 5

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

Comment on attachment 8348848 [details] [diff] [review]
http/2 wip nss alpn patch

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

I checked in the patch for adding the TLS padding extension in NSS bug 944157, so the TLS padding extension logic needs to be removed from this patch.

Add an option to enable NPN and/or ALPN to be enabled separately from each other.

I suggest:

#define SSL_PROTOCOL_NEGOTIATE_NPN 1
#define SSL_PROTOCOL_NEGOTIATE_ALPN 2

#define SSL_ENABLE_PROTOCOL_NEGOTIATION 23

Then, this would enable both ALPN and NPN:

      SSL_OptionSet(fd, SSL_ENABLE_PROTOCOL_NEGOTIATION,  
                    SSL_PROTOCOL_NEGOTIATE_NPN | SSL_PROTOCOL_NEGOTIATE_ALPN);

Also, in the Gecko patch, add the pref (defaulted off) that we agreed to previously. I suspect it is likely the IETF is going to make things such that we have to change the default value, but we don't have to do it now.

Please do this ALPN patch to NSS on the nss:libraries component so that we can check it into NSS and then avoid having to maintain the private patch.

Comment 6

[Patrick McManus [:mcmanus]]

> 
>       SSL_OptionSet(fd, SSL_ENABLE_PROTOCOL_NEGOTIATION,  
>                     SSL_PROTOCOL_NEGOTIATE_NPN |
> SSL_PROTOCOL_NEGOTIATE_ALPN);

I'll go ahead with this, but the third argument to optionSet is technically a bool (though its really a prbool, which is really an int of some kind iirc). not sure if the bitmask has precedent or not in nss.

Comment 7

[Patrick McManus [:mcmanus]]

(In reply to Patrick McManus [:mcmanus] from comment #6)
> > 
> >       SSL_OptionSet(fd, SSL_ENABLE_PROTOCOL_NEGOTIATION,  
> >                     SSL_PROTOCOL_NEGOTIATE_NPN |
> > SSL_PROTOCOL_NEGOTIATE_ALPN);
> 
> I'll go ahead with this, but the third argument to optionSet is technically
> a bool (though its really a prbool, which is really an int of some kind
> iirc). not sure if the bitmask has precedent or not in nss.

actually, I think I'll just make it two different options.

Comment 8

[Patrick McManus [:mcmanus]]

Attachment: ALPN support for gecko

Comment 9

[Patrick McManus [:mcmanus]]

> Please do this ALPN patch to NSS on the nss:libraries component so that we
> can check it into NSS and then avoid having to maintain the private patch.

959664

Comment 10

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

Comment on attachment 8359843 [details] [diff] [review]
ALPN support for gecko

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

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +915,5 @@
>  static const bool REQUIRE_SAFE_NEGOTIATION_DEFAULT = false;
>  static const bool ALLOW_UNRESTRICTED_RENEGO_DEFAULT = false;
>  static const bool FALSE_START_ENABLED_DEFAULT = true;
> +static const bool NPN_ENABLED_DEFAULT = true;
> +static const bool ALPN_ENABLED_DEFAULT = false;

In PSM, we're not doing this anymore unless we use the constant in two places. Please just inline the default values into the SSL_OptionSetDefault calls.

@@ +1279,5 @@
> +                                              NPN_ENABLED_DEFAULT));
> +
> +    SSL_OptionSetDefault(SSL_ENABLE_ALTERNATE_PROTOCOL_NEGOTIATION,
> +                         Preferences::GetBool("security.ssl.enable_alpn",
> +                                              ALPN_ENABLED_DEFAULT));

Besides inlining the constants here, please add a comment that notes that this doesn't affect WebRTC because WebRTC doesn't call SSL_SetNextProto or that other function.

Comment 11

[Patrick McManus [:mcmanus]]

Attachment: ALPN support for gecko

Comment 12

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

Attachment: bug-890994-v2.patch

I adjusted the comments slightly before pushing this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0e66cae4f8

Comment 13

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

Pat, could you please verify that False Start when ALPN is negotiated, or otherwise file a bug if it doesn't. From inspecting the code, it looks like it works, but I didn't test it.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a0e66cae4f8

Comment 15

[Patrick McManus [:mcmanus]]

thanks brian.

Comment 16

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #13)
> Pat, could you please verify that False Start when ALPN is negotiated, or
> otherwise file a bug if it doesn't. From inspecting the code, it looks like
> it works, but I didn't test it.

I confirmed false start using an alpn negotiation at https://www.google.com/

interestingly google favors alpn over npn when both are offered.