Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nss: use TLSv1.3 as default if supported #4187

Closed
wants to merge 1 commit into from

Conversation

@Lekensteyn
Copy link
Member

commented Aug 3, 2019

SSL_VersionRangeGetDefault returns (TLSv1.0, TLSv1.2) as supported
range in NSS 3.45. It looks like the intention is to raise the minimum
version rather than lowering the maximum, so adjust accordingly. Note
that the caller (nss_setup_connect) initializes the version range to
(TLSv1.0, TLSv1.3), so there is no need to check for >= TLSv1.0 again.


Fixes version negotiation when no --tlsv1.3 option is provided.

before$ src/curl https://wireshark.org -vI   
*   Trying 104.25.218.21:443...
...
* SSL connection using TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256

after$ src/curl https://wireshark.org -vI 
*   Trying 104.25.219.21:443...
...
* SSL connection using TLS_AES_128_GCM_SHA256

This fixes an issue with PR #3262, @kdudka rightfully observed that the proposed change is no-op (the maximum version in sslver was ignored).

nss: use TLSv1.3 as default if supported
SSL_VersionRangeGetDefault returns (TLSv1.0, TLSv1.2) as supported
range in NSS 3.45. It looks like the intention is to raise the minimum
version rather than lowering the maximum, so adjust accordingly. Note
that the caller (nss_setup_connect) initializes the version range to
(TLSv1.0, TLSv1.3), so there is no need to check for >= TLSv1.0 again.

@Lekensteyn Lekensteyn requested review from bagder and kdudka Aug 3, 2019

@Lekensteyn Lekensteyn added the SSL/TLS label Aug 3, 2019

@bagder
bagder approved these changes Aug 3, 2019
Copy link
Member

left a comment

I'm not an expert here but it seems fine!

@kdudka

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

Thanks for the patch! However I do not agree that SSL_VersionRangeGetDefault() returns something like minimum version. The function instead returns the system default version range for the protocol.

The idea behind the current implementation is that libcurl by default respects system default. If the default needs to be changed, the administrator can change the default system-wide. If a specific application needs a different version range, it can override the default. But it hardly makes sense to use a different default for all libcurl-based applications than for all other applications on the same system.

We used to hard-wire the TLS range in libcurl and it only caused problems because we had to update multiple components of an enterprise Linux distribution each time we needed to change the default. Hard-wiring some default range in libcurl now again sounds to me like a step backwards.

And I do not think that PR #3262 is anyhow related to this request. The PR was about ensuring that maximum TLS version is never below minimum TLS version, which is a natural API requirement.

@jay

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

I'm not sure I understand this. Don't they both set the minimum version to at least TLSv1.0? So what is the difference here except this change does not set maximum at runtime instead relying on curl's detected compile-time maximum? I'm with @kdudka if a default runtime maximum is available let's continue to use that. I can't find it documented either way for maximum so I hesitate to say there's a right way.

@kdudka

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

To be clear, we make sure that minimum accepted version is at least TLS 1.0 because it is the documented libcurl API since 7.39.0. However, the system policy can be stricter such that a higher TLS version is required by default.

@Lekensteyn

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2019

However I do not agree that SSL_VersionRangeGetDefault() returns something like minimum version. The function instead returns the system default version range for the protocol.

That is true, but the default maximum version is currently constrained to TLS 1.2:
https://github.com/nss-dev/nss/blob/NSS_3_45_RTM/lib/ssl/sslsock.c#L95-L109

The system policy can only tighten the lower and upper bounds of the defaults (TLSv1.0 - TLSv1.2).

We used to hard-wire the TLS range in libcurl and it only caused problems because we had to update multiple components of an enterprise Linux distribution each time we needed to change the default.

Is this about changing the minimum version (SSLv3 -> TLSv1.0) or the maximum as well? Why would you like to reduce the maximum version (e.g. TLSv1.3 -> TLSv1.2)?

The only reason I can think of is that you have some incompatibility with middleboxes that do TLS interception, but that does not seem a valid reason to disable TLS 1.3 by default.

If the current hard-coded maximum version default in NSS is a bug, then I would suggest pushing the current patch, and maybe add another version that allows the maximum version to be reduced if a fixed NSS version is detected.

Hard-wiring some default range in libcurl now again sounds to me like a step backwards. And I do not think that PR #3262 is anyhow related to this request. The PR was about ensuring that maximum TLS version is never below minimum TLS version, which is a natural API requirement.

PR #3262 increased the maximum possible version here in 0c44809, but that has no effect since it is overwritten in nss_init_sslver by the weaker hard-coded NSS default.

@kdudka

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2019

The system policy can only tighten the lower and upper bounds of the defaults (TLSv1.0 - TLSv1.2).

