commit ec6fbf1ca613c11dc783182a11bc1d04f6a3fb63 Author: teor <teor@torproject.org> Date: Thu Aug 29 12:39:44 2019 +1000 nodelist: Use safe string functions in describe.c Rewrite format_node_description() and router_get_verbose_nickname() to use strlcpy() and strlcat(). The previous implementation used memcpy() and pointer arithmetic, which was error-prone. Closes ticket 31545. This is CID 1452819. --- changes/ticket31545 | 5 ++ src/feature/nodelist/describe.c | 101 +++++++++++++++++++++++++++++++--------- 2 files changed, 84 insertions(+), 22 deletions(-) diff --git a/changes/ticket31545 b/changes/ticket31545 new file mode 100644 index 000000000..58921c2ad --- /dev/null +++ b/changes/ticket31545 @@ -0,0 +1,5 @@ + o Code simplification and refactoring: + - Rewrite format_node_description() and router_get_verbose_nickname() to + use strlcpy() and strlcat(). The previous implementation used memcpy() + and pointer arithmetic, which was error-prone. + Closes ticket 31545. This is CID 1452819. diff --git a/src/feature/nodelist/describe.c b/src/feature/nodelist/describe.c index 7ec18438c..1e4683768 100644 --- a/src/feature/nodelist/describe.c +++ b/src/feature/nodelist/describe.c @@ -29,7 +29,8 @@ * NULL or the null address. The <b>addr32h</b> field is optional and may be * set to 0. * - * Return a pointer to the front of <b>buf</b>, or a string constant on error. + * Return a pointer to the front of <b>buf</b>. + * If buf is NULL, return a string constant describing the error. */ STATIC const char * format_node_description(char *buf, @@ -38,7 +39,7 @@ format_node_description(char *buf, const tor_addr_t *addr, uint32_t addr32h) { - char *cp; + size_t rv = 0; bool has_addr = addr && !tor_addr_is_null(addr); if (!buf) @@ -47,34 +48,67 @@ format_node_description(char *buf, memset(buf, 0, NODE_DESC_BUF_LEN); if (!id_digest) { - memcpy(buf, "<NULL ID DIGEST>", 17); + /* strlcpy() returns the length of the source string it attempted to copy, + * ignoring any required truncation due to the buffer length. */ + rv = strlcpy(buf, "<NULL ID DIGEST>", NODE_DESC_BUF_LEN); + tor_assert_nonfatal(rv < NODE_DESC_BUF_LEN); return buf; } - buf[0] = '$'; - base16_encode(buf+1, HEX_DIGEST_LEN+1, id_digest, DIGEST_LEN); - cp = buf+1+HEX_DIGEST_LEN; + /* strlcat() returns the length of the concatenated string it attempted to + * create, ignoring any required truncation due to the buffer length. */ + rv = strlcat(buf, "$", NODE_DESC_BUF_LEN); + tor_assert_nonfatal(rv < NODE_DESC_BUF_LEN); + + { + char hex_digest[HEX_DIGEST_LEN+1]; + memset(hex_digest, 0, sizeof(hex_digest)); + + base16_encode(hex_digest, sizeof(hex_digest), + id_digest, DIGEST_LEN); + rv = strlcat(buf, hex_digest, NODE_DESC_BUF_LEN); + tor_assert_nonfatal(rv < NODE_DESC_BUF_LEN); + } + if (nickname) { - buf[1+HEX_DIGEST_LEN] = '~'; - strlcpy(buf+1+HEX_DIGEST_LEN+1, nickname, MAX_NICKNAME_LEN+1); - cp += strlen(cp); + rv = strlcat(buf, "~", NODE_DESC_BUF_LEN); + tor_assert_nonfatal(rv < NODE_DESC_BUF_LEN); + rv = strlcat(buf, nickname, NODE_DESC_BUF_LEN); + tor_assert_nonfatal(rv < NODE_DESC_BUF_LEN); } if (addr32h || has_addr) { - memcpy(cp, " at ", 4); - cp += 4; + rv = strlcat(buf, " at ", NODE_DESC_BUF_LEN); + tor_assert_nonfatal(rv < NODE_DESC_BUF_LEN); } if (addr32h) { + int ntoa_rv = 0; + char ipv4_addr_str[INET_NTOA_BUF_LEN]; + memset(ipv4_addr_str, 0, sizeof(ipv4_addr_str)); struct in_addr in; + memset(&in, 0, sizeof(in)); + in.s_addr = htonl(addr32h); - tor_inet_ntoa(&in, cp, INET_NTOA_BUF_LEN); - cp += strlen(cp); + ntoa_rv = tor_inet_ntoa(&in, ipv4_addr_str, sizeof(ipv4_addr_str)); + tor_assert_nonfatal(ntoa_rv >= 0); + + rv = strlcat(buf, ipv4_addr_str, NODE_DESC_BUF_LEN); + tor_assert_nonfatal(rv < NODE_DESC_BUF_LEN); } + /* Both addresses are valid */ if (addr32h && has_addr) { - memcpy(cp, " and ", 5); - cp += 5; + rv = strlcat(buf, " and ", NODE_DESC_BUF_LEN); + tor_assert_nonfatal(rv < NODE_DESC_BUF_LEN); } if (has_addr) { - tor_addr_to_str(cp, addr, TOR_ADDR_BUF_LEN, 1); + const char *str_rv = NULL; + char addr_str[TOR_ADDR_BUF_LEN]; + memset(addr_str, 0, sizeof(addr_str)); + + str_rv = tor_addr_to_str(addr_str, addr, sizeof(addr_str), 1); + tor_assert_nonfatal(str_rv == addr_str); + + rv = strlcat(buf, addr_str, NODE_DESC_BUF_LEN); + tor_assert_nonfatal(rv < NODE_DESC_BUF_LEN); } return buf; @@ -92,6 +126,7 @@ router_describe(const routerinfo_t *ri) if (!ri) return "<null>"; + return format_node_description(buf, ri->cache_info.identity_digest, ri->nickname, @@ -152,6 +187,7 @@ routerstatus_describe(const routerstatus_t *rs) if (!rs) return "<null>"; + return format_node_description(buf, rs->identity_digest, rs->nickname, @@ -171,6 +207,7 @@ extend_info_describe(const extend_info_t *ei) if (!ei) return "<null>"; + return format_node_description(buf, ei->identity_digest, ei->nickname, @@ -188,19 +225,39 @@ extend_info_describe(const extend_info_t *ei) void router_get_verbose_nickname(char *buf, const routerinfo_t *router) { + size_t rv = 0; + if (!buf) return; memset(buf, 0, MAX_VERBOSE_NICKNAME_LEN+1); if (!router) { - memcpy(buf, "<null>", 7); + /* strlcpy() returns the length of the source string it attempted to copy, + * ignoring any required truncation due to the buffer length. */ + rv = strlcpy(buf, "<null>", MAX_VERBOSE_NICKNAME_LEN+1); + tor_assert_nonfatal(rv < MAX_VERBOSE_NICKNAME_LEN+1); return; } - buf[0] = '$'; - base16_encode(buf+1, HEX_DIGEST_LEN+1, router->cache_info.identity_digest, - DIGEST_LEN); - buf[1+HEX_DIGEST_LEN] = '~'; - strlcpy(buf+1+HEX_DIGEST_LEN+1, router->nickname, MAX_NICKNAME_LEN+1); + /* strlcat() returns the length of the concatenated string it attempted to + * create, ignoring any required truncation due to the buffer length. */ + rv = strlcat(buf, "$", MAX_VERBOSE_NICKNAME_LEN+1); + tor_assert_nonfatal(rv < MAX_VERBOSE_NICKNAME_LEN+1); + + { + char hex_digest[HEX_DIGEST_LEN+1]; + memset(hex_digest, 0, sizeof(hex_digest)); + + base16_encode(hex_digest, sizeof(hex_digest), + router->cache_info.identity_digest, DIGEST_LEN); + rv = strlcat(buf, hex_digest, MAX_VERBOSE_NICKNAME_LEN+1); + tor_assert_nonfatal(rv < MAX_VERBOSE_NICKNAME_LEN+1); + } + + rv = strlcat(buf, "~", MAX_VERBOSE_NICKNAME_LEN+1); + tor_assert_nonfatal(rv < MAX_VERBOSE_NICKNAME_LEN+1); + + rv = strlcat(buf, router->nickname, MAX_VERBOSE_NICKNAME_LEN+1); + tor_assert_nonfatal(rv < MAX_VERBOSE_NICKNAME_LEN+1); }