From 3e33f4198d1840fd0aed97d98ba0be8ac0cafd19 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Mon, 6 Jul 2015 12:52:37 +1000 Subject: [PATCH] 4154. [bug] A OPT record should be included with the FORMERR response when there is a malformed EDNS option. [RT #39647] 4153. [bug] Dig should zero non significant +subnet bits. Check that non significant ECS bits are zero on receipt. [RT #39647] --- CHANGES | 8 +++++++ bin/dig/dighost.c | 34 ++++++++++++++++++++++++------ bin/named/client.c | 33 ++++++++++++++++++++--------- bin/tests/system/resolver/tests.sh | 19 +++++++++++++++++ bin/tools/mdig.c | 34 ++++++++++++++++++++++++------ lib/dns/rdata/generic/opt_41.c | 9 +++++++- 6 files changed, 114 insertions(+), 23 deletions(-) diff --git a/CHANGES b/CHANGES index 1756f1b326..f1f6995768 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,11 @@ +4154. [bug] A OPT record should be included with the FORMERR + response when there is a malformed EDNS option. + [RT #39647] + +4153. [bug] Dig should zero non significant +subnet bits. Check + that non significant ECS bits are zero on receipt. + [RT #39647] + 4152. [func] Implement DNS COOKIE option. This replaces the experimental SIT option of BIND 9.10. The following named.conf directives are avaliable: send-cookie, diff --git a/bin/dig/dighost.c b/bin/dig/dighost.c index 0336dcb90d..36441f0c2d 100644 --- a/bin/dig/dighost.c +++ b/bin/dig/dighost.c @@ -2491,13 +2491,19 @@ setup_lookup(dig_lookup_t *lookup) { struct sockaddr *sa; struct sockaddr_in *sin; struct sockaddr_in6 *sin6; + const isc_uint8_t *addr; size_t addrl; + isc_uint8_t mask; sa = &lookup->ecs_addr->type.sa; prefixlen = lookup->ecs_addr->length; /* Round up prefix len to a multiple of 8 */ addrl = (prefixlen + 7) / 8; + if (prefixlen % 8 == 0) + mask = 0xff; + else + mask = 0xffU << (8 - (prefixlen % 8)); INSIST(i < DNS_EDNSOPTIONS); opts[i].code = DNS_OPT_CLIENT_SUBNET; @@ -2506,20 +2512,36 @@ setup_lookup(dig_lookup_t *lookup) { isc_buffer_init(&b, ecsbuf, sizeof(ecsbuf)); if (sa->sa_family == AF_INET) { sin = (struct sockaddr_in *) sa; + addr = (isc_uint8_t *) &sin->sin_addr; + /* family */ isc_buffer_putuint16(&b, 1); + /* source prefix-length */ isc_buffer_putuint8(&b, prefixlen); + /* scope prefix-length */ isc_buffer_putuint8(&b, 0); - isc_buffer_putmem(&b, - (isc_uint8_t *) &sin->sin_addr, - (unsigned int) addrl); + /* address */ + if (addrl > 0) { + isc_buffer_putmem(&b, addr, addrl - 1); + isc_buffer_putuint8(&b, + (addr[addrl - 1] & + mask)); + } } else { sin6 = (struct sockaddr_in6 *) sa; + addr = (isc_uint8_t *) &sin6->sin6_addr; + /* family */ isc_buffer_putuint16(&b, 2); + /* source prefix-length */ isc_buffer_putuint8(&b, prefixlen); + /* scope prefix-length */ isc_buffer_putuint8(&b, 0); - isc_buffer_putmem(&b, - (isc_uint8_t *) &sin6->sin6_addr, - (unsigned int) addrl); + /* address */ + if (addrl > 0) { + isc_buffer_putmem(&b, addr, addrl - 1); + isc_buffer_putuint8(&b, + (addr[addrl - 1] & + mask)); + } } opts[i].value = (isc_uint8_t *) ecsbuf; diff --git a/bin/named/client.c b/bin/named/client.c index fc327ca11e..5103fb6436 100644 --- a/bin/named/client.c +++ b/bin/named/client.c @@ -1514,7 +1514,7 @@ ns_client_addopt(ns_client_t *client, dns_message_t *message, uc = paddr[i]; if (i == addrbytes - 1 && ((client->ecs_addrlen % 8) != 0)) - uc &= (1U << (8 - (client->ecs_addrlen % 8))); + uc &= (0xffU << (8 - (client->ecs_addrlen % 8))); isc_buffer_putuint8(&buf, uc); } @@ -1818,10 +1818,16 @@ process_ecs(ns_client_t *client, isc_buffer_t *buf, size_t optlen) { isc_netaddr_t caddr; int i; + /* + * XXXMUKS: Is there any need to repeat these checks here + * (except query's scope length) when they are done in the OPT + * RDATA fromwire code? + */ + if (optlen < 4U) { ns_client_log(client, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(2), - "EDNS client subnet option too short"); + "EDNS client-subnet option too short"); return (DNS_R_FORMERR); } @@ -1833,8 +1839,8 @@ process_ecs(ns_client_t *client, isc_buffer_t *buf, size_t optlen) { if (scope != 0U) { ns_client_log(client, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(2), - "EDNS client subnet option: invalid scope"); - return (DNS_R_FORMERR); + "EDNS client-subnet option: invalid scope"); + return (DNS_R_OPTERR); } memset(&caddr, 0, sizeof(caddr)); @@ -1849,26 +1855,26 @@ process_ecs(ns_client_t *client, isc_buffer_t *buf, size_t optlen) { invalid_length: ns_client_log(client, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(2), - "EDNS client subnet option: invalid " + "EDNS client-subnet option: invalid " "address length (%u) for %s", addrlen, family == 1 ? "IPv4" : "IPv6"); - return (DNS_R_FORMERR); + return (DNS_R_OPTERR); } caddr.family = AF_INET6; break; default: ns_client_log(client, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(2), - "EDNS client subnet option: invalid family"); - return (DNS_R_FORMERR); + "EDNS client-subnet option: invalid family"); + return (DNS_R_OPTERR); } addrbytes = (addrlen + 7) / 8; if (isc_buffer_remaininglength(buf) < addrbytes) { ns_client_log(client, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(2), - "EDNS client subnet option: address too short"); - return (DNS_R_FORMERR); + "EDNS client-subnet option: address too short"); + return (DNS_R_OPTERR); } paddr = (isc_uint8_t *) &caddr.type; @@ -1877,6 +1883,13 @@ process_ecs(ns_client_t *client, isc_buffer_t *buf, size_t optlen) { optlen--; } + if (addrbytes != 0U && (addrlen % 8) != 0) { + isc_uint8_t bits = ~0 << (8 - (addrlen % 8)); + bits &= paddr[addrbytes - 1]; + if (bits != paddr[addrbytes - 1]) + return (DNS_R_OPTERR); + } + memmove(&client->ecs_addr, &caddr, sizeof(caddr)); client->ecs_addrlen = addrlen; client->ecs_scope = 0; diff --git a/bin/tests/system/resolver/tests.sh b/bin/tests/system/resolver/tests.sh index a8a770ccd9..f8365386b8 100755 --- a/bin/tests/system/resolver/tests.sh +++ b/bin/tests/system/resolver/tests.sh @@ -544,5 +544,24 @@ grep "status: NOTIMP" dig.out.ns5.test${n} > /dev/null || ret=1 if [ $ret != 0 ]; then echo "I:failed"; fi status=`expr $status + $ret` +n=`expr $n + 1` +echo "I:check that EDNS client subnet with non-zeroed bits is handled correctly (${n})" +ret=0 +# 0001 (IPv4) 1f (31 significant bits) 00 (0) ffffffff (255.255.255.255) +$DIG soa . @10.53.0.5 -p 5300 +ednsopt=8:00011f00ffffffff > dig.out.ns5.test${n} || ret=1 +grep "status: FORMERR" dig.out.ns5.test${n} > /dev/null || ret=1 +grep "; EDNS: version:" dig.out.ns5.test${n} > /dev/null || ret=1 +if [ $ret != 0 ]; then echo "I:failed"; fi +status=`expr $status + $ret` + +n=`expr $n + 1` +echo "I:check that dig +subnet zeros address bits correctly (${n})" +ret=0 +$DIG soa . @10.53.0.5 -p 5300 +subnet=255.255.255.255/23 > dig.out.ns5.test${n} || ret=1 +grep "status: NOERROR" dig.out.ns5.test${n} > /dev/null || ret=1 +grep "CLIENT-SUBNET: 255.255.254.0/23/0" dig.out.ns5.test${n} > /dev/null || ret=1 +if [ $ret != 0 ]; then echo "I:failed"; fi +status=`expr $status + $ret` + echo "I:exit status: $status" exit $status diff --git a/bin/tools/mdig.c b/bin/tools/mdig.c index b3f49949da..c1fe59ec21 100644 --- a/bin/tools/mdig.c +++ b/bin/tools/mdig.c @@ -588,7 +588,9 @@ sendquery(struct query *query, isc_task_t *task) struct sockaddr *sa; struct sockaddr_in *sin; struct sockaddr_in6 *sin6; + const isc_uint8_t *addr; size_t addrl; + isc_uint8_t mask; isc_buffer_t b; sa = &query->ecs_addr->type.sa; @@ -596,6 +598,10 @@ sendquery(struct query *query, isc_task_t *task) /* Round up prefix len to a multiple of 8 */ addrl = (prefixlen + 7) / 8; + if (prefixlen % 8 == 0) + mask = 0xff; + else + mask = 0xffU << (8 - (prefixlen % 8)); INSIST(i < DNS_EDNSOPTIONS); opts[i].code = DNS_OPT_CLIENT_SUBNET; @@ -604,20 +610,36 @@ sendquery(struct query *query, isc_task_t *task) isc_buffer_init(&b, ecsbuf, sizeof(ecsbuf)); if (sa->sa_family == AF_INET) { sin = (struct sockaddr_in *) sa; + addr = (isc_uint8_t *) &sin->sin_addr; + /* family */ isc_buffer_putuint16(&b, 1); + /* source prefix-length */ isc_buffer_putuint8(&b, prefixlen); + /* scope prefix-length */ isc_buffer_putuint8(&b, 0); - isc_buffer_putmem(&b, - (isc_uint8_t *) &sin->sin_addr, - (unsigned int) addrl); + /* address */ + if (addrl > 0) { + isc_buffer_putmem(&b, addr, addrl - 1); + isc_buffer_putuint8(&b, + (addr[addrl - 1] & + mask)); + } } else { sin6 = (struct sockaddr_in6 *) sa; + addr = (isc_uint8_t *) &sin6->sin6_addr; + /* family */ isc_buffer_putuint16(&b, 2); + /* source prefix-length */ isc_buffer_putuint8(&b, prefixlen); + /* scope prefix-length */ isc_buffer_putuint8(&b, 0); - isc_buffer_putmem(&b, - (isc_uint8_t *) &sin6->sin6_addr, - (unsigned int) addrl); + /* address */ + if (addrl > 0) { + isc_buffer_putmem(&b, addr, addrl - 1); + isc_buffer_putuint8(&b, + (addr[addrl - 1] & + mask)); + } } opts[i].value = (isc_uint8_t *) ecsbuf; diff --git a/lib/dns/rdata/generic/opt_41.c b/lib/dns/rdata/generic/opt_41.c index 7ae5e865bb..853bfbb39a 100644 --- a/lib/dns/rdata/generic/opt_41.c +++ b/lib/dns/rdata/generic/opt_41.c @@ -150,6 +150,13 @@ fromwire_opt(ARGS_FROMWIRE) { addrbytes = (addrlen + 7) / 8; if (addrbytes + 4 != length) return (DNS_R_OPTERR); + + if (addrbytes != 0U && (addrlen % 8) != 0) { + isc_uint8_t bits = ~0 << (8 - (addrlen % 8)); + bits &= sregion.base[addrbytes - 1]; + if (bits != sregion.base[addrbytes - 1]) + return (DNS_R_OPTERR); + } isc_region_consume(&sregion, addrbytes); break; } @@ -163,7 +170,7 @@ fromwire_opt(ARGS_FROMWIRE) { break; case DNS_OPT_COOKIE: if (length != 8 && (length < 16 || length > 40)) - return (DNS_R_FORMERR); + return (DNS_R_OPTERR); isc_region_consume(&sregion, length); break; default: