HTTP/2 requires that decisions around ALPN protocol selection are made based on things like the negotiated protocol version and cipher suite selection, as well as things like whether compression is enabled. See http://http2.github.io/http2-spec/#TLSUsage One way to ensure that this is implemented correctly is to implement a custom ALPN handler function, and check that all the necessary prerequisites are in place before selecting "h2" (the token for HTTP/2). However, all this information is locked away at this point. The only way to get that sort of information normally is to call SSL_GetChannelInfo. But there are guards in SSL_GetChannelInfo (http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslinfo.c#24) that prevent it from releasing any information prior to the handshake being essentially complete. It seems like we could remove the guards, however it's not clear to me if this is 100% safe to do so. (I've also found this necessary to implement more rigorous testing in bug 1082959.)

see also https://bugzilla.mozilla.org/show_bug.cgi?id=1054349 which seeks to make things a little better on the npn side. For the client the ALPN side isn't that interesting, but obviously the roles are reversed for the server and nss covers both. fwiw gecko client h2 support currently just checks the client hello version in the ALPN callback to guard against offering h2 with intolerance fallbacks in play - we managed to trigger that with twitter. Beyond that it relies on config - which could certainly be improved but can probably all be done within psm. the selection of various bits of 9.2.2 are all really in the hands of the server anyhow with alpn, so it needs to be confirmed after the handshake but before any data is sent... that is triggered as a precondition on the first application layer I/O event and generates INADEQUATE_SECURITY if it fails (which obviously again requires h2 at the alpn level)

So I'm a little fuzzy on why we have those guards, but a couple a reasons could be: 1) not to leak critical information before the handshake completes. There are a number of attacks that use the stage of failure of the SSL protocol to get information (particularly if the attacker is using a corrupted key blob). There are places in out implementation where we ignore the actual failure and substitute a fake data structure (like a fake key), and keep the protocol going. We then fail when we do the handshake hashes. While there isn't an issue with the application getting information beforehand, we have to be careful of the application leaking that information before the handshake succeeds or fails. 2) the information on the SSL socket is preliminary until the handshake succeeds. Not only do we have the case where we may already quietly be in a failed handshake condition, but even before the handshake completes, we may have a case of an attacker 'feeding us information' and we don't know it's solid until the TLS handshake completes. 3) most likely: The SSL socket state does not change until we've completed the cipher change spec request. Before that the socket state is unencrypted (or the previous encryption), so we'll be returning bad data. I think 3 is the most problematic, and we should verify that you are indeed getting correct cipher spec information. Maybe a callback to tell the application when it's safe to first look at the cipher suite information is a better solution? bob

I suggest changing libssl to call the ALPN/NPN callback just prior to when it would send the NPN message in ssl3_SendClientSecondRound. Then enoughFirstHsDone will be true and also all the information you need to check the HTTP/2 9.2.2 requirements should be available (including, in particular, the length of the DHE/ECDHE key). Otherwise, you should make a convincing argument regarding why it is OK to remove the enoughFirstHsDone check.

Bob, I was thinking that I could expand the information that is included here to surface the "enoughFirstHsDone" state, so that we could guard against this. I could even make leave the guard enabled if the info struct we're passed is too small to include the new information so that old applications don't make mistakes. Yes, the information is a little unreliable, but that was already the case: zero values are sometimes valid and sometimes not. Brian, This isn't a problem for clients as much as it is a problem for servers. See bug 1054349.

This is a far more conservative change: it adds a check that the input is large enough to receive the newly added infoComplete attribute and only then does it expose all the state prior to this being true. Thus, code compiled against older header files won't see a behaviour change.

