Patchwork [2,of,5,V3] sslutil: config option to specify TLS protocol version

login
register
mail settings
Submitter Pierre-Yves David
Date July 15, 2016, 1:18 a.m.
Message ID <0ce977b1-2f97-9cfe-f428-6e6cf268eb1a@ens-lyon.org>
Download mbox | patch
Permalink /patch/15863/
State Not Applicable
Headers show

Comments

Pierre-Yves David - July 15, 2016, 1:18 a.m.
That patch seems fine to me, but I've a test failure on my test machine
(works fine locally)

 #if sslcontext

ERROR: test-https.t output changed

Can you look into it?

Patch 1 is pushed, see comment about Patch 3 in the other email.


On 07/14/2016 06:50 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1468468118 25200
> #      Wed Jul 13 20:48:38 2016 -0700
> # Node ID 6a6d56e1391ff7e1468ef1b44b7e4c5cbe406f7b
> # Parent  944f37469b94ea956566a037f9b7ae136a99b119
> 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-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 versions Mercurial should use.
> By providing this option, one 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 a global config option under
> [hostsecurity] for that purpose.
> 
> wrapserversocket() has been taught a hidden config option to define
> the explicit protocol to use. This is queried in this function and
> not passed as an argument because I don't want to expose this dangerous
> option as part of the Python API. There is a risk someone could footgun
> themselves. But the config option is a devel option, has a warning
> comment, and I doubt most people are using `hg serve` to run a
> production HTTPS server (I would have something not Mercurial/Python
> handle TLS). If this is problematic, we can go back to using a
> custom extension in tests to coerce the server into bad behavior.
> 
> diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
> --- a/mercurial/help/config.txt
> +++ b/mercurial/help/config.txt
> @@ -995,20 +995,32 @@ For example::
>  
>      [hostfingerprints]
>      hg.intevation.de = fc:e2:8d:d9:51:cd:cb:c1:4d:18:6b:b7:44:8d:49:72:57:e6:cd:33
>      hg.intevation.org = fc:e2:8d:d9:51:cd:cb:c1:4d:18:6b:b7:44:8d:49:72:57:e6:cd:33
>  
>  ``hostsecurity``
>  ----------------
>  
> -Used to specify per-host security settings.
> -
> -Options in this section have the form ``hostname``:``setting``. This allows
> -multiple settings to be defined on a per-host basis.
> +Used to specify global and per-host security settings for connecting to
> +other machines.
> +
> +The following options control default behavior for all hosts.
> +
> +``minimumprotocol``
> +    Defines the minimum channel encryption protocol to use.
> +
> +    By default, the highest version of TLS - 1.0 or greater - supported by
> +    both client and server is used.
> +
> +    Allowed values are: ``tls1.0`` (the default), ``tls1.1``, ``tls1.2``.
> +
> +Options in the ``[hostsecurity]`` section can have the form
> +``hostname``:``setting``. This allows multiple settings to be defined on a
> +per-host basis.
>  
>  The following per-host settings can be defined.
>  
>  ``fingerprints``
>      A list of hashes of the DER encoded peer/remote certificate. Values have
>      the form ``algorithm``:``fingerprint``. e.g.
>      ``sha256:c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714caef0c4f2``.
>  
> @@ -1021,16 +1033,20 @@ 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``.
>  
> +``minimumprotocol``
> +    This behaves like ``minimumprotocol`` as described above except it
> +    only applies to the host on which it is defined.
> +
>  ``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.
> @@ -1053,16 +1069,23 @@ The following per-host settings can be d
>  
>  For example::
>  
>      [hostsecurity]
>      hg.example.com:fingerprints = sha256:c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714caef0c4f2
>      hg2.example.com:fingerprints = sha1:914f1aff87249c09b6859b88b1906d30756491ca, sha1:fc:e2:8d:d9:51:cd:cb:c1:4d:18:6b:b7:44:8d:49:72:57:e6:cd:33
>      foo.example.com:verifycertsfile = /etc/ssl/trusted-ca-certs.pem
>  
> +To change the default minimum protocol version to TLS 1.2 but to allow TLS 1.1
> +when connecting to ``hg.example.com``::
> +
> +    [hostsecurity]
> +    minimumprotocol = tls1.2
> +    hg.example.com:minimumprotocol = tls1.1
> +
>  ``http_proxy``
>  --------------
>  
>  Used to access web-based Mercurial repositories through a HTTP
>  proxy.
>  
>  ``host``
>      Host name and (optional) port of the proxy server, for example
> diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
> --- a/mercurial/sslutil.py
> +++ b/mercurial/sslutil.py
> @@ -24,24 +24,23 @@ 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.1',
> +    '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:
> @@ -131,34 +130,45 @@ def _hostsettings(ui, hostname):
>          # ssl.CERT_* constant used by SSLContext.verify_mode.
>          'verifymode': None,
>          # Defines extra ssl.OP* bitwise options to set.
>          'ctxoptions': None,
>      }
>  
>      # Despite its name, PROTOCOL_SSLv23 selects the highest protocol
>      # that both ends support, including TLS protocols. On legacy stacks,
> -    # the highest it likely goes in TLS 1.0. On modern stacks, it can
> +    # the highest it likely goes is TLS 1.0. On modern stacks, it can
>      # 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
> -    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
> +    # Allow minimum 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)))
> +
> +    key = 'minimumprotocol'
> +    # Default to TLS 1.0+ as that is what browsers are currently doing.
> +    protocol = ui.config('hostsecurity', key, 'tls1.0')
> +    validateprotocol(protocol, key)
> +
> +    key = '%s:minimumprotocol' % hostname
> +    protocol = ui.config('hostsecurity', key, protocol)
> +    validateprotocol(protocol, key)
> +
> +    s['protocol'], s['ctxoptions'] = protocolsettings(protocol)
>  
>      # 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') % (
> @@ -241,16 +251,52 @@ def _hostsettings(ui, hostname):
>              s['verifymode'] = ssl.CERT_NONE
>  
>      assert s['protocol'] is not None
>      assert s['ctxoptions'] is not None
>      assert s['verifymode'] is not None
>  
>      return s
>  
> +def protocolsettings(protocol):
> +    """Resolve the protocol and context options for a config value."""
> +    if protocol not in configprotocols:
> +        raise ValueError('protocol value not supported: %s' % protocol)
> +
> +    # Legacy ssl module only supports up to TLS 1.0. Ideally we'd use
> +    # PROTOCOL_SSLv23 and options to disable SSLv2 and SSLv3. However,
> +    # SSLContext.options doesn't work in our implementation since we use
> +    # a fake SSLContext on these Python versions.
> +    if not modernssl:
> +        if protocol != 'tls1.0':
> +            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'))
> +
> +        return ssl.PROTOCOL_TLSv1, 0
> +
> +    # WARNING: returned options don't work unless the modern ssl module
> +    # is available. Be careful when adding options here.
> +
> +    # SSLv2 and SSLv3 are broken. We ban them outright.
> +    options = ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3
> +
> +    if protocol == 'tls1.0':
> +        # Defaults above are to use TLS 1.0+
> +        pass
> +    elif protocol == 'tls1.1':
> +        options |= ssl.OP_NO_TLSv1
> +    elif protocol == 'tls1.2':
> +        options |= ssl.OP_NO_TLSv1 | ssl.OP_NO_TLSv1_1
> +    else:
> +        raise error.Abort(_('this should not happen'))
> +
> +    return ssl.PROTOCOL_SSLv23, options
> +
>  def wrapsocket(sock, keyfile, certfile, ui, serverhostname=None):
>      """Add SSL/TLS to a socket.
>  
>      This is a glorified wrapper for ``ssl.wrap_socket()``. It makes sane
>      choices based on what security options are available.
>  
>      In addition to the arguments supported by ``ssl.wrap_socket``, we allow
>      the following additional arguments:
> @@ -297,28 +343,35 @@ def wrapsocket(sock, keyfile, certfile, 
>          # This is a no-op on old Python.
>          sslcontext.load_default_certs()
>          caloaded = True
>      else:
>          caloaded = False
>  
>      try:
>          sslsocket = sslcontext.wrap_socket(sock, server_hostname=serverhostname)
> -    except ssl.SSLError:
> +    except ssl.SSLError as e:
>          # If we're doing certificate verification and no CA certs are loaded,
>          # that is almost certainly the reason why verification failed. Provide
>          # a hint to the user.
>          # Only modern ssl module exposes SSLContext.get_ca_certs() so we can
>          # only show this warning if modern ssl is available.
>          if (caloaded and settings['verifymode'] == ssl.CERT_REQUIRED and
>              modernssl and not sslcontext.get_ca_certs()):
>              ui.warn(_('(an attempt was made to load CA certificates but none '
>                        'were loaded; see '
>                        'https://mercurial-scm.org/wiki/SecureConnections for '
>                        'how to configure Mercurial to avoid this error)\n'))
> +        # Try to print more helpful error messages for known failures.
> +        if util.safehasattr(e, 'reason'):
> +            if e.reason == 'UNSUPPORTED_PROTOCOL':
> +                ui.warn(_('(could not negotiate a common protocol; see '
> +                          'https://mercurial-scm.org/wiki/SecureConnections '
> +                          'for how to configure Mercurial to avoid this '
> +                          'error)\n'))
>          raise
>  
>      # check if wrap_socket failed silently because socket had been
>      # closed
>      # - see http://bugs.python.org/issue13721
>      if not sslsocket.cipher():
>          raise error.Abort(_('ssl connection failed'))
>  
> @@ -341,17 +394,47 @@ def wrapserversocket(sock, ui, certfile=
>  
>      ``cafile`` defines the path to certificate authorities.
>  
>      ``requireclientcert`` specifies whether to require client certificates.
>  
>      Typically ``cafile`` is only defined if ``requireclientcert`` is true.
>      """
>      if modernssl:
> -        sslcontext = ssl.create_default_context(purpose=ssl.Purpose.CLIENT_AUTH)
> +        protocol, options = protocolsettings('tls1.0')
> +
> +        # This config option is intended for use in tests only. It is a giant
> +        # footgun to kill security. Don't define it.
> +        exactprotocol = ui.config('devel', 'serverexactprotocol')
> +        if exactprotocol == 'tls1.0':
> +            protocol = ssl.PROTOCOL_TLSv1
> +        elif exactprotocol == 'tls1.1':
> +            protocol = ssl.PROTOCOL_TLSv1_1
> +        elif exactprotocol == 'tls1.2':
> +            protocol = ssl.PROTOCOL_TLSv1_2
> +        elif exactprotocol:
> +            raise error.Abort(_('invalid value for serverexactprotocol: %s') %
> +                              exactprotocol)
> +
> +        # The protocol is pinned as SSLContext construction time and can't be
> +        # changed after the fact (even though the class allows you to assign
> +        # the variable it has no effect). Unfortunately,
> +        # ``create_default_context`` doesn't allow us to pass a custom
> +        # protocol.
> +        if exactprotocol:
> +            sslcontext = ssl.SSLContext(protocol)
> +            sslcontext.options |= options
> +            # Emulate create_default_context() behavior.
> +            sslcontext.options |= ssl.OP_CIPHER_SERVER_PREFERENCE
> +            sslcontext.options |= ssl.OP_SINGLE_DH_USE
> +            sslcontext.options |= ssl.OP_SINGLE_ECDH_USE
> +        else:
> +            sslcontext = ssl.create_default_context(
> +                purpose=ssl.Purpose.CLIENT_AUTH)
> +
>      else:
>          sslcontext = SSLContext(ssl.PROTOCOL_TLSv1)
>  
>      # We don't need to apply options to the context here because either
>      # a) create_default_context() has appropriate defaults and we don't have
>      # any custom settings to apply b) legacy ssl module is already using the
>      # appropriate protocol.
>  
> diff --git a/tests/test-https.t b/tests/test-https.t
> --- a/tests/test-https.t
> +++ b/tests/test-https.t
> @@ -387,18 +387,78 @@ Test https with cert problems through pr
>    abort: error: *certificate verify failed* (glob)
>    [255]
>    $ http_proxy=http://localhost:$HGPORT1/ hg -R copy-pull pull \
>    > --config web.cacerts="$CERTSDIR/pub-expired.pem" https://localhost:$HGPORT2/
>    pulling from https://localhost:$HGPORT2/
>    abort: error: *certificate verify failed* (glob)
>    [255]
>  
> +  $ killdaemons.py hg0.pid
> +  $ killdaemons.py proxy.pid
> +  $ killdaemons.py hg2.pid
> +
> +#if sslcontext
> +Start servers running supported TLS versions
> +
> +  $ cd test
> +  $ hg serve -p $HGPORT -d --pid-file=../hg0.pid --certificate=$PRIV \
> +  > --config devel.serverexactprotocol=tls1.0
> +  $ cat ../hg0.pid >> $DAEMON_PIDS
> +  $ hg serve -p $HGPORT1 -d --pid-file=../hg1.pid --certificate=$PRIV \
> +  > --config devel.serverexactprotocol=tls1.1
> +  $ cat ../hg1.pid >> $DAEMON_PIDS
> +  $ hg serve -p $HGPORT2 -d --pid-file=../hg2.pid --certificate=$PRIV \
> +  > --config devel.serverexactprotocol=tls1.2
> +  $ cat ../hg2.pid >> $DAEMON_PIDS
> +  $ cd ..
> +
> +Clients talking same TLS versions work
> +
> +  $ P="$CERTSDIR" hg --config hostsecurity.minimumprotocol=tls1.0 id https://localhost:$HGPORT/
> +  5fed3813f7f5
> +  $ P="$CERTSDIR" hg --config hostsecurity.minimumprotocol=tls1.1 id https://localhost:$HGPORT1/
> +  5fed3813f7f5
> +  $ P="$CERTSDIR" hg --config hostsecurity.minimumprotocol=tls1.2 id https://localhost:$HGPORT2/
> +  5fed3813f7f5
> +
> +Clients requiring newer TLS version than what server supports fail
> +
> +  $ P="$CERTSDIR" hg --config hostsecurity.minimumprotocol=tls1.1 id https://localhost:$HGPORT/
> +  (could not negotiate a common protocol; see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error)
> +  abort: error: *unsupported protocol* (glob)
> +  [255]
> +  $ P="$CERTSDIR" hg --config hostsecurity.minimumprotocol=tls1.2 id https://localhost:$HGPORT/
> +  (could not negotiate a common protocol; see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error)
> +  abort: error: *unsupported protocol* (glob)
> +  [255]
> +  $ P="$CERTSDIR" hg --config hostsecurity.minimumprotocol=tls1.2 id https://localhost:$HGPORT1/
> +  (could not negotiate a common protocol; see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error)
> +  abort: error: *unsupported protocol* (glob)
> +  [255]
> +
> +The per-host config option overrides the default
> +
> +  $ P="$CERTSDIR" hg id https://localhost:$HGPORT/ \
> +  > --config hostsecurity.minimumprotocol=tls1.2 \
> +  > --config hostsecurity.localhost:minimumprotocol=tls1.0
> +  5fed3813f7f5
> +
> +The per-host config option by itself works
> +
> +  $ P="$CERTSDIR" hg id https://localhost:$HGPORT/ \
> +  > --config hostsecurity.localhost:minimumprotocol=tls1.2
> +  (could not negotiate a common protocol; see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error)
> +  abort: error: *unsupported protocol* (glob)
> +  [255]
>  
>    $ killdaemons.py hg0.pid
> +  $ killdaemons.py hg1.pid
> +  $ killdaemons.py hg2.pid
> +#endif
>  
>  #if sslcontext
>  
>  Start hgweb that requires client certificates:
>  
>    $ cd test
>    $ hg serve -p $HGPORT -d --pid-file=../hg0.pid --certificate=$PRIV \
>    > --config devel.servercafile=$PRIV --config devel.serverrequirecert=true
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

Patch

--- /home/marmoute/mercurial-testing/tests/test-https.t
+++ /home/marmoute/mercurial-testing/tests/test-https.t.err
@@ -394,6 +394,7 @@ 

   $ killdaemons.py hg0.pid
   $ killdaemons.py proxy.pid
+  $TESTTMP.sh: line 184: 28996 Terminated              tinyproxy.py
$HGPORT1 localhost > proxy.log < /dev/null 2>&1
   $ killdaemons.py hg2.pid