1093724 - update the TLS version fallback limit pref loading code to only accept values in the range [0,3]

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(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.

Comment 1

[Martin Thomson [:mt:]]

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.

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Add a range check to the TLS version fallback limit pref loading code

- 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.

Comment 3

[Masatoshi Kimura [:emk]]

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

Comment 4

[Masatoshi Kimura [:emk]]

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.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: non-working patch

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

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Add a range check to the TLS version fallback limit pref loading code, v2

Fixed the link failure.

Comment 7

[Martin Thomson [:mt:]]

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

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Add a range check to the TLS version fallback limit pref loading code, v3

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

Comment 9

[Masatoshi Kimura [:emk]]

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

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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;

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Add a range check to the TLS version prefs loading code

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 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.").

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: patch for checkin

(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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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