Patchwork [3,of,3,RFC] sslutil: config option to specify TLS protocol version

# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1467873039 25200
#      Wed Jul 06 23:30:39 2016 -0700
# Node ID 961c91677f9b2394b3b8140c28059d5fc5eee00f
# Parent  66a5adf42ac7bc0031e772b407403e809e67fb6e
[RFC] sslutil: config option to specify TLS protocol version

Currently, Mercurial will use TLS 1.0 or newer when connecting to
remote servers, selecting the highest TLS version supported by both
peers. On older Pythons, only TLS 1.0 is available. On newer Pythons,
TLS 1.1 and 1.2 should be available.

Security professionals recommend avoiding TLS 1.0 if possible.
PCI DSS 3.1 "strongly encourages" the use of TLS 1.2.

Known attacks like BEAST and POODLE exist against TLS 1.0 (although
mitigations are available and properly configured servers aren't
vulnerable).

Web browsers and many other applications continue to support TLS
1.0 for compatibility reasons.

Security-minded people may want to not take any risks running
TLS 1.0 (or even TLS 1.1). This patch gives those people a config
option to explicitly control which TLS version Mercurial should use.
By providing this option, people can require newer TLS versions
before they are formally deprecated by Mercurial/Python/OpenSSL/etc
and lower their security exposure. This option also provides an
easy mechanism to change protocol policies in Mercurial. If there
is a 0-day and TLS 1.0 is completely broken, we can act quickly
without changing much code.

Because setting the minimum TLS protocol is something you'll likely
want to do globally, this patch introduces the special
"hostsecurity._default_" config option to hold settings that apply
to all hosts. Only the "protocol" sub-option currently works.

Open Issues:

* Need to write tests
* Is "hostsecurity._default_" really the best way to do that?
* Do we want to support both the explicit and "+" values. I could be
  convinced that only the e.g. "tls1.1+" values are needed. Why would
  you only choose TLS 1.1 when TLS 1.2 is available? That feels like
  a footgun that could accidentally lock you into old security when
  TLS 1.3 is available.
* Support "hostsecurity._default_:verifycertsfile"? (would effectively
  be web.cacerts for clients)
* Should we issue a warning if TLS 1.0 is used? What if TLS 1.0 is all
  that is available?
* Should we stop using TLS 1.0 if TLS 1.1+ is available by default?
* Should we also provide a config option that sets value for
  SSLContext.set_ciphers()?

On Wed, Jul 06, 2016 at 11:31:37PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1467873039 25200
> #      Wed Jul 06 23:30:39 2016 -0700
> # Node ID 961c91677f9b2394b3b8140c28059d5fc5eee00f
> # Parent  66a5adf42ac7bc0031e772b407403e809e67fb6e
> [RFC] sslutil: config option to specify TLS protocol version

queued patches 1 and 2, thanks. Comments inline below for some questions.

>
> Currently, Mercurial will use TLS 1.0 or newer when connecting to
> remote servers, selecting the highest TLS version supported by both
> peers. On older Pythons, only TLS 1.0 is available. On newer Pythons,
> TLS 1.1 and 1.2 should be available.
>
> Security professionals recommend avoiding TLS 1.0 if possible.
> PCI DSS 3.1 "strongly encourages" the use of TLS 1.2.
>
> Known attacks like BEAST and POODLE exist against TLS 1.0 (although
> mitigations are available and properly configured servers aren't
> vulnerable).
>
> Web browsers and many other applications continue to support TLS
> 1.0 for compatibility reasons.
>
> Security-minded people may want to not take any risks running
> TLS 1.0 (or even TLS 1.1). This patch gives those people a config
> option to explicitly control which TLS version Mercurial should use.
> By providing this option, people can require newer TLS versions
> before they are formally deprecated by Mercurial/Python/OpenSSL/etc
> and lower their security exposure. This option also provides an
> easy mechanism to change protocol policies in Mercurial. If there
> is a 0-day and TLS 1.0 is completely broken, we can act quickly
> without changing much code.
>
> Because setting the minimum TLS protocol is something you'll likely
> want to do globally, this patch introduces the special
> "hostsecurity._default_" config option to hold settings that apply
> to all hosts. Only the "protocol" sub-option currently works.
>
> Open Issues:
>
> * Need to write tests
> * Is "hostsecurity._default_" really the best way to do that?
> * Do we want to support both the explicit and "+" values. I could be
>   convinced that only the e.g. "tls1.1+" values are needed. Why would
>   you only choose TLS 1.1 when TLS 1.2 is available? That feels like
>   a footgun that could accidentally lock you into old security when
>   TLS 1.3 is available.

