resolver: fixed few bugs

Fixed a bug which caused resolver to return a wrong target name with a
missed ".". The bug was in the part of message_name_get() where a pointer
is handled.
Added checks for buffer overflow.
Added 2 test cases to check a corner case and error handling.

src/resolver.c

@@ -23,6 +23,7 @@
 
 #include "ostypes.h"
 #include "snprintf.h"
+#include "util.h"               /* xmpp_min */
 #include "resolver.h"
 
 #define MESSAGE_HEADER_LEN 12
@@ -65,6 +66,26 @@ static uint8_t message_header_rcode(const struct message_header *header)
     return header->octet3 & 0x0f;
 }
 
+/*
+ * Append a label or a dot to the target name with buffer overflow checks.
+ * Returns length of the non-truncated resulting string, may be bigger than
+ * name_max.
+ */
+static size_t message_name_append_safe(char *name, size_t name_len,
+                                       size_t name_max,
+                                       const char *tail, size_t tail_len)
+{
+    size_t copy_len;
+
+    copy_len = name_max > name_len ? name_max - name_len : 0;
+    copy_len = xmpp_min(tail_len, copy_len);
+    if (copy_len > 0)
+        strncpy(&name[name_len], tail, copy_len);
+
+    return name_len + tail_len;
+}
+
+/* Returns length of the compressed name. This is NOT the same as strlen(). */
 static unsigned message_name_get(const unsigned char *buf, size_t buf_len,
                                  unsigned buf_offset,
                                  char *name, size_t name_max)
