diff --git a/src/resolver.c b/src/resolver.c index 087ef1e..16a8668 100644 --- a/src/resolver.c +++ b/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[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. */ @@ -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) - 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; } @@ -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 */ - 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/resolver.h b/src/resolver.h index a690d76..ac4653e 100644 --- a/src/resolver.h +++ b/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__ */ diff --git a/src/util.h b/src/util.h index 1a7baf7..0168753 100644 --- 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/test_resolver.c b/tests/test_resolver.c index 05844ea..31c873b 100644 --- a/tests/test_resolver.c +++ b/tests/test_resolver.c @@ -13,6 +13,7 @@ #include #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;