priority: be backwards compatible with priority strings starting with NONEtmp-be-backwards-compatible-with-prio

That is, we allow priority strings which do not enable any groups to
work, by disabling TLS1.3. For example
'NONE:+VERS-TLS-ALL:+MAC-ALL:+RSA:+AES-128-GCM:+SIGN-ALL:+COMP-NULL'
is still operational, but no TLS1.3 is enabled when specified.

Resolves: #549

Signed-off-by: Nikos Mavrogiannopoulos <nmav@redhat.com>

6 files changed, 39 insertions, 15 deletions

diff --git a/doc/cha-gtls-app.texi b/doc/cha-gtls-app.texi
index 9a4cf29933..c7a87a5a22 100644
--- a/doc/cha-gtls-app.texi
+++ b/doc/cha-gtls-app.texi
@@ -1185,10 +1185,10 @@ verification profile.
 Means nothing is enabled.  This disables even protocol versions.
 It should be followed by the algorithms to be enabled. Note that
 using this option to build a priority string gives detailed control
-into the resulting settings, however it creates non-portable applications.
-With new revisions of the TLS protocol new priority items are routinely added
-requiring such a string to be continuously updated with the library. As
-such, we advice against using that option for applications targetting multiple versions
+into the resulting settings, however with new revisions of the TLS protocol
+new priority items are routinely added, and such strings are not
+forward compatible with new protocols. As such, we
+advice against using that option for applications targetting multiple versions
 of the GnuTLS library, and recommend using the defaults (see above) or
 adjusting the defaults via @funcref{gnutls_set_default_priority_append}.
 
diff --git a/lib/priority.c b/lib/priority.c
index 00681c53e8..09937526ea 100644
--- a/lib/priority.c
+++ b/lib/priority.c
@@ -1201,6 +1201,15 @@ static void add_dh(gnutls_priority_t priority_cache)
 	}
 }
 
