Patchwork [08,of,11,V4] sslutil: config option to specify TLS protocol version

login
register
mail settings
Submitter Gregory Szorc
Date July 15, 2016, 4:09 a.m.
Message ID <e8fd30210061ff32370c.1468555749@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/15872/
State Superseded
Headers show

Comments

Gregory Szorc - July 15, 2016, 4:09 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1468554442 25200
#      Thu Jul 14 20:47:22 2016 -0700
# Node ID e8fd30210061ff32370c6cfc3388bd9457b94e30
# Parent  528d399eb97148ff3c7344404578b825410cc4e9
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.
timeless - Aug. 9, 2016, 9:50 p.m.
I'm not a huge fan of this abort:
+    # 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)))

It means that when a system updates to hg-default that supports this,
and I later build a newer hg that supports 1.3 or something, I'll
start getting aborts when using the system default, which means I'll
have to spend lots and lots of time trying to coerce the system
version to work at all.

Having to have two configs per repo or similar is a recipe for pain,
one to which I do not look forward.

We've had similar problems in the past -- the broken versions of
mercurial which can't safely handle the obsolete store.

Patch

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,38 +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)))
 
-    # Prevent CRIME.
-    # There is no guarantee this attribute is defined on the module.
-    s['ctxoptions'] |= getattr(ssl, 'OP_NO_COMPRESSION', 0)
+    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') % (
@@ -245,16 +251,56 @@  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'))
+
+    # Prevent CRIME.
+    # There is no guarantee this attribute is defined on the module.
+    options |= getattr(ssl, 'OP_NO_COMPRESSION', 0)
+
+    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:
@@ -301,28 +347,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'))
 
@@ -344,24 +397,36 @@  def wrapserversocket(sock, ui, certfile=
     file via ``certfile`` (the private key must come first in the file).
 
     ``cafile`` defines the path to certificate authorities.
 
     ``requireclientcert`` specifies whether to require client certificates.
 
     Typically ``cafile`` is only defined if ``requireclientcert`` is true.
     """
+    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)
+
     if modernssl:
         # We can't use ssl.create_default_context() for reasons described
         # above. Do most of what it does inline.
-        sslcontext = SSLContext(ssl.PROTOCOL_SSLv23)
-        # SSLv2 and SSLv3 are broken. Ban them outright.
-        sslcontext.options |= OP_NO_SSLv2 | OP_NO_SSLv3
-        # Prevent CRIME
-        sslcontext.options |= getattr(ssl, 'OP_NO_COMPRESSION', 0)
+        sslcontext = SSLContext(protocol)
+        sslcontext.options |= options
         # Improve forward secrecy.
         sslcontext.options |= getattr(ssl, 'OP_SINGLE_DH_USE', 0)
         sslcontext.options |= getattr(ssl, 'OP_SINGLE_ECDH_USE', 0)
 
         # The the list of more secure ciphers if found in the ssl module.
         if util.safehasattr(ssl, '_RESTRICTED_SERVER_CIPHERS'):
             sslcontext.options |= getattr(ssl, 'OP_CIPHER_SERVER_PREFERENCE', 0)
             sslcontext.set_ciphers(ssl._RESTRICTED_SERVER_CIPHERS)
diff --git a/tests/test-https.t b/tests/test-https.t
--- a/tests/test-https.t
+++ b/tests/test-https.t
@@ -340,21 +340,89 @@  Fingerprints
   (check hostfingerprint configuration)
   [255]
 
 
 - ignores that certificate doesn't match hostname
   $ hg -R copy-pull id https://127.0.0.1:$HGPORT/ --config hostfingerprints.127.0.0.1=ecd87cd6b386d04fc1b8b41c9d8f5e168eef1c03
   5fed3813f7f5
 
-HGPORT1 is reused below for tinyproxy tests. Kill that server.
+Ports used by next test. Kill servers.
+
+  $ killdaemons.py hg0.pid
   $ killdaemons.py hg1.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
 
 Prepare for connecting through proxy
 
+  $ hg serve -R test -p $HGPORT -d --pid-file=hg0.pid --certificate=$PRIV
+  $ cat hg0.pid >> $DAEMON_PIDS
+  $ hg serve -R test -p $HGPORT2 -d --pid-file=hg2.pid --certificate=server-expired.pem
+  $ cat hg2.pid >> $DAEMON_PIDS
+tinyproxy.py doesn't fully detach, so killing it may result in extra output
+from the shell. So don't kill it.
   $ tinyproxy.py $HGPORT1 localhost >proxy.log </dev/null 2>&1 &
   $ while [ ! -f proxy.pid ]; do sleep 0; done
   $ cat proxy.pid >> $DAEMON_PIDS
 
   $ echo "[http_proxy]" >> copy-pull/.hg/hgrc
   $ echo "always=True" >> copy-pull/.hg/hgrc
   $ echo "[hostfingerprints]" >> copy-pull/.hg/hgrc
   $ echo "localhost =" >> copy-pull/.hg/hgrc