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 16322916bfaee1c7b894f4a93f97e5196cd32b24..bc660714b1d71a6bb36118a6d234f90114c1665a 100644 (file)
--- 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 c508b0de8a32a19836675d7f86e869f61e120dee..5858912c8db8986f35511d6fd2a7b691860b5613 100644 (file)
--- 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);
        }