From 121a043f6dd1b6c6171dae3c041cb50693eae63f Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Fri, 22 Jan 2016 20:15:31 +0000 Subject: Avoid copying buffer after dn_expand() fails If dn_expand() returns an error we could copy from an uninitialised output buffer or append the previous domain name again. Signed-off-by: Ben Hutchings --- src/script.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/script.c b/src/script.c index f272c19..49f39c4 100644 --- a/src/script.c +++ b/src/script.c @@ -118,9 +118,10 @@ static void fqdn_to_env(const char *name, const uint8_t *fqdn, size_t len) char *buf = realloc(NULL, len + buf_len + 2); memcpy(buf, name, buf_len); buf[buf_len++] = '='; - int l = 1; - while (l > 0 && fqdn < fqdn_end) { - l = dn_expand(fqdn, fqdn_end, fqdn, &buf[buf_len], buf_size - buf_len); + while (fqdn < fqdn_end) { + int l = dn_expand(fqdn, fqdn_end, fqdn, &buf[buf_len], buf_size - buf_len); + if (l <= 0) + break; fqdn += l; buf_len += strlen(&buf[buf_len]); buf[buf_len++] = ' '; -- cgit v1.2.1 From a6bbd1d7f5c25b092f143b579860a44e5b0f929e Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Fri, 22 Jan 2016 18:02:04 +0000 Subject: Fix potential buffer overflow in entry_to_env It appears that an entry of type ENTRY_PREFIX with iaid != 1 and an exclusion can expand to a string of length up to 154 bytes, whereas we allocate only 144 bytes per entry. Also, in case of truncation, snprintf() returns the length of the un-truncated output so we must not use this to increment buf_len. Finally some of the lengths given to snprintf() are unnecessarily generous. Reduce them so we don't have to increase the allocated length per entry further. Signed-off-by: Ben Hutchings --- src/script.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/script.c b/src/script.c index 49f39c4..83fbea5 100644 --- a/src/script.c +++ b/src/script.c @@ -157,7 +157,10 @@ static void entry_to_env(const char *name, const void *data, size_t len, enum en { size_t buf_len = strlen(name); const struct odhcp6c_entry *e = data; - char *buf = realloc(NULL, buf_len + 2 + (len / sizeof(*e)) * 144); + // Worst case: ENTRY_PREFIX with iaid != 1 and exclusion + const size_t max_entry_len = (INET6_ADDRSTRLEN-1 + 5 + 22 + 15 + 10 + + INET6_ADDRSTRLEN-1 + 11 + 1); + char *buf = realloc(NULL, buf_len + 2 + (len / sizeof(*e)) * max_entry_len); memcpy(buf, name, buf_len); buf[buf_len++] = '='; @@ -165,28 +168,34 @@ static void entry_to_env(const char *name, const void *data, size_t len, enum en inet_ntop(AF_INET6, &e[i].target, &buf[buf_len], INET6_ADDRSTRLEN); buf_len += strlen(&buf[buf_len]); if (type != ENTRY_HOST) { - buf_len += snprintf(&buf[buf_len], 6, "/%"PRIu16, e[i].length); + snprintf(&buf[buf_len], 6, "/%"PRIu16, e[i].length); + buf += strlen(&buf[buf_len]); if (type == ENTRY_ROUTE) { buf[buf_len++] = ','; if (!IN6_IS_ADDR_UNSPECIFIED(&e[i].router)) { inet_ntop(AF_INET6, &e[i].router, &buf[buf_len], INET6_ADDRSTRLEN); buf_len += strlen(&buf[buf_len]); } - buf_len += snprintf(&buf[buf_len], 24, ",%u", e[i].valid); - buf_len += snprintf(&buf[buf_len], 12, ",%u", e[i].priority); + snprintf(&buf[buf_len], 23, ",%u,%u", e[i].valid, e[i].priority); + buf += strlen(&buf[buf_len]); } else { - buf_len += snprintf(&buf[buf_len], 24, ",%u,%u", e[i].preferred, e[i].valid); + snprintf(&buf[buf_len], 23, ",%u,%u", e[i].preferred, e[i].valid); + buf += strlen(&buf[buf_len]); } - if (type == ENTRY_PREFIX && ntohl(e[i].iaid) != 1) - buf_len += snprintf(&buf[buf_len], 16, ",class=%08x", ntohl(e[i].iaid)); + if (type == ENTRY_PREFIX && ntohl(e[i].iaid) != 1) { + snprintf(&buf[buf_len], 16, ",class=%08x", ntohl(e[i].iaid)); + buf += strlen(&buf[buf_len]); + } if (type == ENTRY_PREFIX && e[i].priority) { // priority and router are abused for prefix exclusion - buf_len += snprintf(&buf[buf_len], 12, ",excluded="); + snprintf(&buf[buf_len], 11, ",excluded="); + buf_len += strlen(&buf[buf_len]); inet_ntop(AF_INET6, &e[i].router, &buf[buf_len], INET6_ADDRSTRLEN); buf_len += strlen(&buf[buf_len]); - buf_len += snprintf(&buf[buf_len], 24, "/%u", e[i].priority); + snprintf(&buf[buf_len], 12, "/%u", e[i].priority); + buf_len += strlen(&buf[buf_len]); } } buf[buf_len++] = ' '; -- cgit v1.2.1 From 62968599557ac81b0f811481f6b06886ddcf0cdb Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Fri, 22 Jan 2016 19:07:52 +0000 Subject: Fix off-by-one in buffer length in int_to_env We need to allow for '=', negative sign, 10 digits and the null terminator, adding up to 13 bytes not 12. Signed-off-by: Ben Hutchings --- src/script.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/script.c b/src/script.c index 83fbea5..89cb0d6 100644 --- a/src/script.c +++ b/src/script.c @@ -227,7 +227,7 @@ static void search_to_env(const char *name, const uint8_t *start, size_t len) static void int_to_env(const char *name, int value) { - size_t len = 12 + strlen(name); + size_t len = 13 + strlen(name); char *buf = realloc(NULL, len); snprintf(buf, len, "%s=%d", name, value); putenv(buf); -- cgit v1.2.1 From fe22a82fa0bd187a39d130d593c4d56d4749a174 Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Thu, 28 Jan 2016 00:49:22 +0000 Subject: Fix possible stack buffer overflow in s46_to_env when copying IPv6 prefixes An 8-bit prefix-length field can be as large as 255, but values larger than 128 will result in a buffer overflow when copying to in6. Signed-off-by: Ben Hutchings --- src/script.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/script.c b/src/script.c index 89cb0d6..3579331 100644 --- a/src/script.c +++ b/src/script.c @@ -282,7 +282,8 @@ static void s46_to_env(enum odhcp6c_state state, const uint8_t *data, size_t len size_t prefix6len = rule->prefix6_len; prefix6len = (prefix6len % 8 == 0) ? prefix6len / 8 : prefix6len / 8 + 1; - if (olen < sizeof(struct dhcpv6_s46_rule) + prefix6len) + if (prefix6len > sizeof(in6) || + olen < sizeof(struct dhcpv6_s46_rule) + prefix6len) continue; memcpy(&in6, rule->ipv6_prefix, prefix6len); @@ -311,7 +312,8 @@ static void s46_to_env(enum odhcp6c_state state, const uint8_t *data, size_t len size_t prefix6len = dmr->dmr_prefix6_len; prefix6len = (prefix6len % 8 == 0) ? prefix6len / 8 : prefix6len / 8 + 1; - if (olen < sizeof(struct dhcpv6_s46_dmr) + prefix6len) + if (prefix6len > sizeof(in6) || + olen < sizeof(struct dhcpv6_s46_dmr) + prefix6len) continue; memcpy(&in6, dmr->dmr_ipv6_prefix, prefix6len); @@ -330,7 +332,8 @@ static void s46_to_env(enum odhcp6c_state state, const uint8_t *data, size_t len size_t prefix6len = bind->bindprefix6_len; prefix6len = (prefix6len % 8 == 0) ? prefix6len / 8 : prefix6len / 8 + 1; - if (olen < sizeof(struct dhcpv6_s46_v4v6bind) + prefix6len) + if (prefix6len > sizeof(in6) || + olen < sizeof(struct dhcpv6_s46_v4v6bind) + prefix6len) continue; memcpy(&in6, bind->bind_ipv6_prefix, prefix6len); -- cgit v1.2.1 From 7b22e48fbf23d20d92a4bae581f39ac3704d8bb2 Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Thu, 28 Jan 2016 01:14:04 +0000 Subject: Change odhcp6c_insert_state to return a success/failure indicator Some callers will need to free resources on failure. Signed-off-by: Ben Hutchings --- src/odhcp6c.c | 6 ++++-- src/odhcp6c.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/odhcp6c.c b/src/odhcp6c.c index 133ade7..1714d62 100644 --- a/src/odhcp6c.c +++ b/src/odhcp6c.c @@ -516,11 +516,11 @@ void odhcp6c_add_state(enum odhcp6c_state state, const void *data, size_t len) memcpy(n, data, len); } -void odhcp6c_insert_state(enum odhcp6c_state state, size_t offset, const void *data, size_t len) +int odhcp6c_insert_state(enum odhcp6c_state state, size_t offset, const void *data, size_t len) { ssize_t len_after = state_len[state] - offset; if (len_after < 0) - return; + return -1; uint8_t *n = odhcp6c_resize_state(state, len); if (n) { @@ -529,6 +529,8 @@ void odhcp6c_insert_state(enum odhcp6c_state state, size_t offset, const void *d memmove(sdata + offset + len, sdata + offset, len_after); memcpy(sdata + offset, data, len); } + + return 0; } size_t odhcp6c_remove_state(enum odhcp6c_state state, size_t offset, size_t len) diff --git a/src/odhcp6c.h b/src/odhcp6c.h index d202790..928f82f 100644 --- a/src/odhcp6c.h +++ b/src/odhcp6c.h @@ -335,7 +335,7 @@ bool odhcp6c_is_bound(void); void odhcp6c_clear_state(enum odhcp6c_state state); void odhcp6c_add_state(enum odhcp6c_state state, const void *data, size_t len); void odhcp6c_append_state(enum odhcp6c_state state, const void *data, size_t len); -void odhcp6c_insert_state(enum odhcp6c_state state, size_t offset, const void *data, size_t len); +int odhcp6c_insert_state(enum odhcp6c_state state, size_t offset, const void *data, size_t len); size_t odhcp6c_remove_state(enum odhcp6c_state state, size_t offset, size_t len); void* odhcp6c_move_state(enum odhcp6c_state state, size_t *len); void* odhcp6c_get_state(enum odhcp6c_state state, size_t *len); -- cgit v1.2.1 From b0d1c5805a6b76c3b198728cdfd93e351d5eb196 Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Thu, 28 Jan 2016 01:16:31 +0000 Subject: Fix memory leak in dhcpv6_add_server_cand in case odhcp6c_insert_state fails If we fail to store information from the new server, the associated NA and PD options will never be freed. An attacker could use this for denial-of-service. Signed-off-by: Ben Hutchings --- src/dhcpv6.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/dhcpv6.c b/src/dhcpv6.c index cfa3f29..c2a3e3d 100644 --- a/src/dhcpv6.c +++ b/src/dhcpv6.c @@ -1362,6 +1362,7 @@ static void dhcpv6_handle_ia_status_code(const enum dhcpv6_msg orig, } } +// Note this always takes ownership of cand->ia_na and cand->ia_pd static void dhcpv6_add_server_cand(const struct dhcpv6_server_cand *cand) { size_t cand_len, i; @@ -1384,7 +1385,10 @@ static void dhcpv6_add_server_cand(const struct dhcpv6_server_cand *cand) break; } - odhcp6c_insert_state(STATE_SERVER_CAND, i * sizeof(*c), cand, sizeof(*cand)); + if (odhcp6c_insert_state(STATE_SERVER_CAND, i * sizeof(*c), cand, sizeof(*cand))) { + free(cand->ia_na); + free(cand->ia_pd); + } } static void dhcpv6_clear_all_server_cand(void) -- cgit v1.2.1 From abe9d1b0739857f4a0d25005f9f0523153a6fe23 Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Thu, 28 Jan 2016 01:28:32 +0000 Subject: Check for unsupported PD exclusion configuration in dhcpv6_parse_ia We currently only support PD exclusions that only affect bits 64-95 of the address, so we require: 32 <= PD prefix length < exclusion prefix length <= 64 The first inequality was not validated, and this could result in a buffer overflow when generating the next request message. Signed-off-by: Ben Hutchings --- src/dhcpv6.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dhcpv6.c b/src/dhcpv6.c index c2a3e3d..2d8124f 100644 --- a/src/dhcpv6.c +++ b/src/dhcpv6.c @@ -1185,7 +1185,7 @@ static int dhcpv6_parse_ia(void *opt, void *end) if (elen > 64) elen = 64; - if (elen <= 32 || elen <= entry.length) { + if (entry.length < 32 || elen <= entry.length) { ok = false; continue; } -- cgit v1.2.1 From f92e692eefa05ddb7b0b2817260b03262f30090e Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Thu, 28 Jan 2016 01:44:10 +0000 Subject: Fix potential log forgery via status string We should not include any control characters from the server status message when logging it; in particular if we include '\n' this could result in additional arbitrary log lines. In dhcpv6_log_status_code, replace all control characters with '?'. Signed-off-by: Ben Hutchings --- src/dhcpv6.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/dhcpv6.c b/src/dhcpv6.c index 2d8124f..08fe236 100644 --- a/src/dhcpv6.c +++ b/src/dhcpv6.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -1290,16 +1291,22 @@ static int dhcpv6_calc_refresh_timers(void) static void dhcpv6_log_status_code(const uint16_t code, const char *scope, - const void *status_msg, const int len) + const void *status_msg, int len) { - uint8_t buf[len + 3]; + const char *src = status_msg; + char buf[len + 3]; + char *dst = buf; - memset(buf, 0, sizeof(buf)); if (len) { - buf[0] = '('; - memcpy(&buf[1], status_msg, len); - buf[len + 1] = ')'; + *dst++ = '('; + while (len--) { + *dst = isprint((unsigned char)*src) ? *src : '?'; + src++; + dst++; + } + *dst++ = ')'; } + *dst = 0; syslog(LOG_WARNING, "Server returned %s status %i %s", scope, code, buf); -- cgit v1.2.1 From 78bc7d9c835404ad1b5d2eb26de390bd45e26d2a Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Thu, 28 Jan 2016 02:09:47 +0000 Subject: Add missing option length checks in dhcpv6_handle_advert These might be redundant with checks elsewhere but it's better to be safe. Signed-off-by: Ben Hutchings --- src/dhcpv6.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/dhcpv6.c b/src/dhcpv6.c index 08fe236..e27d899 100644 --- a/src/dhcpv6.c +++ b/src/dhcpv6.c @@ -813,7 +813,8 @@ static int dhcpv6_handle_advert(enum dhcpv6_msg orig, const int rc, if (inf_max_rt >= DHCPV6_INF_MAX_RT_MIN && inf_max_rt <= DHCPV6_INF_MAX_RT_MAX) cand.inf_max_rt = inf_max_rt; - } else if (otype == DHCPV6_OPT_IA_PD && request_prefix) { + } else if (otype == DHCPV6_OPT_IA_PD && request_prefix && + olen >= -4 + sizeof(struct dhcpv6_ia_hdr)) { struct dhcpv6_ia_hdr *h = (struct dhcpv6_ia_hdr*)&odata[-4]; uint8_t *oend = odata + olen, *d; dhcpv6_for_each_option(&h[1], oend, otype, olen, d) { @@ -823,7 +824,8 @@ static int dhcpv6_handle_advert(enum dhcpv6_msg orig, const int rc, have_pd = p->prefix; } } - } else if (otype == DHCPV6_OPT_IA_NA) { + } else if (otype == DHCPV6_OPT_IA_NA && + olen >= -4 + sizeof(struct dhcpv6_ia_hdr)) { struct dhcpv6_ia_hdr *h = (struct dhcpv6_ia_hdr*)&odata[-4]; uint8_t *oend = odata + olen, *d; dhcpv6_for_each_option(&h[1], oend, otype, olen, d) -- cgit v1.2.1