From 2840bd0becee307f4ee896b26e9f29baac03c347 Mon Sep 17 00:00:00 2001

From: Andreas Schneider <asn@samba.org>

Date: Mon, 15 Jun 2020 11:50:16 +0200

Subject: [PATCH 1/2] s3:lib:tls: Use better priority lists for modern GnuTLS

We should use the default priority list. That is a good practice,

because TLS protocol hardening and phasing out of legacy algorithms,

is easier to co-ordinate when happens at a single place. See crypto

policies of Fedora.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14408

Signed-off-by: Andreas Schneider <asn@samba.org>

Reviewed-by: Alexander Bokovoy <ab@samba.org>

Autobuild-User(master): Andreas Schneider <asn@cryptomilk.org>

Autobuild-Date(master): Wed Jun 17 17:42:02 UTC 2020 on sn-devel-184

---

 docs-xml/smbdotconf/security/tlspriority.xml | 10 ++---

 lib/param/loadparm.c                         | 10 ++++-

 python/samba/tests/docs.py                   | 20 ++++++++++

 source3/param/loadparm.c                     | 11 +++++-

 source4/lib/tls/tls_tstream.c                | 40 +++++++++++++++-----

 wscript_configure_system_gnutls              |  3 ++

 6 files changed, 76 insertions(+), 18 deletions(-)

diff --git a/docs-xml/smbdotconf/security/tlspriority.xml b/docs-xml/smbdotconf/security/tlspriority.xml

index d7214a4c1ea..6d1f0dcb912 100644

--- a/docs-xml/smbdotconf/security/tlspriority.xml

+++ b/docs-xml/smbdotconf/security/tlspriority.xml

@@ -7,15 +7,15 @@

    to be supported in the parts of Samba that use GnuTLS, specifically

    the AD DC.

    </para>

-   <para>The default turns off SSLv3, as this protocol is no longer considered

-   secure after CVE-2014-3566 (otherwise known as POODLE) impacted SSLv3 use

-   in HTTPS applications.

-   </para>

+   <para>The string is appended to the default priority list of GnuTLS.</para>

    <para>The valid options are described in the

    <ulink url="http://gnutls.org/manual/html_node/Priority-Strings.html">GNUTLS

    Priority-Strings documentation at http://gnutls.org/manual/html_node/Priority-Strings.html</ulink>

    </para>

+   <para>By default it will try to find a config file matching "SAMBA", but if

+   that does not exist will use the entry for "SYSTEM" and last fallback to

+   NORMAL. In all cases the SSL3.0 protocol will be disabled.</para>

  </description>

- <value type="default">NORMAL:-VERS-SSL3.0</value>

+ <value type="default">@SAMBA,SYSTEM,NORMAL:!-VERS-SSL3.0</value>

 </samba:parameter>

diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c

index 63291283905..8fdd844fbaa 100644

--- a/lib/param/loadparm.c

+++ b/lib/param/loadparm.c

@@ -2803,7 +2803,15 @@ struct loadparm_context *loadparm_init(TALLOC_CTX *mem_ctx)

 	lpcfg_do_global_parameter(lp_ctx, "tls keyfile", "tls/key.pem");

 	lpcfg_do_global_parameter(lp_ctx, "tls certfile", "tls/cert.pem");

 	lpcfg_do_global_parameter(lp_ctx, "tls cafile", "tls/ca.pem");

-	lpcfg_do_global_parameter(lp_ctx, "tls priority", "NORMAL:-VERS-SSL3.0");

+#ifdef HAVE_GNUTLS_SET_DEFAULT_PRIORITY_APPEND

+	lpcfg_do_global_parameter(lp_ctx,

+				  "tls priority",

+				  "@SAMBA,SYSTEM,NORMAL:!-VERS-SSL3.0");

+#else

+	lpcfg_do_global_parameter(lp_ctx,

+				  "tls priority",

+				  "NORMAL:-VERS-SSL3.0");

+#endif

 	lpcfg_do_global_parameter(lp_ctx, "nsupdate command", "/usr/bin/nsupdate -g");

diff --git a/python/samba/tests/docs.py b/python/samba/tests/docs.py

index 32a16a98fbc..789865221cb 100644

--- a/python/samba/tests/docs.py

+++ b/python/samba/tests/docs.py

@@ -26,6 +26,21 @@ import os

 import subprocess

 import xml.etree.ElementTree as ET

