From 29d31ba919cbcc4a6d8046db1abd013250028043 Mon Sep 17 00:00:00 2001 From: cpq Date: Thu, 17 Dec 2020 19:14:17 +0000 Subject: [PATCH] More boundary checks for mg_dns_parse() --- mongoose.c | 22 ++++++++++++++-------- src/dns.c | 22 ++++++++++++++-------- test/unit_test.c | 13 +++++++++---- 3 files changed, 37 insertions(+), 20 deletions(-) diff --git a/mongoose.c b/mongoose.c index 5f59a03e..73672160 100644 --- a/mongoose.c +++ b/mongoose.c @@ -165,25 +165,28 @@ void mg_resolve_cancel(struct mg_mgr *mgr, struct mg_connection *c) { static size_t mg_dns_parse_name(const uint8_t *s, const uint8_t *e, size_t off, char *to, size_t tolen, int depth) { size_t i = 0, j = 0; + if (tolen > 0) to[0] = '\0'; if (depth > 5) return 0; while (&s[off + i + 1] < e && s[off + i] > 0) { size_t n = s[off + i]; if (n & 0xc0) { size_t ptr = (((n & 0x3f) << 8) | s[off + i + 1]) - 12; // 12 is hdr len if (&s[ptr + 1] < e && (s[ptr] & 0xc0) == 0) { - j = mg_dns_parse_name(s, e, ptr, to, tolen, depth + 1); + mg_dns_parse_name(s, e, ptr, to, tolen, depth + 1); } i++; break; } if (&s[off + i + n + 1] >= e) break; - if (j > 0 && j < tolen) to[j++] = '.'; - if (j + n < tolen) memcpy(&to[j], &s[off + i + 1], n); + if (j + n + 1 >= tolen) return 0; // Error - overflow + if (j > 0) to[j++] = '.'; + memcpy(&to[j], &s[off + i + 1], n); j += n; i += n + 1; + to[j] = '\0'; // Zero-terminate this chunk + LOG(LL_DEBUG, ("-- %zu/%zu %zu %zu", i, e - s, j, n)); } - if (j < tolen) to[j] = '\0'; // Zero-terminate the name - to[tolen - 1] = '\0'; // Just in case + if (tolen > 0) to[tolen - 1] = '\0'; // Make sure make sure it is nul-term return i; } @@ -195,14 +198,17 @@ int mg_dns_parse(const uint8_t *buf, size_t len, struct mg_dns_message *dm) { struct mg_dns_header *h = (struct mg_dns_header *) buf; const uint8_t *s = buf + sizeof(*h), *e = &buf[len]; size_t i, j = 0, n, ok = 0; - if (len < sizeof(*h)) return ok; + if (len < sizeof(*h)) return ok; // Too small, headers dont fit + if (len > 512) return ok; // Too large, we don't expect that + if (mg_ntohs(h->num_questions) > 2) return 0; // Sanity + if (mg_ntohs(h->num_answers) > 5) return 0; // Sanity for (i = 0; i < mg_ntohs(h->num_questions); i++) { j += mg_dns_parse_name(s, e, j, dm->name, sizeof(dm->name), 0) + 5; - // LOG(LL_INFO, ("QUE [%s]", name)); + LOG(LL_INFO, ("QUE %zu %zu [%s]", i, j, dm->name)); } for (i = 0; i < mg_ntohs(h->num_answers); i++) { j += mg_dns_parse_name(s, e, j, dm->name, sizeof(dm->name), 0) + 9; - // LOG(LL_DEBUG, ("NAME %s", name)); + LOG(LL_DEBUG, ("NAME %s", dm->name)); if (&s[j] + 2 > e) break; n = ((int) s[j] << 8) | s[j + 1]; if (&s[j] + 2 + n > e) break; diff --git a/src/dns.c b/src/dns.c index 63cf0bac..9a9ffd1e 100644 --- a/src/dns.c +++ b/src/dns.c @@ -39,25 +39,28 @@ void mg_resolve_cancel(struct mg_mgr *mgr, struct mg_connection *c) { static size_t mg_dns_parse_name(const uint8_t *s, const uint8_t *e, size_t off, char *to, size_t tolen, int depth) { size_t i = 0, j = 0; + if (tolen > 0) to[0] = '\0'; if (depth > 5) return 0; while (&s[off + i + 1] < e && s[off + i] > 0) { size_t n = s[off + i]; if (n & 0xc0) { size_t ptr = (((n & 0x3f) << 8) | s[off + i + 1]) - 12; // 12 is hdr len if (&s[ptr + 1] < e && (s[ptr] & 0xc0) == 0) { - j = mg_dns_parse_name(s, e, ptr, to, tolen, depth + 1); + mg_dns_parse_name(s, e, ptr, to, tolen, depth + 1); } i++; break; } if (&s[off + i + n + 1] >= e) break; - if (j > 0 && j < tolen) to[j++] = '.'; - if (j + n < tolen) memcpy(&to[j], &s[off + i + 1], n); + if (j + n + 1 >= tolen) return 0; // Error - overflow + if (j > 0) to[j++] = '.'; + memcpy(&to[j], &s[off + i + 1], n); j += n; i += n + 1; + to[j] = '\0'; // Zero-terminate this chunk + LOG(LL_DEBUG, ("-- %zu/%zu %zu %zu", i, e - s, j, n)); } - if (j < tolen) to[j] = '\0'; // Zero-terminate the name - to[tolen - 1] = '\0'; // Just in case + if (tolen > 0) to[tolen - 1] = '\0'; // Make sure make sure it is nul-term return i; } @@ -69,14 +72,17 @@ int mg_dns_parse(const uint8_t *buf, size_t len, struct mg_dns_message *dm) { struct mg_dns_header *h = (struct mg_dns_header *) buf; const uint8_t *s = buf + sizeof(*h), *e = &buf[len]; size_t i, j = 0, n, ok = 0; - if (len < sizeof(*h)) return ok; + if (len < sizeof(*h)) return ok; // Too small, headers dont fit + if (len > 512) return ok; // Too large, we don't expect that + if (mg_ntohs(h->num_questions) > 2) return 0; // Sanity + if (mg_ntohs(h->num_answers) > 5) return 0; // Sanity for (i = 0; i < mg_ntohs(h->num_questions); i++) { j += mg_dns_parse_name(s, e, j, dm->name, sizeof(dm->name), 0) + 5; - // LOG(LL_INFO, ("QUE [%s]", name)); + LOG(LL_INFO, ("QUE %zu %zu [%s]", i, j, dm->name)); } for (i = 0; i < mg_ntohs(h->num_answers); i++) { j += mg_dns_parse_name(s, e, j, dm->name, sizeof(dm->name), 0) + 9; - // LOG(LL_DEBUG, ("NAME %s", name)); + LOG(LL_DEBUG, ("NAME %s", dm->name)); if (&s[j] + 2 > e) break; n = ((int) s[j] << 8) | s[j + 1]; if (&s[j] + 2 + n > e) break; diff --git a/test/unit_test.c b/test/unit_test.c index 752c4fb1..20e4c274 100644 --- a/test/unit_test.c +++ b/test/unit_test.c @@ -964,8 +964,6 @@ static void test_dns(void) { 0x74, 0x61, 0x03, 0x63, 0x6f, 0x6d, 0, 0, 1, 0, 1, 0xc0, 0x0c, 0, 1, 0, 1, 0, 0, 2, 0x57, 0, 4, 0x94, 0xfb, 0x36, 0xec}; - // char *data = mg_file_read("dns.bin"); - // ASSERT(data != NULL); ASSERT(mg_dns_parse(NULL, 0, &dm) == 0); ASSERT(mg_dns_parse(data, sizeof(data), &dm) == 1); ASSERT(strcmp(dm.name, "cesanta.com") == 0); @@ -973,8 +971,15 @@ static void test_dns(void) { memset(&dm, 0, sizeof(dm)); ASSERT(mg_dns_parse(data, sizeof(data), &dm) == 1); ASSERT(strcmp(dm.name, "") == 0); - // ASSERT(mg_dns_parse((uint8_t *) data, strlen(data), &dm) == 0); - // free(data); + +#if 0 + { + char *data = mg_file_read("dns.bin"); + ASSERT(data != NULL); + ASSERT(mg_dns_parse((uint8_t *) data, mg_file_size("dns.bin"), &dm) == 0); + free(data); + } +#endif } static void test_util(void) {