(In reply to David Keeler (:keeler) [use needinfo?] from bug 1076983 comment #30) > ::: security/manager/ssl/src/nsNSSIOLayer.cpp > @@ +1626,5 @@ > > + int32_t limit = 1; // 1 = TLS 1.0 > > + Preferences::GetInt("security.tls.version.fallback-limit", &limit); > > + limit += SSL_LIBRARY_VERSION_3_0; > > + mVersionFallbackLimit = (uint16_t)limit; > > + if (limit != (int32_t)mVersionFallbackLimit) { // overflow check > > I think we should just map { 0, 1, 2, 3 } to { SSL 3.0, TLS 1.0, TLS 1.1, > TLS 1.2 } and use the default (1) if the pref value is outside that range.

Let's make sure that any element we add is in the one place: we're adding TLS 1.3 soon and I don't want to miss it.

- The old code had an undefined behavior if the pref value was larger than (INT32_MAX - 3). - SSL_LIBRARY_VERSION_MAX_SUPPORTED should be updated once NSS supported TLS 1.3.

> (INT32_MAX - 3). Correction: (INT32_MAX - SSL_LIBRARY_VERSION_3_0)

Comment on attachment 8530497 [details] [diff] [review] Add a range check to the TLS version fallback limit pref loading code Oops, SSL_LIBRARY_VERSION_MAX_SUPPORTED was a private macro.

Link fails with LNK2019... 1:16.79 Unified_cpp_manager_ssl_src1.obj : error LNK2019: unresolved external symbol __imp__SSL_VersionRangeGetSupported referenced in function "public: void __thiscall nsSSLIOLayerHelpers::loadVersionFallbackLimit(void)"(?loadVersionFallbackLimit@nsSSLIOLayerHelpers@@QAEXXZ) 1:16.79 1:16.80 xul.dll : fatal error LNK1120: 1 unresolved externals 1:16.80

Fixed the link failure.

Comment on attachment 8530519 [details] [diff] [review] Add a range check to the TLS version fallback limit pref loading code, v2 Review of attachment 8530519 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +1848,3 @@ > limit += SSL_LIBRARY_VERSION_3_0; > + SSLVersionRange range; > + if (SSL_VersionRangeGetSupported(ssl_variant_stream, &range) != SECSuccess || This is good, but you should probably factor this out as well: http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp#876

SSL_VersionRangeSetDefault() will validate the version range, so we don't have to do it on our own.

Try runs: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5eef27007b5e https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bd567241a2ee

Comment on attachment 8530626 [details] [diff] [review] Add a range check to the TLS version fallback limit pref loading code, v3 Review of attachment 8530626 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for having a look at this. However, I was thinking of something more like this: SECStatus FillTLSVersionRange(SSLVersionRange& rangeOut, uint32_t minFromPrefs, uint32_t maxFromPrefs, SSLVersionRange defaults) { // determine what versions are supported (use SSL_VersionRangeGetSupported) // convert min/maxFromPrefs to the internal representation (i.e. add SSL_LIBRARY_VERSION_3_0) // if min/maxFromPrefs are outside of the supported range (or if maxFromPrefs < minFromPrefs), set them to the defaults // fill out rangeOut } Then in nsNSSComponent::setEnabledTLSVersions: minFromPrefs = Preferences::GetUint(...); maxFromPrefs = Preferences::GetUint(...); SSLVersionRange defaults = { ... }; SSLVersionRange filledInRange; SECStatus srv = FillTLSVersionRange(filledInRange, minFromPrefs, maxFromPrefs, defaults); if (srv != SECSuccess) { return NS_ERROR_FAILURE; } srv = SSL_VersionRangeSetDefault(ssl_variant_stream, &filledInRange); if (srv != SECSuccess) { return NS_ERROR_FAILURE; } Similarly, in nsSSLIOLayerHelpers::loadVersionFallbackLimit: dummyMin = 0; limit = Preferences::GetUint(...); SSLVersionRange defaults = { ... }; SSLVersionRange filledInRange; SECStatus srv = FillTLSVersionRange(filledInRange, dummyMin, limit, defaults); if (srv != SECSuccess) { return NS_ERROR_FAILURE; } mVersionFallbackLimit = filledInRange.max;

Basically implementing commen #10 except: 1. Making the return value of FillTLSVersionRange void because this function will never fail. 2. Using minFromPrefs = maxFromPrefs = limit instead of dummyMin so that bug 1106470 will not break loadVersionFallbackLimit.

Comment on attachment 8530982 [details] [diff] [review] Add a range check to the TLS version prefs loading code Review of attachment 8530982 [details] [diff] [review]: ----------------------------------------------------------------- Great - r=me with comments addressed. ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +718,5 @@ > } > } > > +/*static*/ void > +nsNSSComponent::FillTLSVersionRange(SSLVersionRange& rangeOut, We should document that this performs the conversion from pref values like 0, 1, ... to the internal values of SSL_LIBRARY_VERSION_3_0, SSL_LIBRARY_VERSION_TLS_1_0, ... @@ +737,5 @@ > + maxFromPrefs += SSL_LIBRARY_VERSION_3_0; > + // if min/maxFromPrefs are invalid, use defaults > + if (minFromPrefs > maxFromPrefs || > + minFromPrefs < range.min || maxFromPrefs > range.max) { > + rangeOut = defaults; If you wanted, you could just say 'rangeOut = defaults;' at the top of this function and only have 'return;' here and in the other check. Not a big deal, though. @@ +916,4 @@ > > + SSLVersionRange defaults = { > + SSL_LIBRARY_VERSION_3_0 + PSM_DEFAULT_MIN_TLS_VERSION, > + SSL_LIBRARY_VERSION_3_0 + PSM_DEFAULT_MAX_TLS_VERSION It would be nice to not have the + SSL_LIBRARY_VERSION_3_0 conversion here as well - let's just use SSL_LIBRARY_VERSION_TLS_1_0 and SSL_LIBRARY_VERSION_TLS_1_2, respectively. That way we can get rid of PSM_DEFAULT_MIN/MAX_TLS_VERSION (although it would be good to keep the note about "0 means SSL 3.0, 1 means TLS 1.0, 2 means TLS 1.1, etc.").

(In reply to David Keeler (:keeler) [use needinfo?] from comment #12) > ::: security/manager/ssl/src/nsNSSComponent.cpp > @@ +718,5 @@ > It would be nice to not have the + SSL_LIBRARY_VERSION_3_0 conversion here > as well - let's just use SSL_LIBRARY_VERSION_TLS_1_0 and > SSL_LIBRARY_VERSION_TLS_1_2, respectively. That way we can get rid of > PSM_DEFAULT_MIN/MAX_TLS_VERSION (although it would be good to keep the note > about "0 means SSL 3.0, 1 means TLS 1.0, 2 means TLS 1.1, etc."). Unfortunately Preferences::GetUint() still requires PSM_DEFAULT_MIN/MAX_TLS_VERSION for default values. I would not like to keep sync with one more places every time the default version range is changed. Resolved other review comments. https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6153ab2308