+config_h = os.path.join("bin/default/include/config.h")

+config_hash = dict()

+

+if os.path.exists(config_h):

+    config_hash = dict()

+    f = open(config_h, 'r')

+    try:

+        lines = f.readlines()

+        config_hash = dict((x[0], ' '.join(x[1:]))

+                           for x in map(lambda line: line.strip().split(' ')[1:],

+                                        list(filter(lambda line: (line[0:7] == '#define') and (len(line.split(' ')) > 2), lines))))

+    finally:

+        f.close()

+

+have_gnutls_system_config_support = ("HAVE_GNUTLS_SET_DEFAULT_PRIORITY_APPEND" in config_hash)

 class TestCase(samba.tests.TestCaseInTempDir):

@@ -127,6 +142,11 @@ class SmbDotConfTests(TestCase):

         'smbd max async dosmode',

     ])

+    # 'tls priority' has a legacy default value if we don't link against a

+    # modern GnuTLS version.

+    if not have_gnutls_system_config_support:

+        special_cases.add('tls priority')

+

     def setUp(self):

         super(SmbDotConfTests, self).setUp()

         # create a minimal smb.conf file for testparm

diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c

index d3d81f6ece5..2b1a63998d6 100644

--- a/source3/param/loadparm.c

+++ b/source3/param/loadparm.c

@@ -885,8 +885,15 @@ static void init_globals(struct loadparm_context *lp_ctx, bool reinit_globals)

 	lpcfg_string_set(Globals.ctx, &Globals._tls_keyfile, "tls/key.pem");

 	lpcfg_string_set(Globals.ctx, &Globals._tls_certfile, "tls/cert.pem");

 	lpcfg_string_set(Globals.ctx, &Globals._tls_cafile, "tls/ca.pem");

-	lpcfg_string_set(Globals.ctx, &Globals.tls_priority,

-			 "NORMAL:-VERS-SSL3.0");

+#ifdef HAVE_GNUTLS_SET_DEFAULT_PRIORITY_APPEND

+	lpcfg_string_set(Globals.ctx,

+			 &Globals.tls_priority,

+			 "@SAMBA,SYSTEM,NORMAL:!-VERS-SSL3.0");

+#else

+	lpcfg_string_set(Globals.ctx,

+			 &Globals.tls_priority,

+			 "NORMAL!-VERS-SSL3.0");

+#endif

 	lpcfg_string_set(Globals.ctx, &Globals.share_backend, "classic");

diff --git a/source4/lib/tls/tls_tstream.c b/source4/lib/tls/tls_tstream.c

index 55bca036776..d984addeec5 100644

--- a/source4/lib/tls/tls_tstream.c

+++ b/source4/lib/tls/tls_tstream.c

@@ -1035,16 +1035,26 @@ struct tevent_req *_tstream_tls_connect_send(TALLOC_CTX *mem_ctx,

 		return tevent_req_post(req, ev);

 	}

-	ret = gnutls_priority_set_direct(tlss->tls_session,

-					 tls_params->tls_priority,

-					 &error_pos);

+	ret = gnutls_set_default_priority(tlss->tls_session);

 	if (ret != GNUTLS_E_SUCCESS) {

-		DEBUG(0,("TLS %s - %s.  Check 'tls priority' option at '%s'\n",

-			 __location__, gnutls_strerror(ret), error_pos));

+		DBG_ERR("TLS %s - %s. Failed to set default priorities\n",

+			__location__, gnutls_strerror(ret));

 		tevent_req_error(req, EINVAL);

 		return tevent_req_post(req, ev);

 	}

+	if (strlen(tls_params->tls_priority) > 0) {

+		ret = gnutls_priority_set_direct(tlss->tls_session,

+						 tls_params->tls_priority,

+						 &error_pos);

+		if (ret != GNUTLS_E_SUCCESS) {

+			DEBUG(0,("TLS %s - %s.  Check 'tls priority' option at '%s'\n",

+				 __location__, gnutls_strerror(ret), error_pos));

+			tevent_req_error(req, EINVAL);

+			return tevent_req_post(req, ev);

+		}

+	}

+

 	ret = gnutls_credentials_set(tlss->tls_session,

 				     GNUTLS_CRD_CERTIFICATE,

 				     tls_params->x509_cred);

