Merge branch 'tmp-safe-padding-removal-made-optional' into 'master'

Safe padding removal was made optional

Closes #466

See merge request gnutls/gnutls!669

8 files changed, 141 insertions, 32 deletions

diff --git a/doc/cha-gtls-app.texi b/doc/cha-gtls-app.texi
index c23c848d90..59b448547c 100644
--- a/doc/cha-gtls-app.texi
+++ b/doc/cha-gtls-app.texi
@@ -1294,10 +1294,10 @@ and @ref{tab:prio-special2}.
 will enable compatibility mode. It might mean that violations
 of the protocols are allowed as long as maximum compatibility with
 problematic clients and servers is achieved. More specifically this
-string would disable TLS record random padding, tolerate packets
-over the maximum allowed TLS record, and add a padding to TLS Client
-Hello packet to prevent it being in the 256-512 range which is known
-to be causing issues with a commonly used firewall (see the %DUMBFW option).
+string will tolerate packets over the maximum allowed TLS record,
+and add a padding to TLS Client Hello packet to prevent it being in the
+256-512 range which is known to be causing issues with a commonly used
+firewall (see the %DUMBFW option).
 
 @item %DUMBFW @tab
 will add a private extension with bogus data that make the client
diff --git a/doc/cha-intro-tls.texi b/doc/cha-intro-tls.texi
index ca3fa92daa..0c82f0853b 100644
--- a/doc/cha-intro-tls.texi
+++ b/doc/cha-intro-tls.texi
@@ -243,6 +243,14 @@ interface is provided by @funcref{gnutls_record_send2}, and is made
 available when under TLS1.3; alternatively @funcref{gnutls_record_can_use_length_hiding}
 can be queried.
 
+Note that this interface is not sufficient to completely hide the length of the
+data. The application code may reveal the data transferred by leaking its
+data processing time, or by leaking the TLS1.3 record processing time by
+GnuTLS. That is because under TLS1.3 the padding removal time depends on the
+padding data for an efficient implementation. To make that processing
+constant time the @funcref{gnutls_init} function must be called with
+the flag @code{GNUTLS_SAFE_PADDING_CHECK}.
+
 @showfuncdesc{gnutls_record_send2}
 
 Older GnuTLS versions provided an API suitable for cases where the sender
