User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.70 Safari/533.4 Build Identifier: I am splitting out the versioning API changes and the patches for it from bug 565047 as they are pretty much independent of the TLS 1.1 implementation itself. I'll be uploading the patches immediately. Reproducible: Always

This edition of the version range API is designed to be fully backward-compatible with the old SSL_OptionSet version API. In particular, even when TLS 1.1 and TLS 1.2 support are added, the *ordering* of calls to SSL_OptionSet() won't have any effect on what the resulting enabled versions are. As a result, SSL2 is managed separately from SSL3.0 and TLS, and SSL_ENABLE_TLS turns all versions of TLS on or off. See my comment at https://bugzilla.mozilla.org/show_bug.cgi?id=565047#c21 for a justification. I added SSL_CanBypassInVersionRange, which is just like SSL_CanBypass but uses the the new version range API, because there could be cases where bypass would work for only certain versions of TLS (especially when TLS 1.2 support is added, since it has a new PRF based on SHA-2). SSL_CanBypass and its protocol masks would be deprecated. The documentation for SSL_CanBypass was duplicated in the header and the source file and they weren't in sync; I just removed the documentation in the source file. I added SSL_VersionRangeCheck to the version range API. I changed the signature of SSL_VersionRangeSet to include an explicit version range; before it just set the socket's version range to the default. I also removed the check that the peer's version number starts with "3"; see http://my.opera.com/yngve/blog/2010/06/02/renego-patched-servers-a-long-term-interoperability-time-bomb-brewing For now, I left the command line tools (tstclnt and selfserv) unmodified to demonstrate backward compatibility with the old API. Changes to the command-line syntax of selfserv and tstclnt will be needed when TLS 1.1 and TLS 1.2 are implemented. I am not sure what the command line syntax should be; I was thinking something like [-V2] for "enable SSL2" and "[-V version[-version]]", where version is one of "SSL3", "TLS1.0", "TLS1.1", or "TLS1.2" for enabling a specific version range. Perhaps a separate bug should be filed for the command-line tool part?

I see a big collision coming between the patches for this bug and the patches for bug 565047. > I also removed the check that the peer's version number starts with "3"; It's appropriate for a server to permit larger version numbers from a client than the maximum version number the server supports, but not for a client to accept larger version numbers from a server than what the client supports.