(In reply to Robert Relyea from comment #3) > Maybe a callback to tell the application when it's safe to first look at the > cipher suite information is a better solution? I filed bug 1133285 to implement this callback.

Current plan, as discussed on nss-dev is to provide additional fields in the struct that are unconditionally set. An additional flag (maybe a state variable) will indicate whether the final values have been set.

Martin, do you expect that this will be finished soon? If so, it would be great to add the additional information that would be exposed if we went ahead with the patch in bug 1137538. Otherwise, I think we'll go ahead with that bug on its own.

It shouldn't take very long to actually do. I just need to sit down and do it :) Let me increase the priority. I've been hacking on more unit tests for NSS, but this should help me land some more PSM tests so it's good either way.

Brian, this should address the concern that you had in bug 1133566.

My bad, I let the function fail when KEA parameters are missing, but the key exchange algorithms are known too late for this to be useful at the right time.

Comment on attachment 8571609 [details] [diff] [review] bug1084669.patch Sorry about the churn; I've reconsidered and I think we need a clearer signal that values are good to use. That means touching the Hello handlers :(

Now with extra flags that signal when the values are valid. I was pretty careful to look for all instances where the values might have changed. I've tested these changes for the version on bug 1082959, the KEA parameters need testing.

Comment on attachment 8571674 [details] [diff] [review] bug1084669.patch Review of attachment 8571674 [details] [diff] [review]: ----------------------------------------------------------------- FYI: I will be able to look at this in two weeks but I won't be able to look at it until then. Also, I'm no longer a NSS peer so an NSS peer will need to review this too.

Comment on attachment 8571674 [details] [diff] [review] bug1084669.patch :kaie, would you mind having a look at this? Brian doesn't have the time right now and it seems like :keeler needs something like this relatively soon.

Comment on attachment 8571674 [details] [diff] [review] bug1084669.patch Review of attachment 8571674 [details] [diff] [review]: ----------------------------------------------------------------- Here is some preliminary feedback. It would be better to address this feedback and submit a new patch that I can review when I return from vacation. The flags need to be reset during renegotiation. Where is that done? There should be PORT_Asserts in some function that is called at the beginning of a handshake (renegotiation or no) that assert that all the flags are unset. Ideally, we'd have an automated test for this too, but I'm not sure if that is practical. The point of this bug is to ensure that certain values are available when certain callbacks are called. Please add the PORT_Asserts that verify that the bits that are supposed to be set are actually set before calling each callback (false start, auth certificate, finished, etc.). ::: lib/ssl/sslt.h @@ +99,5 @@ > ssl_compression_deflate = 1 /* RFC 3749 */ > } SSLCompressionMethod; > > +typedef enum { > + ssl_info_set_all = 0, What does ssl_info_set_all mean? It should be documented here, if it is necessary. @@ +103,5 @@ > + ssl_info_set_all = 0, > + ssl_info_set_version = 1, > + ssl_info_set_cipher_suite = 2, > + ssl_info_set_kea = 3 > +} SSLInfoValueSet; Let's not use an enum for bitfield values, but rather only for enumerated things. Instead, let's use a bitfield, where every bitfield member has a name that clearly corresponds to the name of the data member it is for. For example, a boolean bitfield "preliminaryKEATypeSet" for preliminaryKEAType. Note that libssl uses bitfields already, e.g. for ss->opt. Also, you are missing a flag to indicate when the authentication type is set, and there's no documentation indicating that the authentication type is set when the KEA flag is set. IMO, it is better to just have a separate explicit auth type flag. @@ +130,5 @@ > const char * compressionMethodName; > SSLCompressionMethod compressionMethod; > + > + /* The following fields are added in NSS 3.19 */ > + /* The following fields are only set for SSL3 and TLS sessions. */ What is the advantage of having all of these be new fields in this structure, and having SSL_GetChannelInfo fill them in, versus defining a new structure that a new function would fill in? To me, a new separate structure and a new separate function make things much more clear. In particular, it seems like a caller would either want to access the old structure values or the new structure values, but never both. Also, it isn't clear whether/how it is safe to access the older structure values. Document (and verify) that these flags are reset at the start of a renegotiation. @@ +141,5 @@ > + PRUint16 preliminaryProtocolVersion; > + /* cipher suite: test */ > + PRUint16 preliminaryCipherSuite; > + /* key exchange algorithm: test ssl_inf_set_kea */ > + SSLKEAType preliminaryKeaType; s/Kea/KEA/. @@ +142,5 @@ > + /* cipher suite: test */ > + PRUint16 preliminaryCipherSuite; > + /* key exchange algorithm: test ssl_inf_set_kea */ > + SSLKEAType preliminaryKeaType; > + SSLSignType preliminaryAuthType; In my patch in the other bug, I also used SSLSignType, mostly because that is what is handy. However, note that if NSS ever implements a public key authentication mechanism that isn't signature based (as has been proposed in the IETF TLS WG mailing list), then SSLSignType won't make sense. So, perhaps SSLAuthType should be used instead. Also, please provide the same level of documentation as I provided in bug 1133566. In particular, it is important to document how RSA key exchange, which doesn't use a signature, is indicated. Also, indicate whether preliminaryAuthType is calculated based on the negotiated cipher suite or based on the type of the public key of the certificate or whatever. (It should be calculated based on the negotiated cipher suite only.)

Thanks for the early feedback Brian. The plan discussed yesterday was to defer this change until 3.18.1, once all the Firefox root CA changes are settled. Enjoy your vacation.