@@ -1284,16 +1294,26 @@ struct tevent_req *_tstream_tls_accept_send(TALLOC_CTX *mem_ctx,

 		return tevent_req_post(req, ev);

 	}

-	ret = gnutls_priority_set_direct(tlss->tls_session,

-					 tlsp->tls_priority,

-					 &error_pos);

+	ret = gnutls_set_default_priority(tlss->tls_session);

 	if (ret != GNUTLS_E_SUCCESS) {

-		DEBUG(0,("TLS %s - %s.  Check 'tls priority' option at '%s'\n",

-			 __location__, gnutls_strerror(ret), error_pos));

+		DBG_ERR("TLS %s - %s. Failed to set default priorities\n",

+			__location__, gnutls_strerror(ret));

 		tevent_req_error(req, EINVAL);

 		return tevent_req_post(req, ev);

 	}

+	if (strlen(tlsp->tls_priority) > 0) {

+		ret = gnutls_priority_set_direct(tlss->tls_session,

+						 tlsp->tls_priority,

+						 &error_pos);

+		if (ret != GNUTLS_E_SUCCESS) {

+			DEBUG(0,("TLS %s - %s.  Check 'tls priority' option at '%s'\n",

+				 __location__, gnutls_strerror(ret), error_pos));

+			tevent_req_error(req, EINVAL);

+			return tevent_req_post(req, ev);

+		}

+	}

+

 	ret = gnutls_credentials_set(tlss->tls_session, GNUTLS_CRD_CERTIFICATE,

 				     tlsp->x509_cred);

 	if (ret != GNUTLS_E_SUCCESS) {

diff --git a/wscript_configure_system_gnutls b/wscript_configure_system_gnutls

index b2b955f3c90..631405fa34c 100644

--- a/wscript_configure_system_gnutls

+++ b/wscript_configure_system_gnutls

@@ -20,6 +20,9 @@ conf.SET_TARGET_TYPE('gnutls', 'SYSLIB')

 # Check for gnutls_pkcs7_get_embedded_data_oid (>= 3.5.5) required by libmscat

 conf.CHECK_FUNCS_IN('gnutls_pkcs7_get_embedded_data_oid', 'gnutls')

+# Check for gnutls_set_default_priority_append (>= 3.6.3)

+conf.CHECK_FUNCS_IN('gnutls_set_default_priority_append', 'gnutls')

+

 # Check for gnutls_aead_cipher_encryptv2

 #

 # This is available since version 3.6.10, but 3.6.10 has a bug which got fixed

-- 

2.26.2

From fdcf9f23f659025f174b32109a273e80b2ad289e Mon Sep 17 00:00:00 2001

From: Andreas Schneider <asn@samba.org>

Date: Tue, 30 Jun 2020 17:12:17 +0200

Subject: [PATCH 2/2] tls: Use NORMAL:-VERS-SSL3.0 as the default configuration

This seems to be really broken in GnuTLS and the documentation is also

not correct.

This partially reverts 53e3a959b958a3b099df6ecc5f6e294e96bd948e

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14408

Signed-off-by: Andreas Schneider <asn@samba.org>

Reviewed-by: Alexander Bokovoy <ab@samba.org>

Autobuild-User(master): Andrew Bartlett <abartlet@samba.org>

Autobuild-Date(master): Wed Jul  1 14:56:33 UTC 2020 on sn-devel-184

---

 docs-xml/smbdotconf/security/tlspriority.xml |  6 ++----

 lib/param/loadparm.c                         |  6 ------

 python/samba/tests/docs.py                   | 21 --------------------

 source3/param/loadparm.c                     |  8 +-------

 4 files changed, 3 insertions(+), 38 deletions(-)

diff --git a/docs-xml/smbdotconf/security/tlspriority.xml b/docs-xml/smbdotconf/security/tlspriority.xml

index 6d1f0dcb912..471dc25ba3b 100644

--- a/docs-xml/smbdotconf/security/tlspriority.xml

+++ b/docs-xml/smbdotconf/security/tlspriority.xml

@@ -12,10 +12,8 @@

    <ulink url="http://gnutls.org/manual/html_node/Priority-Strings.html">GNUTLS

    Priority-Strings documentation at http://gnutls.org/manual/html_node/Priority-Strings.html</ulink>

    </para>

-   <para>By default it will try to find a config file matching "SAMBA", but if

-   that does not exist will use the entry for "SYSTEM" and last fallback to

-   NORMAL. In all cases the SSL3.0 protocol will be disabled.</para>

+   <para>The SSL3.0 protocol will be disabled.</para>

  </description>

- <value type="default">@SAMBA,SYSTEM,NORMAL:!-VERS-SSL3.0</value>

+ <value type="default">NORMAL:-VERS-SSL3.0</value>

 </samba:parameter>

diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c

index 8fdd844fbaa..4e7e3f599dd 100644

--- a/lib/param/loadparm.c

+++ b/lib/param/loadparm.c

@@ -2803,15 +2803,9 @@ struct loadparm_context *loadparm_init(TALLOC_CTX *mem_ctx)

 	lpcfg_do_global_parameter(lp_ctx, "tls keyfile", "tls/key.pem");

 	lpcfg_do_global_parameter(lp_ctx, "tls certfile", "tls/cert.pem");

 	lpcfg_do_global_parameter(lp_ctx, "tls cafile", "tls/ca.pem");

-#ifdef HAVE_GNUTLS_SET_DEFAULT_PRIORITY_APPEND

-	lpcfg_do_global_parameter(lp_ctx,

-				  "tls priority",

-				  "@SAMBA,SYSTEM,NORMAL:!-VERS-SSL3.0");

-#else

 	lpcfg_do_global_parameter(lp_ctx,

 				  "tls priority",

 				  "NORMAL:-VERS-SSL3.0");

-#endif

 	lpcfg_do_global_parameter(lp_ctx, "nsupdate command", "/usr/bin/nsupdate -g");

diff --git a/python/samba/tests/docs.py b/python/samba/tests/docs.py

index 789865221cb..654a192b510 100644

--- a/python/samba/tests/docs.py

+++ b/python/samba/tests/docs.py

@@ -26,22 +26,6 @@ import os

 import subprocess

 import xml.etree.ElementTree as ET

-config_h = os.path.join("bin/default/include/config.h")

-config_hash = dict()

-

-if os.path.exists(config_h):

-    config_hash = dict()

-    f = open(config_h, 'r')

-    try:

-        lines = f.readlines()

-        config_hash = dict((x[0], ' '.join(x[1:]))

-                           for x in map(lambda line: line.strip().split(' ')[1:],

-                                        list(filter(lambda line: (line[0:7] == '#define') and (len(line.split(' ')) > 2), lines))))

-    finally:

-        f.close()

-

-have_gnutls_system_config_support = ("HAVE_GNUTLS_SET_DEFAULT_PRIORITY_APPEND" in config_hash)

-

 class TestCase(samba.tests.TestCaseInTempDir):

     def _format_message(self, parameters, message):

@@ -142,11 +126,6 @@ class SmbDotConfTests(TestCase):

         'smbd max async dosmode',

     ])

