From: Grigorii Demidov <grigorii.demidov@nic.cz>
Date: Thu, 15 Nov 2018 13:29:55 +0000 (+0100)
Subject: daemon TLS: increase wire-buffer size
X-Git-Tag: v3.2.0~21^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2d87427c9a80e978e20e5f5cf4055e006ba7fc8c;p=thirdparty%2Fknot-resolver.git

daemon TLS: increase wire-buffer size

When decoding large packets, gnutls gives the application chunks
of size 16kb. So that tls session wirebuffer must be at least
KNOT_WIRE_MAX_PKTSIZE + 16kb.  (message re-formatted by vcunat)
---

diff --git a/NEWS b/NEWS
index dcd94a168..26f441aa9 100644
--- a/NEWS
+++ b/NEWS
@@ -18,7 +18,7 @@ Bugfixes
 --------
 - http module: only run prometheus in parent process if using --forks=N,
   as the submodule collects metrics from all sub-processes as well.
-- policy.TLS_FORWARD fixes (!714)
+- TLS fixes for corner cases (!714, !700)
 - fix build with -DNOVERBOSELOG (#424)
 
 Improvements
diff --git a/daemon/io.c b/daemon/io.c
index d752a5106..ee65d4d93 100644
--- a/daemon/io.c
+++ b/daemon/io.c
@@ -108,7 +108,7 @@ static int udp_bind_finalize(uv_handle_t *handle)
 {
 	check_bufsize(handle);
 	/* Handle is already created, just create context. */
-	struct session *s = session_new(handle);
+	struct session *s = session_new(handle, false);
 	assert(s);
 	session_flags(s)->outgoing = false;
 	return io_start_read(handle);
@@ -254,7 +254,8 @@ static void _tcp_accept(uv_stream_t *master, int status, bool tls)
 	if (!client) {
 		return;
 	}
-	int res = io_create(master->loop, (uv_handle_t *)client, SOCK_STREAM, AF_UNSPEC);
+	int res = io_create(master->loop, (uv_handle_t *)client,
+			    SOCK_STREAM, AF_UNSPEC, tls);
 	if (res) {
 		if (res == UV_EMFILE) {
 			worker->too_many_open = true;
@@ -268,22 +269,20 @@ static void _tcp_accept(uv_stream_t *master, int status, bool tls)
 	}
 
 	/* struct session was allocated \ borrowed from memory pool. */
-	struct session *session = client->data;
-	assert(session_flags(session)->outgoing == false);
+	struct session *s = client->data;
+	assert(session_flags(s)->outgoing == false);
+	assert(session_flags(s)->has_tls == tls);
 
 	if (uv_accept(master, (uv_stream_t *)client) != 0) {
 		/* close session, close underlying uv handles and
 		 * deallocate (or return to memory pool) memory. */
-		session_close(session);
+		session_close(s);
 		return;
 	}
 
 	/* Set deadlines for TCP connection and start reading.
 	 * It will re-check every half of a request time limit if the connection
 	 * is idle and should be terminated, this is an educated guess. */
-	struct session *s = client->data;
-	assert(session_flags(s)->outgoing == false);
-
 	struct sockaddr *peer = session_get_peer(s);
 	int peer_len = sizeof(union inaddr);
 	int ret = uv_tcp_getpeername(client, peer, &peer_len);
@@ -297,7 +296,6 @@ static void _tcp_accept(uv_stream_t *master, int status, bool tls)
 	uint64_t idle_in_timeout = net->tcp.in_idle_timeout;
 
 	uint64_t timeout = KR_CONN_RTT_MAX / 2;
-	session_flags(s)->has_tls = tls;
 	if (tls) {
 		timeout += TLS_MAX_HANDSHAKE_TIME;
 		struct tls_ctx_t *ctx = session_tls_get_server_ctx(s);
@@ -415,7 +413,7 @@ int tcp_bindfd_tls(uv_tcp_t *handle, int fd, int tcp_backlog)
 	return _tcp_bindfd(handle, fd, tls_accept, tcp_backlog);
 }
 
-int io_create(uv_loop_t *loop, uv_handle_t *handle, int type, unsigned family)
+int io_create(uv_loop_t *loop, uv_handle_t *handle, int type, unsigned family, bool has_tls)
 {
 	int ret = -1;
 	if (type == SOCK_DGRAM) {
@@ -427,7 +425,7 @@ int io_create(uv_loop_t *loop, uv_handle_t *handle, int type, unsigned family)
 	if (ret != 0) {
 		return ret;
 	}
-	struct session *s = session_new(handle);
+	struct session *s = session_new(handle, has_tls);
 	if (s == NULL) {
 		ret = -1;
 	}
diff --git a/daemon/io.h b/daemon/io.h
index 34e33fccf..1d41569b2 100644
--- a/daemon/io.h
+++ b/daemon/io.h
@@ -35,8 +35,10 @@ void tcp_timeout_trigger(uv_timer_t *timer);
 
 /** Initialize the handle, incl. ->data = struct session * instance.
  * \param type = SOCK_*
- * \param family = AF_* */
-int io_create(uv_loop_t *loop, uv_handle_t *handle, int type, unsigned family);
+ * \param family = AF_*
+ * \param has_tls has meanings only when type is SOCK_STREAM */
+int io_create(uv_loop_t *loop, uv_handle_t *handle, int type,
+	      unsigned family, bool has_tls);
 void io_deinit(uv_handle_t *handle);
 void io_free(uv_handle_t *handle);
 
diff --git a/daemon/session.c b/daemon/session.c
index 74aa038f5..4e18b6a3e 100644
--- a/daemon/session.c
+++ b/daemon/session.c
@@ -26,6 +26,8 @@
 #include "daemon/io.h"
 #include "lib/generic/queue.h"
 
+#define TLS_CHUNK_SIZE (16 * 1024)
+
 /* Per-session (TCP or UDP) persistent structure,
  * that exists between remote counterpart and a local socket.
  */
@@ -301,7 +303,7 @@ struct session *session_get(uv_handle_t *h)
 	return h->data;
 }
 
-struct session *session_new(uv_handle_t *handle)
+struct session *session_new(uv_handle_t *handle, bool has_tls)
 {
 	if (!handle) {
 		return NULL;
@@ -314,13 +316,20 @@ struct session *session_new(uv_handle_t *handle)
 	queue_init(session->waiting);
 	session->tasks = trie_create(NULL);
 	if (handle->type == UV_TCP) {
-		uint8_t *wire_buf = malloc(KNOT_WIRE_MAX_PKTSIZE);
+		size_t wire_buffer_size = KNOT_WIRE_MAX_PKTSIZE;
+		if (has_tls) {
+			/* When decoding large packets,
+			 * gnutls gives the application chunks of size 16 kb each. */
+			wire_buffer_size += TLS_CHUNK_SIZE;
+			session->sflags.has_tls = true;
+		}
+		uint8_t *wire_buf = malloc(wire_buffer_size);
 		if (!wire_buf) {
 			free(session);
 			return NULL;
 		}
 		session->wire_buf = wire_buf;
-		session->wire_buf_size = KNOT_WIRE_MAX_PKTSIZE;
+		session->wire_buf_size = wire_buffer_size;
 	} else if (handle->type == UV_UDP) {
 		/* We use the singleton buffer from worker for all UDP (!)
 		 * libuv documentation doesn't really guarantee this is OK,
@@ -533,13 +542,13 @@ knot_pkt_t *session_produce_packet(struct session *session, knot_mm_t *mm)
 	const uv_handle_t *handle = session->handle;
 	uint8_t *msg_start = &session->wire_buf[session->wire_buf_start_idx];
 	ssize_t wirebuf_msg_data_size = session->wire_buf_end_idx - session->wire_buf_start_idx;
-	uint16_t msg_size = wirebuf_msg_data_size;
+	uint16_t msg_size = 0;
 	
 	if (!handle) {
 		session->sflags.wirebuf_error = true;
 		return NULL;
 	} else if (handle->type == UV_TCP) {
-		if (msg_size < 2) {
+		if (wirebuf_msg_data_size < 2) {
 			return NULL;
 		}
 		msg_size = knot_wire_read_u16(msg_start);
@@ -550,13 +559,21 @@ knot_pkt_t *session_produce_packet(struct session *session, knot_mm_t *mm)
 		if (msg_size + 2 > wirebuf_msg_data_size) {
 			return NULL;
 		}
+		if (msg_size == 0) {
+			session->sflags.wirebuf_error = true;
+			return NULL;
+		}
 		msg_start += 2;
+	} else if (wirebuf_msg_data_size < UINT16_MAX) {
+		msg_size = wirebuf_msg_data_size;
+	} else {
+		session->sflags.wirebuf_error = true;
+		return NULL;
 	}
 
+
 	knot_pkt_t *pkt = knot_pkt_new(msg_start, msg_size, mm);
-	if (pkt) {
-		session->sflags.wirebuf_error = false;
-	}
+	session->sflags.wirebuf_error = (pkt == NULL);
 	return pkt;
 }
 
@@ -679,7 +696,7 @@ size_t session_wirebuf_get_len(struct session *session)
 
 size_t session_wirebuf_get_size(struct session *session)
 {
-	return sizeof(session->wire_buf);
+	return session->wire_buf_size;
 }
 
 uint8_t *session_wirebuf_get_free_start(struct session *session)
diff --git a/daemon/session.h b/daemon/session.h
index a537213e0..654f93193 100644
--- a/daemon/session.h
+++ b/daemon/session.h
@@ -32,8 +32,9 @@ struct session_flags {
 	bool wirebuf_error : 1; /**< True: last operation with wirebuf ended up with an error. */
 };
 
-/* Allocate new session for a libuv handle. */
-struct session *session_new(uv_handle_t *handle);
+/* Allocate new session for a libuv handle.
+ * If handle->tyoe is UV_UDP, tls parameter will be ignored. */
+struct session *session_new(uv_handle_t *handle, bool has_tls);
 /* Clear and free given session. */
 void session_free(struct session *session);
 /* Clear session. */
diff --git a/daemon/worker.c b/daemon/worker.c
index aafcf9027..5173eacd2 100644
--- a/daemon/worker.c
+++ b/daemon/worker.c
@@ -131,7 +131,8 @@ static inline struct worker_ctx *get_worker(void)
 
 /*! @internal Create a UDP/TCP handle for an outgoing AF_INET* connection.
  *  socktype is SOCK_* */
-static uv_handle_t *ioreq_spawn(struct worker_ctx *worker, int socktype, sa_family_t family)
+static uv_handle_t *ioreq_spawn(struct worker_ctx *worker,
+				int socktype, sa_family_t family, bool has_tls)
 {
 	bool precond = (socktype == SOCK_DGRAM || socktype == SOCK_STREAM)
 			&& (family == AF_INET  || family == AF_INET6);
@@ -147,7 +148,7 @@ static uv_handle_t *ioreq_spawn(struct worker_ctx *worker, int socktype, sa_fami
 	if (!handle) {
 		return NULL;
 	}
-	int ret = io_create(worker->loop, handle, socktype, family);
+	int ret = io_create(worker->loop, handle, socktype, family, has_tls);
 	if (ret) {
 		if (ret == UV_EMFILE) {
 			worker->too_many_open = true;
@@ -941,7 +942,7 @@ static uv_handle_t *retransmit(struct qr_task *task)
 		if (kr_resolve_checkout(&ctx->req, NULL, (struct sockaddr *)choice, SOCK_DGRAM, task->pktbuf) != 0) {
 			return ret;
 		}
-		ret = ioreq_spawn(ctx->worker, SOCK_DGRAM, choice->sin6_family);
+		ret = ioreq_spawn(ctx->worker, SOCK_DGRAM, choice->sin6_family, false);
 		if (!ret) {
 			return ret;
 		}
@@ -1202,52 +1203,54 @@ static int tcp_task_existing_connection(struct session *session, struct qr_task
 	return kr_ok();
 }
 
-static int tcp_task_make_connection(struct session *session, struct qr_task *task,
-				    const struct sockaddr *addr /* , knot_pkt_t *packet */)
+static int tcp_task_make_connection(struct qr_task *task, const struct sockaddr *addr)
 {
 	struct request_ctx *ctx = task->ctx;
 	struct worker_ctx *worker = ctx->worker;
 
+	/* Check if there must be TLS */
+	struct engine *engine = worker->engine;
+	struct network *net = &engine->net;
+	const char *key = tcpsess_key(addr);
+	struct tls_client_ctx_t *tls_ctx = NULL;
+	struct tls_client_paramlist_entry *entry = map_get(&net->tls_client_params, key);
+	if (entry) {
+		/* Address is configured to be used with TLS.
+		 * We need to allocate auxiliary data structure. */
+		tls_ctx = tls_client_ctx_new(entry, worker);
+		if (!tls_ctx) {
+			return kr_error(EINVAL);
+		}
+	}
+
 	uv_connect_t *conn = malloc(sizeof(uv_connect_t));
 	if (!conn) {
+		tls_client_ctx_free(tls_ctx);
 		return kr_error(EINVAL);
 	}
-	uv_handle_t *client = ioreq_spawn(worker, SOCK_STREAM,
-						  addr->sa_family);
+	bool has_tls = (tls_ctx != NULL);
+	uv_handle_t *client = ioreq_spawn(worker, SOCK_STREAM, addr->sa_family, has_tls);
 	if (!client) {
+		tls_client_ctx_free(tls_ctx);
 		free(conn);
 		return kr_error(EINVAL);
 	}
-	session = client->data;
+	struct session *session = client->data;
+	assert(session_flags(session)->has_tls == has_tls);
+	if (has_tls) {
+		tls_client_ctx_set_session(tls_ctx, session);
+		session_tls_set_client_ctx(session, tls_ctx);
+	}
 
 	/* Add address to the waiting list.
 	 * Now it "is waiting to be connected to." */
 	int ret = worker_add_tcp_waiting(ctx->worker, addr, session);
 	if (ret < 0) {
 		free(conn);
+		session_close(session);
 		return kr_error(EINVAL);
 	}
 
-	/* Check if there must be TLS */
-	struct engine *engine = ctx->worker->engine;
-	struct network *net = &engine->net;
-	const char *key = tcpsess_key(addr);
-	struct tls_client_paramlist_entry *entry = map_get(&net->tls_client_params, key);
-	if (entry) {
-		/* Address is configured to be used with TLS.
-		 * We need to allocate auxiliary data structure. */
-		assert(session_tls_get_client_ctx(session) == NULL);
-		struct tls_client_ctx_t *tls_ctx = tls_client_ctx_new(entry, worker);
-		if (!tls_ctx) {
-			worker_del_tcp_waiting(ctx->worker, addr);
-			free(conn);
-			return kr_error(EINVAL);
-		}
-		tls_client_ctx_set_session(tls_ctx, session);
-		session_tls_set_client_ctx(session, tls_ctx);
-		session_flags(session)->has_tls = true;
-	}
-
 	conn->data = session;
 	/*  Store peer address for the session. */
 	struct sockaddr *peer = session_get_peer(session);
@@ -1259,6 +1262,7 @@ static int tcp_task_make_connection(struct session *session, struct qr_task *tas
 	if (ret != 0) {
 		worker_del_tcp_waiting(ctx->worker, addr);
 		free(conn);
+		session_close(session);
 		return kr_error(EINVAL);
 	}
 
@@ -1273,6 +1277,7 @@ static int tcp_task_make_connection(struct session *session, struct qr_task *tas
 		session_timer_stop(session);
 		worker_del_tcp_waiting(ctx->worker, addr);
 		free(conn);
+		session_close(session);
 		return kr_error(EAGAIN);
 	}
 
@@ -1283,6 +1288,7 @@ static int tcp_task_make_connection(struct session *session, struct qr_task *tas
 		session_timer_stop(session);
 		worker_del_tcp_waiting(ctx->worker, addr);
 		free(conn);
+		session_close(session);
 		return kr_error(EINVAL);
 	}
 
@@ -1321,7 +1327,7 @@ static int tcp_task_step(struct qr_task *task,
 		ret = tcp_task_existing_connection(session, task);
 	} else {
 		/* Make connection. */
-		ret = tcp_task_make_connection(session, task, addr);
+		ret = tcp_task_make_connection(task, addr);
 	}
 
 	if (ret != kr_ok()) {