I'm currently working on unit tests for this. (In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #18) > The flags need to be reset during renegotiation. Where is that done? There > should be PORT_Asserts in some function that is called at the beginning of a > handshake (renegotiation or no) that assert that all the flags are unset. > Ideally, we'd have an automated test for this too, but I'm not sure if that > is practical. Good call. Damn I hate renegotiation. > The point of this bug is to ensure that certain values are available when > certain callbacks are called. Please add the PORT_Asserts that verify that > the bits that are supposed to be set are actually set before calling each > callback (false start, auth certificate, finished, etc.). Will do. Since I'm going to recommend use of the function only prior to finished, I think that it is safe to avoid the asserts for finished. > ::: lib/ssl/sslt.h > @@ +99,5 @@ > > ssl_compression_deflate = 1 /* RFC 3749 */ > > } SSLCompressionMethod; > > > > +typedef enum { > > + ssl_info_set_all = 0, > > What does ssl_info_set_all mean? It should be documented here, if it is > necessary. I think that I've decided that it is unnecessary now; it was originally there to carry the value of enoughFirstHSDone. > Let's not use an enum for bitfield values, but rather only for enumerated > things. > > Instead, let's use a bitfield, where every bitfield member has a name that > clearly corresponds to the name of the data member it is for. For example, a > boolean bitfield "preliminaryKEATypeSet" for preliminaryKEAType. I was concerned that that would not be properly portable. It's fine syntactic sugar for internal use, but I'm not confident that we can maintain ABI compatibility. Even if that were possible, the info struct becomes a bit less manageable when adding new values if a bitfield is used. > @@ +130,5 @@ > > const char * compressionMethodName; > > SSLCompressionMethod compressionMethod; > > + > > + /* The following fields are added in NSS 3.19 */ > > + /* The following fields are only set for SSL3 and TLS sessions. */ > > What is the advantage of having all of these be new fields in this > structure, and having SSL_GetChannelInfo fill them in, versus defining a new > structure that a new function would fill in? To me, a new separate structure > and a new separate function make things much more clear. In particular, it > seems like a caller would either want to access the old structure values or > the new structure values, but never both. Also, it isn't clear whether/how > it is safe to access the older structure values. Yes, you are right; a new function is a lot cleaner. > Document (and verify) that these flags are reset at the start of a > renegotiation. Gotcha. > @@ +142,5 @@ > > + /* cipher suite: test */ > > + PRUint16 preliminaryCipherSuite; > > + /* key exchange algorithm: test ssl_inf_set_kea */ > > + SSLKEAType preliminaryKeaType; > > + SSLSignType preliminaryAuthType; > > In my patch in the other bug, I also used SSLSignType, mostly because that > is what is handy. However, note that if NSS ever implements a public key > authentication mechanism that isn't signature based (as has been proposed in > the IETF TLS WG mailing list), then SSLSignType won't make sense. So, > perhaps SSLAuthType should be used instead. > > Also, please provide the same level of documentation as I provided in bug > 1133566. In particular, it is important to document how RSA key exchange, > which doesn't use a signature, is indicated. > > Also, indicate whether preliminaryAuthType is calculated based on the > negotiated cipher suite or based on the type of the public key of the > certificate or whatever. (It should be calculated based on the negotiated > cipher suite only.) There is no value in exposing information here that is derived solely from the cipher suite if the cipher suite is provided. Calling SSL_GetCipherSuiteInfo is the right way to manage that. That leaves me wondering what the actual point of the KEAType and SignType (or AuthType) parameters is here. I didn't originally question your assertion about the need for these parameters, but if the need can be handled by exposing the cipher suite, then I'd prefer to do that instead. I'm going to drop these, unless there is something I've missed, that is.

(In reply to Martin Thomson [:mt] from comment #21) > > The point of this bug is to ensure that certain values are available when > > certain callbacks are called. Please add the PORT_Asserts that verify that > > the bits that are supposed to be set are actually set before calling each > > callback (false start, auth certificate, finished, etc.). > > Will do. Since I'm going to recommend use of the function only prior to > finished, I think that it is safe to avoid the asserts for finished. IMO it makes sense to allow it for all callbacks including the handshake (finished) callback. I can't see why it would be better to not allow/recommend/verify-it-works for the handshake callback. > > Let's not use an enum for bitfield values, but rather only for enumerated > > things. > > > > Instead, let's use a bitfield, where every bitfield member has a name that > > clearly corresponds to the name of the data member it is for. For example, a > > boolean bitfield "preliminaryKEATypeSet" for preliminaryKEAType. > > I was concerned that that would not be properly portable. It's fine > syntactic sugar for internal use, but I'm not confident that we can maintain > ABI compatibility. Even if that were possible, the info struct becomes a > bit less manageable when adding new values if a bitfield is used. OK. It would be good to find another way to get the same advantage that the bitfield offers--a very clear mapping of the name of the flags to the fields they are flagging. > That leaves me wondering what the actual point of the KEAType and SignType > (or AuthType) parameters is here. I didn't originally question your > assertion about the need for these parameters, but if the need can be > handled by exposing the cipher suite, then I'd prefer to do that instead. > > I'm going to drop these, unless there is something I've missed, that is. The interface I designed for bug 1133566 was for certificate verification, and certificate verification doesn't need all the cipher suite info, just those two aspects of it. But, for this more general interface, I agree it is fine to expose the cipher suite and then have the caller call SSL_GetCipherSuiteInfo to get whatever details are needed.

OK, I think that I got all the feedback sorted out.

This one adds version testing to all of the existing tests (in addition to the asserts). It's a good thing I did this actually, because I missed something the first time around.

Comment on attachment 8577358 [details] [diff] [review] bug1084669-1.patch Review of attachment 8577358 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but some things should be cleaned up. ::: lib/ssl/ssl.h @@ +898,5 @@ > + * Caller supplies the info struct. Function fills it in. > + * The information here will be zeroed prior to details being confirmed. > + * Details are usually confirmed when the Finished message is sent, however > + * false start allows for earlier confirmation, if enabled. > + */ I did not review this change to SSL_GetChannelInfo. @@ +903,4 @@ > SSL_IMPORT SECStatus SSL_GetChannelInfo(PRFileDesc *fd, SSLChannelInfo *info, > PRUintn len); > +/* Get preliminary information about a channel. > + * This function can be called from various callbacks (such as the s/can/may only/ @@ +906,5 @@ > + * This function can be called from various callbacks (such as the > + * SSLAuthCertificate or SSLGetClientAuthData hooks) during the handshake > + * to get information about the channel prior to it being confirmed. > + * Note that values are marked as being unset when renegotiation is initiated. > + * If the channel is already established, use SSL_GetChannelInfo instead. */ This is confusing. Is this function allowed to be used in HandshakeCallback or not? "various callbacks" indicates "yes," but the last sentence indicates "no." The code seems to correctly allow the handshake callback to use the function. ::: lib/ssl/ssl3con.c @@ +908,5 @@ > return SECFailure; > } > > ss->version = PR_MIN(peerVersion, ss->vrange.max); > + ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_version; ssl3_NegotiateVersion is called by ssl3_SendClientHello during session resumption and ssl2_HandleV3HandshakeRecord, and we shouldn't set this flag iun those cases. We shouldn't set ss->ssl3.hs.preliminaryInfo until we've received the server hello. I suggest moving this statement to all of ssl3_HandleServerHello, ssl3_HandleClientHello, and ssl3_HandleV2ClientHello instead. @@ +4898,5 @@ > rv = ssl3_InitState(ss); > if (rv != SECSuccess) { > return rv; /* ssl3_InitState has set the error code. */ > } > + /* These MUST be reset every handshake */ s/MUST/must/. Shouting is just confusing here, since we don't shout elsewhere (usually). Also, end sentences with a period. @@ +4900,5 @@ > return rv; /* ssl3_InitState has set the error code. */ > } > + /* These MUST be reset every handshake */ > + ss->ssl3.hs.sendingSCSV = PR_FALSE; > + PORT_Assert(ss->ssl3.hs.preliminaryInfo == 0); For the same reason that we set sendingSCSV = PR_FALSE here, we should also set ss->ssl3.hs.preliminaryInfo = 0 here, instead of just asserting. This would centralize the resetting logic into one place for clients so that client-initiated and server-initiated renegotiation are handled in the same place. @@ +5006,5 @@ > */ > if (sid->version >= ss->vrange.min && > sid->version <= ss->clientHelloVersion) { > ss->version = ss->clientHelloVersion; > + ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_version; This seems wrong as explained above. Here we're setting the version that will be sent in the ClientHello, but this hasn't been negotiated (agreed to) by the server yet, unless/until it resumes that session. We should only be setting these flags in ssl3_Handle*. This is doubly wrong considering bug 1134548. @@ +5406,5 @@ > > if (IS_DTLS(ss)) { > dtls_RehandshakeCleanup(ss); > } > + ss->ssl3.hs.preliminaryInfo = 0; This isn't needed if ssl3_SendClientHello does it as I suggest above. @@ +7085,5 @@ > ss->ssl3.hs.ws = wait_hello_done; > > if (ss->getClientAuthData != NULL) { > + PORT_Assert(ss->ssl3.hs.preliminaryInfo & ssl_preinfo_version); > + PORT_Assert(ss->ssl3.hs.preliminaryInfo & ssl_preinfo_cipher_suite); Suggestion: 1. Define a ssl_preinfo_all that is all such flags OR'd together. 2. Express such assertions as PORT_Assert(ss->ssl3.hs.preliminaryInfo & ssl_preinfo_all) or PORT_Assert(ss->ssl3.hs.preliminaryInfo & (ssl_preinfo_all & ~ssl_preinfo_flag_that_isn't_expected_to_be_set). This way, you won't ever forget to update these new assertions when you add new flags. @@ +7191,5 @@ > SSL_TRC(3, ("%d: SSL[%d]: no false start due to weak cipher", > SSL_GETPID(), ss->fd)); > } else { > + PORT_Assert(ss->ssl3.hs.preliminaryInfo & ssl_preinfo_version); > + PORT_Assert(ss->ssl3.hs.preliminaryInfo & ssl_preinfo_cipher_suite); ditto. @@ +7647,5 @@ > > PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) ); > PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss)); > PORT_Assert( ss->ssl3.initialized ); > + PORT_Assert(ss->ssl3.hs.preliminaryInfo == 0); Again, it seems clearer to just reset the flags here, not just assert they are zero here. Then we can remove the other places where a server would reset the flags. @@ +8248,5 @@ > if (ssl3_ExtensionNegotiated(ss, ssl_server_name_xtn)) { > int ret = 0; > if (ss->sniSocketConfig) do { /* not a loop */ > + PORT_Assert(ss->ssl3.hs.preliminaryInfo & ssl_preinfo_version); > + PORT_Assert(ss->ssl3.hs.preliminaryInfo & ssl_preinfo_cipher_suite); ditto assertion suggestion. @@ +10014,5 @@ > > ss->ssl3.hs.authCertificatePending = PR_FALSE; > > + PORT_Assert(ss->ssl3.hs.preliminaryInfo & ssl_preinfo_version); > + PORT_Assert(ss->ssl3.hs.preliminaryInfo & ssl_preinfo_cipher_suite); ditto. @@ +11805,5 @@ > ss->ssl3.prSpec = ss->ssl3.pwSpec = &ss->ssl3.specs[1]; > ss->ssl3.hs.sendingSCSV = PR_FALSE; > ssl3_InitCipherSpec(ss, ss->ssl3.crSpec); > ssl3_InitCipherSpec(ss, ss->ssl3.prSpec); > + ss->ssl3.hs.preliminaryInfo = 0; Assuming that the new function is only allowed to be called within the callback functions, this shouldn't be needed, AFAICT. @@ +12096,5 @@ > ss->sec.uncache(sid); /* remove it from whichever cache it's in. */ > ssl_FreeSID(sid); /* dec ref count and free if zero. */ > ss->sec.ci.sid = NULL; > } > + ss->ssl3.hs.preliminaryInfo = 0; IMO, it is better to omit this and put the resetting in the places I suggest above. ::: lib/ssl/ssl3ext.c @@ +629,5 @@ > if (rv != SECSuccess) > return rv; > > PORT_Assert(ss->nextProtoCallback); > + PORT_Assert(ss->ssl3.hs.preliminaryInfo & ssl_preinfo_version); Ditto flag assertion suggestion. In particular, you are missing the assertion that the cipher suite flag is set. ::: lib/ssl/sslimpl.h @@ +855,5 @@ > #define backupHash md5 > PK11Context * md5; > PK11Context * sha; > > + Remove this unrelated change (blank line). ::: lib/ssl/sslsecur.c @@ +134,5 @@ > > + PORT_Assert(ss->version < SSL_LIBRARY_VERSION_3_0 || > + ss->ssl3.hs.preliminaryInfo & ssl_preinfo_version); > + PORT_Assert(ss->version < SSL_LIBRARY_VERSION_3_0 || > + ss->ssl3.hs.preliminaryInfo & ssl_preinfo_cipher_suite); Ditto assertion suggestion. Also, move these assertions to be directly above the call to handshakeCallback. ::: lib/ssl/sslt.h @@ +56,1 @@ > #define kt_kea_size ssl_kea_size Undo all these unrelated changes or move them to another changeset. @@ +56,4 @@ > #define kt_kea_size ssl_kea_size > > typedef enum { > + ssl_sign_null = 0, Ditto. @@ +62,5 @@ > ssl_sign_ecdsa = 3 > } SSLSignType; > > typedef enum { > + ssl_auth_null = 0, Ditto. @@ +88,5 @@ > + ssl_mac_null = 0, > + ssl_mac_md5 = 1, > + ssl_mac_sha = 2, > + ssl_hmac_md5 = 3, /* TLS HMAC version of mac_md5 */ > + ssl_hmac_sha = 4, /* TLS HMAC version of mac_sha */ Ditto. @@ +98,5 @@ > ssl_compression_null = 0, > ssl_compression_deflate = 1 /* RFC 3749 */ > } SSLCompressionMethod; > > + Ditto. @@ +125,5 @@ > SSLCompressionMethod compressionMethod; > } SSLChannelInfo; > > +/* Preliminary channel info is added in NSS 3.19 */ > +#define ssl_preinfo_version (1U << 0) Let's not bother trying to align the values. It never ends up working well. Instead, just use one space. @@ +128,5 @@ > +/* Preliminary channel info is added in NSS 3.19 */ > +#define ssl_preinfo_version (1U << 0) > +#define ssl_preinfo_cipher_suite (1U << 1) > + > +typedef struct SSLPreliminaryChannelInfoStr { In new code, let's not add multiple spaces between the type and variable name. In particular, let's not try to align variable names in declaration blocks any more. It never ends up working well. @@ +129,5 @@ > +#define ssl_preinfo_version (1U << 0) > +#define ssl_preinfo_cipher_suite (1U << 1) > + > +typedef struct SSLPreliminaryChannelInfoStr { > + PRUint32 length; Document that length must be set to the size of the struct.

Comment on attachment 8577360 [details] [diff] [review] bug1084669-2.patch Review of attachment 8577360 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I don't have time to check this, particularly since I'm not familiar with the testing framework.

Comment on attachment 8577358 [details] [diff] [review] bug1084669-1.patch Review of attachment 8577358 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/ssl/ssl3con.c @@ +7647,5 @@ > > PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) ); > PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss)); > PORT_Assert( ss->ssl3.initialized ); > + PORT_Assert(ss->ssl3.hs.preliminaryInfo == 0); In particular, won't this assertion sometimes fail, in particularly when the client initiates renegotiation by sending a ClientHello without the server asking for one?

