From db9781d4a2ed15c4b34bb5c97ea68b8f598992fc Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 14 Sep 2016 08:22:15 +1000 Subject: [PATCH] 4468. [bug] Address ECS option handling issues. [RT #43191] (cherry picked from commit df1729011335b7991e748c2ad185309cb3f8e945) --- CHANGES | 2 + bin/dig/dig.docbook | 2 +- bin/dig/dighost.c | 81 ++++++++++++++++++++----------- bin/named/client.c | 70 +++++++++++++++----------- bin/tests/system/digdelv/tests.sh | 31 +++++++++++- lib/dns/message.c | 10 +--- lib/dns/rdata/generic/opt_41.c | 3 -- lib/dns/tests/rdata_test.c | 4 +- 8 files changed, 130 insertions(+), 73 deletions(-) diff --git a/CHANGES b/CHANGES index cddea583db..04d026d793 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ +4468. [bug] Address ECS option handling issues. [RT #43191] + --- 9.11.0rc2 released --- 4467. [security] It was possible to trigger a assertion when rendering diff --git a/bin/dig/dig.docbook b/bin/dig/dig.docbook index 28756c77b6..994d579fec 100644 --- a/bin/dig/dig.docbook +++ b/bin/dig/dig.docbook @@ -1020,7 +1020,7 @@ dig +subnet=0.0.0.0/0, or simply dig +subnet=0 for short, sends an EDNS - client-subnet option with an empty address and a source + CLIENT-SUBNET option with an empty address and a source prefix-length of zero, which signals a resolver that the client's address information must not be used when resolving diff --git a/bin/dig/dighost.c b/bin/dig/dighost.c index 675bf70fdd..74cdcb36b6 100644 --- a/bin/dig/dighost.c +++ b/bin/dig/dighost.c @@ -1066,11 +1066,24 @@ parse_netprefix(isc_sockaddr_t **sap, const char *value) { isc_uint32_t prefix_length = 0xffffffff; char *slash = NULL; isc_boolean_t parsed = ISC_FALSE; + isc_boolean_t prefix_parsed = ISC_FALSE; char buf[sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:XXX.XXX.XXX.XXX/128")]; if (strlcpy(buf, value, sizeof(buf)) >= sizeof(buf)) fatal("invalid prefix '%s'\n", value); + sa = isc_mem_allocate(mctx, sizeof(*sa)); + if (sa == NULL) + fatal("out of memory"); + memset(sa, 0, sizeof(*sa)); + + if (strcmp(buf, "0") == 0) { + sa->type.sa.sa_family = AF_UNSPEC; + parsed = ISC_TRUE; + prefix_length = 0; + goto done; + } + slash = strchr(buf, '/'); if (slash != NULL) { *slash = '\0'; @@ -1079,16 +1092,9 @@ parse_netprefix(isc_sockaddr_t **sap, const char *value) { fatal("invalid prefix length in '%s': %s\n", value, isc_result_totext(result)); } + prefix_parsed = ISC_TRUE; } - if (strcmp(buf, "0") == 0) { - parsed = ISC_TRUE; - prefix_length = 0; - } - - sa = isc_mem_allocate(mctx, sizeof(*sa)); - if (sa == NULL) - fatal("out of memory"); if (inet_pton(AF_INET6, buf, &in6) == 1) { parsed = ISC_TRUE; isc_sockaddr_fromin6(sa, &in6, 0); @@ -1099,7 +1105,7 @@ parse_netprefix(isc_sockaddr_t **sap, const char *value) { isc_sockaddr_fromin(sa, &in4, 0); if (prefix_length > 32) prefix_length = 32; - } else if (prefix_length != 0xffffffff && prefix_length != 0) { + } else if (prefix_parsed) { int i; for (i = 0; i < 3 && strlen(buf) < sizeof(buf) - 2; i++) { @@ -1118,10 +1124,8 @@ parse_netprefix(isc_sockaddr_t **sap, const char *value) { if (!parsed) fatal("invalid address '%s'", value); +done: sa->length = prefix_length; - if (prefix_length == 0) - sa->type.sa.sa_family = AF_UNSPEC; - *sap = sa; return (ISC_R_SUCCESS); @@ -2510,8 +2514,8 @@ setup_lookup(dig_lookup_t *lookup) { } if (lookup->ecs_addr != NULL) { - isc_uint8_t addr[16], family; - int proto; + isc_uint8_t addr[16]; + isc_uint16_t family; isc_uint32_t plen; struct sockaddr *sa; struct sockaddr_in *sin; @@ -2528,46 +2532,65 @@ setup_lookup(dig_lookup_t *lookup) { opts[i].code = DNS_OPT_CLIENT_SUBNET; opts[i].length = (isc_uint16_t) addrl + 4; check_result(result, "isc_buffer_allocate"); - isc_buffer_init(&b, ecsbuf, sizeof(ecsbuf)); - /* If prefix length is zero, don't set family */ - proto = sa->sa_family; - if (plen == 0) - proto = AF_UNSPEC; - - switch (proto) { + /* + * XXXMUKS: According to RFC7871, "If there is + * no ADDRESS set, i.e., SOURCE PREFIX-LENGTH is + * set to 0, then FAMILY SHOULD be set to the + * transport over which the query is sent." + * + * However, at this point we don't know what + * transport(s) we'll be using, so we can't + * set the value now. For now, we're using + * IPv4 as the default the +subnet option + * used an IPv4 prefix, or for +subnet=0, + * and IPv6 if the +subnet option used an + * IPv6 prefix. + * + * (For future work: preserve the offset into + * the buffer where the family field is; + * that way we can update it in send_udp() + * or send_tcp_connect() once we know + * what it outght to be.) + */ + switch (sa->sa_family) { case AF_UNSPEC: INSIST(plen == 0); - family = 0; + family = 1; break; case AF_INET: + INSIST(plen <= 32); family = 1; sin = (struct sockaddr_in *) sa; - memmove(addr, &sin->sin_addr, 4); + memmove(addr, &sin->sin_addr, addrl); break; case AF_INET6: + INSIST(plen <= 128); family = 2; sin6 = (struct sockaddr_in6 *) sa; - memmove(addr, &sin6->sin6_addr, 16); + memmove(addr, &sin6->sin6_addr, addrl); break; default: INSIST(0); } - /* Mask off last address byte */ - if (addrl > 0 && (plen % 8) != 0) - addr[addrl - 1] &= ~0U << (8 - (plen % 8)); - + isc_buffer_init(&b, ecsbuf, sizeof(ecsbuf)); /* family */ isc_buffer_putuint16(&b, family); /* source prefix-length */ isc_buffer_putuint8(&b, plen); /* scope prefix-length */ isc_buffer_putuint8(&b, 0); + /* address */ - if (addrl > 0) + if (addrl > 0) { + /* Mask off last address byte */ + if ((plen % 8) != 0) + addr[addrl - 1] &= + ~0U << (8 - (plen % 8)); isc_buffer_putmem(&b, addr, (unsigned)addrl); + } opts[i].value = (isc_uint8_t *) ecsbuf; i++; diff --git a/bin/named/client.c b/bin/named/client.c index df6e5d1c68..f4469d8363 100644 --- a/bin/named/client.c +++ b/bin/named/client.c @@ -1614,35 +1614,57 @@ ns_client_addopt(ns_client_t *client, dns_message_t *message, client->ecs_addr.family == AF_INET6 || client->ecs_addr.family == AF_UNSPEC)) { - int i, addrbytes = (client->ecs_addrlen + 7) / 8; - isc_uint8_t *paddr; isc_buffer_t buf; + isc_uint8_t addr[16]; + isc_uint32_t plen, addrl; + isc_uint16_t family; + + /* Add CLIENT-SUBNET option. */ + + plen = client->ecs_addrlen; + + /* Round up prefix len to a multiple of 8 */ + addrl = (plen + 7) / 8; + + switch (client->ecs_addr.family) { + case AF_UNSPEC: + INSIST(plen == 0); + family = 0; + break; + case AF_INET: + INSIST(plen <= 32); + family = 1; + memmove(addr, &client->ecs_addr.type, addrl); + break; + case AF_INET6: + INSIST(plen <= 128); + family = 2; + memmove(addr, &client->ecs_addr.type, addrl); + break; + default: + INSIST(0); + } - /* Add client subnet option. */ isc_buffer_init(&buf, ecs, sizeof(ecs)); - if (client->ecs_addr.family == AF_UNSPEC || - client->ecs_addrlen == 0) - isc_buffer_putuint16(&buf, 0); - else if (client->ecs_addr.family == AF_INET) - isc_buffer_putuint16(&buf, 1); - else - isc_buffer_putuint16(&buf, 2); + /* family */ + isc_buffer_putuint16(&buf, family); + /* source prefix-length */ isc_buffer_putuint8(&buf, client->ecs_addrlen); + /* scope prefix-length */ isc_buffer_putuint8(&buf, client->ecs_scope); - paddr = (isc_uint8_t *) &client->ecs_addr.type; - for (i = 0; i < addrbytes; i++) { - unsigned char uc; - uc = paddr[i]; - if (i == addrbytes - 1 && - ((client->ecs_addrlen % 8) != 0)) - uc &= (0xffU << (8 - - (client->ecs_addrlen % 8))); - isc_buffer_putuint8(&buf, uc); + /* address */ + if (addrl > 0) { + /* Mask off last address byte */ + if ((plen % 8) != 0) + addr[addrl - 1] &= + ~0U << (8 - (plen % 8)); + isc_buffer_putmem(&buf, addr, + (unsigned) addrl); } ednsopts[count].code = DNS_OPT_CLIENT_SUBNET; - ednsopts[count].length = addrbytes + 4; + ednsopts[count].length = addrl + 4; ednsopts[count].value = ecs; count++; } @@ -1975,14 +1997,6 @@ process_ecs(ns_client_t *client, isc_buffer_t *buf, size_t optlen) { return (DNS_R_OPTERR); } - if (addrlen == 0U && family != 0U) { - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(2), - "EDNS client-subnet option: " - "source == 0 but family != 0"); - return (DNS_R_OPTERR); - } - memset(&caddr, 0, sizeof(caddr)); switch (family) { case 0: diff --git a/bin/tests/system/digdelv/tests.sh b/bin/tests/system/digdelv/tests.sh index da225d86a8..adcc2617c1 100644 --- a/bin/tests/system/digdelv/tests.sh +++ b/bin/tests/system/digdelv/tests.sh @@ -281,7 +281,9 @@ if [ -x ${DIG} ] ; then echo "I:checking dig +subnet=0/0 ($n)" ret=0 $DIG $DIGOPTS +tcp @10.53.0.2 +subnet=0/0 A a.example > dig.out.test$n 2>&1 || ret=1 - grep "CLIENT-SUBNET: 0/0/0" < dig.out.test$n > /dev/null || ret=1 + grep "status: NOERROR" < dig.out.test$n > /dev/null || ret=1 + grep "CLIENT-SUBNET: 0.0.0.0/0/0" < dig.out.test$n > /dev/null || ret=1 + grep "10.0.0.1" < dig.out.test$n > /dev/null || ret=1 if [ $ret != 0 ]; then echo "I:failed"; fi status=`expr $status + $ret` @@ -289,7 +291,9 @@ if [ -x ${DIG} ] ; then echo "I:checking dig +subnet=0 ($n)" ret=0 $DIG $DIGOPTS +tcp @10.53.0.2 +subnet=0 A a.example > dig.out.test$n 2>&1 || ret=1 - grep "CLIENT-SUBNET: 0/0/0" < dig.out.test$n > /dev/null || ret=1 + grep "status: NOERROR" < dig.out.test$n > /dev/null || ret=1 + grep "CLIENT-SUBNET: 0.0.0.0/0/0" < dig.out.test$n > /dev/null || ret=1 + grep "10.0.0.1" < dig.out.test$n > /dev/null || ret=1 if [ $ret != 0 ]; then echo "I:failed"; fi status=`expr $status + $ret` @@ -297,7 +301,30 @@ if [ -x ${DIG} ] ; then echo "I:checking dig +subnet=::/0 ($n)" ret=0 $DIG $DIGOPTS +tcp @10.53.0.2 +subnet=::/0 A a.example > dig.out.test$n 2>&1 || ret=1 + grep "status: NOERROR" < dig.out.test$n > /dev/null || ret=1 + grep "CLIENT-SUBNET: ::/0/0" < dig.out.test$n > /dev/null || ret=1 + grep "10.0.0.1" < dig.out.test$n > /dev/null || ret=1 + if [ $ret != 0 ]; then echo "I:failed"; fi + status=`expr $status + $ret` + + n=`expr $n + 1` + echo "I:checking dig +ednsopt=8:00000000 (family=0, source=0, scope=0) ($n)" + ret=0 + $DIG $DIGOPTS +tcp @10.53.0.2 +ednsopt=8:00000000 A a.example > dig.out.test$n 2>&1 || ret=1 + grep "status: NOERROR" < dig.out.test$n > /dev/null || ret=1 grep "CLIENT-SUBNET: 0/0/0" < dig.out.test$n > /dev/null || ret=1 + grep "10.0.0.1" < dig.out.test$n > /dev/null || ret=1 + if [ $ret != 0 ]; then echo "I:failed"; fi + status=`expr $status + $ret` + + n=`expr $n + 1` + echo "I:checking dig +ednsopt=8:00030000 (family=3, source=0, scope=0) ($n)" + ret=0 + $DIG $DIGOPTS +qr +tcp @10.53.0.2 +ednsopt=8:00030000 A a.example > dig.out.test$n 2>&1 || ret=1 + grep "status: FORMERR" < dig.out.test$n > /dev/null || ret=1 + grep "CLIENT-SUBNET: 00 03 00 00" < dig.out.test$n > /dev/null || ret=1 + lines=`grep "CLIENT-SUBNET: 00 03 00 00" dig.out.test$n | wc -l` + [ ${lines:-0} -eq 1 ] || ret=1 if [ $ret != 0 ]; then echo "I:failed"; fi status=`expr $status + $ret` diff --git a/lib/dns/message.c b/lib/dns/message.c index 81a702a1d0..c5d6ca22e2 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -3255,9 +3255,6 @@ render_ecs(isc_buffer_t *ecsbuf, isc_buffer_t *target) { addrlen = isc_buffer_getuint8(ecsbuf); scopelen = isc_buffer_getuint8(ecsbuf); - if (addrlen == 0 && family != 0) - return (DNS_R_OPTERR); - addrbytes = (addrlen + 7) / 8; if (isc_buffer_remaininglength(ecsbuf) < addrbytes) return (DNS_R_OPTERR); @@ -3265,7 +3262,6 @@ render_ecs(isc_buffer_t *ecsbuf, isc_buffer_t *target) { if (addrbytes > sizeof(addr)) return (DNS_R_OPTERR); - ADD_STRING(target, ": "); memset(addr, 0, sizeof(addr)); for (i = 0; i < addrbytes; i ++) addr[i] = isc_buffer_getuint8(ecsbuf); @@ -3287,12 +3283,10 @@ render_ecs(isc_buffer_t *ecsbuf, isc_buffer_t *target) { inet_ntop(AF_INET6, addr, addr_text, sizeof(addr_text)); break; default: - snprintf(addr_text, sizeof(addr_text), - "Unsupported family %u", family); - ADD_STRING(target, addr_text); - return (ISC_R_SUCCESS); + return (DNS_R_OPTERR); } + ADD_STRING(target, ": "); ADD_STRING(target, addr_text); snprintf(addr_text, sizeof(addr_text), "/%d/%d", addrlen, scopelen); ADD_STRING(target, addr_text); diff --git a/lib/dns/rdata/generic/opt_41.c b/lib/dns/rdata/generic/opt_41.c index cd02399472..9a741ea98c 100644 --- a/lib/dns/rdata/generic/opt_41.c +++ b/lib/dns/rdata/generic/opt_41.c @@ -125,9 +125,6 @@ fromwire_opt(ARGS_FROMWIRE) { scope = uint8_fromregion(&sregion); isc_region_consume(&sregion, 1); - if (addrlen == 0U && family != 0U) - return (DNS_R_OPTERR); - switch (family) { case 0: /* diff --git a/lib/dns/tests/rdata_test.c b/lib/dns/tests/rdata_test.c index 4b794fba31..31c0f8f875 100644 --- a/lib/dns/tests/rdata_test.c +++ b/lib/dns/tests/rdata_test.c @@ -97,7 +97,7 @@ ATF_TC_BODY(edns_client_subnet, tc) { 0x00, 0x08, 0x00, 0x04, 0x00, 0x01, 0x00, 0x00 }, - 8, ISC_FALSE + 8, ISC_TRUE }, { /* Option code family 2 (ipv6) , source 0, scope 0 */ @@ -105,7 +105,7 @@ ATF_TC_BODY(edns_client_subnet, tc) { 0x00, 0x08, 0x00, 0x04, 0x00, 0x02, 0x00, 0x00 }, - 8, ISC_FALSE + 8, ISC_TRUE }, { /* extra octet */