@@ -72,25 +93,41 @@ static unsigned message_name_get(const unsigned char *buf, size_t buf_len,
     size_t name_len = 0;
     unsigned i = buf_offset;
     unsigned pointer;
+    unsigned rc;
     unsigned char label_len;
 
-    while ((label_len = buf[i++]) != 0) {
+
-        /* label */
+    while (1) {
+        if (i >= buf_len) return 0;
+        label_len = buf[i++];
+        if (label_len == 0) break;
+
+        /* Label */
         if ((label_len & 0xc0) == 0) {
+            if (i + label_len - 1 >= buf_len) return 0;
             if (name != NULL) {
-                if (name_len != 0)
+                name_len = message_name_append_safe(name, name_len, name_max,
-                    name[name_len++] = '.';
+                                                    (char *)&buf[i], label_len);
-                strncpy(&name[name_len], (char *)&buf[i], label_len);
+                name_len = message_name_append_safe(name, name_len, name_max,
+                                                    ".", 1);
             }
             i += label_len;
-            name_len += label_len;
 
-        /* pointer */
+        /* Pointer */
         } else if ((label_len & 0xc0) == 0xc0) {
+            if (i >= buf_len) return 0;
             pointer = (label_len & 0x3f) << 8 | buf[i++];
-            (void)message_name_get(buf, buf_len, pointer, &name[name_len],
+            if (name != NULL && name_len >= name_max && name_max > 0) {
-                                   name_max - name_len);
+                /* We have filled the name buffer. Don't pass it recursively. */
-            /* pointer is always the last */
+                name[name_max - 1] = '\0';
+                name = NULL;
+                name_max = 0;
+            }
+            rc = message_name_get(buf, buf_len, pointer,
+                                  name != NULL ? &name[name_len] : NULL,
+                                  name_max > name_len ? name_max - name_len : 0);
+            if (rc == 0) return 0;
+            /* Pointer is always the last. */
             break;
 
         /* The 10 and 01 combinations are reserved for future use. */
@@ -98,8 +135,21 @@ static unsigned message_name_get(const unsigned char *buf, size_t buf_len,
             return 0;
         }
     }
-    if (label_len == 0 && name != NULL)
+    if (label_len == 0) {
-        name[name_len] = '\0';
+        if (name_len == 0) name_len = 1;
+        /*
+         * At this point name_len is length of the resulting name,
+         * including '\0'. This value can be exported to allocate buffer
+         * of precise size.
+         */
+        if (name != NULL && name_max > 0) {
+            /*
+             * Overwrite leading '.' with a '\0'. If the resulting name is
+             * bigger than name_max it is truncated.
+             */
+            name[xmpp_min(name_len, name_max) - 1] = '\0';
+        }
+    }
 
     return i - buf_offset;
 }
@@ -167,6 +217,15 @@ static void resolver_srv_list_sort(resolver_srv_rr_t **srv_rr_list)
     *srv_rr_list = rr_head;
 }
 
+#define BUF_OVERFLOW_CHECK(ptr, len) do {         \
+    if ((ptr) >= (len)) {                         \
+        if (*srv_rr_list != NULL)                 \
+            resolver_srv_free(ctx, *srv_rr_list); \
+        *srv_rr_list = NULL;                      \
+        return XMPP_DOMAIN_NOT_FOUND;             \
+    }                                             \
+} while (0)
+
 int resolver_srv_lookup_buf(xmpp_ctx_t *ctx, const unsigned char *buf,
                             size_t len, resolver_srv_rr_t **srv_rr_list)
 {
@@ -200,17 +259,20 @@ int resolver_srv_lookup_buf(xmpp_ctx_t *ctx, const unsigned char *buf,
 
     /* skip question section */
     for (i = 0; i < header.qdcount; ++i) {
+        BUF_OVERFLOW_CHECK(j, len);
         name_len = message_name_len(buf, len, j);
-        if (name_len == 0) {
+        /* error in name format */
-            /* error in name format */
+        if (name_len == 0) return XMPP_DOMAIN_NOT_FOUND;
-            return XMPP_DOMAIN_NOT_FOUND;
-        }
         j += name_len + 4;
     }
 
     for (i = 0; i < header.ancount; ++i) {
+        BUF_OVERFLOW_CHECK(j, len);
         name_len = message_name_len(buf, len, j);
+        /* error in name format */
+        if (name_len == 0) return XMPP_DOMAIN_NOT_FOUND;
         j += name_len;
+        BUF_OVERFLOW_CHECK(j + 16, len);
         type = xmpp_ntohs_ptr(&buf[j]);
         class = xmpp_ntohs_ptr(&buf[j + 2]);
         rdlength = xmpp_ntohs_ptr(&buf[j + 8]);

src/resolver.h

@@ -65,4 +65,5 @@ int resolver_srv_lookup(xmpp_ctx_t *ctx, const char *service, const char *proto,
  *  @param srv_rr_list a list allocated by lookup functions
  */
 void resolver_srv_free(xmpp_ctx_t *ctx, resolver_srv_rr_t *srv_rr_list);
+
 #endif /* __LIBSTROPHE_RESOLVER_H__ */

src/util.h

@@ -18,6 +18,9 @@
 
 #include "ostypes.h"
 
+/* TODO evaluate x and y only once */
+#define xmpp_min(x, y) ((x) < (y) ? (x) : (y))
+
 /* string functions */
 char *xmpp_strtok_r(char *s, const char *delim, char **saveptr);
 

tests/test_resolver.c

@@ -13,6 +13,7 @@
 #include <stdio.h>
 
 #include "strophe.h"
+#include "rand.h"
 #include "resolver.h"
 #include "test.h"
 
@@ -84,7 +85,6 @@ static const unsigned char data3[] = {
     0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x03, 0x63, 0x6f,
     0x6d, 0x00,
 };
-
 /* res_query("_xmpp-client._tcp.jabber.calyxinstitute.org", C_IN, T_SRV, ...) */
 static const unsigned char data4[] = {
     0x8d, 0x58, 0x81, 0x80, 0x00, 0x01, 0x00, 0x02, 0x00, 0x00,    // .X........
@@ -103,6 +103,20 @@ static const unsigned char data4[] = {
     0x61, 0x6c, 0x79, 0x78, 0x69, 0x6e, 0x73, 0x74, 0x69, 0x74,    // alyxinstit
     0x75, 0x74, 0x65, 0x03, 0x6f, 0x72, 0x67, 0x00,                // ute.org.
 };
+/* hacked data2 with two empty-string targets. */
+static const unsigned char data5[] = {
+    0xf2, 0x98, 0x81, 0x80, 0x00, 0x01, 0x00, 0x02,
+    0x00, 0x00, 0x00, 0x00, 0x0c, 0x5f, 0x78, 0x6d,
+    0x70, 0x70, 0x2d, 0x63, 0x6c, 0x69, 0x65, 0x6e,
+    0x74, 0x04, 0x5f, 0x74, 0x63, 0x70, 0x06, 0x6a,
+    0x61, 0x62, 0x62, 0x65, 0x72, 0x03, 0x6f, 0x72,
+    0x67, 0x00, 0x00, 0x21, 0x00, 0x01, 0xc0, 0x0c,
+    0x00, 0x21, 0x00, 0x01, 0x00, 0x00, 0x03, 0x83,
+    0x00, 0x07, 0x00, 0x1e, 0x00, 0x1e, 0x14, 0x66,
+    0x00, 0xc0, 0x0c, 0x00, 0x21, 0x00, 0x01, 0x00,
+    0x00, 0x03, 0x83, 0x00, 0x08, 0x00, 0x1f, 0x00,
+    0x1e, 0x14, 0x66, 0xc0, 0x40,
+};
 
 static const struct {
     const unsigned char *data;
@@ -139,6 +153,13 @@ static const struct {
         .port = 5222,
         .target_nr = 2,
     },
+    {
+        .data = data5,
+        .len = sizeof(data5),
+        .target = "",
+        .port = 5222,
+        .target_nr = 2,
+    },
 };
 
 static int srv_rr_list_len(resolver_srv_rr_t *list)
@@ -153,8 +174,10 @@ static int srv_rr_list_len(resolver_srv_rr_t *list)
 int main(int argc, char **argv)
 {
     xmpp_ctx_t *ctx;
+    xmpp_rand_t *rand;
     resolver_srv_rr_t *srv_rr_list;
     char *domain;
+    unsigned char *buf;
     unsigned short port;
     size_t i;
     int ret;
@@ -187,6 +210,25 @@ int main(int argc, char **argv)
         resolver_srv_free(ctx, srv_rr_list);
     }
 
+    /*
+     * The next test case must not crash and is supposed to be checked
+     * under valgrind.
+     */
+    printf("Test of a broken message: ");
+    rand = xmpp_rand_new(ctx);
+    assert(rand != NULL);
+    assert(sizeof(data2) > 64);
+    buf = xmpp_alloc(ctx, sizeof(data2));
+    assert(buf != NULL);
+    memcpy(buf, data2, 64);
+    xmpp_rand_bytes(rand, &buf[64], sizeof(data2) - 64);
+    ret = resolver_srv_lookup_buf(ctx, buf, sizeof(data2), &srv_rr_list);
+    if (ret == XMPP_DOMAIN_FOUND && srv_rr_list != NULL)
+        resolver_srv_free(ctx, srv_rr_list);
+    xmpp_free(ctx, buf);
+    xmpp_rand_free(ctx, rand);
+    printf("ok\n");
+
     xmpp_ctx_free(ctx);
 
     return 0;