-    # 'tls priority' has a legacy default value if we don't link against a

-    # modern GnuTLS version.

-    if not have_gnutls_system_config_support:

-        special_cases.add('tls priority')

-

     def setUp(self):

         super(SmbDotConfTests, self).setUp()

         # create a minimal smb.conf file for testparm

diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c

index 2b1a63998d6..901f01b1c6a 100644

--- a/source3/param/loadparm.c

+++ b/source3/param/loadparm.c

@@ -885,15 +885,9 @@ static void init_globals(struct loadparm_context *lp_ctx, bool reinit_globals)

 	lpcfg_string_set(Globals.ctx, &Globals._tls_keyfile, "tls/key.pem");

 	lpcfg_string_set(Globals.ctx, &Globals._tls_certfile, "tls/cert.pem");

 	lpcfg_string_set(Globals.ctx, &Globals._tls_cafile, "tls/ca.pem");

-#ifdef HAVE_GNUTLS_SET_DEFAULT_PRIORITY_APPEND

 	lpcfg_string_set(Globals.ctx,

 			 &Globals.tls_priority,

-			 "@SAMBA,SYSTEM,NORMAL:!-VERS-SSL3.0");

-#else

-	lpcfg_string_set(Globals.ctx,

-			 &Globals.tls_priority,

-			 "NORMAL!-VERS-SSL3.0");

-#endif

+			 "NORMAL:-VERS-SSL3.0");

 	lpcfg_string_set(Globals.ctx, &Globals.share_backend, "classic");

-- 

2.26.2