From patchwork Fri Jul 15 01:18:33 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [2,of,5,V3] sslutil: config option to specify TLS protocol version From: Pierre-Yves David X-Patchwork-Id: 15863 Message-Id: <0ce977b1-2f97-9cfe-f428-6e6cf268eb1a@ens-lyon.org> To: Gregory Szorc , "mercurial-devel@mercurial-scm.org" Date: Fri, 15 Jul 2016 03:18:33 +0200 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 > # 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 : 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 > --- /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