Setting target milestone to 3.18.2 since there's no 3.19 target milestone. dveditz, I've forgottom how to get new target milestones added, but I know you know how. Could you please add target milestones 3.19+ to the NSS product and then set the target milestone of this bug to 3.19? Thanks!

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #25) I'll take all your suggestions, unless noted. I'll post an interdiff if you want to review that instead of the updated patches. > ::: lib/ssl/ssl.h > @@ +898,5 @@ > > + * Caller supplies the info struct. Function fills it in. > > + * The information here will be zeroed prior to details being confirmed. > > + * Details are usually confirmed when the Finished message is sent, however > > + * false start allows for earlier confirmation, if enabled. > > + */ > > I did not review this change to SSL_GetChannelInfo. This is a documentation update only, since there wasn't any here. I've done another copy-edit pass for the next round. > @@ +903,4 @@ > > SSL_IMPORT SECStatus SSL_GetChannelInfo(PRFileDesc *fd, SSLChannelInfo *info, > > PRUintn len); > > +/* Get preliminary information about a channel. > > + * This function can be called from various callbacks (such as the > > s/can/may only/ I don't think that we need to prohibit its use beyond HandshakeCallback. The reason we might prefer SSL_GetChannelInfo over this is that the information there is both final and more complete. Is this less confusing? /* Get preliminary information about a channel. * This function can be called prior to handshake details being confirmed (see * SSL_GetChannelInfo above for what that means). Thus, information provided by * this function is available to SSLAuthCertificate, SSLGetClientAuthData, * SSLSNISocketConfig, and other callbacks that might be called during the * processing of the first flight of client of server handshake messages. * Values are marked as being unavailable when renegotiation is initiated. */ I've explained more precisely in the description of SSL_GetChannelInfo when information can be expected to be available. > @@ +4900,5 @@ > > return rv; /* ssl3_InitState has set the error code. */ > > } > > + /* These MUST be reset every handshake */ > > + ss->ssl3.hs.sendingSCSV = PR_FALSE; > > + PORT_Assert(ss->ssl3.hs.preliminaryInfo == 0); > > For the same reason that we set sendingSCSV = PR_FALSE here, we should also > set ss->ssl3.hs.preliminaryInfo = 0 here, instead of just asserting. This > would centralize the resetting logic into one place for clients so that > client-initiated and server-initiated renegotiation are handled in the same > place. I've moved what was in ssl3_RedoHandshake to here, and also to ssl3_SendHelloRequest and ssl3_HandleClientHello as you recommended anyway. > @@ +5006,5 @@ > > */ > > if (sid->version >= ss->vrange.min && > > sid->version <= ss->clientHelloVersion) { > > ss->version = ss->clientHelloVersion; > > + ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_version; > > This seems wrong as explained above. Here we're setting the version that > will be sent in the ClientHello, but this hasn't been negotiated (agreed to) > by the server yet, unless/until it resumes that session. Yes, I agree, we should only capture what has been "negotiated", even if it hasn't yet been confirmed sufficiently. > 1. Define a ssl_preinfo_all that is all such flags OR'd together. > 2. Express such assertions as PORT_Assert(ss->ssl3.hs.preliminaryInfo & > ssl_preinfo_all) or PORT_Assert(ss->ssl3.hs.preliminaryInfo & > (ssl_preinfo_all & ~ssl_preinfo_flag_that_isn't_expected_to_be_set). I added and removed this a couple of times; I couldn't decide whether it was worthwhile. That method of checking is definitely more robust though. > @@ +7647,5 @@ > > > > PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) ); > > PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss)); > > PORT_Assert( ss->ssl3.initialized ); > > + PORT_Assert(ss->ssl3.hs.preliminaryInfo == 0); > > Again, it seems clearer to just reset the flags here, not just assert they > are zero here. Then we can remove the other places where a server would > reset the flags. OK. > @@ +11805,5 @@ > > ss->ssl3.prSpec = ss->ssl3.pwSpec = &ss->ssl3.specs[1]; > > ss->ssl3.hs.sendingSCSV = PR_FALSE; > > ssl3_InitCipherSpec(ss, ss->ssl3.crSpec); > > ssl3_InitCipherSpec(ss, ss->ssl3.prSpec); > > + ss->ssl3.hs.preliminaryInfo = 0; > > Assuming that the new function is only allowed to be called within the > callback functions, this shouldn't be needed, AFAICT. If we are using calloc() or equivalent; doubly so. I like to be explicit about this stuff. > ::: lib/ssl/ssl3ext.c > @@ +629,5 @@ > > if (rv != SECSuccess) > > return rv; > > > > PORT_Assert(ss->nextProtoCallback); > > + PORT_Assert(ss->ssl3.hs.preliminaryInfo & ssl_preinfo_version); > > Ditto flag assertion suggestion. In particular, you are missing the > assertion that the cipher suite flag is set. Extensions are processed prior to cipher suite selection. We use extensions to enable or disable ECC, for instance. That complicates the HTTP/2 stuff we are using for this, but I don't see any clean way to reorganize the handshake to make cipher suite available at this point. > ::: lib/ssl/sslt.h > @@ +56,1 @@ > > #define kt_kea_size ssl_kea_size > > Undo all these unrelated changes or move them to another changeset. Sorry about that. It can wait until we apply clang-format. > Let's not bother trying to align the values. It never ends up working well. > Instead, just use one space. I'm pleased you think that way. Saves me a lot of effort.

