[PATCH gwproxy v3 6/9] dnsparser: Fix serialize_answ function

[Ahmad Gani <reyuki@gnuweeb.org>]

This changes ensure memory read access is always valid. It fixes a logic
bug in serialize_answ that can lead to a memory error: invalid read.

Use array indexing instead of pointer arithmetic.

It also changes the handling of variable-length fields received from the
network payload. Because the payload originates from an external source,
we avoid trusting it blindly and return EINVAL instead of EAGAIN.

If EAGAIN is encountered, it's supposed to resend the request, but
the current implementation hasn't done it yet.

return EINVAL if calculate_question_len == 0

Signed-off-by: Ahmad Gani <reyuki@gnuweeb.org>
---
 src/gwproxy/dnsparser.c | 91 +++++++++++++++++++++++++++--------------
 1 file changed, 60 insertions(+), 31 deletions(-)

diff --git a/src/gwproxy/dnsparser.c b/src/gwproxy/dnsparser.c
index f65450fa922c..744dc61581ef 100644
--- a/src/gwproxy/dnsparser.c
+++ b/src/gwproxy/dnsparser.c
@@ -45,8 +45,10 @@ static ssize_t calculate_question_len(uint8_t *in, size_t in_len)
 
 	tot_len = 0;
 	while (true) {
-		if (*p == 0x0)
+		if (*p == 0x0) {
+			tot_len++;
 			break;
+		}
 
 		if (tot_len >= in_len)
 			return -ENAMETOOLONG;
@@ -56,25 +58,27 @@ static ssize_t calculate_question_len(uint8_t *in, size_t in_len)
 		p += advance_len;
 	}
 
+	tot_len += 4;
+
 	return  tot_len;
 }
 
 int serialize_answ(uint16_t txid, uint8_t *in, size_t in_len, gwdns_answ_data *out)
 {
-	size_t advance_len, first_len;
+	size_t idx;
 	gwdns_header_pkt *hdr;
 	uint16_t raw_flags;
 	int ret;
 
-	advance_len = sizeof(*hdr);
-	if (in_len < advance_len)
+	idx = sizeof(*hdr);
+	if (idx >= in_len)
 		return -EAGAIN;
 
 	hdr = (void *)in;
 	if (memcmp(&txid, &hdr->id, sizeof(txid)))
 		return -EINVAL;
 
-	memcpy(&raw_flags, &in[2], sizeof(raw_flags));
+	memcpy(&raw_flags, &hdr->flags, sizeof(raw_flags));
 	raw_flags = ntohs(raw_flags);
 	/* QR MUST 1 = response from dns server */
 	if (!DNS_QR(raw_flags))
@@ -93,25 +97,26 @@ int serialize_answ(uint16_t txid, uint8_t *in, size_t in_len, gwdns_answ_data *o
 	if (!hdr->ancount)
 		return -ENODATA;
 
-	in += advance_len;
-	in_len -= advance_len;
-
-	first_len = 1 + in[0];
-	advance_len = first_len + 1 + 2 + 2;
-	if (in_len < advance_len)
-		return -EAGAIN;
+	/*
+	 * Check the sizes upfront.
+	 *
+	 * 1 bytes for variable-length
+	 * in[idx] for the length of first name
+	 * 1 bytes for null terminator
+	 * 2 bytes for qtype
+	 * 2 bytes for qclass
+	 */
+	if ((size_t)(1 + in[idx] + 1 + 2 + 2) >= in_len)
+		return -EINVAL;
 
-	ret = calculate_question_len(in, in_len);
-	if (ret < 0)
+	ret = calculate_question_len(&in[idx], in_len - idx);
+	if (ret <= 0)
 		return -EINVAL;
 
-	advance_len -= first_len;
-	advance_len += ret;
-	if (in_len < advance_len)
+	idx += ret;
+	if (idx >= in_len)
 		return -EAGAIN;
 
-	in += advance_len;
-	in_len -= advance_len;
 	out->hdr.ancount = 0;
 	out->rr_answ = malloc(hdr->ancount * sizeof(uint8_t *));
 	if (!out->rr_answ)
@@ -127,22 +132,38 @@ int serialize_answ(uint16_t txid, uint8_t *in, size_t in_len, gwdns_answ_data *o
 
 		out->rr_answ[i] = item;
 
-		memcpy(&is_compressed, in, sizeof(is_compressed));
+		memcpy(&is_compressed, &in[idx], sizeof(is_compressed));
 		is_compressed = DNS_IS_COMPRESSED(ntohs(is_compressed));
 		assert(is_compressed);
-		in += 2; // NAME
+		idx += 2; // NAME
+		if (idx >= in_len) {
+			ret = -EAGAIN;
+			goto exit_free;
+		}
 
-		memcpy(&item->rr_type, in, 2);
+		memcpy(&item->rr_type, &in[idx], 2);
 		item->rr_type = ntohs(item->rr_type);
-		in += 2; // TYPE
-		memcpy(&item->rr_class, in, 2);
+		idx += 2; // TYPE
+		if (idx >= in_len) {
+			ret = -EAGAIN;
+			goto exit_free;
+		}
+		memcpy(&item->rr_class, &in[idx], 2);
 		item->rr_class = ntohs(item->rr_class);
-		in += 2; // CLASS
-		memcpy(&item->ttl, in, 4);
+		idx += 2; // CLASS
+		if (idx >= in_len) {
+			ret = -EAGAIN;
+			goto exit_free;
+		}
+		memcpy(&item->ttl, &in[idx], 4);
 		item->ttl = be32toh(item->ttl);
-		in += 4; // TTL
+		idx += 4; // TTL
+		if (idx >= in_len) {
+			ret = -EAGAIN;
+			goto exit_free;
+		}
 
-		memcpy(&rdlength, in, sizeof(rdlength));
+		memcpy(&rdlength, &in[idx], sizeof(rdlength));
 		rdlength = ntohs(rdlength);
 		if (item->rr_type != TYPE_AAAA && item->rr_type != TYPE_A) {
 			ret = -EINVAL;
@@ -160,7 +181,11 @@ int serialize_answ(uint16_t txid, uint8_t *in, size_t in_len, gwdns_answ_data *o
 			goto exit_free;
 		}
 		item->rdlength = rdlength;
-		in += 2;
+		idx += 2;
+		if (idx >= in_len) {
+			ret = -EAGAIN;
+			goto exit_free;
+		}
 
 		/*
 		 * considering if condition above,
@@ -175,8 +200,12 @@ int serialize_answ(uint16_t txid, uint8_t *in, size_t in_len, gwdns_answ_data *o
 			free(item);
 			goto exit_free;
 		}
-		memcpy(item->rdata, in, rdlength);
-		in += rdlength;
+		memcpy(item->rdata, &in[idx], rdlength);
+		idx += rdlength;
+		if (idx > in_len) {
+			ret = -EINVAL;
+			goto exit_free;
+		}
 		out->hdr.ancount++;
 	}
 
-- 
Ahmad Gani