Merge pull request #10740 from miri64/sock_dns/bug/i10739

sock_dns: fix out-of-bound errors

Makefile.dep

@@ -689,6 +689,7 @@ endif
 
 ifneq (,$(filter sock_dns,$(USEMODULE)))
   USEMODULE += sock_util
+  USEMODULE += posix
 endif
 
 ifneq (,$(filter sock_util,$(USEMODULE)))

sys/include/net/dns.h

@@ -0,0 +1,42 @@
+/*
+ * Copyright (C) 2019 Freie Universität Berlin
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @defgroup    net_dns DNS defines
+ * @ingroup     net
+ * @brief       Generic DNS values
+ * @{
+ *
+ * @file
+ * @brief   Generic DNS values
+ *
+ * @author  Martine Lenders <m.lenders@fu-berlin.de>
+ */
+#ifndef NET_DNS_H
+#define NET_DNS_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @name    Field lengths
+ * @{
+ */
+#define RR_TYPE_LENGTH      (2U)
+#define RR_CLASS_LENGTH     (2U)
+#define RR_TTL_LENGTH       (4U)
+#define RR_RDLENGTH_LENGTH  (2U)
+/** @} */
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* NET_DNS_H */
+/** @} */

sys/net/application_layer/dns/dns.c

@@ -15,9 +15,11 @@
  * @}
  */
 
+#include <arpa/inet.h>
 #include <string.h>
 #include <stdio.h>
 
+#include "net/dns.h"
 #include "net/sock/udp.h"
 #include "net/sock/dns.h"
 
@@ -73,55 +75,91 @@ 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;
+    unsigned res = 0;
 
+    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[res]) {
-        bufpos += *bufpos + 1;
+        res += bufpos[res] + 1;
+        if ((&bufpos[res]) >= buflim) {
+            /* out-of-bound */
+            return -EBADMSG;
+        }
     }
-    return (bufpos - buf + 1);
+    return res + 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);
-        bufpos += 4;    /* skip type and class of query */
+        if (tmp < 0) {
+            return tmp;
+        }
+        bufpos += tmp;
+        /* skip type and class of query */
+        bufpos += (RR_TYPE_LENGTH + RR_CLASS_LENGTH);
     }
 
     for (unsigned n = 0; n < ntohs(hdr->ancount); n++) {
-        bufpos += _skip_hostname(bufpos);
+        ssize_t tmp = _skip_hostname(buf, len, bufpos);
-        uint16_t _type = ntohs(_get_short(bufpos));
+        if (tmp < 0) {
-        bufpos += 2;
+            return tmp;
-        uint16_t class = ntohs(_get_short(bufpos));
+        }
-        bufpos += 2;
+        bufpos += tmp;
-        bufpos += 4; /* skip ttl */
+        if ((bufpos + RR_TYPE_LENGTH + RR_CLASS_LENGTH + RR_TTL_LENGTH) >= buflim) {
-
-        unsigned addrlen = ntohs(_get_short(bufpos));
-        bufpos += 2;
-        if ((bufpos + addrlen) > (buf + len)) {
             return -EBADMSG;
         }
+        uint16_t _type = ntohs(_get_short(bufpos));
+        bufpos += RR_TYPE_LENGTH;
+        uint16_t class = ntohs(_get_short(bufpos));
+        bufpos += RR_CLASS_LENGTH;
+        bufpos += RR_TTL_LENGTH; /* skip ttl */
 
+        unsigned addrlen = ntohs(_get_short(bufpos));
         /* 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 += RR_RDLENGTH_LENGTH;
+        if ((bufpos + addrlen) >= buflim) {
+            return -EBADMSG;
+        }
 
         memcpy(addr_out, bufpos, addrlen);
         return addrlen;
@@ -174,7 +212,6 @@ int sock_dns_query(const char *domain_name, void *addr_out, int family)
 
     ssize_t res = sock_udp_create(&sock_dns, NULL, &sock_dns_server, 0);
     if (res) {
-            puts("a");
         goto out;
     }