Merge remote-tracking branch 'upstream/master'

Conflicts: Makefile.am README.markdown src/resolver.h tests/check_parser.c tests/test_resolver.c
profanity-im · Sep 17, 2016 · e3db0e9 · e3db0e9
2 parents c4580c9 + 9931ad4
commit e3db0e9
Show file tree

Hide file tree

Showing 7 changed files with 175 additions and 39 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -1,4 +1,6 @@
 0.9.1
+	- Fixed bug #95 (DNS lookup failing on Cygwin)
+	- Removed dependency on the check package
 
 0.9.0
 	- IPv6 support

diff --git a/Makefile.am b/Makefile.am
@@ -75,9 +75,9 @@ TESTS = tests/check_parser tests/test_sha1 tests/test_md5 tests/test_rand \
 check_PROGRAMS = $(TESTS)
 
 tests_check_parser_SOURCES = tests/check_parser.c tests/test.h
-tests_check_parser_CFLAGS = @check_CFLAGS@ $(PARSER_CFLAGS) $(MESODE_FLAGS) \
+tests_check_parser_CFLAGS = $(PARSER_CFLAGS) $(MESODE_FLAGS) \
 	-I$(top_srcdir)/src
-tests_check_parser_LDADD = @check_LIBS@ $(MESODE_LIBS)
+tests_check_parser_LDADD = $(MESODE_LIBS)
 tests_check_parser_LDFLAGS = -static
 
 tests_test_ctx_SOURCES = tests/test_ctx.c

diff --git a/configure.ac b/configure.ac
@@ -35,9 +35,6 @@ PARSER_LIBS=$expat_LIBS
 AC_ARG_ENABLE([tls],
     [AS_HELP_STRING([--disable-tls], [disable TLS support])])
 
-PKG_CHECK_MODULES([check], [check >= 0.9.4],
-    [], [AC_MSG_WARN([libcheck not found; unit tests will not be compilable])])
-
 if test "x$enable_tls" != xno; then
     PKG_CHECK_MODULES([openssl], [openssl],
         [PC_REQUIRES="openssl ${PC_REQUIRES}"],

diff --git a/src/resolver.c b/src/resolver.c
@@ -20,6 +20,7 @@
 
 #include "ostypes.h"
 #include "snprintf.h"
+#include "util.h"               /* xmpp_min */
 #include "resolver.h"
 
 #define MESSAGE_HEADER_LEN 12
@@ -55,41 +56,90 @@ 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)
 {
     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[name_len++] = '.';
-                strncpy(&name[name_len], (char *)&buf[i], label_len);
+                name_len = message_name_append_safe(name, name_len, name_max,
+                                                    (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],
-                                   name_max - name_len);
-            /* pointer is always the last */
+            if (name != NULL && name_len >= name_max && name_max > 0) {
+                /* We have filled the name buffer. Don't pass it recursively. */
+                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. */
         } else {
             return 0;
         }
     }
-    if (label_len == 0 && name != NULL)
-        name[name_len] = '\0';
+    if (label_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;
 }
@@ -157,6 +207,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)
 {
@@ -190,17 +249,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 */
-            return XMPP_DOMAIN_NOT_FOUND;
-        }
+        /* error in name format */
+        if (name_len == 0) 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]);

diff --git a/src/util.h b/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);
 

diff --git a/tests/check_parser.c b/tests/check_parser.c
@@ -10,27 +10,34 @@
 */
 
 #include <stdlib.h>
+#include <stdio.h>
 #include <string.h>
 
-#include <check.h>
-
 #include <mesode.h>
 #include "parser.h"
 
 #include "test.h"
 
-START_TEST(create_destroy)
+#define fail_unless(expr) do {                  \
+    int result = (expr);                        \
+    if (!result) {                              \
+        printf("%s:%d: Assertion failed: %s\n", \
+               __FILE__, __LINE__, #expr);      \
+        exit(1);                                \
+    }                                           \
+} while (0)
+
+static void create_destroy(void)
 {
     xmpp_ctx_t *ctx;
     parser_t *parser;
 
     ctx = xmpp_ctx_new(NULL, NULL);
     parser = parser_new(ctx, NULL, NULL, NULL, NULL);
-    fail_unless(parser != NULL, "Parser creation failed.");
+    fail_unless(parser != NULL);
     parser_free(parser);
     xmpp_ctx_free(ctx);
 }
