Patchwork [3,of,5,V3] sslutil: require TLS 1.1+ when supported

login
register
mail settings
Submitter Gregory Szorc
Date July 14, 2016, 4:50 a.m.
Message ID <b4527c8cec88824c1593.1468471814@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/15837/
State Superseded
Headers show

Comments

Gregory Szorc - July 14, 2016, 4:50 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1468470954 25200
#      Wed Jul 13 21:35:54 2016 -0700
# Node ID b4527c8cec88824c15936f64e7d5ea59c5d54bee
# Parent  6a6d56e1391ff7e1468ef1b44b7e4c5cbe406f7b
sslutil: require TLS 1.1+ when supported

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).

I asked Eric Rescorla - Mozilla's resident crypto expert - whether
Mercurial should drop support for TLS 1.0. His response was
"if you can get away with it." Essentially, a number of servers on
the Internet don't support TLS 1.1+. This is why web browsers
continue to support TLS 1.0 despite desires from security experts.

This patch changes Mercurial's default behavior on modern Python
versions to require TLS 1.1+, thus avoiding known security issues
with TLS 1.0 and making Mercurial more secure by default. Rather
than drop TLS 1.0 support wholesale, we still allow TLS 1.0 to be
used if configured. This is a compromise solution - ideally we'd
disallow TLS 1.0. However, since we're not sure how many Mercurial
servers don't support TLS 1.1+ and we're not sure how much user
inconvenience this change will bring, I think it is prudent to ship
an escape hatch that still allows usage of TLS 1.0. In the default
case our users get better security. In the worst case, they are no
worse off than before this patch.

This patch has no effect when running on Python versions that don't
support TLS 1.1+.

As the added test shows, connecting to a server that doesn't
support TLS 1.1+ will display a warning message with a link to
our wiki, where we can guide people to configure their client to
allow less secure connections.
Pierre-Yves David - July 15, 2016, 1:18 a.m.
On 07/14/2016 06:50 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1468470954 25200
> #      Wed Jul 13 21:35:54 2016 -0700
> # Node ID b4527c8cec88824c15936f64e7d5ea59c5d54bee
> # Parent  6a6d56e1391ff7e1468ef1b44b7e4c5cbe406f7b
> sslutil: require TLS 1.1+ when supported

This change is scary (as in, a large base of our user will probably
explode) but I think I agree we should do it.
However, I would probably advocate to actually change the default at the
beginning of the 4.0 cycle to have a longer period to test it.

If other agree, I would be happy to take a V2, were the default is
unchanged but the documentation recommend tls1.1 for newer python. The
rest of the series looks fine to me.

Cheers
Gregory Szorc - July 15, 2016, 2:47 a.m.
On Thu, Jul 14, 2016 at 6:18 PM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 07/14/2016 06:50 AM, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1468470954 25200
> > #      Wed Jul 13 21:35:54 2016 -0700
> > # Node ID b4527c8cec88824c15936f64e7d5ea59c5d54bee
> > # Parent  6a6d56e1391ff7e1468ef1b44b7e4c5cbe406f7b
> > sslutil: require TLS 1.1+ when supported
>
> This change is scary (as in, a large base of our user will probably
> explode) but I think I agree we should do it.
> However, I would probably advocate to actually change the default at the
> beginning of the 4.0 cycle to have a longer period to test it.
>
> If other agree, I would be happy to take a V2, were the default is
> unchanged but the documentation recommend tls1.1 for newer python. The
> rest of the series looks fine to me.
>

I recognize that this and the patch after it (a warning when only TLS 1.0
is available) are scary. I would like to hear another opinion before I drop
them. I plan on sending a revised series in an hour or two. I'll hold the
course with dropping default compatibility with TLS 1.0 until someone else
says otherwise.
Augie Fackler - July 15, 2016, 5:21 p.m.
On Thu, Jul 14, 2016 at 07:47:48PM -0700, Gregory Szorc wrote:
> On Thu, Jul 14, 2016 at 6:18 PM, Pierre-Yves David <
> pierre-yves.david@ens-lyon.org> wrote:
>
> >
> >
> > On 07/14/2016 06:50 AM, Gregory Szorc wrote:
> > > # HG changeset patch
> > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > # Date 1468470954 25200
> > > #      Wed Jul 13 21:35:54 2016 -0700
> > > # Node ID b4527c8cec88824c15936f64e7d5ea59c5d54bee
> > > # Parent  6a6d56e1391ff7e1468ef1b44b7e4c5cbe406f7b
> > > sslutil: require TLS 1.1+ when supported
> >
> > This change is scary (as in, a large base of our user will probably
> > explode) but I think I agree we should do it.
> > However, I would probably advocate to actually change the default at the
> > beginning of the 4.0 cycle to have a longer period to test it.
> >
> > If other agree, I would be happy to take a V2, were the default is
> > unchanged but the documentation recommend tls1.1 for newer python. The
> > rest of the series looks fine to me.
> >
>
> I recognize that this and the patch after it (a warning when only TLS 1.0
> is available) are scary. I would like to hear another opinion before I drop
> them. I plan on sending a revised series in an hour or two. I'll hold the
> course with dropping default compatibility with TLS 1.0 until someone else
> says otherwise.

Per the discussion we had on the RFC round of this
(http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/95932/focus=96079),
I'm still a big fan of pushing for TLS 1.1 with a hint on how to
enable TLS 1.0.

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
--- a/mercurial/help/config.txt
+++ b/mercurial/help/config.txt
@@ -1003,20 +1003,28 @@  For example::
 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``.
+    By default, the highest version of TLS supported by both client and server
+    is used.
+
+    Allowed values are: ``tls1.0``, ``tls1.1``, ``tls1.2``.
+
+    When running on an old Python version, only ``tls1.0`` is allowed since
+    old versions of Python only support up to TLS 1.0.
+
+    When running a Python that supports modern TLS versions, the default is
+    ``tls1.1``. ``tls1.0`` can still be used to allow TLS 1.0. However, this
+    weakens security and should only be used as a feature of last resort if
+    a server does not support TLS 1.1+.
 
 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``
diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -149,19 +149,27 @@  def _hostsettings(ui, hostname):
     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)))
 
+    # Legacy Python can only do TLS 1.0. We default to TLS 1.1+ where we
+    # can because TLS 1.0 has known vulnerabilities (like BEAST and POODLE).
+    # We allow users to downgrade to TLS 1.0+ via config options in case a
+    # legacy server is encountered.
+    if modernssl:
+        defaultprotocol = 'tls1.1'
+    else:
+        defaultprotocol = 'tls1.0'
+
     key = 'minimumprotocol'
-    # Default to TLS 1.0+ as that is what browsers are currently doing.
-    protocol = ui.config('hostsecurity', key, 'tls1.0')
+    protocol = ui.config('hostsecurity', key, defaultprotocol)
     validateprotocol(protocol, key)
 
     key = '%s:minimumprotocol' % hostname
     protocol = ui.config('hostsecurity', key, protocol)
     validateprotocol(protocol, key)
 
     s['protocol'], s['ctxoptions'] = protocolsettings(protocol)
 
diff --git a/tests/test-https.t b/tests/test-https.t
--- a/tests/test-https.t
+++ b/tests/test-https.t
@@ -417,16 +417,21 @@  Clients talking same TLS versions work
   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 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.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]