gnutls_session_get_data2: fix operation without a timeout callback

When TLS1.3 was introduced, gnutls_session_get_data2 was modified
to assume that the callbacks set included the timeout one which was
not previously necessary except for some special cases. This corrects
that issue and makes sure that gnutls_session_get_data2() does not
fail (but not necessarily succeed), if that timeout callback is not
set.

Resolves: #823

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

8 files changed, 182 insertions, 18 deletions

diff --git a/NEWS b/NEWS
index e0320042c3..538256a0b7 100644
--- a/NEWS
+++ b/NEWS
@@ -15,6 +15,10 @@ See the end for copying conditions.
 ** libgnutls: add gnutls_aead_cipher_encryptv2 and gnutls_aead_cipher_decryptv2
    functions that will perform in-place encryption/decryption on data buffers (#718).
 
+** libgnutls: Corrected issue in gnutls_session_get_data2() which could fail under
+   TLS1.3, if a timeout callback was not set using gnutls_transport_set_pull_timeout_function()
+   (#823).
+
 ** libgnutls: added interoperability tests with gnutls 2.12.x; addressed
    issue with large record handling due to random padding (#811).
 
diff --git a/doc/cha-upgrade.texi b/doc/cha-upgrade.texi
index 286790de5b..91c6803ec6 100644
--- a/doc/cha-upgrade.texi
+++ b/doc/cha-upgrade.texi
@@ -241,7 +241,9 @@ TLS 1.3 is done via session tickets, c.f. @funcref{gnutls_session_ticket_enable_
 @item @funcref{gnutls_session_get_data2}, @funcref{gnutls_session_get_data}
 @tab These functions may introduce a slight delay under TLS 1.3 for few
 milliseconds. Check output of @funcref{gnutls_session_get_flags} for GNUTLS_SFLAGS_SESSION_TICKET
-before calling this function to avoid delays.
+before calling this function to avoid delays. To work efficiently under
+TLS 1.3 this function requires the application setting
+@funcref{gnutls_transport_set_pull_timeout_function}.
 
 @item SRP and RSA-PSK key exchanges are not supported under TLS 1.3
 @tab SRP and RSA-PSK key exchanges are not supported in TLS 1.3, so when these key exchanges are present in a priority string, TLS 1.3 is disabled.
diff --git a/lib/buffers.c b/lib/buffers.c
index 32202c6ffb..f3749b70e2 100644
--- a/lib/buffers.c
+++ b/lib/buffers.c
@@ -741,9 +741,7 @@ int _gnutls_io_check_recv(gnutls_session_t session, unsigned int ms)
 	gnutls_transport_ptr_t fd = session->internals.transport_recv_ptr;
 	int ret = 0, err;
 
-	if (unlikely
-	    (session->internals.pull_timeout_func == gnutls_system_recv_timeout
-	     && session->internals.pull_func != system_read)) {
+	if (NO_TIMEOUT_FUNC_SET(session)) {
 		_gnutls_debug_log("The pull function has been replaced but not the pull timeout.\n");
 		return gnutls_assert_val(GNUTLS_E_PULL_ERROR);
 	}
diff --git a/lib/buffers.h b/lib/buffers.h
index 7b76423c4b..7f30b0ade1 100644
--- a/lib/buffers.h
+++ b/lib/buffers.h
@@ -37,6 +37,9 @@ inline static int _gnutls_record_buffer_get_size(gnutls_session_t session)
 	return session->internals.record_buffer.byte_length;
 }
 
+#define NO_TIMEOUT_FUNC_SET(session) unlikely(session->internals.pull_timeout_func == gnutls_system_recv_timeout \
+	     && session->internals.pull_func != system_read)
+
 /*-
  * record_check_unprocessed:
  * @session: is a #gnutls_session_t structure.
diff --git a/lib/session.c b/lib/session.c
index 6deda99c07..71bcb40515 100644
--- a/lib/session.c
+++ b/lib/session.c
@@ -109,6 +109,12 @@ gnutls_session_get_data(gnutls_session_t session,
  * received, will return session resumption data corresponding to the last
  * received ticket.
  *
+ * Note that this function under TLS1.3 requires a callback to be set with
+ * gnutls_transport_set_pull_timeout_function() for successful operation. There
+ * was a bug before 3.6.10 which could make this function fail if that callback
+ * was not set. On later versions if not set, the function will return a successful
+ * error code, but will return dummy data that cannot lead to a resumption.
+ *
  * Returns: On success, %GNUTLS_E_SUCCESS (0) is returned, otherwise
  *   an error code is returned.
  **/
@@ -128,10 +134,17 @@ gnutls_session_get_data2(gnutls_session_t session, gnutls_datum_t *data)
 		 * the value(s). */
 		ertt += 60;
 
-		/* wait for a message with timeout */
-		ret = _gnutls_recv_in_buffers(session, GNUTLS_APPLICATION_DATA, -1, ertt);
-		if (ret < 0 && (gnutls_error_is_fatal(ret) && ret != GNUTLS_E_TIMEDOUT)) {
-			return gnutls_assert_val(ret);
+		/* we cannot use a read with timeout if the caller has not set
+		 * a callback with gnutls_transport_set_pull_timeout_function() */
+		if (NO_TIMEOUT_FUNC_SET(session) || (session->internals.flags & GNUTLS_NONBLOCK)) {
+			if (!(session->internals.flags & GNUTLS_NONBLOCK))
+				_gnutls_debug_log("TLS1.3 works efficiently if a callback with gnutls_transport_set_pull_timeout_function() is set\n");
+		} else {
+			/* wait for a message with timeout */
+			ret = _gnutls_recv_in_buffers(session, GNUTLS_APPLICATION_DATA, -1, ertt);
+			if (ret < 0 && (gnutls_error_is_fatal(ret) && ret != GNUTLS_E_TIMEDOUT)) {
+				return gnutls_assert_val(ret);
+			}
 		}
 
 		if (!(session->internals.hsk_flags & HSK_TICKET_RECEIVED)) {
diff --git a/lib/system_override.c b/lib/system_override.c
index f2fc5749d9..9179bf5c8a 100644
--- a/lib/system_override.c
+++ b/lib/system_override.c
@@ -106,15 +106,13 @@ gnutls_transport_set_pull_function(gnutls_session_t session,
  * int (*gnutls_pull_timeout_func)(gnutls_transport_ptr_t, unsigned int ms);
  *
  * This callback is necessary when gnutls_handshake_set_timeout() or 
- * gnutls_record_set_timeout() are set, and for calculating the DTLS mode
- * timeouts.
- *
- * In short, this callback should be set when a custom pull function is
- * registered. The callback will not be used when the session is in TLS mode with
- * non-blocking sockets. That is, when %GNUTLS_NONBLOCK is specified for a TLS
- * session in gnutls_init(). For compatibility with future GnuTLS versions
- * it is recommended to always set this function when a custom pull function
- * is registered.
+ * gnutls_record_set_timeout() are set, under TLS1.3 and for enforcing the DTLS
+ * mode timeouts when in blocking mode.
+ *
+ * For compatibility with future GnuTLS versions this callback must be set when
+ * a custom pull function is registered. The callback will not be used when the
+ * session is in TLS mode with non-blocking sockets. That is, when %GNUTLS_NONBLOCK
+ * is specified for a TLS session in gnutls_init().
  *
  * The helper function gnutls_system_recv_timeout() is provided to
  * simplify writing callbacks. 
diff --git a/tests/Makefile.am b/tests/Makefile.am
index f08f76d0dd..c8458c2ba5 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -214,7 +214,8 @@ ctests += mini-record-2 simple gnutls_hmac_fast set_pkcs12_cred cert certuniquei
 	 null_retrieve_function tls-record-size-limit tls-crt_type-neg \
 	 resume-with-stek-expiration resume-with-previous-stek rawpk-api \
 	 tls-record-size-limit-asym dh-compute ecdh-compute sign-verify-data-newapi \
-	 sign-verify-newapi sign-verify-deterministic iov aead-cipher-vec
+	 sign-verify-newapi sign-verify-deterministic iov aead-cipher-vec \
+	 tls13-without-timeout-func
 
 if HAVE_SECCOMP_TESTS
 ctests += dtls-with-seccomp tls-with-seccomp dtls-client-with-seccomp tls-client-with-seccomp
diff --git a/tests/tls13-without-timeout-func.c b/tests/tls13-without-timeout-func.c
new file mode 100644
index 0000000000..8235835928
--- /dev/null
+++ b/tests/tls13-without-timeout-func.c
@@ -0,0 +1,145 @@
+/*
+ * Copyright (C) 2019 Red Hat, Inc.
+ *
+ * Author: Nikos Mavrogiannopoulos
+ *
+ * This file is part of GnuTLS.
+ *
+ * GnuTLS is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GnuTLS is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <assert.h>
+#include <gnutls/gnutls.h>
+#include "utils.h"
+#include "eagain-common.h"
+#include "cert-common.h"
+
+/* This tests TLS1.3 and gnutls_session_get_data2() when no
+ * callback with gnutls_transport_set_pull_timeout_function()
+ * is set */
+
+const char *side;
+
+static void tls_log_func(int level, const char *str)
+{
+	fprintf(stderr, "%s|<%d>| %s", side, level, str);
+}
+
+static time_t mytime(time_t * t)
+{
+	time_t then = 1461671166;
+
+	if (t)
+		*t = then;
+
+	return then;
+}
+
+static ssize_t
+server_pull_fail(gnutls_transport_ptr_t tr, void *data, size_t len)
+{
+	fail("unexpected call to pull callback detected\n");
+	return -1;
+}
+
+void doit(void)
+{
+	int ret;
+	/* Server stuff. */
+	gnutls_certificate_credentials_t serverx509cred;
+	gnutls_session_t server;
+	int sret = GNUTLS_E_AGAIN;
+	/* Client stuff. */
+	gnutls_certificate_credentials_t clientx509cred;
+	gnutls_session_t client;
+	int cret = GNUTLS_E_AGAIN;
+	gnutls_datum_t data;
+	char buf[128];
+
+	/* General init. */
+	global_init();
+	gnutls_global_set_log_function(tls_log_func);
+	if (debug)
+		gnutls_global_set_log_level(6);
+
+	gnutls_global_set_time_function(mytime);
+
+	assert(gnutls_certificate_allocate_credentials(&serverx509cred) >= 0);
+	assert(gnutls_certificate_set_x509_key_mem(serverx509cred,
+					    &server_cert, &server_key,
+					    GNUTLS_X509_FMT_PEM) >= 0);
+
+	assert(gnutls_init(&server, GNUTLS_SERVER) >= 0);
+	gnutls_credentials_set(server, GNUTLS_CRD_CERTIFICATE,
+				serverx509cred);
+	assert(gnutls_set_default_priority(server)>=0);
+	gnutls_transport_set_push_function(server, server_push);
+	gnutls_transport_set_pull_function(server, server_pull);
+	gnutls_transport_set_ptr(server, server);
+
+	assert(gnutls_certificate_allocate_credentials(&clientx509cred)>=0);
+
+	assert(gnutls_certificate_set_x509_trust_mem(clientx509cred, &ca_cert, GNUTLS_X509_FMT_PEM)>=0);
+
+	assert(gnutls_init(&client, GNUTLS_CLIENT)>=0);
+
+	assert(gnutls_credentials_set(client, GNUTLS_CRD_CERTIFICATE,
+				      clientx509cred)>=0);
+
+	assert(gnutls_priority_set_direct(client, "NORMAL:-VERS-ALL:+VERS-TLS1.3", NULL)>=0);
+	gnutls_transport_set_push_function(client, client_push);
+	gnutls_transport_set_pull_function(client, client_pull);
+	gnutls_transport_set_ptr(client, client);
+
+	HANDSHAKE(client, server);
+
+	ret = gnutls_record_recv(client, buf, sizeof(buf));
+	if (ret < 0 && ret != GNUTLS_E_AGAIN) {
+		fail("unexpected error: %s\n", gnutls_strerror(ret));
+	}
+
+	gnutls_transport_set_pull_function(server, server_pull_fail);
+
+	ret = gnutls_session_get_data2(client, &data);
+	if (ret != 0) {
+		fail("unexpected error: %s\n", gnutls_strerror(ret));
+	}
+	gnutls_free(data.data);
+	gnutls_transport_set_pull_function(server, server_pull);
+
+	ret = gnutls_record_recv(client, buf, sizeof(buf));
+	if (ret < 0 && ret != GNUTLS_E_AGAIN) {
+		fail("unexpected error: %s\n", gnutls_strerror(ret));
+	}
+
+	gnutls_bye(client, GNUTLS_SHUT_RDWR);
+	gnutls_bye(server, GNUTLS_SHUT_RDWR);
+
+	gnutls_deinit(client);
+	gnutls_deinit(server);
+
+	gnutls_certificate_free_credentials(serverx509cred);
+	gnutls_certificate_free_credentials(clientx509cred);
+
+	gnutls_global_deinit();
+}