I do not think the above statement is correct. Could you please provide any reference?

Is this about changing the minimum version (SSLv3 -> TLSv1.0) or the maximum as well? Why would you like to reduce the maximum version (e.g. TLSv1.3 -> TLSv1.2)?

The only reason I can think of is that you have some incompatibility with middleboxes that do TLS interception, but that does not seem a valid reason to disable TLS 1.3 by default.

These are valid discussion points for a request at Mozilla to change the default in NSS.

If the current hard-coded maximum version default in NSS is a bug,

It is not. Otherwise you would tell us Mozilla bug ID to begin with?

@Lekensteyn

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2019

The system policy can only tighten the lower and upper bounds of the defaults (TLSv1.0 - TLSv1.2).

I do not think the above statement is correct. Could you please provide any reference?

I linked the relevant source code above. Specifically, this is my train of thought:

SECStatus
SSL_VersionRangeGetDefault(SSLProtocolVariant protocolVariant,
                           SSLVersionRange *vrange)
{
    // ...
    // VERSION_DEFAULTS point to TLS 1.0 - TLS 1.2
    *vrange = *VERSIONS_DEFAULTS(protocolVariant);
    return ssl3_CreateOverlapWithPolicy(protocolVariant, vrange, vrange);
}

VERSION_DEFAULTS point to TLS 1.0 - TLS 1.2 as can be seen here:

static SSLVersionRange versions_defaults_stream = {
    SSL_LIBRARY_VERSION_TLS_1_0,
    SSL_LIBRARY_VERSION_TLS_1_2
};

static SSLVersionRange versions_defaults_datagram = {
    SSL_LIBRARY_VERSION_TLS_1_1,
    SSL_LIBRARY_VERSION_TLS_1_2
};

#define VERSIONS_DEFAULTS(variant) \
    (variant == ssl_variant_stream ? &versions_defaults_stream : &versions_defaults_datagram)

Heading over to ssl3_CreateOverlapWithPolicy, note that the "input" and "overlap" parameters are both the range (TLS 1.0, TLS 1.2). There is nothing here that suggests that it can be extended to include TLS 1.3.

/*
 * Assumes that rangeParam values are within the supported boundaries,
 * but should contain all potentially allowed versions, even if they contain
 * conflicting versions.
 * Will return the overlap, or a NONE range if system policy is invalid.
 */
static SECStatus
ssl3_CreateOverlapWithPolicy(SSLProtocolVariant protocolVariant,
                             SSLVersionRange *input,
                             SSLVersionRange *overlap)
{
    // assume it retrieves the system policy which is unconstrained by default based on
    // lib/nss/nssoptions.c setting tlsVersionMinPolicy=1, tlsVersionMaxPolicy=0xffff
    rv = ssl3_GetEffectiveVersionPolicy(protocolVariant,
                                        &effectivePolicyBoundary);
    // ...
    // Here we can see that the policy can only tighten the bounds as documented.
    vrange.min = PR_MAX(input->min, effectivePolicyBoundary.min);
    vrange.max = PR_MIN(input->max, effectivePolicyBoundary.max);

If the current hard-coded maximum version default in NSS is a bug,

It is not. Otherwise you would tell us Mozilla bug ID to begin with?

There is no bug ID because nobody seems to have run into it before. It is possible that curl is using the wrong APIs. Why would the current behavior of disabling the more secure TLS 1.3 protocol version be considered desirable behavior?

A major user of NSS is Firefox. Let's see how they use the affected symbol via https://searchfox.org/mozilla-central/search?q=SSL_VersionRangeGetDefault
That query points to only one user, and that user is not using the returned value to restrict the negotiated TLS version:
https://searchfox.org/mozilla-central/rev/7a5022a3/security/manager/ssl/nsNSSCallbacks.cpp#1280
I guess that the "the range of SSL3/TLS versions enabled by default" description in the documentation for SSL_VersionRangeGetDefault refers to the NSS library default which is apparently not the most secure option. As users such as Firefox already enable TLS 1.3 by default, I suppose it is safe to allow any application to use this as well?

Based on this insight, I think that perhaps this SSL_VersionRangeGetDefault call should be dropped from cURL as well? The SSL_VersionRangeGetSupported function already appears to apply the system policy via ssl3_CreateOverlapWithPolicy.

@kdudka

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2019

@Lekensteyn You are right. NSS does not support changing of the default TLS version range with crypto policies yet. I have reported a bug at Mozilla to change the default TLS version range in NSS:

https://bugzilla.mozilla.org/1573118

But it may take some time before the change reaches a stable NSS release, so I am fine with applying your patch as a mid-term solution.

@kdudka
kdudka approved these changes Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.