diff options
| -rw-r--r-- | lib/doh.c | 76 | ||||
| -rw-r--r-- | lib/doh.h | 6 | ||||
| -rw-r--r-- | tests/unit/unit1655.c | 118 |
3 files changed, 147 insertions, 53 deletions
@@ -86,12 +86,36 @@ UNITTEST DOHcode doh_encode(const char *host, unsigned char *orig = dnsp; const char *hostp = host; - /* The expected output length does not depend on the number of dots within - * the host name. It will always be two more than the length of the host - * name, one for the size and one trailing null. In case there are dots, - * each dot adds one size but removes the need to store the dot, net zero. + /* The expected output length is 16 bytes more than the length of + * the QNAME-encoding of the host name. + * + * A valid DNS name may not contain a zero-length label, except at + * the end. For this reason, a name beginning with a dot, or + * containing a sequence of two or more consecutive dots, is invalid + * and cannot be encoded as a QNAME. + * + * If the host name ends with a trailing dot, the corresponding + * QNAME-encoding is one byte longer than the host name. If (as is + * also valid) the hostname is shortened by the omission of the + * trailing dot, then its QNAME-encoding will be two bytes longer + * than the host name. + * + * Each [ label, dot ] pair is encoded as [ length, label ], + * preserving overall length. A final [ label ] without a dot is + * also encoded as [ length, label ], increasing overall length + * by one. The encoding is completed by appending a zero byte, + * representing the zero-length root label, again increasing + * the overall length by one. */ - const size_t expected_len = 12 + ( 1 + hostlen + 1) + 4; + + size_t expected_len; + DEBUGASSERT(hostlen); + expected_len = 12 + 1 + hostlen + 4; + if(host[hostlen-1]!='.') + expected_len++; + + if(expected_len > (256 + 16)) /* RFCs 1034, 1035 */ + return DOH_DNS_NAME_TOO_LONG; if(len < expected_len) return DOH_TOO_SMALL_BUFFER; @@ -109,31 +133,30 @@ UNITTEST DOHcode doh_encode(const char *host, *dnsp++ = '\0'; *dnsp++ = '\0'; /* ARCOUNT */ - /* store a QNAME */ - do { - char *dot = strchr(hostp, '.'); + /* encode each label and store it in the QNAME */ + while(*hostp) { size_t labellen; - bool found = false; - if(dot) { - found = true; + char *dot = strchr(hostp, '.'); + if(dot) labellen = dot - hostp; - } else labellen = strlen(hostp); - if(labellen > 63) { - /* too long label, error out */ + if((labellen > 63) || (!labellen)) { + /* label is too long or too short, error out */ *olen = 0; return DOH_DNS_BAD_LABEL; } + /* label is non-empty, process it */ *dnsp++ = (unsigned char)labellen; memcpy(dnsp, hostp, labellen); dnsp += labellen; - hostp += labellen + 1; - if(!found) { - *dnsp++ = 0; /* terminating zero */ - break; - } - } while(1); + hostp += labellen; + /* advance past dot, but only if there is one */ + if(dot) + hostp++; + } /* next label */ + + *dnsp++ = 0; /* append zero-length label for root */ /* There are assigned TYPE codes beyond 255: use range [1..65535] */ *dnsp++ = (unsigned char)(255 & (dnstype>>8)); /* upper 8 bit TYPE */ @@ -144,8 +167,8 @@ UNITTEST DOHcode doh_encode(const char *host, *olen = dnsp - orig; - /* verify that our assumption of length is valid, since - * this has lead to buffer overflows in this function */ + /* verify that our estimation of length is valid, since + * this has led to buffer overflows in this function */ DEBUGASSERT(*olen == expected_len); return DOH_OK; } @@ -586,6 +609,9 @@ static DOHcode rdata(unsigned char *doh, if(rc) return rc; break; + case DNS_TYPE_DNAME: + /* explicit for clarity; just skip; rely on synthesized CNAME */ + break; default: /* unsupported type, just skip it */ break; @@ -647,8 +673,10 @@ UNITTEST DOHcode doh_decode(unsigned char *doh, return DOH_DNS_OUT_OF_RANGE; type = get16bit(doh, index); - if((type != DNS_TYPE_CNAME) && (type != dnstype)) - /* Not the same type as was asked for nor CNAME */ + if((type != DNS_TYPE_CNAME) /* may be synthesized from DNAME */ + && (type != DNS_TYPE_DNAME) /* if present, accept and ignore */ + && (type != dnstype)) + /* Not the same type as was asked for nor CNAME nor DNAME */ return DOH_DNS_UNEXPECTED_TYPE; index += 2; @@ -55,14 +55,16 @@ typedef enum { DOH_DNS_UNEXPECTED_TYPE, /* 9 */ DOH_DNS_UNEXPECTED_CLASS, /* 10 */ DOH_NO_CONTENT, /* 11 */ - DOH_DNS_BAD_ID /* 12 */ + DOH_DNS_BAD_ID, /* 12 */ + DOH_DNS_NAME_TOO_LONG /* 13 */ } DOHcode; typedef enum { DNS_TYPE_A = 1, DNS_TYPE_NS = 2, DNS_TYPE_CNAME = 5, - DNS_TYPE_AAAA = 28 + DNS_TYPE_AAAA = 28, + DNS_TYPE_DNAME = 39 /* RFC6672 */ } DNStype; #define DOH_MAX_ADDR 24 diff --git a/tests/unit/unit1655.c b/tests/unit/unit1655.c index 7fea134d5..cccaab8da 100644 --- a/tests/unit/unit1655.c +++ b/tests/unit/unit1655.c @@ -36,43 +36,99 @@ static void unit_stop(void) UNITTEST_START -/* introduce a scope and prove the corner case with write overflow, - * so we can prove this test would detect it and that it is properly fixed +/* + * Prove detection of write overflow using a short buffer and a name + * of maximal valid length. + * + * Prove detection of other invalid input. */ do { - const char *bad = "this.is.a.hostname.where.each.individual.part.is.within." - "the.sixtythree.character.limit.but.still.long.enough.to." - "trigger.the.the.buffer.overflow......it.is.chosen.to.be." - "of.a.length.such.that.it.causes.a.two.byte.buffer......." - "overwrite.....making.it.longer.causes.doh.encode.to....." - ".return.early.so.dont.change.its.length.xxxx.xxxxxxxxxxx" - "..xxxxxx.....xx..........xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" - "xxxxxxxxxxxxxxxxxxxxxxxxxx.xxxxxxxxxxxxxxxx..x......xxxx" - "xxxx..xxxxxxxxxxxxxxxxxxx.x...xxxx.x.x.x...xxxxx"; + const char *max = + /* ..|....1.........2.........3.........4.........5.........6... */ + /* 3456789012345678901234567890123456789012345678901234567890123 */ + "this.is.a.maximum-length.hostname." /* 34: 34 */ + "with-no-label-of-greater-length-than-the-sixty-three-characters." + /* 64: 98 */ + "specified.in.the.RFCs." /* 22: 120 */ + "and.with.a.QNAME.encoding.whose.length.is.exactly." /* 50: 170 */ + "the.maximum.length.allowed." /* 27: 197 */ + "that.is.two-hundred.and.fifty-six." /* 34: 231 */ + "including.the.last.null." /* 24: 255 */ + ""; + const char *toolong = + /* ..|....1.........2.........3.........4.........5.........6... */ + /* 3456789012345678901234567890123456789012345678901234567890123 */ + "here.is.a.hostname.which.is.just.barely.too.long." /* 49: 49 */ + "to.be.encoded.as.a.QNAME.of.the.maximum.allowed.length." + /* 55: 104 */ + "which.is.256.including.a.final.zero-length.label." /* 49: 153 */ + "representing.the.root.node.so.that.a.name.with." /* 47: 200 */ + "a.trailing.dot.may.have.up.to." /* 30: 230 */ + "255.characters.never.more." /* 26: 256 */ + ""; + const char *emptylabel = + "this.is.an.otherwise-valid.hostname." + ".with.an.empty.label."; + const char *outsizelabel = + "this.is.an.otherwise-valid.hostname." + "with-a-label-of-greater-length-than-the-sixty-three-characters-" + "specified.in.the.RFCs."; + + struct test { + const char *name; + const DOHcode expected_result; + }; /* plays the role of struct dnsprobe in urldata.h */ struct demo { - unsigned char dohbuffer[512]; + unsigned char dohbuffer[255 + 16]; /* deliberately short buffer */ unsigned char canary1; unsigned char canary2; unsigned char canary3; }; - size_t olen = 100000; - struct demo victim; - DOHcode d; - victim.canary1 = 87; /* magic numbers, arbritrarily picked */ - victim.canary2 = 35; - victim.canary3 = 41; - d = doh_encode(bad, DNS_TYPE_A, victim.dohbuffer, - sizeof(victim.dohbuffer), &olen); - fail_unless(victim.canary1 == 87, "one byte buffer overwrite has happened"); - fail_unless(victim.canary2 == 35, "two byte buffer overwrite has happened"); - fail_unless(victim.canary3 == 41, - "three byte buffer overwrite has happened"); - if(d == DOH_OK) { - fail_unless(olen <= sizeof(victim.dohbuffer), "wrote outside bounds"); - fail_unless(olen > strlen(bad), "unrealistic low size"); + const struct test playlist[4] = { + { toolong, DOH_DNS_NAME_TOO_LONG }, /* expect early failure */ + { emptylabel, DOH_DNS_BAD_LABEL }, /* also */ + { outsizelabel, DOH_DNS_BAD_LABEL }, /* also */ + { max, DOH_OK } /* expect buffer overwrite */ + }; + + for(int i = 0; i < (int)(sizeof(playlist)/sizeof(*playlist)); i++) { + const char *name = playlist[i].name; + size_t olen = 100000; + struct demo victim; + DOHcode d; + + victim.canary1 = 87; /* magic numbers, arbritrarily picked */ + victim.canary2 = 35; + victim.canary3 = 41; + d = doh_encode(name, DNS_TYPE_A, victim.dohbuffer, + sizeof(struct demo), /* allow room for overflow */ + &olen); + + fail_unless(d == playlist[i].expected_result, + "result returned was not as expected"); + if(d == playlist[i].expected_result) { + if(name == max) { + fail_if(victim.canary1 == 87, + "demo one-byte buffer overwrite did not happen"); + } + else { + fail_unless(victim.canary1 == 87, + "one-byte buffer overwrite has happened"); + } + fail_unless(victim.canary2 == 35, + "two-byte buffer overwrite has happened"); + fail_unless(victim.canary3 == 41, + "three-byte buffer overwrite has happened"); + } + else { + if(d == DOH_OK) { + fail_unless(olen <= sizeof(victim.dohbuffer), "wrote outside bounds"); + fail_unless(olen > strlen(name), "unrealistic low size"); + } + } } } while(0); @@ -84,6 +140,7 @@ do { const size_t magic1 = 9765; size_t olen1 = magic1; const char *sunshine1 = "a.com"; + const char *dotshine1 = "a.com."; const char *sunshine2 = "aa.com"; size_t olen2; DOHcode ret2; @@ -94,6 +151,13 @@ do { fail_if(olen1 == magic1, "olen has not been assigned properly"); fail_unless(olen1 > strlen(sunshine1), "bad out length"); + /* with a trailing dot, the response should have the same length */ + olen2 = magic1; + ret2 = doh_encode(dotshine1, dnstype, buffer, buflen, &olen2); + fail_unless(ret2 == DOH_OK, "dotshine case should pass fine"); + fail_if(olen2 == magic1, "olen has not been assigned properly"); + fail_unless(olen1 == olen2, "olen should not grow for a trailing dot"); + /* add one letter, the response should be one longer */ olen2 = magic1; ret2 = doh_encode(sunshine2, dnstype, buffer, buflen, &olen2); |