From: Vladimír Čunát <vladimir.cunat@nic.cz>
Date: Mon, 13 May 2019 14:46:48 +0000 (+0200)
Subject: daemon TCP to upstream: don't send wrong message length
X-Git-Tag: v4.1.0~26^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=10a113d7d4ef3188dfcb03b92e9e41111bc77343;p=thirdparty%2Fknot-resolver.git

daemon TCP to upstream: don't send wrong message length

See the added comments.  Such bugs are tricky, because the old code
would typically work just fine, only if libuv/OS decided to postpone
copying the data (perhaps large load), we would send two bytes from
this address on C stack - their later value (hard to predict what).

Security risks: the two bytes might theoretically contain information
that was more or less private and we just send it to some DNS server
(possibly over unencrypted TCP), but ATM I find it very unlikely that
this bug could be practically exploited.
---

diff --git a/NEWS b/NEWS
index 16322916b..bc660714b 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,11 @@
+Knot Resolver 4.x.y (2019-0m-dd)
+================================
+
+Bugfixes
+--------
+- TCP to upstream: don't send wrong message length (unlikely, !816)
+
+
 Knot Resolver 4.0.0 (2019-04-18)
 ================================
 
diff --git a/daemon/worker.c b/daemon/worker.c
index c508b0de8..5858912c8 100644
--- a/daemon/worker.c
+++ b/daemon/worker.c
@@ -626,13 +626,28 @@ static int qr_task_send(struct qr_task *task, struct session *session,
 		ret = uv_udp_send(send_req, (uv_udp_t *)handle, &buf, 1, addr, &on_send);
 	} else if (handle->type == UV_TCP) {
 		uv_write_t *write_req = (uv_write_t *)ioreq;
-		uint16_t pkt_size = htons(pkt->size);
-		uv_buf_t buf[2] = {
-			{ (char *)&pkt_size, sizeof(pkt_size) },
-			{ (char *)pkt->wire, pkt->size }
+		/* We need to write message length in native byte order,
+		 * but we don't have a convenient place to store those bytes.
+		 * The problem is that all memory referenced from buf[] MUST retain
+		 * its contents at least until on_write() is called, and I currently
+		 * can't see any convenient place outside the `pkt` structure.
+		 * So we use directly the *individual* bytes in pkt->size.
+		 * The call to htonl() and the condition will probably be inlinable. */
+		int lsbi, slsbi; /* (second) least significant byte index */
+		if (htonl(1) == 1) { /* big endian */
+			lsbi  = sizeof(pkt->size) - 1;
+			slsbi = sizeof(pkt->size) - 2;
+		} else {
+			lsbi  = 0;
+			slsbi = 1;
+		}
+		uv_buf_t buf[3] = {
+			{ (char *)&pkt->size + slsbi, 1 },
+			{ (char *)&pkt->size + lsbi,  1 },
+			{ (char *)pkt->wire, pkt->size },
 		};
 		write_req->data = task;
-		ret = uv_write(write_req, (uv_stream_t *)handle, buf, 2, &on_write);
+		ret = uv_write(write_req, (uv_stream_t *)handle, buf, 3, &on_write);
 	} else {
 		assert(false);
 	}