aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--lib/doh.c76
-rw-r--r--lib/doh.h6
-rw-r--r--tests/unit/unit1655.c118
3 files changed, 147 insertions, 53 deletions
diff --git a/lib/doh.c b/lib/doh.c
index d1795789e..73ccce95e 100644
--- a/lib/doh.c
+++ b/lib/doh.c
@@ -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;
diff --git a/lib/doh.h b/lib/doh.h
index f522d3308..fc053eddf 100644
--- a/lib/doh.h
+++ b/lib/doh.h
@@ -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);