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

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: keeler, Assigned: emk)

Tracking

unspecified
mozilla37
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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

Comment 2

5 years ago
- 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.
Attachment #8530497 - Flags: review?(dkeeler)
(Assignee)

Comment 3

5 years ago
> (INT32_MAX - 3).
Correction: (INT32_MAX - SSL_LIBRARY_VERSION_3_0)
(Assignee)

Comment 4

5 years ago
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.
Attachment #8530497 - Attachment is obsolete: true
Attachment #8530497 - Flags: review?(dkeeler)
(Assignee)

Comment 5

5 years ago
Posted patch non-working patch (obsolete) — Splinter Review
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
(Assignee)

Comment 6

5 years ago
Fixed the link failure.
Attachment #8530509 - Attachment is obsolete: true
Attachment #8530519 - Flags: review?(dkeeler)
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
(Assignee)

Comment 8

5 years ago
SSL_VersionRangeSetDefault() will validate the version range, so we don't have to do it on our own.
Attachment #8530626 - Flags: review?(dkeeler)
(Assignee)

Updated

5 years ago
Attachment #8530519 - Attachment is obsolete: true
Attachment #8530519 - Flags: review?(dkeeler)
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;
Attachment #8530626 - Flags: review?(dkeeler) → review-
(Assignee)

Comment 11

4 years ago
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.
Attachment #8530626 - Attachment is obsolete: true
Attachment #8530982 - Flags: review?(dkeeler)
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.").
Attachment #8530982 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 13

4 years ago
(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
Assignee: nobody → VYV03354
Attachment #8530982 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8533686 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6f6153ab2308
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.