[tor/master] nodelist: Use safe string functions in describe.c - tor-commits

4 Sep 2019

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);
 }

    