I think that it should really just be a "minimum version" of TLS, so
tls1.1 means 1.1 or newer. The footgun aspect is important.

> * Support "hostsecurity._default_:verifycertsfile"? (would effectively
>   be web.cacerts for clients)
> * Should we issue a warning if TLS 1.0 is used? What if TLS 1.0 is all
>   that is available?

I'm not sure. Browsers are still allowing TLS1, so we probably should
just silently allow it by default too.

> * Should we stop using TLS 1.0 if TLS 1.1+ is available by default?

I don't know that I'd go this far yet.

> * Should we also provide a config option that sets value for
>   SSLContext.set_ciphers()?

Probably.

>
> diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
> --- a/mercurial/help/config.txt
> +++ b/mercurial/help/config.txt
> @@ -1021,16 +1021,24 @@ The following per-host settings can be d
>      host and Mercurial will require the remote certificate to match one
>      of the fingerprints specified. This means if the server updates its
>      certificate, Mercurial will abort until a new fingerprint is defined.
>      This can provide stronger security than traditional CA-based validation
>      at the expense of convenience.
>
>      This option takes precedence over ``verifycertsfile``.
>
> +``protocol``
> +    Defines the channel encryption protocol to use.
> +
> +    By default, the highest TLS version 1.0 or greater protocol supported by
> +    both client and server is used. This option can be used to explicitly use
> +    a specific protocol version or to bump the minimum version that would
> +    otherwise be used.
> +
>  ``verifycertsfile``
>      Path to file a containing a list of PEM encoded certificates used to
>      verify the server certificate. Environment variables and ``~user``
>      constructs are expanded in the filename.
>
>      The server certificate or the certificate's certificate authority (CA)
>      must match a certificate from this file or certificate verification
>      will fail and connections to the server will be refused.
> diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
> --- a/mercurial/sslutil.py
> +++ b/mercurial/sslutil.py
> @@ -24,24 +24,26 @@ from . import (
>  # Python 2.7.9+ overhauled the built-in SSL/TLS features of Python. It added
>  # support for TLS 1.1, TLS 1.2, SNI, system CA stores, etc. These features are
>  # all exposed via the "ssl" module.
>  #
>  # Depending on the version of Python being used, SSL/TLS support is either
>  # modern/secure or legacy/insecure. Many operations in this module have
>  # separate code paths depending on support in Python.
>
> -hassni = getattr(ssl, 'HAS_SNI', False)
> +configprotocols = set([
> +    'tls1.0',
> +    'tls1.0+',
> +    'tls1.1',
> +    'tls1.1+',
> +    'tls1.2',
> +    'tls1.2+',
> +])
>
> -try:
> -    OP_NO_SSLv2 = ssl.OP_NO_SSLv2
> -    OP_NO_SSLv3 = ssl.OP_NO_SSLv3
> -except AttributeError:
> -    OP_NO_SSLv2 = 0x1000000
> -    OP_NO_SSLv3 = 0x2000000
> +hassni = getattr(ssl, 'HAS_SNI', False)
>
>  try:
>      # ssl.SSLContext was added in 2.7.9 and presence indicates modern
>      # SSL/TLS features are available.
>      SSLContext = ssl.SSLContext
>      modernssl = True
>      _canloaddefaultcerts = util.safehasattr(SSLContext, 'load_default_certs')
>  except AttributeError:
> @@ -140,25 +142,65 @@ def _hostsettings(ui, hostname):
>      # support TLS 1.2.
>      #
>      # The PROTOCOL_TLSv* constants select a specific TLS version
>      # only (as opposed to multiple versions). So the method for
>      # supporting multiple TLS versions is to use PROTOCOL_SSLv23 and
>      # disable protocols via SSLContext.options and OP_NO_* constants.
>      # However, SSLContext.options doesn't work unless we have the
>      # full/real SSLContext available to us.
> -    if modernssl:
> -        s['protocol'] = ssl.PROTOCOL_SSLv23
> +
> +    # Allow TLS protocol to be specified in the config.
> +    def validateprotocol(protocol, key):
> +        if protocol not in configprotocols:
> +            raise error.Abort(
> +                _('unsupported protocol from hostsecurity.%s: %s') %
> +                (key, protocol),
> +                hint=_('valid protocols: %s') %
> +                     ' '.join(sorted(configprotocols)))
> +
> +    protocol = ui.config('hostsecurity', '_default_:protocol', 'tls1.0+')
> +    validateprotocol(protocol, '_default_:protocol')
> +
> +    protocol = ui.config('hostsecurity', '%s:protocol' % hostname, protocol)
> +    validateprotocol(protocol, '%s.protocol' % hostname)
> +
> +    # Legacy ssl module only supports TLS 1.0.
> +    if not modernssl:
> +        if protocol in ('tls1.0', 'tls1.0+'):
> +            s['protocol'] = ssl.PROTOCOL_TLSv1
> +            s['ctxoptions'] = 0
> +        else:
> +            raise error.Abort(_('current Python does not support protocol '
> +                                'setting %s') % protocol,
> +                              hint=_('upgrade Python or disable setting since '
> +                                     'only TLS 1.0 is supported'))
>      else:
> -        s['protocol'] = ssl.PROTOCOL_TLSv1
> -
> -    # SSLv2 and SSLv3 are broken. We ban them outright.
> -    # WARNING: ctxoptions doesn't have an effect unless the modern ssl module
> -    # is available. Be careful when adding flags!
> -    s['ctxoptions'] = OP_NO_SSLv2 | OP_NO_SSLv3
> +        if protocol == 'tls1.0':
> +            s['protocol'] = ssl.PROTOCOL_TLSv1
> +            s['ctxoptions'] = 0
> +        elif protocol == 'tls1.0+':
> +            s['protocol'] = ssl.PROTOCOL_SSLv23
> +            s['ctxoptions'] = ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3
> +        elif protocol == 'tls1.1':
> +            s['protocol'] = ssl.PROTOCOL_TLSv1_1
> +            s['ctxoptions'] = 0
> +        elif protocol == 'tls1.1+':
> +            s['protocol'] = ssl.PROTOCOL_SSLv23
> +            s['ctxoptions'] = (ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3 |
> +                               ssl.OP_NO_TLSv1_1)
> +        elif protocol == 'tls1.2':
> +            s['protocol'] = ssl.PROTOCOL_TLSv1_2
> +            s['ctxoptions'] = 0
> +        elif protocol == 'tls1.2+':
> +            s['protocol'] = ssl.PROTOCOL_SSLv23
> +            s['ctxoptions'] = (ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3 |
> +                               ssl.OP_NO_TLSv1_1 | ssl.OP_NO_TLSv1_2)
> +        else:
> +            raise error.Abort(_('this should not happen'))
>
>      # Look for fingerprints in [hostsecurity] section. Value is a list
>      # of <alg>:<fingerprint> strings.
>      fingerprints = ui.configlist('hostsecurity', '%s:fingerprints' % hostname,
>                                   [])
>      for fingerprint in fingerprints:
>          if not (fingerprint.startswith(('sha1:', 'sha256:', 'sha512:'))):
>              raise error.Abort(_('invalid fingerprint for %s: %s') % (
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

On Tue, Jul 12, 2016 at 9:42 AM, Augie Fackler <raf@durin42.com> wrote:

> On Wed, Jul 06, 2016 at 11:31:37PM -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1467873039 25200
> > #      Wed Jul 06 23:30:39 2016 -0700
> > # Node ID 961c91677f9b2394b3b8140c28059d5fc5eee00f
> > # Parent  66a5adf42ac7bc0031e772b407403e809e67fb6e
> > [RFC] sslutil: config option to specify TLS protocol version
>
> queued patches 1 and 2, thanks. Comments inline below for some questions.
>
> >
> > Currently, Mercurial will use TLS 1.0 or newer when connecting to
> > remote servers, selecting the highest TLS version supported by both
> > peers. On older Pythons, only TLS 1.0 is available. On newer Pythons,
> > TLS 1.1 and 1.2 should be available.
> >
> > Security professionals recommend avoiding TLS 1.0 if possible.
> > PCI DSS 3.1 "strongly encourages" the use of TLS 1.2.
> >
> > Known attacks like BEAST and POODLE exist against TLS 1.0 (although
> > mitigations are available and properly configured servers aren't
> > vulnerable).
> >
> > Web browsers and many other applications continue to support TLS
> > 1.0 for compatibility reasons.
> >
> > Security-minded people may want to not take any risks running
> > TLS 1.0 (or even TLS 1.1). This patch gives those people a config
> > option to explicitly control which TLS version Mercurial should use.
> > By providing this option, people can require newer TLS versions
> > before they are formally deprecated by Mercurial/Python/OpenSSL/etc
> > and lower their security exposure. This option also provides an
> > easy mechanism to change protocol policies in Mercurial. If there
> > is a 0-day and TLS 1.0 is completely broken, we can act quickly
> > without changing much code.
> >
> > Because setting the minimum TLS protocol is something you'll likely
> > want to do globally, this patch introduces the special
> > "hostsecurity._default_" config option to hold settings that apply
> > to all hosts. Only the "protocol" sub-option currently works.
> >
> > Open Issues:
> >
> > * Need to write tests
> > * Is "hostsecurity._default_" really the best way to do that?
> > * Do we want to support both the explicit and "+" values. I could be
> >   convinced that only the e.g. "tls1.1+" values are needed. Why would
> >   you only choose TLS 1.1 when TLS 1.2 is available? That feels like
> >   a footgun that could accidentally lock you into old security when
> >   TLS 1.3 is available.
>
> I think that it should really just be a "minimum version" of TLS, so
> tls1.1 means 1.1 or newer. The footgun aspect is important.
>

OK. Will change this.


>
> > * Support "hostsecurity._default_:verifycertsfile"? (would effectively
> >   be web.cacerts for clients)
> > * Should we issue a warning if TLS 1.0 is used? What if TLS 1.0 is all
> >   that is available?
>
> I'm not sure. Browsers are still allowing TLS1, so we probably should
> just silently allow it by default too.
>

> > * Should we stop using TLS 1.0 if TLS 1.1+ is available by default?
>
> I don't know that I'd go this far yet.
>

I just asked Eric Rescorla "should Mercurial drop TLS 1.0 support" and his
response was "if you can get away with it." Basically a large portion of
the Internet still doesn't run TLS 1.1+ and dropping TLS 1.0 would make
that Internet unavailable. Just how many Mercurial servers don't run TLS
1.1+, I have no idea and there is no easy way to find out.

I'll throw out an idea. We could make TLS 1.1+ the default (on modern
Python versions since legacy Python only supports TLS 1.0) and provide an
option to allow TLS 1.0+. When connecting to a server that doesn't support
TLS 1.1+, we can suggest users try the legacy config. When TLS 1.0 is
insecure, we can drop the config option to allow it. There is also a
related discussion about whether we should print warnings on legacy Pythons
that don't support TLS 1.1+.


>
> > * Should we also provide a config option that sets value for
> >   SSLContext.set_ciphers()?
>
> Probably.
>
> >
> > diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
> > --- a/mercurial/help/config.txt
> > +++ b/mercurial/help/config.txt
> > @@ -1021,16 +1021,24 @@ The following per-host settings can be d
> >      host and Mercurial will require the remote certificate to match one
> >      of the fingerprints specified. This means if the server updates its
> >      certificate, Mercurial will abort until a new fingerprint is
> defined.
> >      This can provide stronger security than traditional CA-based
> validation
> >      at the expense of convenience.
> >
> >      This option takes precedence over ``verifycertsfile``.
> >
> > +``protocol``
> > +    Defines the channel encryption protocol to use.
> > +
> > +    By default, the highest TLS version 1.0 or greater protocol
> supported by
> > +    both client and server is used. This option can be used to
> explicitly use
> > +    a specific protocol version or to bump the minimum version that
> would
> > +    otherwise be used.
> > +
> >  ``verifycertsfile``
> >      Path to file a containing a list of PEM encoded certificates used to
> >      verify the server certificate. Environment variables and ``~user``
> >      constructs are expanded in the filename.
> >
> >      The server certificate or the certificate's certificate authority
> (CA)
> >      must match a certificate from this file or certificate verification
> >      will fail and connections to the server will be refused.
> > diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
> > --- a/mercurial/sslutil.py
> > +++ b/mercurial/sslutil.py
> > @@ -24,24 +24,26 @@ from . import (
> >  # Python 2.7.9+ overhauled the built-in SSL/TLS features of Python. It
> added
> >  # support for TLS 1.1, TLS 1.2, SNI, system CA stores, etc. These
> features are
> >  # all exposed via the "ssl" module.
> >  #
> >  # Depending on the version of Python being used, SSL/TLS support is
> either
> >  # modern/secure or legacy/insecure. Many operations in this module have
> >  # separate code paths depending on support in Python.
> >
> > -hassni = getattr(ssl, 'HAS_SNI', False)
> > +configprotocols = set([
> > +    'tls1.0',
> > +    'tls1.0+',
> > +    'tls1.1',
> > +    'tls1.1+',
> > +    'tls1.2',
> > +    'tls1.2+',
> > +])
> >
> > -try:
> > -    OP_NO_SSLv2 = ssl.OP_NO_SSLv2
> > -    OP_NO_SSLv3 = ssl.OP_NO_SSLv3
> > -except AttributeError:
> > -    OP_NO_SSLv2 = 0x1000000
> > -    OP_NO_SSLv3 = 0x2000000
> > +hassni = getattr(ssl, 'HAS_SNI', False)
> >
> >  try:
> >      # ssl.SSLContext was added in 2.7.9 and presence indicates modern
> >      # SSL/TLS features are available.
> >      SSLContext = ssl.SSLContext
> >      modernssl = True
> >      _canloaddefaultcerts = util.safehasattr(SSLContext,
> 'load_default_certs')
> >  except AttributeError:
> > @@ -140,25 +142,65 @@ def _hostsettings(ui, hostname):
> >      # support TLS 1.2.
> >      #
> >      # The PROTOCOL_TLSv* constants select a specific TLS version
> >      # only (as opposed to multiple versions). So the method for
> >      # supporting multiple TLS versions is to use PROTOCOL_SSLv23 and
> >      # disable protocols via SSLContext.options and OP_NO_* constants.
> >      # However, SSLContext.options doesn't work unless we have the
> >      # full/real SSLContext available to us.
> > -    if modernssl:
> > -        s['protocol'] = ssl.PROTOCOL_SSLv23
> > +
> > +    # Allow TLS protocol to be specified in the config.
> > +    def validateprotocol(protocol, key):
> > +        if protocol not in configprotocols:
> > +            raise error.Abort(
> > +                _('unsupported protocol from hostsecurity.%s: %s') %
> > +                (key, protocol),
> > +                hint=_('valid protocols: %s') %
> > +                     ' '.join(sorted(configprotocols)))
> > +
> > +    protocol = ui.config('hostsecurity', '_default_:protocol',
> 'tls1.0+')
> > +    validateprotocol(protocol, '_default_:protocol')
> > +
> > +    protocol = ui.config('hostsecurity', '%s:protocol' % hostname,
> protocol)
> > +    validateprotocol(protocol, '%s.protocol' % hostname)
> > +
> > +    # Legacy ssl module only supports TLS 1.0.
> > +    if not modernssl:
> > +        if protocol in ('tls1.0', 'tls1.0+'):
> > +            s['protocol'] = ssl.PROTOCOL_TLSv1
> > +            s['ctxoptions'] = 0
> > +        else:
> > +            raise error.Abort(_('current Python does not support
> protocol '
> > +                                'setting %s') % protocol,
> > +                              hint=_('upgrade Python or disable setting
> since '
> > +                                     'only TLS 1.0 is supported'))
> >      else:
> > -        s['protocol'] = ssl.PROTOCOL_TLSv1
> > -
> > -    # SSLv2 and SSLv3 are broken. We ban them outright.
> > -    # WARNING: ctxoptions doesn't have an effect unless the modern ssl
> module
> > -    # is available. Be careful when adding flags!
> > -    s['ctxoptions'] = OP_NO_SSLv2 | OP_NO_SSLv3
> > +        if protocol == 'tls1.0':
> > +            s['protocol'] = ssl.PROTOCOL_TLSv1
> > +            s['ctxoptions'] = 0
> > +        elif protocol == 'tls1.0+':
> > +            s['protocol'] = ssl.PROTOCOL_SSLv23
> > +            s['ctxoptions'] = ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3
> > +        elif protocol == 'tls1.1':
> > +            s['protocol'] = ssl.PROTOCOL_TLSv1_1
> > +            s['ctxoptions'] = 0
> > +        elif protocol == 'tls1.1+':
> > +            s['protocol'] = ssl.PROTOCOL_SSLv23
> > +            s['ctxoptions'] = (ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3 |
> > +                               ssl.OP_NO_TLSv1_1)
> > +        elif protocol == 'tls1.2':
> > +            s['protocol'] = ssl.PROTOCOL_TLSv1_2
> > +            s['ctxoptions'] = 0
> > +        elif protocol == 'tls1.2+':
> > +            s['protocol'] = ssl.PROTOCOL_SSLv23
> > +            s['ctxoptions'] = (ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3 |
> > +                               ssl.OP_NO_TLSv1_1 | ssl.OP_NO_TLSv1_2)
> > +        else:
> > +            raise error.Abort(_('this should not happen'))
> >
> >      # Look for fingerprints in [hostsecurity] section. Value is a list
> >      # of <alg>:<fingerprint> strings.
> >      fingerprints = ui.configlist('hostsecurity', '%s:fingerprints' %
> hostname,
> >                                   [])
> >      for fingerprint in fingerprints:
> >          if not (fingerprint.startswith(('sha1:', 'sha256:',
> 'sha512:'))):
> >              raise error.Abort(_('invalid fingerprint for %s: %s') % (
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

> On Jul 12, 2016, at 5:44 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> 
>> > * Should we stop using TLS 1.0 if TLS 1.1+ is available by default?
>> 
>> I don't know that I'd go this far yet.
> 
> I just asked Eric Rescorla "should Mercurial drop TLS 1.0 support" and his response was "if you can get away with it." Basically a large portion of the Internet still doesn't run TLS 1.1+ and dropping TLS 1.0 would make that Internet unavailable. Just how many Mercurial servers don't run TLS 1.1+, I have no idea and there is no easy way to find out.
> 
> I'll throw out an idea. We could make TLS 1.1+ the default (on modern Python versions since legacy Python only supports TLS 1.0) and provide an option to allow TLS 1.0+. When connecting to a server that doesn't support TLS 1.1+, we can suggest users try the legacy config. When TLS 1.0 is insecure, we can drop the config option to allow it. There is also a related discussion about whether we should print warnings on legacy Pythons that don't support TLS 1.1+.

Works for me. Make it so in the resend?