From 1033130b6c506f2dde790ce3939f57c57de06afa Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Sun, 9 Jan 2022 23:21:55 +0000 Subject: [PATCH] Handle malformed query packets sensibly. Previously, hash_questions() would return a random hash if the packet was malformed, and probably the hash of a previous query. Now handle this as an error. --- src/forward.c | 74 +++++++++++++++++++++++++++----------------- src/hash-questions.c | 8 ++--- 2 files changed, 49 insertions(+), 33 deletions(-) diff --git a/src/forward.c b/src/forward.c index f22c080..51f7ccb 100644 --- a/src/forward.c +++ b/src/forward.c @@ -272,6 +272,15 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr, /* new query */ if (!forward) { + /* If the query is malformed, we can't forward it because + we can't get a reliable hash to recognise the answer. */ + if (!hash) + { + flags = 0; + ede = EDE_INVALID_DATA; + goto reply; + } + if (lookup_domain(daemon->namebuff, gotname, &first, &last)) flags = is_local_answer(now, first, daemon->namebuff); else @@ -865,8 +874,10 @@ static void dnssec_validate(struct frec *forward, struct dns_header *header, STAT_ISEQUAL(status, STAT_NEED_KEY) ? T_DNSKEY : T_DS, server->edns_pktsz); flags = STAT_ISEQUAL(status, STAT_NEED_KEY) ? FREC_DNSKEY_QUERY : FREC_DS_QUERY; - hash = hash_questions(header, nn, daemon->namebuff); + if (!(hash = hash_questions(header, nn, daemon->namebuff))) + return; + if ((new = lookup_frec_by_query(hash, flags, FREC_DNSKEY_QUERY | FREC_DS_QUERY))) { forward->next_dependent = new->dependent; @@ -1694,13 +1705,16 @@ static ssize_t tcp_talk(int first, int last, int start, unsigned char *packet, unsigned char *payload = &packet[2]; struct dns_header *header = (struct dns_header *)payload; unsigned char c1, c2; - unsigned char hash[HASH_SIZE]; + unsigned char hash[HASH_SIZE], *hashp; unsigned int rsize; (void)mark; (void)have_mark; - memcpy(hash, hash_questions(header, (unsigned int)qsize, daemon->namebuff), HASH_SIZE); + if (!(hashp = hash_questions(header, (unsigned int)qsize, daemon->namebuff))) + return 0; + + memcpy(hash, hashp, HASH_SIZE); while (1) { @@ -1781,7 +1795,7 @@ static ssize_t tcp_talk(int first, int last, int start, unsigned char *packet, someone might be attempting to insert bogus values into the cache by sending replies containing questions and bogus answers. Try another server, or give up */ - if (memcmp(hash, hash_questions(header, rsize, daemon->namebuff), HASH_SIZE) != 0) + if (!(hashp = hash_questions(header, rsize, daemon->namebuff)) || memcmp(hash, hashp, HASH_SIZE) != 0) continue; serv->flags |= SERV_GOT_TCP; @@ -2546,28 +2560,29 @@ static struct frec *lookup_frec(unsigned short id, int fd, void *hash, int *firs struct server *s; int first, last; struct randfd_list *fdl; - - for(f = daemon->frec_list; f; f = f->next) - if (f->sentto && f->new_id == id && - (memcmp(hash, f->hash, HASH_SIZE) == 0)) - { - filter_servers(f->sentto->arrayposn, F_SERVER, firstp, lastp); - /* sent from random port */ - for (fdl = f->rfds; fdl; fdl = fdl->next) - if (fdl->rfd->fd == fd) - return f; - - /* Sent to upstream from socket associated with a server. - Note we have to iterate over all the possible servers, since they may - have different bound sockets. */ - for (first = *firstp, last = *lastp; first != last; first++) - { - s = daemon->serverarray[first]; - if (s->sfd && s->sfd->fd == fd) + if (hash) + for (f = daemon->frec_list; f; f = f->next) + if (f->sentto && f->new_id == id && + (memcmp(hash, f->hash, HASH_SIZE) == 0)) + { + filter_servers(f->sentto->arrayposn, F_SERVER, firstp, lastp); + + /* sent from random port */ + for (fdl = f->rfds; fdl; fdl = fdl->next) + if (fdl->rfd->fd == fd) return f; - } - } + + /* Sent to upstream from socket associated with a server. + Note we have to iterate over all the possible servers, since they may + have different bound sockets. */ + for (first = *firstp, last = *lastp; first != last; first++) + { + s = daemon->serverarray[first]; + if (s->sfd && s->sfd->fd == fd) + return f; + } + } return NULL; } @@ -2576,11 +2591,12 @@ static struct frec *lookup_frec_by_query(void *hash, unsigned int flags, unsigne { struct frec *f; - for(f = daemon->frec_list; f; f = f->next) - if (f->sentto && - (f->flags & flagmask) == flags && - memcmp(hash, f->hash, HASH_SIZE) == 0) - return f; + if (hash) + for(f = daemon->frec_list; f; f = f->next) + if (f->sentto && + (f->flags & flagmask) == flags && + memcmp(hash, f->hash, HASH_SIZE) == 0) + return f; return NULL; } diff --git a/src/hash-questions.c b/src/hash-questions.c index f41023b..4b1dd91 100644 --- a/src/hash-questions.c +++ b/src/hash-questions.c @@ -55,7 +55,7 @@ unsigned char *hash_questions(struct dns_header *header, size_t plen, char *name char *cp, c; if (!extract_name(header, plen, &p, name, 1, 4)) - break; /* bad packet */ + return NULL; /* bad packet */ for (cp = name; (c = *cp); cp++) if (c >= 'A' && c <= 'Z') @@ -67,7 +67,7 @@ unsigned char *hash_questions(struct dns_header *header, size_t plen, char *name p += 4; if (!CHECK_LEN(header, p, plen, 0)) - break; /* bad packet */ + return NULL; /* bad packet */ } hash->digest(ctx, hash->digest_size, digest); @@ -109,7 +109,7 @@ unsigned char *hash_questions(struct dns_header *header, size_t plen, char *name char *cp, c; if (!extract_name(header, plen, &p, name, 1, 4)) - break; /* bad packet */ + return NULL; /* bad packet */ for (cp = name; (c = *cp); cp++) if (c >= 'A' && c <= 'Z') @@ -121,7 +121,7 @@ unsigned char *hash_questions(struct dns_header *header, size_t plen, char *name p += 4; if (!CHECK_LEN(header, p, plen, 0)) - break; /* bad packet */ + return NULL; /* bad packet */ } sha256_final(&ctx, digest);