This is a slightly old interdiff. The only change I made after this was to correct the modified PORT_Assert lines.

Arguably, these renegotiation tests could go elsewhere, but I wanted to check that I wasn't tripping any assertions anywhere, or had made wrong assumptions about where the preliminary info was set/unset. Seems like as good a place to land them as any.

Comment on attachment 8586416 [details] [diff] [review] bug1084669-2.patch Review of attachment 8586416 [details] [diff] [review]: ----------------------------------------------------------------- This looks rightish but I have some questions. ::: external_tests/ssl_gtest/tls_agent.cc @@ +57,5 @@ > EXPECT_EQ(SECSuccess, rv); > if (rv != SECSuccess) return false; > + > + rv = SSL_SetCanFalseStartCallback(ssl_fd_, CanFalseStartCallback, > + reinterpret_cast<void*>(this)); No cast needed here. @@ +72,5 @@ > EXPECT_EQ(SECSuccess, rv); > if (rv != SECSuccess) return false; > > + rv = SSL_HandshakeCallback(ssl_fd_, HandshakeCallback, > + reinterpret_cast<void*>(this)); No cast needed here. @@ +177,5 @@ > + SSLPreliminaryChannelInfo info; > + ASSERT_EQ(SECSuccess, > + SSL_GetPreliminaryChannelInfo(ssl_fd_, &info, sizeof(info))); > + ASSERT_TRUE((info.valuesSet & ssl_preinfo_version) != 0); > + ASSERT_TRUE((info.valuesSet & ssl_preinfo_cipher_suite) != 0); Do you need != 0? Is this just a type checking issue in the ASSERT? @@ +195,5 @@ > +} > + > +void TlsAgent::Connected() { > + LOG("Handshake success"); > + ASSERT_TRUE(handshake_callback_called_); Why are you asserting this but not the false start callback? @@ +208,5 @@ > + ASSERT_EQ(expected_version_, info_.protocolVersion); > + } > + if (expected_cipher_suite_) { > + ASSERT_EQ(expected_cipher_suite_, info_.cipherSuite); > + } Why would these ever be nonzero? ::: external_tests/ssl_gtest/tls_agent.h @@ +166,5 @@ > return SSL_SNI_CURRENT_CONFIG_IS_USED; > } > > + static SECStatus CanFalseStartCallback(PRFileDesc *fd, void *arg, > + PRBool *canFalseStart) { Indent.

Comment on attachment 8586420 [details] [diff] [review] bug1084669-3.patch Review of attachment 8586420 [details] [diff] [review]: ----------------------------------------------------------------- I think there may be a glitch here. ::: external_tests/ssl_gtest/ssl_loopback_unittest.cc @@ +208,5 @@ > + Handshake(); > + CheckConnected(); > +} > + > +TEST_P(TlsConnectStream, ConnectResumed) { Is there a reason this was changed to Stream from Generic and moved? ::: external_tests/ssl_gtest/tls_agent.h @@ +82,5 @@ > void StartConnect(); > void CheckKEAType(SSLKEAType type) const; > > void Handshake(); > + // Marks the state as CONNECTING in anticipation of renegotiation. s/state/internal state/

Comment on attachment 8586414 [details] [diff] [review] bug1084669-1.patch Review of attachment 8586414 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, with the comments addressed. ::: lib/ssl/ssl3con.c @@ +7463,5 @@ > rv = ssl3_FlushHandshake(ss, 0); > if (rv != SECSuccess) { > return rv; /* error code set by ssl3_FlushHandshake */ > } > + ss->ssl3.hs.preliminaryInfo = 0; Seems like this should be removed. We don't reset it before a client-initiated renegotiation, so why reset it before a server-initiated renegotiation? It seems like if it is important to do so then there's something likely missing for the client-initiated renegotiation case. @@ +7661,5 @@ > > PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) ); > PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss)); > PORT_Assert( ss->ssl3.initialized ); > + ss->ssl3.hs.preliminaryInfo = 0; In particular, I think this makes the resetting in ssl3_SendHelloRequest unnecessary. ::: lib/ssl/ssl3ext.c @@ +632,5 @@ > > PORT_Assert(ss->nextProtoCallback); > + PORT_Assert((ss->ssl3.hs.preliminaryInfo & > + ssl_preinfo_all & ~ssl_preinfo_cipher_suite) == > + (ssl_preinfo_all & ~ssl_preinfo_cipher_suite)); Please document why the version isn't available in a TODO comment. (It wouldn't be hard to make it so that the version is available here, but I guess there's no immediate need to do that work.) ::: lib/ssl/sslt.h @@ +123,5 @@ > const char * compressionMethodName; > SSLCompressionMethod compressionMethod; > } SSLChannelInfo; > > +/* Preliminary channel info is added in NSS 3.19 */ I suggest removing "added in NSS 3.19". we don't usually add that kind of comment in the source code, at least not in the last few years.