(In reply to comment #2) > I see a big collision coming between the patches for this bug and the > patches for bug 565047. No worries. All of the issues I've reported this weekend are part of my work on bug 565047. I will make a note over on that bug about it. > > I also removed the check that the peer's version number starts with "3"; > > It's appropriate for a server to permit larger version numbers from a client > than the maximum version number the server supports, but not for a client to > accept larger version numbers from a server than what the client supports. Yes, that logic is in ssl3_NegotiateVersion. ssl3_NegotiateVersion handles 0x0401 the same way it would handle 0x03FF. The check that the first byte is 0x03 doesn't matter. But, maybe I am misunderstanding you.

The API for application to use to query the min/max supported version may need to be modified. When TLS 1.2 is implemented, it will require the PKCS#11 implementation to provide new mechanisms based on the new TLS 1.2 PRF in non-bypass mode. However, AFAICT, official PKCS#11 identifiers for these mechanisms haven't even been assigned yet, which implies that *no* PKCS#11 modules support them. In particular, the FIPS 140-2-validated softoken doesn't support them, so until a new version of softoken is validated, FIPS mode and TLS 1.2 will be mutually exclusive. Theoretically in the future a PKCS#11 module could *only* support the TLS 1.2 mechanisms, meaning that SSL 3.0 and TLS 1.0 and TLS 1.1 would not be supported. Consequently, the function that returns the min/max versions supported must also know the values of the FIPS and bypass settings, as well as which PKCS#11 module will be used. Also, I wrote the patch so that the maximum implemented version is enabled by default. However, for backwards compatibility with existing libssl users, and particularly to avoid performance regressions that are inevitable as a result of the additional overhead of the TLS 1.1+ CBC mode and the use of SHA-256 in TLS 1.2, the default maximum version enabled by default should probably remain TLS 1.0, and the SSL_ENABLE_TLS option should only toggle TLS 1.0 on and off. I think that the whole idea of "default maximum version supported" should be deprecated--that is, applications that use NSS should be encouraged to explicitly set the range of versions supported during initialization using the new API. The new API should make it easy for the application to always get the maximum version supported.

I would like to change the API that I proposed to the following: Remove SSL_LIBRARY_VERSION_MAX_SUPPORTED, because it would be a compile-time constant, when really an application is better off getting the maximum version at runtime, when libssl is a shared library. Also, as I mentioned above, there are multiple definitions of "supported," and some of those depend on information only available at runtime even for a given release of libssl. /* Returns the maximum version understood by libssl */ SSL3ProtocolVersion SSL_MaxVersionSupportedBySSL(); /* Returns the maximum version that is possible to use, * based on the currently installed tokens. This will * always be less than or equal to the result of * MaxVersionSupportedBySSL. */ SSL3ProtocolVersion SSL_MaxVersionSupportedByTokens( PRBool bypassMode); /* Returns the minimum version understood by libssl, * except it does not consider SSL 2.0. * * If bypassMode is true, then the result will be * limited to versions in which bypass mode will work. */ SSL3ProtocolVersion SSL_MinVersionSupportedBySSL(); /* Returns the maximum version that is possible to use, * based on the currently installed tokens. This will * always be greater than or equal to the result of * MinVersionSupportedBySSL. * * If bypassMode is true, then the result will be * limited to versions in which bypass mode will work. */ SSL3ProtocolVersion SSL_MinVersionSupportedByTokens( PRBool bypassMode); /* Set the maximum value advertised in client hellos * and the maximum version accepted by servers. Fails * if larger than SSL_MaxVersionSupportedBySSL. */ SECStatus SSL_SetMaxVersion(PRFileDesc *fd, SSL3ProtocolVersion); /* Set the maximum value advertised in client hellos * and the maximum version accepted by servers. Fails * if less than SSL_MaxVersionSupportedBySSL. */ SECStatus SSL_SetMinVersion(PRFileDesc *fd, SSL3ProtocolVersion); Note that this would remove the SSLVersionRange header and would

...not provide the functions for setting the defaults. I think we should avoid providing such "get/set the defaults" functions for future options, as the duplication is annoying.

Comment on attachment 450895 [details] [diff] [review] Patch to implement new API for SSL3 and TLS 1.0 Review of attachment 450895 [details] [diff] [review]: ----------------------------------------------------------------- Overall the patch is good. Thanks for writing it. 1. The 'vr' argument name is too short. Please change it to 'range', 'vrange', or 'versionRange'. 2. I don't understand the need for SSL_CanBypassInVersionRange that you explained in comment 1. I suggest that we not add this function prematurely. You can just add a TODO comment for now. ::: lib/ssl/derive.c @@ -852,2 +840,5 @@ > > } > > > > +SECStatus > > +SSL_CanBypassInVersionRange(CERTCertificate *cert, SECKEYPrivateKey *srvPrivkey, > > + SSLVersionRange const * vr, Put 'const' before SSLVersionRange. @@ -854,0 +842,13 @@ > > +SECStatus > > +SSL_CanBypassInVersionRange(CERTCertificate *cert, SECKEYPrivateKey *srvPrivkey, > > + SSLVersionRange const * vr, > > + PRUint16 *ciphersuites, int nsuites, NaN more ... BUG: the test should be vr->min <= SSL_LIBRARY_VERSION_3_1_TLS_1_0 ::: lib/ssl/ssl.h @@ -110,4 +112,4 @@ > > /* (off by default) */ > > #define SSL_V2_COMPATIBLE_HELLO 12 /* send v3 client hello in v2 fmt */ > > /* (on by default) */ > > -#define SSL_ENABLE_TLS 13 /* enable TLS (on by default) */ > > +#define SSL_ENABLE_TLS 13 /* enable TLS (on by default). The comment should clarify whetner this option enables TLS 1.0 or all versions of TLS. It is a bad idea for this option to mean "enable all versions of TLS". Perhaps SSL_ENABLE_TLS=PR_FALSE should disable all versions of TLS (to avoid leaving a hole in the version range), but SSL_ENABLE_TLS=PR_TRUE should enable oly TLS 1.0? @@ -144,1 +148,5 @@ > > > > +/* SSL version range setting API. This API should be used to control SSL3 & > > +** TLS support instead of the older SSL_Option* API; however, the SSL_Option* > > +** API MUST still be used to control SSL2 support. By default, SSL3 and all > > +** implemented versions of TLS are enabled. It seems bad to enable all implemented versions of TLS by default. @@ -145,0 +149,13 @@ > > +/* SSL version range setting API. This API should be used to control SSL3 & > > +** TLS support instead of the older SSL_Option* API; however, the SSL_Option* > > +** API MUST still be used to control SSL2 support. By default, SSL3 and all > > +** implemented versions of TLS are enabled. NaN more ... In the comments for the various Get functions, change Sets |vr| to Sets |*vr| @@ -602,5 @@ > **/ > - > -/* protocol mask bits */ > -#define SSL_CBP_SSL3 0x0001 /* test SSL v3 mechanisms */ > -#define SSL_CBP_TLS1_0 0x0002 /* test TLS v1.0 mechanisms */ Keep the comments on these three lines (604-606). Just add the "Deprecated" comment, but preserve the original comments. ::: lib/ssl/ssl3con.c @@ +637,5 @@ > if (!ss) { > PORT_SetError(SEC_ERROR_INVALID_ARGS); > return 0; > } > + if (ss->vrange.min == SSL_LIBRARY_VERSION_NO_SSL3_OR_TLS) { This expression occurs several times. It may be a good idea to create a macro for it, such as #define SSL_ALL_VERSIONS_DISABLED(vrange) \ ((vrange)->min == SSL_LIBRARY_VERSION_NO_SSL3_OR_TLS) (SSL_NO_VERSIONS_ENABLED would be another good name for the macro.) Then this would read: if (SSL_ALL_VERSIONS_DISABLED(&ss->vrange)) { @@ +798,2 @@ > /* what are we doing here? */ > + PORT_Assert(ss->vrange.min != SSL_LIBRARY_VERSION_NO_SSL3_OR_TLS); This assertion is confusing. After looking at this assertion for several minutes, I finally realized that it asserts that we should not reach here. PORT_Assert(0) or PR_NOT_REACHED("SSL or TLS should be enabled") would make the intention more obvious. @@ -4901,5 @@ > version = (SSL3ProtocolVersion)temp; > > - /* this is appropriate since the negotiation is complete, and we only > - ** know SSL 3.x. > - */ This comment and the code should be preserved but updated. Change "we only know SSL 3.x" to "the server chose an SSL version newer than what we advertised". Change the test if (MSB(version) != MSB(SSL_LIBRARY_VERSION_3_0)) { to if (version > ss->vrange.max) { This test is not performed by ssl3_NegotiateVersion, so we must do the test separately. (This is the problem that Nelson noted in comment 2.) ::: lib/ssl/sslcon.c @@ +3172,5 @@ > cp = msg = ss->sec.ci.sendBuf.buf; > msg[0] = SSL_MT_CLIENT_HELLO; > + ss->clientHelloVersion = ss->vrange.min != SSL_LIBRARY_VERSION_NO_SSL3_OR_TLS > + ? ss->vrange.max > + : SSL_LIBRARY_VERSION_2; Format this statement as: ss->clientHelloVersion = (ss->vrange.min != SSL_LIBRARY_VERSION_NO_SSL3_OR_TLS) ? ss->vrange.max : SSL_LIBRARY_VERSION_2; ::: lib/ssl/sslimpl.h @@ +1293,5 @@ > { if (!ss->opt.noLocks) PZ_ExitMonitor( (ss)->xmitBufLock); } > #define ssl_HaveXmitBufLock(ss) \ > (PZ_InMonitor((ss)->xmitBufLock)) > > +#define SSL_LIBRARY_VERSION_NO_SSL3_OR_TLS 0 I suggest naming this macro SSL_LIBRARY_VERSION_NONE. ::: lib/ssl/sslproto.h @@ -47,4 +47,4 @@ > > /* All versions less than 3_0 are treated as SSL version 2 */ > > #define SSL_LIBRARY_VERSION_2 0x0002 > > #define SSL_LIBRARY_VERSION_3_0 0x0300 > > -#define SSL_LIBRARY_VERSION_3_1_TLS 0x0301 > > +#define SSL_LIBRARY_VERSION_3_1_TLS_1_0 0x0301 SSL_LIBRARY_VERSION_3_1_TLS_1_0 contains redundant info (SSL 3.1 is TLS 1.0). I think the original name SSL_LIBRARY_VERSION_3_1_TLS is fine. We would add SSL_LIBRARY_VERSION_3_2_TLS for TLS 1.1 and SSL_LIBRARY_VERSION_3_3_TLS for TLS 1.2. Alternatively, we can rename this macro SSL_LIBRARY_VERSION_TLS_1_0, and add SSL_LIBRARY_VERSION_TLS_1_1 and SSL_LIBRARY_VERSION_TLS_1_2 later. ::: lib/ssl/sslsock.c @@ +565,5 @@ > return PR_CallOnce(&setupBypassOnce, &SSL_BypassRegisterShutdown); > } > > +static void > +ssl_enable_tls(SSLVersionRange * vrange, PRBool on) { These two functions are very difficult to understand. Please add comments to explain in prose what the nested if statements do. Then, add two comments to explain what these two functions do (they handle the SSL_ENABLE_TLS and SSL_ENABLE_SSL3 options). @@ +878,5 @@ > + on = versions_defaults.max >= SSL_LIBRARY_VERSION_3_1_TLS_1_0; > + break; > + case SSL_ENABLE_SSL3: > + on = versions_defaults.min == SSL_LIBRARY_VERSION_3_0; > + break; Please leave these two cases where they were to minimize the diffs. @@ -1364,1 +1413,5 @@ > > > > +PRBool > > +ssl3_VersionIsSupported(SSL3ProtocolVersion v) { > > + return v == SSL_LIBRARY_VERSION_3_1_TLS_1_0 || > > + v == SSL_LIBRARY_VERSION_3_0; It is better to write this as return v <= SSL_LIBRARY_VERSION_3_1_TLS_1_0 && v >= SSL_LIBRARY_VERSION_3_0; It'll be easier to update when we add TLS 1.1 and TLS 1.2 support. You can even replace SSL_LIBRARY_VERSION_3_1_TLS_1_0 with SSL_LIBRARY_VERSION_MAX_SUPPORTED. (I'm not sure if you still want to remove SSL_LIBRARY_VERSION_MAX_SUPPORTED.) @@ +1417,5 @@ > + v == SSL_LIBRARY_VERSION_3_0; > +} > + > +SECStatus > +SSL_VersionRangeCheck(const SSLVersionRange *vr) { I think this function should return PRBool and be named SSL_VersionRangeValid. @@ +1422,5 @@ > + if (!vr || > + !ssl3_VersionIsSupported(vr->min) || > + !ssl3_VersionIsSupported(vr->max) || > + vr->min > vr->max) { > + PORT_SetError(SEC_ERROR_INVALID_ARGS); We should add a new error code for invalid SSL version range. SEC_ERROR_INVALID_ARGS is too generic to be useful in a bug report.

Comment on attachment 450895 [details] [diff] [review] Patch to implement new API for SSL3 and TLS 1.0 I reviewed Nelson's patch v7 (attachment 510370 [details] [diff] [review]) in Bug 565047 carefully. I like some of his ideas: 1. He added functions to make version tests more readable. For example, ssl_V3xIsEnabled (similar to my proposed SSL_ALL_VERSION_DISABLED) ssl_VersionIsInRange 2. He interprets SSL_ENABLE_TLS in the same way I proposed: Enable TLS 1.0, OR Disable all TLS 1.x 3. In struct sslOptionsStr, he leaves the original enableSSL2, enableSSL3, and enableTLS bits there and simply renames them unused1, unused2, and unused3. This reduces the diffs. I have some more comments about this patch. In ssl2_BeginClientHandshake: > while (sid) { /* this isn't really a loop */ > /* if we're not doing this SID's protocol any more, drop it. */ >- if (((sid->version < SSL_LIBRARY_VERSION_3_0) && !ss->opt.enableSSL2) || >- ((sid->version == SSL_LIBRARY_VERSION_3_0) && !ss->opt.enableSSL3) || >- ((sid->version > SSL_LIBRARY_VERSION_3_0) && !ss->opt.enableTLS)) { >+ if ((sid->version < SSL_LIBRARY_VERSION_3_0 && !ss->opt.enableSSL2) || >+ (sid->version >= SSL_LIBRARY_VERSION_3_0 && >+ (ss->vrange.min == SSL_LIBRARY_VERSION_NO_SSL3_OR_TLS || >+ sid->version < ss->vrange.min || >+ sid->version > ss->vrange.max))) { Nelson did a better job with this conditional expression. It becomes simply: if (!ssl_VersionIsEnabled(ss, sid->version)) { I hope we can do something like that. >+static void >+ssl_enable_tls(SSLVersionRange * vrange, PRBool on) { ... >+static void >+ssl_enable_ssl3(SSLVersionRange * vrange, PRBool on) { Let's name these functions ssl_EnableTLS and ssl_EnableSSL3 to follow the NSS naming convention of functions. Regarding that "not reached" assertion in ssl3_NegotiateVersion, we can also just delete it. I believe we need the SSL_LIBRARY_VERSION_MAX_SUPPORTED macro. ssl3_SendClientHello passes it to ssl3_NegotiateVersion.

Here is an updated patch that addresses Wan-Teh's review comments. When updating the patch, I noticed some strange things about the current logic in the NSS trunk: 1. In ssl3_SendClientHello, we search the session cache in search of a session that is compatible with the current socket's settings. However, we don't consider the SSL version (or the SSL policy) when we attempt to select a session to resume. This seemed wrong to me, so I changed it when I updated this patch. 2. In ssl2_HandleV3HandshakeRecord, we call ssl3_NegotiateVersion using the version number encoded in the server's ServerHello record's *header*. From past IETF TLS WG discussions, it seems like we are better off not making decisions based on the record header version of ClientHello and ServerHello messages. So, I changed this logic to use the same logic used in ssl3_SendClientHello. Wan-Teh suggested to add a ssl_VersionIsEnabled function. However, I found that such a function would only be used in one place, so I basically inlined it into that one location. Unfortunately, almost every line of the previous patch changed. Because of that, I took the opportunity to correct the whitespace in the previous patch from leading spaces to leading tabs. If you would like, I can generate an interdiff between this patch and the previous one, ignoring whitespace.

Comment on attachment 603077 [details] [diff] [review] Implement SSL Version Range API [v3] bsmith: I plan to read this patch from scratch, so I don't need you to generate patch interdiffs manually.

The new API is not tested. We should change tstclnt and selfserv to use the new API. I would also like to write more tests (especially testing the SSL_OptionSet behavior) using the loopback framework from bug 702322, but I will need to update the patch in bug 702322 first. I may not have time to get these test changes done before I go on vacation on Saturday.

Does anybody have any opinions on what the command-line interface for tstclnt and selfserv should look like, with respect to enabling/disabling versions? What should the defaults be? (IMO, SSL 2.0 should not be enabled by default any more.)

I agree that SSLv2 shouldn't be the default any more. I propose that it be SSLv3-<highest version supported>. Arguably, however, it should be TLSv1-<highest version supported>, to allow for extensions. This whole -2 -3 -T scheme is starting to break down as we have more versions. Conveniently we have a -V flag free in both programs. In the spirit of the current API, I suggest we indicate the acceptable version range as: -V[min][max]. I see two primary options for how to specify min/max. (a) letters/numbers, in which case we could do: 0 = SSLv2 1 = SSLv3 2 = TLSv1 3 = TLSv11 4 = TLSv12 Or alternately, A,B,C,D,E... So, SSLv3-TLS11 would be -V13 (b) absolute names, in which case we would do: ssl3, tls1, tls11, tls12, and probably separate them with a colon, so SSLv3-TLS11 would be -V ssl3:tls11 My preference here would be for (b), because it's clearer, though less in the spirit of the existing command line syntax.

I think we should do -V min:max that EKR suggested. 'min' and 'max' can be either symbolic names (ssl3, tls1, tls11, tls12) or wire protocol versions (3.0, 3.1, 3.2, 3.3). I prefer wire protocol versions. I would not change the -2 option. I hope we can remove the SSLv2 code soon. EKR: NSS still sends extensions when SSLv3 is enabled, unless SSLv3 is the only version of SSL/TLS enabled.

I updated the patch to convert a C++-style comment to a C-style comment. The patch in bug 571722 allows Firefox to use this API. With the patch that I will soon post to bug 565047, Firefox is able to successfully use this API to speak TLS 1.1 with https://mikestoolbox.org after setting security.tls.version.max to 2. I am now updating the test programs based on the previous comments by EKR and Wan-Teh.

(In reply to Brian Smith (:bsmith) from comment #9) > Created attachment 603077 [details] [diff] [review] > Implement SSL Version Range API [v3] ... > When updating the patch, I noticed some strange things about the current > logic in the NSS trunk: ... > 2. In ssl2_HandleV3HandshakeRecord, we call ssl3_NegotiateVersion using the > version number encoded in the server's ServerHello record's *header*. From > past IETF TLS WG discussions, it seems like we are better off not making > decisions based on the record header version of ClientHello and ServerHello > messages. At that stage, the only decision NSS makes based on ss->version is the if (ss->version >= SSL_LIBRARY_VERSION_3_0) { test in ssl_GatherRecord1stHandshake. So setting ss->version to any version >= 3.0 will do. Is this correct? Your change ensures that we will invoke ssl3_GatherCompleteHandshake, even if the record header version is < 3.0. That seems better. But I think the original code will also detect that condition in the ssl3_NegotiateVersion call and return SECFailure.

Comment on attachment 603895 [details] [diff] [review] Implement SSL Version Range API [v4] I have reviewed this patch. In general it is good. Let me describe the high-level problems first. 1. With the imminent support of DTLS, the version range API needs to handle DTLS versions. The patch must specify SSL_VersionRangeGetDefault, SSL_VersionRangeGetSupported, and SSL_VersionRangeSetDefault mean for the DTLS versions. SSL_VersionRangeGet and SSL_VersionRangeSet operate on a fd, which is either stream or datagram oriented, so it is easier to expect how they deal with DTLS. 2. The side effects of SSL_ENABLE_SSL3 and SSL_ENABLE_TLS when newer versions of TLS are supported are difficult to understand. I struggled to find a better solution, but given the constraints that - we don't want holes in the range of enabled versions, and - we want SSL_OptionSet(SSL_ENABLE_TLS, PR_FALSE) to continue to work for SSLv3 fallback I arrived at the same conclusion. So I propose we improve the comments to explain that we don't want to leave holes in the version range. This makes the semantics more intuitive. 3. Consider adding SSLVersionRange as a field of SSLOptions. Although the name "SSLOptions" implies it is a structure of bit flags, we already broke that property by adding the nextProtoNego SECItem. 4. SSL_VersionRangeGetSupported and SSL_VersionRangeIsValid don't seem useful to me.

(In reply to Wan-Teh Chang from comment #16) > At that stage, the only decision NSS makes based on ss->version is the > if (ss->version >= SSL_LIBRARY_VERSION_3_0) { > test in ssl_GatherRecord1stHandshake. So setting ss->version to any > version >= 3.0 will do. Is this correct? That is my understanding too. I decided to make the logic setting ss->version exactly the same as it is in ssl3_SendClientHello because then it is clearer that we will handle a SSL 3.0+ ServerHello similarly. > Your change ensures that we will invoke ssl3_GatherCompleteHandshake, > even if the record header version is < 3.0. That seems better. But > I think the original code will also detect that condition in the > ssl3_NegotiateVersion call and return SECFailure. I am more concerned about the case where the server might send a version like 0x0303 (TLS 1.2) in the server hello record--i.e. a version number that is higher than what we support. I remember from discussions on the TLS WG mailing list that different products do different things here, so I think it is a good idea in general to avoid making a decision based on the version number in the server hello record header, when we can avoid doing so. (In reply to Wan-Teh Chang from comment #17) > 1. With the imminent support of DTLS, the version range API needs to > handle DTLS versions. The patch must specify SSL_VersionRangeGetDefault, > SSL_VersionRangeGetSupported, and SSL_VersionRangeSetDefault mean for > the DTLS versions. Since this will land before DTLS support lands, we should include this change in the DTLS patch. And, the logic should be that these are "logical" version numbers so that SSL_LIBRARY_VERSION_TLS_1_1 means DTLS 1.0, SSL_LIBRARY_VERSION_TLS_1_2 means DTLS 1.2, etc. DTLS connections will need to implement some logic like this before the handshake: if (ss->opt.isDTLS && ss->vrange.min < SSL_LIBRARY_VERSION_TLS_1_1) { SSLVersionRange range = { SSL_LIBRARY_VERSION_TLS_1_1, ss->vrange.max }; rv = SSL_VersionRangeSet(ss, &range); if (rv != SECSuccess) { return rv; } } > SSL_VersionRangeGet and SSL_VersionRangeSet operate on a fd, which is > either stream or datagram oriented, so it is easier to expect how they > deal with DTLS. I think EKR wants DTLS to be able to work on both streams or datagram connections, so we cannot use that property of a fd for such decisions, AFAICT. But, we can use ss->opt.isDTLS or whatever that option will be. > - we want SSL_OptionSet(SSL_ENABLE_TLS, PR_FALSE) to continue to work for > SSLv3 fallback This is a great point to add to the documentation. > So I propose we improve the comments to explain that we don't want to leave > holes in the version range. This makes the semantics more intuitive. I will add this to the documentation too. > 3. Consider adding SSLVersionRange as a field of SSLOptions. Although > the name "SSLOptions" implies it is a structure of bit flags, we already > broke that property by adding the nextProtoNego SECItem. Then we have to write ss->opt.vrange instead of ss->vrange. I prefer the latter because it is shorter and less likely that we will need to wrap lines. Something we can discuss tomorrow. > 4. SSL_VersionRangeGetSupported and SSL_VersionRangeIsValid don't seem useful > to me. Perhaps SSL_VersionRangeIsValid should not be exported. SSL_VersionRangeGetSupported is useful because that is the only way you could implement "Enable the latest version of TLS that the linked-to version of libssl supports". Some applications may want to do that.

(In reply to Brian Smith (:bsmith) from comment #18) > (In reply to Wan-Teh Chang from comment #17) > > 1. With the imminent support of DTLS, the version range API needs to > > handle DTLS versions. The patch must specify SSL_VersionRangeGetDefault, > > SSL_VersionRangeGetSupported, and SSL_VersionRangeSetDefault mean for > > the DTLS versions. > > Since this will land before DTLS support lands, we should include this > change in the DTLS patch. And, the logic should be that these are "logical" > version numbers so that SSL_LIBRARY_VERSION_TLS_1_1 means DTLS 1.0, > SSL_LIBRARY_VERSION_TLS_1_2 means DTLS 1.2, etc. DTLS connections will need > to implement some logic like this before the handshake: > > if (ss->opt.isDTLS && > ss->vrange.min < SSL_LIBRARY_VERSION_TLS_1_1) { > SSLVersionRange range = { SSL_LIBRARY_VERSION_TLS_1_1, > ss->vrange.max }; > rv = SSL_VersionRangeSet(ss, &range); > if (rv != SECSuccess) { > return rv; > } > } This seems OK with me. It's kind of gross, but better than the alternatives. > > SSL_VersionRangeGet and SSL_VersionRangeSet operate on a fd, which is > > either stream or datagram oriented, so it is easier to expect how they > > deal with DTLS. > > I think EKR wants DTLS to be able to work on both streams or datagram > connections, so we cannot use that property of a fd for such decisions, > AFAICT. But, we can use ss->opt.isDTLS or whatever that option will be. This works for me.

Suppose we add TLS 1.2 but can't add DTLS 1.2 at the same time, what should SSL_VersionRangeGetSupported report? For SSL_VersionRangeGet, which takes an fd argument, we can report the right version range based on the value of ss->opt.isDTLS. But SL_VersionRangeGetSupported has to return a version range that applies to both TLS and DTLS sockets. So the current version range API will require we add a new TLS version and its DTLS variant in lock step. By the way, EKR's DTLS code takes advantage of UDP's all-or-nothing read behavior to simplify parsing, so this particular implementation won't work over a byte stream.

(In reply to Wan-Teh Chang from comment #20) > Suppose we add TLS 1.2 but can't add DTLS 1.2 at the same time, > what should SSL_VersionRangeGetSupported report? > > For SSL_VersionRangeGet, which takes an fd argument, we can > report the right version range based on the value of ss->opt.isDTLS. > But SL_VersionRangeGetSupported has to return a version range > that applies to both TLS and DTLS sockets. So the current version > range API will require we add a new TLS version and its DTLS variant > in lock step. Currently, this isn't intended to be a big deal, since DTLS is supposed to be a relatively fixed delta. But I see the concern. What about providing a flag to SSL_VersionRangeGetSupported() to indicate whether we are asking for datagram or stream modes. > By the way, EKR's DTLS code takes advantage of UDP's all-or-nothing > read behavior to simplify parsing, so this particular implementation > won't work over a byte stream. Correct. We would need to fix this at some point, but I'd prefer not to bake it in permanently to the api.

(In reply to Wan-Teh Chang from comment #17) > 1. With the imminent support of DTLS, the version range API needs to > handle DTLS versions. The patch must specify SSL_VersionRangeGetDefault, > SSL_VersionRangeGetSupported, and SSL_VersionRangeSetDefault mean for > the DTLS versions. This is addressed in the latest patch. The API now requires the caller to pass the value SSL_Stream to the various functions to indicate that the caller is interested in changing the versions supported for stream DTLS. I expect that the DTLS patch will add support for a SSL_Datagram value. One open question for the DTLS patch is whether SSL_VersionRangeSet(SSL_Datagram, range) should also switch the socket from stream to datagram mode, instead of having a separate SSL_OptionSet() flag for setting datagram mode. It seems like a good idea to do that to me, but then SSL_VersionRangeSetDefault would be inconsistent if it were used to set the default versions support for stream and datagram mode independently, without changing whether the default variant is stream or datagram. One possible solution would be to remove SSL_VersionRangeGetDefault and SSL_VersionRangeSetDefault from the patch, so that the version range has to be set on each individual socket. Although the patch I wrote for Gecko uses SSL_VersionRangeSetDefault, I would be happy to drop it. > So I propose we improve the comments to explain that we don't want to leave > holes in the version range. This makes the semantics more intuitive. I hope the new documentation is clear enough now. > 3. Consider adding SSLVersionRange as a field of SSLOptions. Although > the name "SSLOptions" implies it is a structure of bit flags, we already > broke that property by adding the nextProtoNego SECItem. We decided not to do this in our face-to-face meeting. > 4. SSL_VersionRangeGetSupported and SSL_VersionRangeIsValid don't seem useful > to me. As mentioned above, SSL_VersionRangeGetSupported is needed in order to support applications that want to always enable the latest version of TLS that the linked-to libssl supports. I have removed VersionRangeIsValid from the API. > } else if (vrange->min == SSL_LIBRARY_VERSION_3_0) { > if (vrange->max > SSL_LIBRARY_VERSION_3_0) { > /* If the mininimum is SSL 3.0 and the maximum is not SSL 3.0, then > * TLS 1.0 must already be enabled, so TLS 1.0 will become the new > * minimum. > */ > vrange->min = SSL_LIBRARY_VERSION_TLS_1_0; > } else { > /* Only SSL 3.0 was enabled, so now no versions are. */ > vrange->min = SSL_LIBRARY_VERSION_NONE; > vrange->max = SSL_LIBRARY_VERSION_NONE; > } > } Previously, we discussed changing the vrange->min = SSL_LIBRARY_VERSION_TLS_1_0; to vrange->min = PR_MAX(vrange->min, SSL_LIBRARY_VERSION_TLS_1_0); and changing the comment. However, I found that hard to understand. Instead I added the else if (vrange->min == SSL_LIBRARY_VERSION_3_0) { condition so that the existing code makes sense.

With this patch, the tests for the two export cipher suites we support will fail. This needs to be corrected before the patch could be checked in. The patch doesn't implement a mechanism for disabling TLS 1.1 while leaving TLS 1.0 enabled. I just realized that we did not handle the case where the application uses SSL_VersionRangeGet/SSL_VersionRangeGetDefault and no versions are enabled. The patch needs to be updated to move SSL_LIBRARY_VERSION_NONE to a public header and to update the documentation in ssl.h to indicate that SSL_LIBRARY_VERSION_NONE is put into vrange->min and vrange->max when no versions are enabled.

Brian: I didn't know you're working on this today, so I cleaned up your v4 patch and checked it in. I only export SSL_VersionRangeGet and SSL_VersionRangeSet in ssl.def. I will merge with your v5 patch and check it in tomorrow or next week. Have a good vacation! Checking in SSLerrs.h; /cvsroot/mozilla/security/nss/lib/ssl/SSLerrs.h,v <-- SSLerrs.h new revision: 1.19; previous revision: 1.18 done Checking in ssl.def; /cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v <-- ssl.def new revision: 1.31; previous revision: 1.30 done Checking in ssl.h; /cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v <-- ssl.h new revision: 1.52; previous revision: 1.51 done Checking in ssl3con.c; /cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v <-- ssl3con.c new revision: 1.169; previous revision: 1.168 done Checking in sslcon.c; /cvsroot/mozilla/security/nss/lib/ssl/sslcon.c,v <-- sslcon.c new revision: 1.47; previous revision: 1.46 done Checking in sslerr.h; /cvsroot/mozilla/security/nss/lib/ssl/sslerr.h,v <-- sslerr.h new revision: 1.20; previous revision: 1.19 done Checking in sslgathr.c; /cvsroot/mozilla/security/nss/lib/ssl/sslgathr.c,v <-- sslgathr.c new revision: 1.13; previous revision: 1.12 done Checking in sslimpl.h; /cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v <-- sslimpl.h new revision: 1.96; previous revision: 1.95 done Checking in sslproto.h; /cvsroot/mozilla/security/nss/lib/ssl/sslproto.h,v <-- sslproto.h new revision: 1.16; previous revision: 1.15 done Checking in sslsock.c; /cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v <-- sslsock.c new revision: 1.83; previous revision: 1.82 done Checking in sslt.h; /cvsroot/mozilla/security/nss/lib/ssl/sslt.h,v <-- sslt.h new revision: 1.19; previous revision: 1.18 done

OK. We will have to sort out the differences. FWIW, v6 of my patch makes the changes mentioned in comment 24.

Wan-Teh and I met on Wednesday to try to resolve the TLS/DTLS issue. Our proposal is that we retain the variant structure proposed by Brian but not have an explicit argument attached to the versions that take an fd. Instead, we're going to tag each fd permanently as stream or datagram, with the default being stream. That way the fd variants will interrogate the fd for its type. The minimal code to implement this with only a single fixed type is attached as a patch. I'll add the code to set the fd to datagram (proposal is to have a new DTLS_import_fd() which does it, rather than an option because it avoids conditions where you change type once you've set other options) and to consult the fd in the mega DTLS patch.

Eric: thank you for creating this patch. I made the folllowing changes: 1. I changed the enum constant SSL_Stream to ssl_variant_stream. I added "variant" because "stream" alone could mean a stream cipher. I found two naming conventions are most commonly used for public enum types in libssl: ssl_foo_bar SSL_FOO_BAR (only used for error codes and SSLNextProtoState) So I go with the first naming convention. I also moved the enum definition to sslt.h to be next to the SSLVersionRange structure definition. Patch checked in on the NSS trunk (NSS 3.13.4). Checking in ssl.h; /cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v <-- ssl.h new revision: 1.53; previous revision: 1.52 done Checking in ssl3con.c; /cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v <-- ssl3con.c new revision: 1.172; previous revision: 1.171 done Checking in sslimpl.h; /cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v <-- sslimpl.h new revision: 1.99; previous revision: 1.98 done Checking in sslsock.c; /cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v <-- sslsock.c new revision: 1.85; previous revision: 1.84 done Checking in sslt.h; /cvsroot/mozilla/security/nss/lib/ssl/sslt.h,v <-- sslt.h new revision: 1.20; previous revision: 1.19 done

I propose the following change to SSL_VersionRangeSetDefault and SSL_VersionRangeSet. Right now they do if (!ssl3_VersionRangeIsValid(protocolVariant, vrange)) { PORT_SetError(SSL_ERROR_INVALID_VERSION_RANGE); return SECFailure; } ssl3_VersionRangeIsValid returns PR_FALSE if the version range is not fully supported. I propose the SSL_VersionRangeSet* functions should clip the requested version range to the supported range, and only return SECFailure if the clipped version range is empty. There are two advantages to this approach. 1. An application can enable all supported versions by requesting { 0xFFFF, 0x0300 }. No need to call SSL_VersionRangeGetSupported first. 2. If we ever remove SSL 3.0 support from NSS, an old application that enables { 0x0301, 0x0300 } will continue to work in degraded mode. It will only fail when we also remove TLS 1.0 support from NSS.

This works for me. In reply to Wan-Teh Chang from comment #29) > Created attachment 606418 [details] [diff] [review] > Updates to v4 patch (checked in) > > Eric: thank you for creating this patch. I made the folllowing > changes: > > 1. I changed the enum constant SSL_Stream to ssl_variant_stream. > I added "variant" because "stream" alone could mean a stream cipher. > I found two naming conventions are most commonly used for public > enum types in libssl: > > ssl_foo_bar > SSL_FOO_BAR (only used for error codes and SSLNextProtoState) > > So I go with the first naming convention. > > I also moved the enum definition to sslt.h to be next to the > SSLVersionRange structure definition. > > Patch checked in on the NSS trunk (NSS 3.13.4). > > Checking in ssl.h; > /cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v <-- ssl.h > new revision: 1.53; previous revision: 1.52 > done > Checking in ssl3con.c; > /cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v <-- ssl3con.c > new revision: 1.172; previous revision: 1.171 > done > Checking in sslimpl.h; > /cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v <-- sslimpl.h > new revision: 1.99; previous revision: 1.98 > done > Checking in sslsock.c; > /cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v <-- sslsock.c > new revision: 1.85; previous revision: 1.84 > done > Checking in sslt.h; > /cvsroot/mozilla/security/nss/lib/ssl/sslt.h,v <-- sslt.h > new revision: 1.20; previous revision: 1.19 > done

(In reply to Wan-Teh Chang from comment #30) > 1. An application can enable all supported versions by requesting > { 0xFFFF, 0x0300 }. No need to call SSL_VersionRangeGetSupported > first. > 2. If we ever remove SSL 3.0 support from NSS, an old application > that enables { 0x0301, 0x0300 } will continue to work in degraded > mode. It will only fail when we also remove TLS 1.0 support from > NSS. I would rather keep things as they are. I don't think we should reduce the max component of the requested range, because this may be materially reducing the security of an application in a way that the application does not expect. For example, if an application requests TLS 1.0 through TLS 1.2 then it would be bad to enable only TLS 1.0 and TLS 1.1, because it may be important for the application to be able to use TLS 1.2 with peers that support it. The good thing about the current approach is that there are no surprises. I think this is a very important property. I think that a real application should do something like this: 1. SSL_VersionRangeGetSupported(&vrange); 2. vrange.max = PR_MAX(vrange.max, <app's min required version>); 3. vrange.min = PR_MAX(vrange.min, <app's min required version>); 4. SSL_VersionRangeSet(&vrange); whereas, tstclnt, selfserv, etc. can get away with just: 1. SSL_VersionRangeGetSupported(&vrange); 2. SSL_VersionRangeSet(&vrange); because they are just test programs.

Comment on attachment 604723 [details] [diff] [review] Implement SSL Version Range API [v6] Brian: this patch is the same as "Implement SSL Version Range API [v5]" (attachment 604707 [details] [diff] [review]), so I suspect you attached the wrong patch file.

I resolved the differences between Brian's "Implement SSL Version Range API [v5]" patch (attachment 604707 [details] [diff] [review]) and my [v4+] patch, and checked them in on the NSS trunk (NSS 3.13.4). Checking in ssl.def; /cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v <-- ssl.def new revision: 1.32; previous revision: 1.31 done Checking in ssl.h; /cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v <-- ssl.h new revision: 1.54; previous revision: 1.53 done Checking in ssl3con.c; /cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v <-- ssl3con.c new revision: 1.173; previous revision: 1.172 done Checking in sslcon.c; /cvsroot/mozilla/security/nss/lib/ssl/sslcon.c,v <-- sslcon.c new revision: 1.48; previous revision: 1.47 done Checking in sslimpl.h; /cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v <-- sslimpl.h new revision: 1.100; previous revision: 1.99 done Checking in sslsock.c; /cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v <-- sslsock.c new revision: 1.86; previous revision: 1.85 done

(In reply to Brian Smith (:bsmith) from comment #32) > > I think that a real application should do something like this: > > 1. SSL_VersionRangeGetSupported(&vrange); > 2. vrange.max = PR_MAX(vrange.max, <app's min required version>); > 3. vrange.min = PR_MAX(vrange.min, <app's min required version>); > 4. SSL_VersionRangeSet(&vrange); That's why I propose steps 1-3 should be done by SSL_VersionRangeSet. (Step 2 has a typo, but that's not important to this dicussion.) Your example in comment 32 shows automatic clipping of vrange.max is a bad idea. Thank you for pointing that out. I will remove the automatic clipping of vrange.max from my proposal. Automatic clipping of vrange.min is important to backward compatibility (see "Note"), but we don't need to do that until we drop support for SSL 3.0. Note: we have an equivalent question for the old SSL_OptionSet API: when we drop support for SSL 2.0, should SSL_OptionSet(fd, SSL_ENABLE_SSL2, PR_TRUE) return SECSuccess (a lie) or SECFailure? For maximum backward compatibility, we had to make SSL_CipherPrefSet lie when we removed the Fortezza cipher suites. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsock.c&rev=1.85&mark=1244,1253-1254#1243 So I believe we'll need to do the same when we drop support for obsolete versions of SSL.

Is somebody currently working on this bug? This is P1 and nothing happened since 5 months.

(In reply to Wan-Teh Chang from comment #35) > That's why I propose steps 1-3 should be done by SSL_VersionRangeSet. > (Step 2 has a typo, but that's not important to this dicussion.) > > Your example in comment 32 shows automatic clipping of vrange.max is > a bad idea. Thank you for pointing that out. I will remove the > automatic clipping of vrange.max from my proposal. Automatic clipping > of vrange.min is important to backward compatibility (see "Note"), but > we don't need to do that until we drop support for SSL 3.0. > > Note: we have an equivalent question for the old SSL_OptionSet API: > when we drop support for SSL 2.0, should > SSL_OptionSet(fd, SSL_ENABLE_SSL2, PR_TRUE) > return SECSuccess (a lie) or SECFailure? Sure, I think it is good to have it lie, for the reason you point it out. So, I guess the remaining work here is to have SSL_VersionRangeSetDefault and SSL_VersionRangeSet clip the minimum (but not maximum) part of the range. Kai has already filed other bugs for modifying the test programs.

(In reply to Brian Smith (:bsmith) from comment #37) > I guess the remaining work here is to have SSL_VersionRangeSetDefault > and SSL_VersionRangeSet clip the minimum (but not maximum) part of the > range. Kai has already filed other bugs for modifying the test programs. Thinking about this more, I am not sure that we need to change the implementation of these functions now. If/when we remove SSL 3.0 support, we can make the version range API lie in the same way as we would when we remove SSL 2.0 support. This would require making a change similar to the one that Wan-Teh suggested in comment 30, but since we'd need to audit all of libssl at the point we'd remove SSL 3.0 support anyway, I don't think we're saving any effort by making that change pre-emptively. Perhaps it would be useful to change the *documentation* of these functions, though, to say that in the future, they may will silently clamp the minimum part of the range as we remove support for older versions of SSL/TLS.

Comment on attachment 604714 [details] [diff] [review] Update selfserv and tstclnt to enable all versions of SSL supported by libssl by default Marked the patch obsolete. We will modify selfserv, strsclnt, and tstclnt in bug 785169.

Marked the bug FIXED.