From b09d07ee2aa891c5b8dd2469c4a73c9dd61e2384 Mon Sep 17 00:00:00 2001
From: Simon Kelley <simon@thekelleys.org.uk>
Date: Wed, 25 Oct 2017 17:48:19 +0100
Subject: [PATCH 1/2] Fix caching logic for validated answers.
The current logic is naive in the case that there is more than
one RRset in an answer (Typically, when a non-CNAME query is answered
by one or more CNAME RRs, and then then an answer RRset.)
If all the RRsets validate, then they are cached and marked as validated,
but if any RRset doesn't validate, then the AD flag is not set (good) and
ALL the RRsets are cached marked as not validated.
This breaks when, eg, the answer contains a validated CNAME, pointing
to a non-validated answer. A subsequent query for the CNAME without do
will get an answer with the AD flag wrongly reset, and worse, the same
query with do will get a cached answer without RRSIGS, rather than
being forwarded.
The code now records the validation of individual RRsets and that
is used to correctly set the "validated" bits in the cache entries.
(cherry picked from commit a6004d7f17687ac2455f724d0b57098c413f128d)
---
src/dnsmasq.c | 2 +
src/dnsmasq.h | 5 +-
src/dnssec.c | 174 +++++++++++++++++++++++++++++---------------------
src/forward.c | 19 ++++--
src/rfc1035.c | 58 ++++++++++++-----
5 files changed, 162 insertions(+), 96 deletions(-)
diff --git a/src/dnsmasq.c b/src/dnsmasq.c
index 50b2029..f3d2671 100644
--- a/src/dnsmasq.c
+++ b/src/dnsmasq.c
@@ -118,6 +118,8 @@ int main (int argc, char **argv)
daemon->namebuff = safe_malloc(MAXDNAME * 2);
daemon->keyname = safe_malloc(MAXDNAME * 2);
daemon->workspacename = safe_malloc(MAXDNAME * 2);
+ /* one char flag per possible RR in answer section. */
+ daemon->rr_status = safe_malloc(256);
}
#endif
diff --git a/src/dnsmasq.h b/src/dnsmasq.h
index 5a68162..89d138a 100644
--- a/src/dnsmasq.h
+++ b/src/dnsmasq.h
@@ -1006,6 +1006,7 @@ extern struct daemon {
#ifdef HAVE_DNSSEC
char *keyname; /* MAXDNAME size buffer */
char *workspacename; /* ditto */
+ char *rr_status; /* 256 bytes as flags for individual RRs */
#endif
unsigned int local_answer, queries_forwarded, auth_answer;
struct frec *frec_list;
@@ -1118,7 +1119,7 @@ size_t setup_reply(struct dns_header *header, size_t qlen,
unsigned long local_ttl);
int extract_addresses(struct dns_header *header, size_t qlen, char *namebuff,
time_t now, char **ipsets, int is_sign, int checkrebind,
- int no_cache, int secure, int *doctored);
+ int no_cache_dnssec, int secure, int *doctored, char *rr_status);
size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
struct in_addr local_addr, struct in_addr local_netmask,
time_t now, int ad_reqd, int do_bit, int have_pseudoheader);
@@ -1151,7 +1152,7 @@ size_t dnssec_generate_query(struct dns_header *header, unsigned char *end, char
int dnssec_validate_by_ds(time_t now, struct dns_header *header, size_t n, char *name, char *keyname, int class);
int dnssec_validate_ds(time_t now, struct dns_header *header, size_t plen, char *name, char *keyname, int class);
int dnssec_validate_reply(time_t now, struct dns_header *header, size_t plen, char *name, char *keyname, int *class,
- int check_unsigned, int *neganswer, int *nons);
+ int check_unsigned, int *neganswer, int *nons, char *rr_status);
int dnskey_keytag(int alg, int flags, unsigned char *rdata, int rdlen);
size_t filter_rrsigs(struct dns_header *header, size_t plen);
unsigned char* hash_questions(struct dns_header *header, size_t plen, char *name);
diff --git a/src/dnssec.c b/src/dnssec.c
index f45c804..3121eb1 100644
--- a/src/dnssec.c
+++ b/src/dnssec.c
@@ -1177,8 +1177,7 @@ int dnssec_validate_ds(time_t now, struct dns_header *header, size_t plen, char
if (qtype != T_DS || qclass != class)
rc = STAT_BOGUS;
else
- rc = dnssec_validate_reply(now, header, plen, name, keyname, NULL, 0, &neganswer, &nons);
- /* Note dnssec_validate_reply() will have cached positive answers */
+ rc = dnssec_validate_reply(now, header, plen, name, keyname, NULL, 0, &neganswer, &nons, NULL);
if (rc == STAT_INSECURE)
rc = STAT_BOGUS;
@@ -1962,18 +1961,25 @@ static int zone_status(char *name, int class, char *keyname, time_t now)
STAT_INSECURE at least one RRset not validated, because in unsigned zone.
STAT_BOGUS signature is wrong, bad packet, no validation where there should be.
STAT_NEED_KEY need DNSKEY to complete validation (name is returned in keyname, class in *class)
- STAT_NEED_DS need DS to complete validation (name is returned in keyname)
+ STAT_NEED_DS need DS to complete validation (name is returned in keyname)
+
+ If non-NULL, rr_status points to a char array which corressponds to the RRs in the
+ answer section (only). This is set to 1 for each RR which is validated, and 0 for any which aren't.
*/
int dnssec_validate_reply(time_t now, struct dns_header *header, size_t plen, char *name, char *keyname,
- int *class, int check_unsigned, int *neganswer, int *nons)
+ int *class, int check_unsigned, int *neganswer, int *nons, char *rr_status)
{
static unsigned char **targets = NULL;
static int target_sz = 0;
unsigned char *ans_start, *p1, *p2;
- int type1, class1, rdlen1, type2, class2, rdlen2, qclass, qtype, targetidx;
+ int type1, class1, rdlen1 = 0, type2, class2, rdlen2, qclass, qtype, targetidx;
int i, j, rc;
+ int secure = STAT_SECURE;
+ if (rr_status)
+ memset(rr_status, 0, ntohs(header->ancount));
+
if (neganswer)
*neganswer = 0;
@@ -2030,7 +2036,10 @@ int dnssec_validate_reply(time_t now, struct dns_header *header, size_t plen, ch
for (p1 = ans_start, i = 0; i < ntohs(header->ancount) + ntohs(header->nscount); i++)
{
- if (!extract_name(header, plen, &p1, name, 1, 10))
+ if (i != 0 && !ADD_RDLEN(header, p1, plen, rdlen1))
+ return STAT_BOGUS;
+
+ if (!extract_name(header, plen, &p1, name, 1, 10))
return STAT_BOGUS; /* bad packet */
GETSHORT(type1, p1);
@@ -2039,106 +2048,125 @@ int dnssec_validate_reply(time_t now, struct dns_header *header, size_t plen, ch
GETSHORT(rdlen1, p1);
/* Don't try and validate RRSIGs! */
- if (type1 != T_RRSIG)
+ if (type1 == T_RRSIG)
+ continue;
+
+ /* Check if we've done this RRset already */
+ for (p2 = ans_start, j = 0; j < i; j++)
{
- /* Check if we've done this RRset already */
- for (p2 = ans_start, j = 0; j < i; j++)
- {
- if (!(rc = extract_name(header, plen, &p2, name, 0, 10)))
- return STAT_BOGUS; /* bad packet */
-
- GETSHORT(type2, p2);
- GETSHORT(class2, p2);
- p2 += 4; /* TTL */
- GETSHORT(rdlen2, p2);
-
- if (type2 == type1 && class2 == class1 && rc == 1)
- break; /* Done it before: name, type, class all match. */
-
- if (!ADD_RDLEN(header, p2, plen, rdlen2))
- return STAT_BOGUS;
- }
+ if (!(rc = extract_name(header, plen, &p2, name, 0, 10)))
+ return STAT_BOGUS; /* bad packet */
+
+ GETSHORT(type2, p2);
+ GETSHORT(class2, p2);
+ p2 += 4; /* TTL */
+ GETSHORT(rdlen2, p2);
+
+ if (type2 == type1 && class2 == class1 && rc == 1)
+ break; /* Done it before: name, type, class all match. */
+ if (!ADD_RDLEN(header, p2, plen, rdlen2))
+ return STAT_BOGUS;
+ }
+
+ if (j != i)
+ {
+ /* Done already: copy the validation status */
+ if (rr_status && (i < ntohs(header->ancount)))
+ rr_status[i] = rr_status[j];
+ }
+ else
+ {
/* Not done, validate now */
- if (j == i)
+ int sigcnt, rrcnt;
+ char *wildname;
+
+ if (!explore_rrset(header, plen, class1, type1, name, keyname, &sigcnt, &rrcnt))
+ return STAT_BOGUS;
+
+ /* No signatures for RRset. We can be configured to assume this is OK and return a INSECURE result. */
+ if (sigcnt == 0)
{
- int sigcnt, rrcnt;
- char *wildname;
-
- if (!explore_rrset(header, plen, class1, type1, name, keyname, &sigcnt, &rrcnt))
- return STAT_BOGUS;
-
- /* No signatures for RRset. We can be configured to assume this is OK and return a INSECURE result. */
- if (sigcnt == 0)
+ if (check_unsigned)
{
- if (check_unsigned)
- {
- rc = zone_status(name, class1, keyname, now);
- if (rc == STAT_SECURE)
- rc = STAT_BOGUS;
- if (class)
- *class = class1; /* Class for NEED_DS or NEED_KEY */
- }
- else
- rc = STAT_INSECURE;
-
- return rc;
+ rc = zone_status(name, class1, keyname, now);
+ if (rc == STAT_SECURE)
+ rc = STAT_BOGUS;
+ if (class)
+ *class = class1; /* Class for NEED_DS or NEED_KEY */
}
+ else
+ rc = STAT_INSECURE;
+ if (rc != STAT_INSECURE)
+ return rc;
+ }
+ else
+ {
/* explore_rrset() gives us key name from sigs in keyname.
Can't overwrite name here. */
strcpy(daemon->workspacename, keyname);
rc = zone_status(daemon->workspacename, class1, keyname, now);
-
- if (rc != STAT_SECURE)
+
+ if (rc == STAT_BOGUS || rc == STAT_NEED_KEY || rc == STAT_NEED_DS)
{
/* Zone is insecure, don't need to validate RRset */
if (class)
*class = class1; /* Class for NEED_DS or NEED_KEY */
return rc;
- }
-
- rc = validate_rrset(now, header, plen, class1, type1, sigcnt, rrcnt, name, keyname, &wildname, NULL, 0, 0, 0);
+ }
- if (rc == STAT_BOGUS || rc == STAT_NEED_KEY || rc == STAT_NEED_DS)
- {
- if (class)
- *class = class1; /* Class for DS or DNSKEY */
- return rc;
- }
- else
+ /* Zone is insecure, don't need to validate RRset */
+ if (rc == STAT_SECURE)
{
+ rc = validate_rrset(now, header, plen, class1, type1, sigcnt,
+ rrcnt, name, keyname, &wildname, NULL, 0, 0, 0);
+
+ if (rc == STAT_BOGUS || rc == STAT_NEED_KEY || rc == STAT_NEED_DS)
+ {
+ if (class)
+ *class = class1; /* Class for DS or DNSKEY */
+ return rc;
+ }
+
/* rc is now STAT_SECURE or STAT_SECURE_WILDCARD */
-
+
+ /* Note that RR is validated */
+ if (rr_status && (i < ntohs(header->ancount)))
+ rr_status[i] = 1;
+
/* Note if we've validated either the answer to the question
or the target of a CNAME. Any not noted will need NSEC or
to be in unsigned space. */
-
for (j = 0; j <targetidx; j++)
if ((p2 = targets[j]))
{
- if (!(rc = extract_name(header, plen, &p2, name, 0, 10)))
+ int rc1;
+ if (!(rc1 = extract_name(header, plen, &p2, name, 0, 10)))
return STAT_BOGUS; /* bad packet */
- if (class1 == qclass && rc == 1 && (type1 == T_CNAME || type1 == qtype || qtype == T_ANY ))
+ if (class1 == qclass && rc1 == 1 && (type1 == T_CNAME || type1 == qtype || qtype == T_ANY ))
targets[j] = NULL;
}
-
- /* An attacker replay a wildcard answer with a different
- answer and overlay a genuine RR. To prove this
- hasn't happened, the answer must prove that
- the gennuine record doesn't exist. Check that here.
- Note that we may not yet have validated the NSEC/NSEC3 RRsets.
- That's not a problem since if the RRsets later fail
- we'll return BOGUS then. */
- if (rc == STAT_SECURE_WILDCARD && !prove_non_existence(header, plen, keyname, name, type1, class1, wildname, NULL))
+
+ /* An attacker replay a wildcard answer with a different
+ answer and overlay a genuine RR. To prove this
+ hasn't happened, the answer must prove that
+ the genuine record doesn't exist. Check that here.
+ Note that we may not yet have validated the NSEC/NSEC3 RRsets.
+ That's not a problem since if the RRsets later fail
+ we'll return BOGUS then. */
+ if (rc == STAT_SECURE_WILDCARD &&
+ !prove_non_existence(header, plen, keyname, name, type1, class1, wildname, NULL))
return STAT_BOGUS;
+
+ rc = STAT_SECURE;
}
}
}
- if (!ADD_RDLEN(header, p1, plen, rdlen1))
- return STAT_BOGUS;
+ if (rc == STAT_INSECURE)
+ secure = STAT_INSECURE;
}
/* OK, all the RRsets validate, now see if we have a missing answer or CNAME target. */
@@ -2172,7 +2200,7 @@ int dnssec_validate_reply(time_t now, struct dns_header *header, size_t plen, ch
}
}
- return STAT_SECURE;
+ return secure;
}
diff --git a/src/forward.c b/src/forward.c
index a729c06..245c448 100644
--- a/src/forward.c
+++ b/src/forward.c
@@ -561,7 +561,8 @@ static size_t process_reply(struct dns_header *header, time_t now, struct server
char **sets = 0;
int munged = 0, is_sign;
size_t plen;
-
+ char *rr_status = NULL;
+
(void)ad_reqd;
(void)do_bit;
(void)bogusanswer;
@@ -651,6 +652,11 @@ static size_t process_reply(struct dns_header *header, time_t now, struct server
server->flags |= SERV_WARNED_RECURSIVE;
}
+#ifdef HAVE_DNSSEC
+ if (option_bool(OPT_DNSSEC_VALID))
+ rr_status = daemon->rr_status;
+#endif
+
if (daemon->bogus_addr && RCODE(header) != NXDOMAIN &&
check_for_bogus_wildcard(header, n, daemon->namebuff, daemon->bogus_addr, now))
{
@@ -676,7 +682,7 @@ static size_t process_reply(struct dns_header *header, time_t now, struct server
cache_secure = 0;
}
- if (extract_addresses(header, n, daemon->namebuff, now, sets, is_sign, check_rebind, no_cache, cache_secure, &doctored))
+ if (extract_addresses(header, n, daemon->namebuff, now, sets, is_sign, check_rebind, no_cache, cache_secure, &doctored, rr_status))
{
my_syslog(LOG_WARNING, _("possible DNS-rebind attack detected: %s"), daemon->namebuff);
munged = 1;
@@ -856,7 +862,7 @@ void reply_query(int fd, int family, time_t now)
if (forward->forwardall == 0 || --forward->forwardall == 1 || RCODE(header) != REFUSED)
{
int check_rebind = 0, no_cache_dnssec = 0, cache_secure = 0, bogusanswer = 0;
-
+
if (option_bool(OPT_NO_REBIND))
check_rebind = !(forward->flags & FREC_NOREBIND);
@@ -896,7 +902,8 @@ void reply_query(int fd, int family, time_t now)
status = dnssec_validate_ds(now, header, n, daemon->namebuff, daemon->keyname, forward->class);
else
status = dnssec_validate_reply(now, header, n, daemon->namebuff, daemon->keyname, &forward->class,
- option_bool(OPT_DNSSEC_NO_SIGN), NULL, NULL);
+ option_bool(OPT_DNSSEC_NO_SIGN) && (server->flags & SERV_DO_DNSSEC),
+ NULL, NULL, daemon->rr_status);
}
/* Can't validate, as we're missing key data. Put this
@@ -1480,7 +1487,9 @@ static int tcp_key_recurse(time_t now, int status, struct dns_header *header, si
else if (status == STAT_NEED_DS)
new_status = dnssec_validate_ds(now, header, n, name, keyname, class);
else
- new_status = dnssec_validate_reply(now, header, n, name, keyname, &class, option_bool(OPT_DNSSEC_NO_SIGN), NULL, NULL);
+ new_status = dnssec_validate_reply(now, header, n, name, keyname, &class,
+ option_bool(OPT_DNSSEC_NO_SIGN) && (server->flags & SERV_DO_DNSSEC),
+ NULL, NULL, daemon->rr_status);
if (new_status != STAT_NEED_DS && new_status != STAT_NEED_KEY)
break;
diff --git a/src/rfc1035.c b/src/rfc1035.c
index f78b5cb..607412f 100644
--- a/src/rfc1035.c
+++ b/src/rfc1035.c
@@ -571,7 +571,8 @@ static int find_soa(struct dns_header *header, size_t qlen, char *name, int *doc
expired and cleaned out that way.
Return 1 if we reject an address because it look like part of dns-rebinding attack. */
int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t now,
- char **ipsets, int is_sign, int check_rebind, int no_cache_dnssec, int secure, int *doctored)
+ char **ipsets, int is_sign, int check_rebind, int no_cache_dnssec,
+ int secure, int *doctored, char *rr_status)
{
unsigned char *p, *p1, *endrr, *namep;
int i, j, qtype, qclass, aqtype, aqclass, ardlen, res, searched_soa = 0;
@@ -582,6 +583,7 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t
#else
(void)ipsets; /* unused */
#endif
+
cache_start_insert();
@@ -590,10 +592,16 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t
{
searched_soa = 1;
ttl = find_soa(header, qlen, name, doctored);
-#ifdef HAVE_DNSSEC
- if (*doctored && secure)
- return 0;
-#endif
+
+ if (*doctored)
+ {
+ if (secure)
+ return 0;
+ if (rr_status)
+ for (i = 0; i < ntohs(header->ancount); i++)
+ if (rr_status[i])
+ return 0;
+ }
}
/* go through the questions. */
@@ -604,7 +612,7 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t
int found = 0, cname_count = CNAME_CHAIN;
struct crec *cpp = NULL;
int flags = RCODE(header) == NXDOMAIN ? F_NXDOMAIN : 0;
- int secflag = secure ? F_DNSSECOK : 0;
+ int cname_short = 0;
unsigned long cttl = ULONG_MAX, attl;
namep = p;
@@ -632,8 +640,9 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t
if (!(p1 = skip_questions(header, qlen)))
return 0;
- for (j = ntohs(header->ancount); j != 0; j--)
+ for (j = 0; j < ntohs(header->ancount); j++)
{
+ int secflag = 0;
unsigned char *tmp = namep;
/* the loop body overwrites the original name, so get it back here. */
if (!extract_name(header, qlen, &tmp, name, 1, 0) ||
@@ -659,11 +668,21 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t
{
if (!extract_name(header, qlen, &p1, name, 1, 0))
return 0;
-
+
+ if (rr_status && rr_status[j])
+ {
+ /* validated RR anywhere in CNAME chain, don't cache. */
+ if (cname_short || aqtype == T_CNAME)
+ return 0;
+
+ secflag = F_DNSSECOK;
+ }
+
if (aqtype == T_CNAME)
{
- if (!cname_count-- || secure)
- return 0; /* looped CNAMES, or DNSSEC, which we can't cache. */
+ if (!cname_count--)
+ return 0; /* looped CNAMES, we can't cache. */
+ cname_short = 1;
goto cname_loop;
}
@@ -685,7 +704,7 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t
ttl = find_soa(header, qlen, NULL, doctored);
}
if (ttl)
- cache_insert(NULL, &addr, now, ttl, name_encoding | F_REVERSE | F_NEG | flags | secflag);
+ cache_insert(NULL, &addr, now, ttl, name_encoding | F_REVERSE | F_NEG | flags | (secure ? F_DNSSECOK : 0));
}
}
else
@@ -713,8 +732,10 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t
if (!(p1 = skip_questions(header, qlen)))
return 0;
- for (j = ntohs(header->ancount); j != 0; j--)
+ for (j = 0; j < ntohs(header->ancount); j++)
{
+ int secflag = 0;
+
if (!(res = extract_name(header, qlen, &p1, name, 0, 10)))
return 0; /* bad packet */
@@ -731,6 +752,10 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t
if (aqclass == C_IN && res != 2 && (aqtype == T_CNAME || aqtype == qtype))
{
+#ifdef HAVE_DNSSEC
+ if (rr_status && rr_status[j])
+ secflag = F_DNSSECOK;
+#endif
if (aqtype == T_CNAME)
{
if (!cname_count--)
@@ -822,7 +847,7 @@ int extract_addresses(struct dns_header *header, size_t qlen, char *name, time_t
pointing at this, inherit its TTL */
if (ttl || cpp)
{
- newc = cache_insert(name, NULL, now, ttl ? ttl : cttl, F_FORWARD | F_NEG | flags | secflag);
+ newc = cache_insert(name, NULL, now, ttl ? ttl : cttl, F_FORWARD | F_NEG | flags | (secure ? F_DNSSECOK : 0));
if (newc && cpp)
{
cpp->addr.cname.target.cache = newc;
@@ -937,7 +962,7 @@ int check_for_local_domain(char *name, time_t now)
/* Note: the call to cache_find_by_name is intended to find any record which matches
ie A, AAAA, CNAME. */
- if ((crecp = cache_find_by_name(NULL, name, now, F_IPV4 | F_IPV6 | F_CNAME |F_NO_RR)) &&
+ if ((crecp = cache_find_by_name(NULL, name, now, F_IPV4 | F_IPV6 | F_CNAME | F_NO_RR)) &&
(crecp->flags & (F_HOSTS | F_DHCP | F_CONFIG)))
return 1;
@@ -1689,8 +1714,9 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
if (qtype == T_CNAME || qtype == T_ANY)
{
- if ((crecp = cache_find_by_name(NULL, name, now, F_CNAME)) &&
- (qtype == T_CNAME || (crecp->flags & (F_HOSTS | F_DHCP | F_CONFIG | (dryrun ? F_NO_RR : 0)))))
+ if ((crecp = cache_find_by_name(NULL, name, now, F_CNAME | (dryrun ? F_NO_RR : 0))) &&
+ (qtype == T_CNAME || (crecp->flags & F_CONFIG)) &&
+ ((crecp->flags & F_CONFIG) || !do_bit || !(crecp->flags & F_DNSSECOK)))
{
if (!(crecp->flags & F_DNSSECOK))
sec_data = 0;
--
2.20.1