sock_dns: fix out-of-bound errors

Fixes RIOT-OS#10739
miri64 · Jan 10, 2019 · 7908809 · 7908809
1 parent d99d72c
commit 7908809
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 10 deletions.
diff --git a/Makefile.dep b/Makefile.dep
@@ -689,6 +689,7 @@ endif
 
 ifneq (,$(filter sock_dns,$(USEMODULE)))
   USEMODULE += sock_util
+  USEMODULE += posix
 endif
 
 ifneq (,$(filter sock_util,$(USEMODULE)))

diff --git a/sys/net/application_layer/dns/dns.c b/sys/net/application_layer/dns/dns.c
@@ -15,6 +15,7 @@
  * @}
  */
 
+#include <arpa/inet.h>
 #include <string.h>
 #include <stdio.h>
 
@@ -73,55 +74,89 @@ static unsigned _get_short(uint8_t *buf)
     return _tmp;
 }
 
-static size_t _skip_hostname(uint8_t *buf)
+static ssize_t _skip_hostname(const uint8_t *buf, size_t len, uint8_t *bufpos)
 {
-    uint8_t *bufpos = buf;
+    const uint8_t *buflim = buf + len;
 
+    if (bufpos >= buflim) {
+        /* out-of-bound */
+        return -EBADMSG;
+    }
     /* handle DNS Message Compression */
     if (*bufpos >= 192) {
+        if ((bufpos + 2) >= buflim) {
+            return -EBADMSG;
+        }
         return 2;
     }
 
-    while(*bufpos) {
+    while (*bufpos) {
         bufpos += *bufpos + 1;
+        if (bufpos >= buflim) {
+            /* out-of-bound */
+            return -EBADMSG;
+        }
     }
     return (bufpos - buf + 1);
 }
 
 static int _parse_dns_reply(uint8_t *buf, size_t len, void* addr_out, int family)
 {
+    const uint8_t *buflim = buf + len;
     sock_dns_hdr_t *hdr = (sock_dns_hdr_t*) buf;
     uint8_t *bufpos = buf + sizeof(*hdr);
 
     /* skip all queries that are part of the reply */
     for (unsigned n = 0; n < ntohs(hdr->qdcount); n++) {
-        bufpos += _skip_hostname(bufpos);
+        ssize_t tmp = _skip_hostname(buf, len, bufpos);
+        if (tmp < 0) {
+            return tmp;
+        }
+        bufpos += tmp;
         bufpos += 4;    /* skip type and class of query */
     }
 
     for (unsigned n = 0; n < ntohs(hdr->ancount); n++) {
-        bufpos += _skip_hostname(bufpos);
+        ssize_t tmp = _skip_hostname(buf, len, bufpos);
+        if (tmp < 0) {
+            return tmp;
+        }
+        bufpos += tmp;
+        if ((bufpos + 2 + 2 + 4) >= buflim) {
+            return -EBADMSG;
+        }
         uint16_t _type = ntohs(_get_short(bufpos));
         bufpos += 2;
         uint16_t class = ntohs(_get_short(bufpos));
         bufpos += 2;
         bufpos += 4; /* skip ttl */
 
         unsigned addrlen = ntohs(_get_short(bufpos));
-        bufpos += 2;
-        if ((bufpos + addrlen) > (buf + len)) {
-            return -EBADMSG;
-        }
-
         /* skip unwanted answers */
         if ((class != DNS_CLASS_IN) ||
                 ((_type == DNS_TYPE_A) && (family == AF_INET6)) ||
                 ((_type == DNS_TYPE_AAAA) && (family == AF_INET)) ||
                 ! ((_type == DNS_TYPE_A) || ((_type == DNS_TYPE_AAAA))
                     )) {
+            if (addrlen > len) {
+                /* buffer wraps around memory space */
+                return -EBADMSG;
+            }
             bufpos += addrlen;
+            /* other out-of-bound is checked in `_skip_hostname()` at start of
+             * loop */
             continue;
         }
+        if (((addrlen != INADDRSZ) && (family == AF_INET)) ||
+            ((addrlen != IN6ADDRSZ) && (family == AF_INET6)) ||
+            ((addrlen != IN6ADDRSZ) && (addrlen != INADDRSZ) &&
+             (family == AF_UNSPEC))) {
+            return -EBADMSG;
+        }
+        bufpos += 2;
+        if ((bufpos + addrlen) >= buflim) {
+            return -EBADMSG;
+        }
 
         memcpy(addr_out, bufpos, addrlen);
         return addrlen;