HTTP/2 has a requirement that it only be used with TLS >= 1.2. We have a case with twitter.com right now where the server is offering h2 in its 1.0 Server Hello NPN extension . Gecko needs to be able to identify that and, in the NPN callback to PSM, not select h2. SSL_GetChannelInfo(), used to get this info, currently does not provide any information until more of the handshake (specifically all of the extensions) have been processed. This is a bit of a catch-22 as the NPN callback is part of that extension processing code. From inspecting the code it seems perfectly fine to allow the protocol version to be identified at any stage - and that's what a one line patch I've included does. Indeed nss often looks at ss->version at these points. The worst case is that it is retrieved before any activity in which case it reports the least trusted initialized (ssl3) value.

in context https://tbpl.mozilla.org/?tree=Try&rev=3a6913b9913a

Comment on attachment 8475589 [details] [diff] [review] SSL_GetChannelInfo() server hello version not available during npn callback Review of attachment 8475589 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. This seems fine. I am surprised that SSL_GetChannelInfo still returns SECSuccess when it leaves most fields blank. It seems strange to return a structure that is all blank except this one field. How would the caller know which fields have been field? I wonder if we should add a new function just to report the negotiated protocol version. Doesn't HTTP/2 require ALPN? So if we need to be strict, it seems that we should also disallow using NPN to negotiate HTTP/2.

My previous comment has a typo. I wrote: How would the caller know which fields have been field? It should read: How would the caller know which fields have been filled?

Comment on attachment 8475589 [details] [diff] [review] SSL_GetChannelInfo() server hello version not available during npn callback Review of attachment 8475589 [details] [diff] [review]: ----------------------------------------------------------------- Patrick: I canceled the review+. I just realized that this patch won't meet the requirements of HTTP/2. http://tools.ietf.org/html/draft-ietf-httpbis-http2-14#section-9.2.2 says: The set of TLS cipher suites that are permitted in HTTP/2 is restricted. HTTP/2 MUST only be used with cipher suites that have ephemeral key exchange, such as the ephemeral Diffie-Hellman (DHE) [TLS12] or the elliptic curve variant (ECDHE) [RFC4492]. Ephemeral key exchange MUST have a minimum size of 2048 bits for DHE or security level of 128 bits for ECDHE. Clients MUST accept DHE sizes of up to 4096 bits. HTTP MUST NOT be used with cipher suites that use stream or block ciphers. Authenticated Encryption with Additional Data (AEAD) modes, such as the Galois Counter Model (GCM) mode for AES [RFC5288] are acceptable. Therefore, having SSL_GetChannelInfo return just the protocol version isn't enough. It seems that the problem is that ss->enoughFirstHsDone is false when the NPN callback is invoked. Right now we call the NPN callback at the end of ssl3_ClientHandleNextProtoNegoXtn (inside the ssl3_SelectAppProtocol function). I think the best solution is to call the NPN callback in ssl3_SendNextProto. NSS makes two calls to ssl3_SendNextProto. One of them is clearly made after setting ss->enoughFirstHsDone to PR_TRUE. The other call (for session resumption handshakes?) is less clear and may need changing.

(In reply to Wan-Teh Chang from comment #3) > Comment on attachment 8475589 [details] [diff] [review] > SSL_GetChannelInfo() server hello version not available during npn callback > > Review of attachment 8475589 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=wtc. This seems fine. I am surprised that SSL_GetChannelInfo > still returns SECSuccess when it leaves most fields blank. It > seems strange to return a structure that is all blank except > this one field. How would the caller know which fields have > been field? I agree its odd. Note the behavior is the same for 0 fields are filled in the current code. The function pretty much only fails if you pass in the wrong structure length or a bogus fd. The practical thing to do is to compare what you are interested in against 0 as the structure is at least cleared anytime it returns SUCCESS. > > Doesn't HTTP/2 require ALPN? So if we need to be strict, it > seems that we should also disallow using NPN to negotiate > HTTP/2. strictly speaking h2 doesn't require alpn. It defines alpn and then also allows you to also negotiate h2 via a big black box called "prior knowledge" before the h2 settings bits are sent. So firefox is absolutely doing alpn (with google.com right now as a matter of fact), but some servers have not yet caught up because alpn hasn't made it into whatever SSL library they are using. Notably it isn't in the OpenSSL release yet (it is on some pre-release version). Twitter is the primary example - they are doing h2 over NPN only for now. Unfortunately twitter offers h2 in their npn list even when sending a server hello less than 1.2 (which is incompatible). If firefox selects h2 during the tls handsake then the h2 protocol stack throws an INADEQUATE_SECURITY error (which is actually defined at the h2 level).. so twitter shouldn't be offering h2 when the hello is less than 1.2 (their bug), and firefox shouldn't be choosing it (this bug and bug 1054347 for the gecko part of it). As a purely practical matter, twitter always takes the maximum version from the client hello and therefore this only comes up when the intolerance code tries to initiate a handshake < 1.2. I've fixed that problem already (filtering the ALPN/NPN list based on the client hello version) so what is left is really the corner case of what happens when the server selects a lower version than is offered and also advertises an incompatible protocol with it.

(In reply to Wan-Teh Chang from comment #5) > Comment on attachment 8475589 [details] [diff] [review] > SSL_GetChannelInfo() server hello version not available during npn callback > > Review of attachment 8475589 [details] [diff] [review]: > ----------------------------------------------------------------- > > Therefore, having SSL_GetChannelInfo return just the protocol version > isn't enough. it certainly isn't enough to enforce that whole requirement during NPN negotiation time - but it is enforced by the h2 stack itself a few beats later before any application layer data is sent. indeed under alpn, where the server chooses the application protocol, that is the only way to enforce it. The idea behind the change proposed here isn't really to enforce that requirement (failure of it requires a particular h2 message anyhow), but to avoid selecting h2 when we know its going to fail the enforcement checks later (and indeed the server should not have offered it). The quality of the implementation is better because errors are avoided. > > It seems that the problem is that ss->enoughFirstHsDone is false when > the NPN callback is invoked. yes > Right now we call the NPN callback at the > end of ssl3_ClientHandleNextProtoNegoXtn (inside the ssl3_SelectAppProtocol > function). I think the best solution is to call the NPN callback in > ssl3_SendNextProto. I thought about this, and opted instead for the more minimal change I was confident in. If we what you describe all of the traditional GetChannelInfo() fields ought to be available at that time, right? That would obviously lead to even more flexible callback behavior - which is good. > NSS makes two calls to ssl3_SendNextProto. One > of them is clearly made after setting ss->enoughFirstHsDone to PR_TRUE. > The other call (for session resumption handshakes?) is less clear and > may need changing. I think that is resumption too. perhaps we just need a state bool to separate the number of times we select the protocol from the number of times we send it. I would also be careful to make sure this happens before the false start callback so that it can continue to use that in its decision making process. I think that is naturally how it will work anyhow without significant changes. based on all of that, would you prefer I try a version of this that moves the npn callback timing or should we stay with the simpler approach already submitted? (I'm also going away on a break starting tomorrow - so if I disappear it isn't a lack of interest :))

Effectively fixed by bug 1084669.