-END_TEST
 
 int cbtest_got_start = 0;
 void cbtest_handle_start(char *name, char **attrs, void *userdata)
@@ -53,7 +60,7 @@ void cbtest_handle_stanza(xmpp_stanza_t *stanza, void *userdata)
         cbtest_got_stanza = 1;
 }
 
-START_TEST(callbacks)
+static void callbacks(void)
 {
     xmpp_ctx_t *ctx;
     parser_t *parser;
@@ -79,16 +86,18 @@ START_TEST(callbacks)
     parser_free(parser);
     xmpp_ctx_free(ctx);
 }
-END_TEST
 
-Suite *parser_suite(void)
+int main()
 {
-    Suite *s = suite_create("Parser");
-    TCase *tc_core = tcase_create("Core");
-    tcase_add_test(tc_core, create_destroy);
-    tcase_add_test(tc_core, callbacks);
-    suite_add_tcase(s, tc_core);
-    return s;
-}
+    printf("XML parser tests.\n");
 
-TEST_MAIN
+    printf("create-destroy: ");
+    create_destroy();
+    printf("ok\n");
+
+    printf("callbacks: ");
+    callbacks();
+    printf("ok\n");
+
+    return 0;
+}
diff --git a/tests/test_resolver.c b/tests/test_resolver.c
@@ -13,6 +13,7 @@
 #include <stdio.h>
 
 #include "mesode.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,34 @@ 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.
 };
+/* res_query("_xmpp-client._tcp.jabber.org", C_IN, T_SRV, ...) with pointers */
+static const unsigned char data5[] = {
+    0x00, 0x00, 0x81, 0x80, 0x00, 0x01, 0x00, 0x02, 0x00, 0x00,    // ..........
+    0x00, 0x00, 0x0c, 0x5f, 0x78, 0x6d, 0x70, 0x70, 0x2d, 0x63,    // ..._xmpp-c
+    0x6c, 0x69, 0x65, 0x6e, 0x74, 0x04, 0x5f, 0x74, 0x63, 0x70,    // lient._tcp
+    0x06, 0x6a, 0x61, 0x62, 0x62, 0x65, 0x72, 0x03, 0x6f, 0x72,    // .jabber.or
+    0x67, 0x00, 0x00, 0x21, 0x00, 0x01, 0xc0, 0x0c, 0x00, 0x21,    // g..!.....!
+    0x00, 0x01, 0x00, 0x00, 0x01, 0x9e, 0x00, 0x12, 0x00, 0x1f,    // ..........
+    0x00, 0x1e, 0x14, 0x66, 0x09, 0x68, 0x65, 0x72, 0x6d, 0x65,    // ...f.herme
+    0x73, 0x32, 0x76, 0x36, 0xc0, 0x1e, 0xc0, 0x0c, 0x00, 0x21,    // s2v6.....!
+    0x00, 0x01, 0x00, 0x00, 0x01, 0x9e, 0x00, 0x10, 0x00, 0x1e,    // ..........
+    0x00, 0x1e, 0x14, 0x66, 0x07, 0x68, 0x65, 0x72, 0x6d, 0x65,    // ...f.herme
+    0x73, 0x32, 0xc0, 0x1e,                                        // s2..
+};
+/* hacked data2 with two empty-string targets. */
+static const unsigned char data6[] = {
+    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,22 +167,38 @@ static const struct {
         .port = 5222,
         .target_nr = 2,
     },
+    {
+        .data = data5,
+        .len = sizeof(data5),
+        .target = "hermes2.jabber.org",
+        .port = 5222,
+        .target_nr = 2,
+    },
+    {
+        .data = data6,
+        .len = sizeof(data6),
+        .target = "",
+        .port = 5222,
+        .target_nr = 2,
+    },
 };
 
 static int srv_rr_list_len(resolver_srv_rr_t *list)
 {
     int nr;
 
-    for(nr = 0; list != NULL; ++nr, list = list->next);
+    for (nr = 0; list != NULL; ++nr, list = list->next);
 
     return nr;
 }
 
 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 +231,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;