(In reply to Eric Rescorla (:ekr) from comment #34) > > + rv = SSL_SetCanFalseStartCallback(ssl_fd_, CanFalseStartCallback, > > + reinterpret_cast<void*>(this)); > > No cast needed here. I know. Just copying you. > > + ASSERT_TRUE((info.valuesSet & ssl_preinfo_cipher_suite) != 0); > > Do you need != 0? Is this just a type checking issue in the ASSERT? Apparently it's OK. BTW, I *hate* implicit conversion, so I generally do this anyway. > > +void TlsAgent::Connected() { > > + LOG("Handshake success"); > > + ASSERT_TRUE(handshake_callback_called_); > > Why are you asserting this but not the false start callback? Because the false start callback isn't guaranteed. Sure, all the suites we negotiate by default probably meet the criteria, but I don't want to make this any more fragile than necessary. > @@ +208,5 @@ > > + ASSERT_EQ(expected_version_, info_.protocolVersion); > > + } > > + if (expected_cipher_suite_) { > > + ASSERT_EQ(expected_cipher_suite_, info_.cipherSuite); > > + } > > Why would these ever be nonzero? Good catch. I don't like if statements either. I wasn't originally capturing values in the HandshakeCallback. (In reply to Eric Rescorla (:ekr) from comment #35) > I think there may be a glitch here. Indeed there is. > ::: external_tests/ssl_gtest/ssl_loopback_unittest.cc > @@ +208,5 @@ > > + Handshake(); > > + CheckConnected(); > > +} > > + > > +TEST_P(TlsConnectStream, ConnectResumed) { > > Is there a reason this was changed to Stream from Generic and moved? I think I messed up a rebase at some point. I'll fix that.

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #36) > ::: lib/ssl/ssl3con.c > @@ +7463,5 @@ > > rv = ssl3_FlushHandshake(ss, 0); > > if (rv != SECSuccess) { > > return rv; /* error code set by ssl3_FlushHandshake */ > > } > > + ss->ssl3.hs.preliminaryInfo = 0; > > Seems like this should be removed. We don't reset it before a > client-initiated renegotiation, so why reset it before a server-initiated > renegotiation? It seems like if it is important to do so then there's > something likely missing for the client-initiated renegotiation case. That means that the server doesn't consider itself in a renegotiation until it sees the ClientHello. I'm OK with that. As you point out, the reset in ssl3_HandleClientHello is sufficient for that. > ::: lib/ssl/ssl3ext.c > @@ +632,5 @@ > > > > PORT_Assert(ss->nextProtoCallback); > > + PORT_Assert((ss->ssl3.hs.preliminaryInfo & > > + ssl_preinfo_all & ~ssl_preinfo_cipher_suite) == > > + (ssl_preinfo_all & ~ssl_preinfo_cipher_suite)); > > Please document why the version isn't available in a TODO comment. (It > wouldn't be hard to make it so that the version is available here, but I > guess there's no immediate need to do that work.) It's the cipher suite that isn't present, not the version. And fixing that is tricky because extension processing is used to determine cipher suite selection. I'll add a comment. It's only ALPN that is affected here. I could add an assertion that the cipher suite is present for NPN, but that's going to make a complex assertion even more complex.

Carrying r=briansmith

Complain and ye shall receive more patches to review. This expands our coverage of the various handshake callbacks, checking to see that they are properly called. I also added a cursory test for false start.

This missed 3.19, please set a new target milestone if you have a good estimate. (3.19.1 target milestone will soon be available once bug 1158958 is fixed.)

Comment on attachment 8588159 [details] [diff] [review] bug1084669-4.patch Review of attachment 8588159 [details] [diff] [review]: ----------------------------------------------------------------- Sorry about the delay. Can you rebase and put this on Rietveld?

Rebased and available at: https://codereview.appspot.com/250320043/ Note that I still have to settle on which release this is going to land in.

kaie, I want to ask about your note in ssl.def: ;+# If the 3.20 release includes any additional functions ;+# besides SSL_DHEGroupPrefSet and SSL_EnableWeakDHEPrimeGroup ;+# they should be labeled as NSS_3.20a Is there anything else that I need to do other than to create a new group with a name of NSS_3.20a ?

Kai, also regarding that change. SSL_DHEGroupPrefSet and SSL_EnabledWeakDHEPrimeGroup weren't added to lib/nss/nss.def. Should they be?

(In reply to Martin Thomson [:mt:] from comment #46) > Is there anything else that I need to do other than to create a new group > with a name of NSS_3.20a ? That's all that Bob suggested to do. (In reply to Martin Thomson [:mt:] from comment #47) > Kai, also regarding that change. SSL_DHEGroupPrefSet and > SSL_EnabledWeakDHEPrimeGroup weren't added to lib/nss/nss.def. Should they > be? The were added to ssl.def because the code is contained in the shared library libssl3.so / ssl3.dll etc. The nss.def file only gets the symbols that are linked to the libnss3.so / nss3.dll

Thanks Kai, http://hg.mozilla.org/projects/nss/rev/282d1b19a8bf http://hg.mozilla.org/projects/nss/rev/ad61cacacc15 http://hg.mozilla.org/projects/nss/rev/03491beff8ef http://hg.mozilla.org/projects/nss/rev/4279526d5c83

Comment on attachment 8631778 [details] [diff] [review] bug1084669-1.patch Review of attachment 8631778 [details] [diff] [review]: ----------------------------------------------------------------- Martin: I suggest some changes. (I only reviewed the public API.) ::: lib/ssl/ssl.def @@ +180,5 @@ > SSL_EnableWeakDHEPrimeGroup; > ;+ local: > ;+*; > ;+}; > +;+NSS_3.20a { # NSS 3.20 release Please remove "a" from "NSS_3.20a". ::: lib/ssl/sslt.h @@ +124,5 @@ > SSLCompressionMethod compressionMethod; > } SSLChannelInfo; > > +/* Preliminary channel info */ > +#define ssl_preinfo_version (1U << 0) These constants should ideally be defined as enumerators. If defined as macros, they should follow the usual macro naming convention: SSL_PREINFO_VERSION, SSL_PREINFO_CIPHER, SSL_PREINFO_ALL. Some enumerators in libSSL are named in lowercase, but NSS macros are almost always named in all capital letters. @@ +126,5 @@ > > +/* Preliminary channel info */ > +#define ssl_preinfo_version (1U << 0) > +#define ssl_preinfo_cipher_suite (1U << 1) > +#define ssl_preinfo_all (ssl_preinfo_version|ssl_preinfo_cipher_suite) SSL_PREINFO_ALL probably should be an internal macro. It seems a little tricky for an NSS application to use SSL_PREINFO_ALL in a backward compatible way. It will need to do SSLPreliminaryChannelInfo info; // Call SSL_GetPreliminaryChannelInfo to have |info| filled in. if ((info.valuesSet & SSL_PREINFO_ALL) == SSL_PREINFO_ALL) { // All values I know about are set. } It should not test info.valuesSet like this: if (info.valuesSet == SSL_PREINFO_ALL) { // All values I know about are set. }

As discussed by email, this will be part of NSS 3.21 I've changed the version in ssl.def to 3.21 https://hg.mozilla.org/projects/nss/rev/98f8d3310867 We need to update the target milestone of this bug when that version is available in bugzilla.