doh: improced both encoding and decoding

Improved estimation of expected_len and updated related comments;
increased strictness of QNAME-encoding, adding error detection for empty
labels and names longer than the overall limit; avoided treating DNAME
as unexpected;

updated unit test 1655 with more thorough set of proofs and tests

Closes #4598

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);