Skip to content

Commit

Permalink
resolver: fixed few bugs
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@pasis
pasis committed Sep 15, 2016
1 parent f0706f5 commit c6aaa96
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 17 deletions.
94 changes: 78 additions & 16 deletions src/resolver.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

#include "ostypes.h"
#include "snprintf.h"
#include "util.h" /* xmpp_min */
#include "resolver.h"

#define MESSAGE_HEADER_LEN 12
Expand Down Expand Up @@ -65,41 +66,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;
}
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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]);
Expand Down
1 change: 1 addition & 0 deletions src/resolver.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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__ */
3 changes: 3 additions & 0 deletions src/util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
44 changes: 43 additions & 1 deletion tests/test_resolver.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <stdio.h>

#include "strophe.h"
#include "rand.h"
#include "resolver.h"
#include "test.h"

Expand Down Expand Up @@ -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........
Expand All @@ -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;
Expand Down Expand Up @@ -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)
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit c6aaa96

Please sign in to comment.