+#define REMOVE_TLS13_IN_LOOP(vers, i) \
+	if (vers->tls13_sem) { \
+		for (j=i+1;j<priority_cache->protocol.algorithms;j++) \
+			priority_cache->protocol.priority[j-1] = priority_cache->protocol.priority[j]; \
+		priority_cache->protocol.algorithms--; \
+		i--; \
+		continue; \
+	}
+
 static int set_ciphersuite_list(gnutls_priority_t priority_cache)
 {
 	unsigned i, j, z;
@@ -1247,16 +1256,10 @@ static int set_ciphersuite_list(gnutls_priority_t priority_cache)
 		if (!vers)
 			continue;
 
-		/* if we have NULL ciphersuites, SRP or RSA-PSK enabled, remove TLS1.3+ protocol
-		 * versions; they cannot be negotiated under TLS1.3. */
+		/* if we have NULL ciphersuites, SRP, or RSA-PSK enabled remove TLS1.3+
+		 * protocol versions; they cannot be negotiated under TLS1.3. */
 		if (have_null || have_srp || have_rsa_psk) {
-			if (vers->tls13_sem) {
-				for (j=i+1;j<priority_cache->protocol.algorithms;j++)
-					priority_cache->protocol.priority[j-1] = priority_cache->protocol.priority[j];
-				priority_cache->protocol.algorithms--;
-				i--;
-				continue;
-			}
+			REMOVE_TLS13_IN_LOOP(vers, i);
 		}
 
 		if (vers->transport == GNUTLS_STREAM) { /* TLS */
@@ -1395,8 +1398,15 @@ static int set_ciphersuite_list(gnutls_priority_t priority_cache)
 		return gnutls_assert_val(GNUTLS_E_NO_PRIORITIES_WERE_SET);
 
 	/* when TLS 1.3 is available we must have groups set */
-	if (unlikely(!have_psk && tlsmax && tlsmax->id >= GNUTLS_TLS1_3 && priority_cache->groups.size == 0))
-		return gnutls_assert_val(GNUTLS_E_NO_PRIORITIES_WERE_SET);
+	if (unlikely(!have_psk && tlsmax && tlsmax->id >= GNUTLS_TLS1_3 && priority_cache->groups.size == 0)) {
+		for (i = 0; i < priority_cache->protocol.algorithms; i++) {
+			vers = version_to_entry(priority_cache->protocol.priority[i]);
+			if (!vers)
+				continue;
+
+			REMOVE_TLS13_IN_LOOP(vers, i);
+		}
+	}
 
 	return 0;
 }
diff --git a/tests/Makefile.am b/tests/Makefile.am
index c0ecfed3f2..295cfcd354 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -61,6 +61,7 @@ EXTRA_DIST = suppressions.valgrind eagain-common.h cert-common.h test-chains.h \
 	ocsp-tests/certs/server_good.key ocsp-tests/certs/server_bad.key ocsp-tests/certs/server_good.template \
 	ocsp-tests/certs/server_bad.template ocsp-tests/certs/ocsp-staple-unrelated.der ocsp-tests/suppressions.valgrind \
 	data/listings-DTLS1.0 data/listings-SSL3.0 data/listings-TLS1.0 data/listings-TLS1.1 \
+	data/listings-legacy1 data/listings-legacy2 \
 	data/listings-SSL3.0-TLS1.1 p11-kit-trust-data/Example_Root_CA.p11-kit server-kx-neg-common.c \
 	p11-kit-trust-data/Example_Root_CA.pem data/test1.cat data/test2.cat \
 	data/test1.cat.data data/test2.cat.data data/test1.cat.out data/test2.cat.out \
diff --git a/tests/cipher-listings.sh b/tests/cipher-listings.sh
index 094ae5f38f..b8f3a602e3 100755
--- a/tests/cipher-listings.sh
+++ b/tests/cipher-listings.sh
@@ -81,6 +81,11 @@ check TLS1.0 "NORMAL:-VERS-ALL:+VERS-TLS1.0"
 check TLS1.1 "NORMAL:-VERS-ALL:+VERS-TLS1.1"
 check SSL3.0-TLS1.1 "NORMAL:-VERS-ALL:+VERS-TLS1.0:+VERS-SSL3.0:+VERS-TLS1.1"
 check DTLS1.0 "NORMAL:-VERS-ALL:+VERS-DTLS1.0"
+# Priority strings prior to 3.6.x did not require the +GROUP option; here we
+# test whether these work as expected.
+check legacy1 "NONE:+VERS-TLS-ALL:+MAC-ALL:+RSA:+AES-128-GCM:+SIGN-ALL:+COMP-NULL"
+check legacy2 "NONE:+VERS-TLS-ALL:+MAC-ALL:+RSA:+CAMELLIA-256-GCM:+SIGN-ALL:+COMP-NULL"
+
 
 rm -f ${TMPFILE}
 rm -f ${TMPFILE2}
diff --git a/tests/data/listings-legacy1 b/tests/data/listings-legacy1
new file mode 100644
index 0000000000..549ca739c9
--- /dev/null
+++ b/tests/data/listings-legacy1
@@ -0,0 +1,4 @@
+Cipher suites for NONE:+VERS-TLS-ALL:+MAC-ALL:+RSA:+AES-128-GCM:+SIGN-ALL:+COMP-NULL
+TLS_RSA_AES_128_GCM_SHA256                        	0x00, 0x9c	TLS1.2
+
+Protocols: VERS-TLS1.2, VERS-TLS1.1, VERS-TLS1.0
diff --git a/tests/data/listings-legacy2 b/tests/data/listings-legacy2
new file mode 100644
index 0000000000..35ce346da5
--- /dev/null
+++ b/tests/data/listings-legacy2
@@ -0,0 +1,4 @@
+Cipher suites for NONE:+VERS-TLS-ALL:+MAC-ALL:+RSA:+CAMELLIA-256-GCM:+SIGN-ALL:+COMP-NULL
+TLS_RSA_CAMELLIA_256_GCM_SHA384                   	0xc0, 0x7b	TLS1.2
+
+Protocols: VERS-TLS1.2, VERS-TLS1.1, VERS-TLS1.0