diff --git a/lib/cipher.c b/lib/cipher.c
index a502a16986..3d999de8a6 100644
--- a/lib/cipher.c
+++ b/lib/cipher.c
@@ -477,7 +477,7 @@ encrypt_packet_tls13(gnutls_session_t session,
 	iov[0].iov_base = plain->data;
 	iov[0].iov_len = plain->size;
 
-	if (pad_size) {
+	if (pad_size || (session->internals.flags & GNUTLS_SAFE_PADDING_CHECK)) {
 		uint8_t *pad = gnutls_calloc(1, 1+pad_size);
 		if (pad == NULL)
 			return gnutls_assert_val(GNUTLS_E_MEMORY_ERROR);
@@ -879,6 +879,8 @@ decrypt_packet_tls13(gnutls_session_t session,
 			*type = plain->data[j-1];
 			length = j-1;
 			length_set = 1;
+			if (!(session->internals.flags & GNUTLS_SAFE_PADDING_CHECK))
+				break;
 		}
 	}
 
diff --git a/lib/includes/gnutls/gnutls.h.in b/lib/includes/gnutls/gnutls.h.in
index be350ecb15..41389a39af 100644
--- a/lib/includes/gnutls/gnutls.h.in
+++ b/lib/includes/gnutls/gnutls.h.in
@@ -375,6 +375,11 @@ typedef enum {
  * @GNUTLS_POST_HANDSHAKE_AUTH: Enable post handshake authentication for server and client. When set and
  *   a server requests authentication after handshake %GNUTLS_E_REAUTH_REQUEST will be returned
  *   by gnutls_record_recv(). A client should then call gnutls_reauth() to re-authenticate.
+ * @GNUTLS_SAFE_PADDING_CHECK: Flag to indicate that the TLS 1.3 padding check will be done in a
+ *   safe way which doesn't leak the pad size based on GnuTLS processing time. This is of use to
+ *   applications which hide the length of transferred data via the TLS1.3 padding mechanism and
+ *   are already taking steps to hide the data processing time. This comes at a performance
+ *   penalty.
  *
  * Enumeration of different flags for gnutls_init() function. All the flags
  * can be combined except @GNUTLS_SERVER and @GNUTLS_CLIENT which are mutually
@@ -400,7 +405,8 @@ typedef enum {
 	GNUTLS_KEY_SHARE_TOP2 = (1<<12),
 	GNUTLS_KEY_SHARE_TOP3 = (1<<13),
 	GNUTLS_POST_HANDSHAKE_AUTH = (1<<14),
-	GNUTLS_NO_AUTO_REKEY = (1<<15)
+	GNUTLS_NO_AUTO_REKEY = (1<<15),
+	GNUTLS_SAFE_PADDING_CHECK = (1<<16)
 } gnutls_init_flags_t;
 
 /* compatibility defines (previous versions of gnutls
diff --git a/lib/record.c b/lib/record.c
index e4bfe34929..ce0ecb672a 100644
--- a/lib/record.c
+++ b/lib/record.c
@@ -490,7 +490,7 @@ _gnutls_send_tlen_int(gnutls_session_t session, content_type_t type,
 		retval = session->internals.record_send_buffer_user_size;
 	} else {
 		if (unlikely((send_data_size == 0 && min_pad == 0)))
-			return gnutls_assert_val(GNUTLS_E_INTERNAL_ERROR);
+			return gnutls_assert_val(GNUTLS_E_INVALID_REQUEST);
 
 		/* now proceed to packet encryption
 		 */
@@ -1760,6 +1760,10 @@ gnutls_record_send(gnutls_session_t session, const void *data,
  * To determine the maximum size of padding, use
  * gnutls_record_get_max_size() and gnutls_record_overhead_size().
  *
+ * Note that in order for GnuTLS to provide constant time processing
+ * of padding and data in TLS1.3, the flag %GNUTLS_SAFE_PADDING_CHECK
+ * must be used in gnutls_init().
+ *
  * Returns: The number of bytes sent, or a negative error code.  The
  *   number of bytes sent might be less than @data_size.  The maximum
  *   number of bytes this function can send in a single call depends
diff --git a/src/benchmark-tls.c b/src/benchmark-tls.c
index 5c43f827d4..285010ae1f 100644
--- a/src/benchmark-tls.c
+++ b/src/benchmark-tls.c
@@ -56,6 +56,7 @@ const char *side = "";
 #define PRIO_AES_CBC_SHA1 "NONE:+VERS-TLS1.0:+AES-128-CBC:+SHA1:+SIGN-ALL:+COMP-NULL:+RSA"
 #define PRIO_TLS12_AES_GCM "NONE:+VERS-TLS1.2:+AES-128-GCM:+AEAD:+SIGN-ALL:+COMP-NULL:+RSA"
 #define PRIO_AES_GCM "NONE:+VERS-TLS1.3:+AES-128-GCM:+AEAD:+SIGN-ALL:+COMP-NULL:+GROUP-ALL"
+#define PRIO_TLS12_AES_CCM "NONE:+VERS-TLS1.2:+AES-128-CCM:+AEAD:+SIGN-ALL:+COMP-NULL:+RSA"
 #define PRIO_AES_CCM "NONE:+VERS-TLS1.3:+AES-128-CCM:+AEAD:+SIGN-ALL:+COMP-NULL:+GROUP-ALL"
 #define PRIO_TLS12_CHACHA_POLY1305 "NONE:+VERS-TLS1.2:+CHACHA20-POLY1305:+AEAD:+SIGN-ALL:+COMP-NULL:+ECDHE-RSA:+CURVE-ALL"
 #define PRIO_CHACHA_POLY1305 "NONE:+VERS-TLS1.3:+CHACHA20-POLY1305:+AEAD:+SIGN-ALL:+COMP-NULL:+ECDHE-RSA:+CURVE-ALL"
@@ -535,6 +536,7 @@ void benchmark_tls(int debug_level, int ciphers)
 
 		test_ciphersuite(PRIO_TLS12_AES_GCM, size);
 		test_ciphersuite(PRIO_AES_GCM, size);
+		test_ciphersuite(PRIO_TLS12_AES_CCM, size);
 		test_ciphersuite(PRIO_AES_CCM, size);
 		test_ciphersuite(PRIO_TLS12_CHACHA_POLY1305, size);
 		test_ciphersuite(PRIO_CHACHA_POLY1305, size);
@@ -547,6 +549,7 @@ void benchmark_tls(int debug_level, int ciphers)
 		     size);
 		test_ciphersuite(PRIO_TLS12_AES_GCM, size);
 		test_ciphersuite(PRIO_AES_GCM, size);
+		test_ciphersuite(PRIO_TLS12_AES_CCM, size);
 		test_ciphersuite(PRIO_AES_CCM, size);
 		test_ciphersuite(PRIO_TLS12_CHACHA_POLY1305, size);
 		test_ciphersuite(PRIO_CHACHA_POLY1305, size);
diff --git a/tests/Makefile.am b/tests/Makefile.am
index fd9c10e497..14d75d42c5 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -192,7 +192,7 @@ ctests += mini-record-2 simple gnutls_hmac_fast set_pkcs12_cred cert certuniquei
 	 send-data-before-handshake recv-data-before-handshake crt_inv_write \
 	 x509sign-verify-error rng-op-nonce rng-op-random rng-op-key x509-dn-decode-compat \
 	 ip-check mini-x509-ipaddr trust-store base64-raw random-art dhex509self \
-	 dss-sig-val sign-pk-api tls-session-ext-override mini-record-pad \
+	 dss-sig-val sign-pk-api tls-session-ext-override record-pad \
 	 tls13-server-kx-neg gnutls_ext_raw_parse_dtls
 
 if HAVE_SECCOMP_TESTS
diff --git a/tests/mini-record-pad.c b/tests/record-pad.c
index 275ec70e06..acbcce9c7b 100644
--- a/tests/mini-record-pad.c
+++ b/tests/record-pad.c
@@ -45,6 +45,7 @@ int main(void)
 #include <gnutls/gnutls.h>
 #include <gnutls/dtls.h>
 #include <signal.h>
+#include <assert.h>
 
 #include "cert-common.h"
 #include "utils.h"
@@ -82,7 +83,16 @@ push(gnutls_transport_ptr_t tr, const void *data, size_t len)
 	return send(fd, data, len, 0);
 }
 
-static void client(int fd, const char *prio)
+struct test_st {
+	const char *name;
+	size_t pad;
+	size_t data;
+	const char *prio;
+	unsigned flags;
+	int sret;
+};
+
+static void client(int fd, struct test_st *test)
 {
 	int ret;
 	char buffer[MAX_BUF + 1];
@@ -101,12 +111,8 @@ static void client(int fd, const char *prio)
 	gnutls_anon_allocate_client_credentials(&anoncred);
 	gnutls_certificate_allocate_credentials(&x509_cred);
 
-	/* Initialize TLS session
-	 */
-	gnutls_init(&session, GNUTLS_CLIENT);
-
-	/* Use default priorities */
-	gnutls_priority_set_direct(session, prio, NULL);
+	assert(gnutls_init(&session, GNUTLS_CLIENT|test->flags)>=0);
+	assert(gnutls_priority_set_direct(session, test->prio, NULL)>=0);
 
 	/* put the anonymous credentials to the current session
 	 */
@@ -138,7 +144,7 @@ static void client(int fd, const char *prio)
 
 	do {
 		do {
-			ret = gnutls_record_recv(session, buffer, MAX_BUF);
+			ret = gnutls_record_recv(session, buffer, sizeof(buffer));
 		} while (ret == GNUTLS_E_AGAIN
 			 || ret == GNUTLS_E_INTERRUPTED);
 	} while (ret > 0);
@@ -157,7 +163,7 @@ static void client(int fd, const char *prio)
 
 	gnutls_bye(session, GNUTLS_SHUT_WR);
 
-      end:
+ end:
 
 	close(fd);
 
@@ -179,7 +185,7 @@ static void terminate(void)
 	exit(1);
 }
 
-static void server(int fd, const char *prio, size_t pad)
+static void server(int fd, struct test_st *test)
 {
 	int ret;
 	char buffer[MAX_BUF + 1];
@@ -205,12 +211,9 @@ static void server(int fd, const char *prio, size_t pad)
 
 	gnutls_anon_allocate_server_credentials(&anoncred);
 
-	gnutls_init(&session, GNUTLS_SERVER);
+	assert(gnutls_init(&session, GNUTLS_SERVER|test->flags)>=0);
 
-	/* avoid calling all the priority functions, since the defaults
-	 * are adequate.
-	 */
-	gnutls_priority_set_direct(session, prio, NULL);
+	assert(gnutls_priority_set_direct(session, test->prio, NULL)>=0);
 
 	gnutls_credentials_set(session, GNUTLS_CRD_ANON, anoncred);
 	gnutls_credentials_set(session, GNUTLS_CRD_CERTIFICATE, x509_cred);
@@ -238,28 +241,40 @@ static void server(int fd, const char *prio, size_t pad)
 
 	gnutls_transport_set_push_function(session, push);
 
+	assert(test->data <= sizeof(buffer));
+
 	total = 0;
 	do {
 		ret =
 		    gnutls_record_send2(session, buffer,
-					sizeof(buffer),
-					pad, 0);
+					test->data,
+					test->pad, 0);
 	} while (ret == GNUTLS_E_AGAIN
 		 || ret == GNUTLS_E_INTERRUPTED);
 
+	if (test->sret < 0) {
+		if (ret >= 0)
+			fail("server: expected failure got success!\n");
+		if (ret != test->sret)
+			fail("server: expected different failure: '%s', got: '%s'\n",
+			     gnutls_strerror(test->sret), gnutls_strerror(ret));
+		goto finish;
+	}
+
 	if (ret < 0) {
 		fail("Error sending packet: %s\n",
 		     gnutls_strerror(ret));
 		terminate();
 	}
 
-	expected = sizeof(buffer) + pad + gnutls_record_overhead_size(session);
+	expected = test->data + test->pad + gnutls_record_overhead_size(session);
 	if (total != expected) {
 		fail("Sent data (%u) are lower than expected (%u)\n",
 		     (unsigned) total, (unsigned) expected);
 		terminate();
 	}
 
+ finish:
 	/* do not wait for the peer to close the connection.
 	 */
 	gnutls_bye(session, GNUTLS_SHUT_WR);
@@ -276,11 +291,13 @@ static void server(int fd, const char *prio, size_t pad)
 		success("server: finished\n");
 }
 
-static void start(const char *prio, size_t pad)
+static void start(struct test_st *test)
 {
 	int fd[2];
 	int ret;
 
+	success("running %s\n", test->name);
+
 	ret = socketpair(AF_UNIX, SOCK_STREAM, 0, fd);
 	if (ret < 0) {
 		perror("socketpair");
@@ -297,11 +314,11 @@ static void start(const char *prio, size_t pad)
 	if (child) {
 		/* parent */
 		close(fd[1]);
-		server(fd[0], prio, pad);
+		server(fd[0], test);
 		kill(child, SIGTERM);
 	} else {
 		close(fd[0]);
-		client(fd[1], prio);
+		client(fd[1], test);
 		exit(0);
 	}
 }
@@ -316,13 +333,82 @@ static void ch_handler(int sig)
 	return;
 }
 
+struct test_st tests[] =
+{
+	{
+		.name = "AES-GCM with max pad",
+		.pad = HIGH(MAX_BUF+1)-(MAX_BUF+1),
+		.data = MAX_BUF,
+		.prio = AES_GCM,
+		.flags = 0
+	},
+	{
+		.name = "AES-GCM with zero pad",
+		.pad = 0,
+		.data = MAX_BUF,
+		.prio = AES_GCM,
+		.flags = 0
+	},
+	{
+		.name = "AES-GCM with 1-byte pad",
+		.pad = 1,
+		.data = MAX_BUF,
+		.prio = AES_GCM,
+		.flags = 0
+	},
+	{
+		.name = "AES-GCM with pad, but no data",
+		.pad = 16,
+		.data = 0,
+		.prio = AES_GCM,
+		.flags = 0
+	},
+	{
+		.name = "AES-GCM with max pad and safe padding check",
+		.pad = HIGH(MAX_BUF+1)-(MAX_BUF+1),
+		.data = MAX_BUF,
+		.prio = AES_GCM,
+		.flags = GNUTLS_SAFE_PADDING_CHECK
+	},
+	{
+		.name = "AES-GCM with zero pad and safe padding check",
+		.pad = 0,
+		.data = MAX_BUF,
+		.prio = AES_GCM,
+		.flags = GNUTLS_SAFE_PADDING_CHECK
+	},
+	{
+		.name = "AES-GCM with 1-byte pad and safe padding check",
+		.pad = 1,
+		.data = MAX_BUF,
+		.prio = AES_GCM,
+		.flags = GNUTLS_SAFE_PADDING_CHECK
+	},
+	{
+		.name = "AES-GCM with pad, but no data and safe padding check",
+		.pad = 16,
+		.data = 0,
+		.prio = AES_GCM,
+		.flags = GNUTLS_SAFE_PADDING_CHECK
+	},
+	{
+		.name = "AES-GCM with pad, but no data and no pad",
+		.pad = 0,
+		.data = 0,
+		.prio = AES_GCM,
+		.flags = GNUTLS_SAFE_PADDING_CHECK,
+		.sret = GNUTLS_E_INVALID_REQUEST
+	},
+};
+
 void doit(void)
 {
+	unsigned i;
 	signal(SIGCHLD, ch_handler);
 
-	start(AES_GCM, HIGH(MAX_BUF+1)-(MAX_BUF+1));
-	start(AES_GCM, 0);
-	start(AES_GCM, 1);
+	for (i=0;i<sizeof(tests)/sizeof(tests[0]);i++) {
+		start(&tests[i]);
+	}
 }
 
 #endif				/* _WIN32 */