From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server-vie001.gnuweeb.org X-Spam-Level: X-Spam-Status: No, score=-1.2 required=5.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,URIBL_ZEN_BLOCKED_OPENDNS autolearn=ham autolearn_force=no version=3.4.6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gnuweeb.org; s=new2025; t=1754376640; bh=pYNbXKa0s940jIWWASQpkVPkBaKzskuZ3CEJaD1udzg=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Transfer-Encoding:Message-ID:Date:From: Reply-To:Subject:To:Cc:In-Reply-To:References:Resent-Date: Resent-From:Resent-To:Resent-Cc:User-Agent:Content-Type: Content-Transfer-Encoding; b=Fa2KiQGkxDL9qVopydOVhRsbyI8jOzxZfPWL9s516reFPQhYcXAbf142fHApzK+mw zb1IrYeQEN0i5Pf/uhVfuPVPPfi410AVEu/GFAxtfyowdRlWor/8z1PeZ4y7K2sYWH KcFnQRgb6Bcje0DYBxDaMWAUNs3KEFg9ocP7M16jY9AUHXX2CzwD4rw8O16VZfapzE K8BEEIdypN3+0q8Ku6j8JviI1Oc9n+vJqvjSr4W47VcUwBaqcIrz5El/gRualzmzBZ W930Rd05PrtrEyALuihQrgQUpF+fMeBHtlbw70tzSeraUeOtipiKrbhfjcnvOI4hSB L1TKtwqur1e8w== Received: from zero (unknown [182.253.151.158]) by server-vie001.gnuweeb.org (Postfix) with ESMTPSA id 53265312801E; Tue, 5 Aug 2025 06:50:39 +0000 (UTC) From: Ahmad Gani To: Ammar Faizi Cc: Ahmad Gani , Alviro Iskandar Setiawan , GNU/Weeb Mailing List Subject: [PATCH gwproxy v3 6/9] dnsparser: Fix serialize_answ function Date: Tue, 5 Aug 2025 13:49:26 +0700 Message-ID: <20250805064933.109080-7-reyuki@gnuweeb.org> X-Mailer: git-send-email 2.50.1 In-Reply-To: <20250805064933.109080-1-reyuki@gnuweeb.org> References: <20250805064933.109080-1-reyuki@gnuweeb.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: 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 --- 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