From 6689e336042d2ca0b075f8db1fe30ed6b47c7d4c Mon Sep 17 00:00:00 2001
From: Simon Kelley <simon@thekelleys.org.uk>
Date: Wed, 11 Nov 2020 23:25:04 +0000
Subject: [PATCH 1/4] Fix remote buffer overflow CERT VU#434904
The problem is in the sort_rrset() function and allows a remote
attacker to overwrite memory. Any dnsmasq instance with DNSSEC
enabled is vulnerable.
---
src/dnssec.c | 273 ++++++++++++++++++++++++++++-----------------------
1 file changed, 152 insertions(+), 121 deletions(-)
diff --git a/src/dnssec.c b/src/dnssec.c
index 8143185..0c703ac 100644
--- a/src/dnssec.c
+++ b/src/dnssec.c
@@ -222,138 +222,147 @@ static int check_date_range(u32 date_start, u32 date_end)
&& serial_compare_32(curtime, date_end) == SERIAL_LT;
}
-/* Return bytes of canonicalised rdata, when the return value is zero, the remaining
- data, pointed to by *p, should be used raw. */
-static int get_rdata(struct dns_header *header, size_t plen, unsigned char *end, char *buff, int bufflen,
- unsigned char **p, u16 **desc)
+/* Return bytes of canonicalised rrdata one by one.
+ Init state->ip with the RR, and state->end with the end of same.
+ Init state->op to NULL.
+ Init state->desc to RR descriptor.
+ Init state->buff with a MAXDNAME * 2 buffer.
+
+ After each call which returns 1, state->op points to the next byte of data.
+ On returning 0, the end has been reached.
+*/
+struct rdata_state {
+ u16 *desc;
+ size_t c;
+ unsigned char *end, *ip, *op;
+ char *buff;
+};
+
+static int get_rdata(struct dns_header *header, size_t plen, struct rdata_state *state)
{
- int d = **desc;
+ int d;
- /* No more data needs mangling */
- if (d == (u16)-1)
+ if (state->op && state->c != 1)
{
- /* If there's more data than we have space for, just return what fits,
- we'll get called again for more chunks */
- if (end - *p > bufflen)
- {
- memcpy(buff, *p, bufflen);
- *p += bufflen;
- return bufflen;
- }
-
- return 0;
+ state->op++;
+ state->c--;
+ return 1;
}
-
- (*desc)++;
-
- if (d == 0 && extract_name(header, plen, p, buff, 1, 0))
- /* domain-name, canonicalise */
- return to_wire(buff);
- else
- {
- /* plain data preceding a domain-name, don't run off the end of the data */
- if ((end - *p) < d)
- d = end - *p;
+
+ while (1)
+ {
+ d = *(state->desc);
- if (d != 0)
+ if (d == (u16)-1)
{
- memcpy(buff, *p, d);
- *p += d;
+ /* all the bytes to the end. */
+ if ((state->c = state->end - state->ip) != 0)
+ {
+ state->op = state->ip;
+ state->ip = state->end;;
+ }
+ else
+ return 0;
+ }
+ else
+ {
+ state->desc++;
+
+ if (d == (u16)0)
+ {
+ /* domain-name, canonicalise */
+ int len;
+
+ if (!extract_name(header, plen, &state->ip, state->buff, 1, 0) ||
+ (len = to_wire(state->buff)) == 0)
+ continue;
+
+ state->c = len;
+ state->op = (unsigned char *)state->buff;
+ }
+ else
+ {
+ /* plain data preceding a domain-name, don't run off the end of the data */
+ if ((state->end - state->ip) < d)
+ d = state->end - state->ip;
+
+ if (d == 0)
+ continue;
+
+ state->op = state->ip;
+ state->c = d;
+ state->ip += d;
+ }
}
- return d;
+ return 1;
}
}
-/* Bubble sort the RRset into the canonical order.
- Note that the byte-streams from two RRs may get unsynced: consider
- RRs which have two domain-names at the start and then other data.
- The domain-names may have different lengths in each RR, but sort equal
-
- ------------
- |abcde|fghi|
- ------------
- |abcd|efghi|
- ------------
-
- leaving the following bytes as deciding the order. Hence the nasty left1 and left2 variables.
-*/
+/* Bubble sort the RRset into the canonical order. */
static int sort_rrset(struct dns_header *header, size_t plen, u16 *rr_desc, int rrsetidx,
unsigned char **rrset, char *buff1, char *buff2)
{
- int swap, quit, i, j;
+ int swap, i, j;
do
{
for (swap = 0, i = 0; i < rrsetidx-1; i++)
{
- int rdlen1, rdlen2, left1, left2, len1, len2, len, rc;
- u16 *dp1, *dp2;
- unsigned char *end1, *end2;
+ int rdlen1, rdlen2;
+ struct rdata_state state1, state2;
+
/* Note that these have been determined to be OK previously,
so we don't need to check for NULL return here. */
- unsigned char *p1 = skip_name(rrset[i], header, plen, 10);
- unsigned char *p2 = skip_name(rrset[i+1], header, plen, 10);
-
- p1 += 8; /* skip class, type, ttl */
- GETSHORT(rdlen1, p1);
- end1 = p1 + rdlen1;
-
- p2 += 8; /* skip class, type, ttl */
- GETSHORT(rdlen2, p2);
- end2 = p2 + rdlen2;
+ state1.ip = skip_name(rrset[i], header, plen, 10);
+ state2.ip = skip_name(rrset[i+1], header, plen, 10);
+ state1.op = state2.op = NULL;
+ state1.buff = buff1;
+ state2.buff = buff2;
+ state1.desc = state2.desc = rr_desc;
- dp1 = dp2 = rr_desc;
+ state1.ip += 8; /* skip class, type, ttl */
+ GETSHORT(rdlen1, state1.ip);
+ if (!CHECK_LEN(header, state1.ip, plen, rdlen1))
+ return rrsetidx; /* short packet */
+ state1.end = state1.ip + rdlen1;
- for (quit = 0, left1 = 0, left2 = 0, len1 = 0, len2 = 0; !quit;)
+ state2.ip += 8; /* skip class, type, ttl */
+ GETSHORT(rdlen2, state2.ip);
+ if (!CHECK_LEN(header, state2.ip, plen, rdlen2))
+ return rrsetidx; /* short packet */
+ state2.end = state2.ip + rdlen2;
+
+ while (1)
{
- if (left1 != 0)
- memmove(buff1, buff1 + len1 - left1, left1);
+ int ok1, ok2;
- if ((len1 = get_rdata(header, plen, end1, buff1 + left1, (MAXDNAME * 2) - left1, &p1, &dp1)) == 0)
- {
- quit = 1;
- len1 = end1 - p1;
- memcpy(buff1 + left1, p1, len1);
- }
- len1 += left1;
-
- if (left2 != 0)
- memmove(buff2, buff2 + len2 - left2, left2);
-
- if ((len2 = get_rdata(header, plen, end2, buff2 + left2, (MAXDNAME *2) - left2, &p2, &dp2)) == 0)
- {
- quit = 1;
- len2 = end2 - p2;
- memcpy(buff2 + left2, p2, len2);
- }
- len2 += left2;
-
- if (len1 > len2)
- left1 = len1 - len2, left2 = 0, len = len2;
- else
- left2 = len2 - len1, left1 = 0, len = len1;
-
- rc = (len == 0) ? 0 : memcmp(buff1, buff2, len);
-
- if (rc > 0 || (rc == 0 && quit && len1 > len2))
- {
- unsigned char *tmp = rrset[i+1];
- rrset[i+1] = rrset[i];
- rrset[i] = tmp;
- swap = quit = 1;
- }
- else if (rc == 0 && quit && len1 == len2)
+ ok1 = get_rdata(header, plen, &state1);
+ ok2 = get_rdata(header, plen, &state2);
+
+ if (!ok1 && !ok2)
{
/* Two RRs are equal, remove one copy. RFC 4034, para 6.3 */
for (j = i+1; j < rrsetidx-1; j++)
rrset[j] = rrset[j+1];
rrsetidx--;
i--;
+ break;
+ }
+ else if (ok1 && (!ok2 || *state1.op > *state2.op))
+ {
+ unsigned char *tmp = rrset[i+1];
+ rrset[i+1] = rrset[i];
+ rrset[i] = tmp;
+ swap = 1;
+ break;
}
- else if (rc < 0)
- quit = 1;
+ else if (ok2 && (!ok1 || *state2.op > *state1.op))
+ break;
+
+ /* arrive here when bytes are equal, go round the loop again
+ and compare the next ones. */
}
}
} while (swap);
@@ -549,15 +558,18 @@ static int validate_rrset(time_t now, struct dns_header *header, size_t plen, in
wire_len = to_wire(keyname);
hash->update(ctx, (unsigned int)wire_len, (unsigned char*)keyname);
from_wire(keyname);
+
+#define RRBUFLEN 300 /* Most RRs are smaller than this. */
for (i = 0; i < rrsetidx; ++i)
{
- int seg;
- unsigned char *end, *cp;
- u16 len, *dp;
+ int j;
+ struct rdata_state state;
+ u16 len;
+ unsigned char rrbuf[RRBUFLEN];
p = rrset[i];
-
+
if (!extract_name(header, plen, &p, name, 1, 10))
return STAT_BOGUS;
@@ -566,12 +578,11 @@ static int validate_rrset(time_t now, struct dns_header *header, size_t plen, in
/* if more labels than in RRsig name, hash *.<no labels in rrsig labels field> 4035 5.3.2 */
if (labels < name_labels)
{
- int k;
- for (k = name_labels - labels; k != 0; k--)
+ for (j = name_labels - labels; j != 0; j--)
{
while (*name_start != '.' && *name_start != 0)
name_start++;
- if (k != 1 && *name_start == '.')
+ if (j != 1 && *name_start == '.')
name_start++;
}
@@ -592,24 +603,44 @@ static int validate_rrset(time_t now, struct dns_header *header, size_t plen, in
if (!CHECK_LEN(header, p, plen, rdlen))
return STAT_BOGUS;
- end = p + rdlen;
+ /* canonicalise rdata and calculate length of same, use
+ name buffer as workspace for get_rdata. */
+ state.ip = p;
+ state.op = NULL;
+ state.desc = rr_desc;
+ state.buff = name;
+ state.end = p + rdlen;
- /* canonicalise rdata and calculate length of same, use name buffer as workspace.
- Note that name buffer is twice MAXDNAME long in DNSSEC mode. */
- cp = p;
- dp = rr_desc;
- for (len = 0; (seg = get_rdata(header, plen, end, name, MAXDNAME * 2, &cp, &dp)) != 0; len += seg);
- len += end - cp;
- len = htons(len);
+ for (j = 0; get_rdata(header, plen, &state); j++)
+ if (j < RRBUFLEN)
+ rrbuf[j] = *state.op;
+
+ len = htons((u16)j);
hash->update(ctx, 2, (unsigned char *)&len);
+
+ /* If the RR is shorter than RRBUFLEN (most of them, in practice)
+ then we can just digest it now. If it exceeds RRBUFLEN we have to
+ go back to the start and do it in chunks. */
+ if (j >= RRBUFLEN)
+ {
+ state.ip = p;
+ state.op = NULL;
+ state.desc = rr_desc;
+
+ for (j = 0; get_rdata(header, plen, &state); j++)
+ {
+ rrbuf[j] = *state.op;
+
+ if (j == RRBUFLEN - 1)
+ {
+ hash->update(ctx, RRBUFLEN, rrbuf);
+ j = -1;
+ }
+ }
+ }
- /* Now canonicalise again and digest. */
- cp = p;
- dp = rr_desc;
- while ((seg = get_rdata(header, plen, end, name, MAXDNAME * 2, &cp, &dp)))
- hash->update(ctx, seg, (unsigned char *)name);
- if (cp != end)
- hash->update(ctx, end - cp, cp);
+ if (j != 0)
+ hash->update(ctx, j, rrbuf);
}
hash->digest(ctx, hash->digest_size, digest);
--
2.26.2