From 8911c78b601d0515814db27abb60021467f65840 Mon Sep 17 00:00:00 2001 From: rootvector2 Date: Mon, 23 Mar 2026 19:40:32 +0530 Subject: [PATCH 1/3] enhance memory safety in DNS parser by adding bounds checks --- modules/network/dns/moddnsparser.c | 59 ++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/modules/network/dns/moddnsparser.c b/modules/network/dns/moddnsparser.c index 21588fca16..60a2d64c44 100644 --- a/modules/network/dns/moddnsparser.c +++ b/modules/network/dns/moddnsparser.c @@ -51,13 +51,19 @@ static void *getQuestionByIndex(xsMachine *the, uint8_t index) position += 12; while (index--) { while (true) { + if (position >= packetEnd) + return NULL; uint8_t tag = *position++; if (0 == tag) break; if (0xC0 == (tag & 0xC0)) { + if (position >= packetEnd) + return NULL; position += 1; break; } + if (position + tag > packetEnd) + return NULL; position += tag; } position += 4; @@ -75,6 +81,9 @@ static void *getAnswerByIndex(xsMachine *the, uint8_t index) uint16_t answers = ((position[6] << 8) | position[7]) + ((position[8] << 8) | position[9]) + ((position[10] << 8) | position[11]); // including authority and additional records position = getQuestionByIndex(the, 0xff); + if (NULL == position) + return NULL; + if ((0xff != index) && (index >= answers)) return NULL; @@ -85,13 +94,19 @@ static void *getAnswerByIndex(xsMachine *the, uint8_t index) uint16_t rdlength; while (true) { + if (position >= packetEnd) + return NULL; uint8_t tag = *position++; if (0 == tag) break; if (0xC0 == (tag & 0xC0)) { + if (position >= packetEnd) + return NULL; position += 1; break; } + if (position + tag > packetEnd) + return NULL; position += tag; } position += 10; @@ -107,16 +122,26 @@ static void *getAnswerByIndex(xsMachine *the, uint8_t index) return position; } +#define kDNSQnameMaxHops 128 + static int parseQname(xsMachine *the, int offset) { + uint8_t *packetEnd; + uint8_t *packetStart = getPacket(the, &packetEnd); + xsUnsignedValue packetLength = (xsUnsignedValue)(packetEnd - packetStart); uint8_t *position; int qnameEndPosition = 0; + int hops = 0; xsVar(1) = xsNewArray(0); - position = offset + (uint8_t *)getPacket(the, NULL);; + position = offset + packetStart; while (true) { char tmp[64]; - uint8_t tag = *position++; + uint8_t tag; + + if (position >= packetEnd) + xsUnknownError("bad name"); + tag = *position++; offset += 1; if (0 == tag) { if (0 == qnameEndPosition) @@ -124,20 +149,30 @@ static int parseQname(xsMachine *the, int offset) break; } if (0xC0 == (tag & 0xC0)) { + int ptr; + if (position >= packetEnd) + xsUnknownError("bad name"); if (0 == qnameEndPosition) qnameEndPosition = offset + 1; - offset = ((tag << 8) | *position) & 0x3FFF; //@@ range check - position = offset + (uint8_t *)getPacket(the, NULL); + ptr = ((tag << 8) | *position) & 0x3FFF; + if ((xsUnsignedValue)ptr >= packetLength) + xsUnknownError("bad name"); + if (++hops > kDNSQnameMaxHops) + xsUnknownError("bad name"); + offset = ptr; + position = offset + packetStart; continue; } if (tag > 63) xsUnknownError("bad name"); + if (position + tag > packetEnd) + xsUnknownError("bad name"); c_memcpy(tmp, position, tag); xsmcSetStringBuffer(xsVar(2), tmp, tag); xsCall1(xsVar(1), xsID_push, xsVar(2)); offset += tag; - position = offset + (uint8_t *)getPacket(the, NULL); + position = offset + packetStart; } return qnameEndPosition; @@ -243,14 +278,24 @@ static void parseQuestionOrAnswer(xsMachine *the, uint8_t *position, uint8_t ans else if (0x0010 == qtype) { // TXT if (rdlength > 1) { + uint8_t *rdataEnd; xsVar(1) = xsNewArray(0); position = offset + (uint8_t *)getPacket(the, NULL); + rdataEnd = position + rdlength; while (rdlength > 1) { uint8_t tag; + uint16_t advance; + if (position >= rdataEnd) + break; tag = *position++; - offset += tag + 1; //@@ bounds check range - rdlength -= tag + 1; + advance = (uint16_t)tag + 1; + if (advance > rdlength) + break; + if (position + tag > rdataEnd) + break; + offset += advance; + rdlength -= advance; c_memcpy(tmp, position, tag); xsmcSetStringBuffer(xsVar(2), tmp, tag); xsCall1(xsVar(1), xsID_push, xsVar(2)); From f53d437fd988066a089e7e0a9c71e9d6ed1fcf72 Mon Sep 17 00:00:00 2001 From: rootvector2 Date: Mon, 23 Mar 2026 22:20:38 +0530 Subject: [PATCH 2/3] address review: fix GC-unsafe pointer usage and consolidate loop bounds checks - Move xsNewArray() before getPacket() in parseQname so GC triggered by xsNewArray cannot invalidate the packetStart/packetEnd pointers - Re-fetch packetStart and packetEnd after xsmcSetStringBuffer and xsCall1 in parseQname loop since both can trigger GC and move memory - Consolidate inner name-label traversal from 'while (true)' with multiple early-return guards to 'while (position < packetEnd)' in both getQuestionByIndex and getAnswerByIndex, relying on the existing post-loop bounds check to catch malformed packets (per phoddie review) --- modules/network/dns/moddnsparser.c | 31 ++++++++++++------------------ 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/modules/network/dns/moddnsparser.c b/modules/network/dns/moddnsparser.c index 60a2d64c44..0d299d3a18 100644 --- a/modules/network/dns/moddnsparser.c +++ b/modules/network/dns/moddnsparser.c @@ -50,20 +50,14 @@ static void *getQuestionByIndex(xsMachine *the, uint8_t index) position += 12; while (index--) { - while (true) { - if (position >= packetEnd) - return NULL; + while (position < packetEnd) { uint8_t tag = *position++; if (0 == tag) break; if (0xC0 == (tag & 0xC0)) { - if (position >= packetEnd) - return NULL; position += 1; break; } - if (position + tag > packetEnd) - return NULL; position += tag; } position += 4; @@ -93,20 +87,14 @@ static void *getAnswerByIndex(xsMachine *the, uint8_t index) while (index--) { uint16_t rdlength; - while (true) { - if (position >= packetEnd) - return NULL; + while (position < packetEnd) { uint8_t tag = *position++; if (0 == tag) break; if (0xC0 == (tag & 0xC0)) { - if (position >= packetEnd) - return NULL; position += 1; break; } - if (position + tag > packetEnd) - return NULL; position += tag; } position += 10; @@ -127,13 +115,15 @@ static void *getAnswerByIndex(xsMachine *the, uint8_t index) static int parseQname(xsMachine *the, int offset) { uint8_t *packetEnd; - uint8_t *packetStart = getPacket(the, &packetEnd); - xsUnsignedValue packetLength = (xsUnsignedValue)(packetEnd - packetStart); + uint8_t *packetStart; + xsUnsignedValue packetLength; uint8_t *position; int qnameEndPosition = 0; int hops = 0; - xsVar(1) = xsNewArray(0); + xsVar(1) = xsNewArray(0); // do this before getPacket: xsNewArray can trigger GC + packetStart = getPacket(the, &packetEnd); + packetLength = (xsUnsignedValue)(packetEnd - packetStart); position = offset + packetStart; while (true) { char tmp[64]; @@ -169,9 +159,12 @@ static int parseQname(xsMachine *the, int offset) if (position + tag > packetEnd) xsUnknownError("bad name"); c_memcpy(tmp, position, tag); - xsmcSetStringBuffer(xsVar(2), tmp, tag); - xsCall1(xsVar(1), xsID_push, xsVar(2)); offset += tag; + xsmcSetStringBuffer(xsVar(2), tmp, tag); // can trigger GC + xsCall1(xsVar(1), xsID_push, xsVar(2)); // can trigger GC + // re-fetch packet pointer since GC may have moved memory + packetStart = getPacket(the, &packetEnd); + packetLength = (xsUnsignedValue)(packetEnd - packetStart); position = offset + packetStart; } From d2309a3b36a487f401ff1c674a8a7e7e5d8e4edc Mon Sep 17 00:00:00 2001 From: rootvector2 Date: Wed, 25 Mar 2026 21:06:28 +0530 Subject: [PATCH 3/3] enhance error handling in parseQuestionOrAnswer for invalid TXT records --- modules/network/dns/moddnsparser.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/network/dns/moddnsparser.c b/modules/network/dns/moddnsparser.c index 0d299d3a18..7cceb5ecb2 100644 --- a/modules/network/dns/moddnsparser.c +++ b/modules/network/dns/moddnsparser.c @@ -280,13 +280,13 @@ static void parseQuestionOrAnswer(xsMachine *the, uint8_t *position, uint8_t ans uint16_t advance; if (position >= rdataEnd) - break; + xsUnknownError("bad txt"); tag = *position++; advance = (uint16_t)tag + 1; if (advance > rdlength) - break; + xsUnknownError("bad txt"); if (position + tag > rdataEnd) - break; + xsUnknownError("bad txt"); offset += advance; rdlength -= advance; c_memcpy(tmp, position, tag);