*** buffer overflow detected ***: terminated (fortify_source ?)

[lanodan]

• Kernel: 5.10.9-gentoo x86_64
• Compiler: Clang
• libc: glibc

$ mtr -rz -c 5 hacktivis.me
Start: 2021-03-28T15:24:18+0200
HOST: cloudchaser                 Loss%   Snt   Last   Avg  Best  Wrst StDev
*** buffer overflow detected ***: terminated
Aborted (core dumped)

Backtrace:

$ $ lldb -c 1616937873-203-mtr\[31213]
(lldb) target create --core "1616937873-203-mtr[31213]"
Core file '/var/crash/1616937873-203-mtr[31213]' (x86_64) was loaded.

(lldb) bt
* thread #1, name = 'mtr', stop reason = signal SIGABRT
  * frame #0: 0x00007ffa5e7cbf91 libc.so.6`__GI_raise(sig=<unavailable>) at raise.c:50:1
    frame #1: 0x00007ffa5e7b52b8 libc.so.6`__GI_abort at abort.c:79:7
    frame #2: 0x00007ffa5e80f437 libc.so.6`__libc_message(action=do_abort, fmt=<unavailable>) at libc_fatal.c:155:5
    frame #3: 0x00007ffa5e894cb2 libc.so.6`__GI___fortify_fail(msg="") at fortify_fail.c:26:5
    frame #4: 0x00007ffa5e893632 libc.so.6`__GI___chk_fail at chk_fail.c:28:3
    frame #5: 0x00007ffa5e893235 libc.so.6`___snprintf_chk(s=<unavailable>, maxlen=<unavailable>, flag=<unavailable>, slen=<unavailable>, format=<unavailable>) at snprintf_chk.c:29:5
    frame #6: 0x0000000000408d06 mtr`fmt_ipinfo [inlined] reverse_host6(addr=0x0000000000413850, buff_length=127) at asn.c:211:9
    frame #7: 0x0000000000408c96 mtr`fmt_ipinfo [inlined] get_ipinfo(ctl=<unavailable>, addr=0x0000000000413850) at asn.c:232
    frame #8: 0x0000000000408c7c mtr`fmt_ipinfo(ctl=0x00007ffedc605da8, addr=0x0000000000413850) at asn.c:299
    frame #9: 0x000000000040734e mtr`report_close(ctl=<unavailable>) at report.c:158:13
    frame #10: 0x0000000000402e5c mtr`main(argc=<unavailable>, argv=<unavailable>) at mtr.c:867:9
    frame #11: 0x00007ffa5e7b6c1b libc.so.6`__libc_start_main(main=(mtr`main at mtr.c:768), argc=5, argv=0x00007ffedc606848, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007ffedc606838) at libc-start.c:314:16
    frame #12: 0x000000000040270a mtr`_start + 42

[rpaaron]

static void reverse_host6(
    struct in6_addr *addr,
    char *buff,
    int buff_length)
{
    int i;
    char *b = buff;
    for (i = (sizeof(*addr) / 2 - 1); i >= 0; i--, b += 4)      /* 64b portion */
        snprintf(b, buff_length,
                 "%x.%x.", addr->s6_addr[i] & 0xf, addr->s6_addr[i] >> 4);

    buff[strlen(buff) - 1] = '\0';
}

I'm guessing because buff_length as an argument to snprintf needs to be decremented as b is incremented?

[rewolff]

The size of the destination buffer can be predicted 100% statically. 128 prints in 32 hex characters with a dot between each of them. So 32*2 = 64. Now the last digit doesn't have a trailing . but there is a tailing \0, so the buffer needs to be 64 characters.... Except that the trailing . IS printed and then removed.

So... the buffer needs to be at least 65 characters. With the current price of say 10 bytes of RAM being less than a microcent, I'd say that taking some margin and declaring the buffer to be say at least 80 characters to be "safe" against such stupid off-by-one errors. (Save on programer-hours at the cost of a few bytes of extra RAM at runtime).

Ah. I missed another detail at first: Only 64 bits are printed, so we're talking about using only 32/33 bytes of the buffer.

I checked the caller of "reverse_host6" and... it uses a buffer of 128 characters where just over 65 would've sufficed. Then those 65 characters are concatenated with another 30 or so characters and placed into another 128-byte buffer. So, although the code is not perfect, this shouldn't cause the buffer overrun.

To fix the code: i = sizeof (... ) should be changed into i = 7 / process the top 8 bytes of the address / (The /2 hinted to me that the s6_addr array was defined as being 16 bit SHORTs... That's the wrong hint to be conveying. ) (*) The b+=4 should have buff_length-=4 added.

The "strlen" at the end is unnecessary: *--b = '\0'; // Remove the last '.'

Then.... The error says that a buffer overflow was detected inside snprintf. IMHO I don't see how that could happen.

I've implemented these changes. Can you guys double check that nothing changed?

(*) P.S. So things would go wrong (not only possibly overflowing the buffer, but also generating wrong results!) when someone extends in6_addr to bigger than 16 bytes.

[yvs2014]

It's not entirely clear what caused a buffer overflow there, but along the way what if we eliminate *printf() there, and clean up the code around its call?

diff --git a/ui/asn.c b/ui/asn.c
index 4790d6a..75c0f62 100644
--- a/ui/asn.c
+++ b/ui/asn.c
@@ -28,6 +28,7 @@
 #include "portability/error.h"
 #endif
 #include <errno.h>
+#include <assert.h>

 #ifdef __APPLE__
 #define BIND_8_COMPAT
@@ -199,19 +200,26 @@ static char *split_txtrec(
 }

 #ifdef ENABLE_IPV6
-/* from dns.c:addr2ip6arpa() */
+#define BYTES_TO_PROCESS       8
 static void reverse_host6(
     struct in6_addr *addr,
     char *buff,
     int buff_length)
 {
-    int i;
-    char *b = buff;
-    // We need to process the top 64 bits, or 8 bytes. 
-    for (i = 8-1; i >= 0; i--, b += 4, buff_length -= 4)
-        snprintf(b, buff_length,
-                 "%x.%x.", addr->s6_addr[i] & 0xf, addr->s6_addr[i] >> 4);
-    *--b = 0;
+    static char hex4bits[] = "0123456789abcdef";
+    int i, j;
+    char c;
+    assert(buff_length >= (4 * BYTES_TO_PROCESS));
+    // We need to process the top 64 bits, or 8 bytes.
+    for (i = 0; i < BYTES_TO_PROCESS; i++) {
+        c = addr->s6_addr[BYTES_TO_PROCESS - 1 - i];
+        j = i * 4;
+        buff[j]     = hex4bits[c & 0xf];
+        buff[j + 1] = '.';
+        buff[j + 2] = hex4bits[(c >> 4) & 0xf];
+        buff[j + 3] = '.';
+    }
+    buff[4 * BYTES_TO_PROCESS - 1] = 0;
 }
 #endif

@@ -229,22 +237,16 @@ static char *get_ipinfo(

     if (ctl->af == AF_INET6) {
 #ifdef ENABLE_IPV6
-        reverse_host6(addr, key, NAMELEN);
-        if (snprintf(lookup_key, NAMELEN, "%s.origin6.asn.cymru.com", key)
-            >= NAMELEN)
+        reverse_host6(addr, key, sizeof(key));
+        if (snprintf(lookup_key, sizeof(lookup_key), "%s.origin6.asn.cymru.com", key) < 0)
             return NULL;
 #else
         return NULL;
 #endif
     } else {
-        unsigned char buff[4];
-        memcpy(buff, addr, 4);
-        if (snprintf
-            (key, NAMELEN, "%d.%d.%d.%d", buff[3], buff[2], buff[1],
-             buff[0]) >= NAMELEN)
+        if (snprintf(key, sizeof(key), "%d.%d.%d.%d", ((unsigned char*)addr)[3], ((unsigned char*)addr)[2], ((unsigned char*)addr)[1], ((unsigned char*)addr)[0]) < 0)
             return NULL;
-        if (snprintf(lookup_key, NAMELEN, "%s.origin.asn.cymru.com", key)
-            >= NAMELEN)
+        if (snprintf(lookup_key, sizeof(lookup_key), "%s.origin.asn.cymru.com", key) < 0)
             return NULL;
     }

[rewolff]

If you do i=BYTES_TOPROCESS-1, j=0 and change the j's to j++ in the assignments that makes things a bit clearer I think.

+    for (i = BYTES_TO_PROCESS-1, j=0; i>= 0; i++) {
+        c = addr->s6_addr[i];
+        buff[j++] = hex4bits[c & 0xf];
+        buff[j++] = '.';
+        buff[j++] = hex4bits[(c >> 4) & 0xf];
+        buff[j++] = '.';
+    }

That said: Minor nitpick, it is worth a try. Is @lanodan still available to test patches?

[yvs2014]

If you do i=BYTES_TOPROCESS-1, j=0 and change the j's to j++ in the assignments that makes things a bit clearer I think.

+    for (i = BYTES_TO_PROCESS-1, j=0; i>= 0; i++) {
+        c = addr->s6_addr[BYTES_TO_PROCESS - 1 - i];
+        buff[j++] = hex4bits[c & 0xf];
+        buff[j++] = '.';
+        buff[j++] = hex4bits[(c >> 4) & 0xf];
+        buff[j++] = '.';
+    }

Good point, thanks. Then

+    char *b;
+    for (i = BYTES_TO_PROCESS - 1, b = buff; i >= 0; i--) {
+        c = addr->s6_addr[i];
+        *b++ = hex4bits[c & 0xf];
+        *b++ = '.';
+        *b++ = hex4bits[(c >> 4) & 0xf];
+        *b++ = '.';
+    }

[rewolff]

Even nicer. :-)

[lanodan]

Sorry for the lag, I'm mostly using email so I didn't get the mention in https://github.com/traviscross/mtr/issues/392#issuecomment-821952944 on testing the patches.

With current HEAD being at dca7750f19319fb3d51d9ce9710b9c64acfa9d08:

• building on 01ce4a5296a26fdf6eea24f7140b51e5c5224602 and later seems to work
• https://github.com/traviscross/mtr/issues/392#issuecomment-821947689 applied on current HEAD seems to work
• with https://github.com/traviscross/mtr/issues/392#issuecomment-821963408 applied on current HEAD it seems to also work

What I applied for #issuecomment-821963408

```diff diff --git a/ui/asn.c b/ui/asn.c index 4790d6a..75c0f62 100644 --- a/ui/asn.c +++ b/ui/asn.c @@ -28,6 +28,7 @@ #include "portability/error.h" #endif #include +#include #ifdef __APPLE__ #define BIND_8_COMPAT @@ -199,19 +200,26 @@ static char *split_txtrec( } #ifdef ENABLE_IPV6 -/* from dns.c:addr2ip6arpa() */ +#define BYTES_TO_PROCESS 8 static void reverse_host6( struct in6_addr *addr, char *buff, int buff_length) { - int i; - char *b = buff; - // We need to process the top 64 bits, or 8 bytes. - for (i = 8-1; i >= 0; i--, b += 4, buff_length -= 4) - snprintf(b, buff_length, - "%x.%x.", addr->s6_addr[i] & 0xf, addr->s6_addr[i] >> 4); - *--b = 0; + static char hex4bits[] = "0123456789abcdef"; + int i, j; + char c; + assert(buff_length >= (4 * BYTES_TO_PROCESS)); + // We need to process the top 64 bits, or 8 bytes. + char *b; + for (i = BYTES_TO_PROCESS - 1, b = buff; i >= 0; i--) { + c = addr->s6_addr[i]; + *b++ = hex4bits[c & 0xf]; + *b++ = '.'; + *b++ = hex4bits[(c >> 4) & 0xf]; + *b++ = '.'; + } + buff[4 * BYTES_TO_PROCESS - 1] = 0; } #endif @@ -229,22 +237,16 @@ static char *get_ipinfo( if (ctl->af == AF_INET6) { #ifdef ENABLE_IPV6 - reverse_host6(addr, key, NAMELEN); - if (snprintf(lookup_key, NAMELEN, "%s.origin6.asn.cymru.com", key) - >= NAMELEN) + reverse_host6(addr, key, sizeof(key)); + if (snprintf(lookup_key, sizeof(lookup_key), "%s.origin6.asn.cymru.com", key) < 0) return NULL; #else return NULL; #endif } else { - unsigned char buff[4]; - memcpy(buff, addr, 4); - if (snprintf - (key, NAMELEN, "%d.%d.%d.%d", buff[3], buff[2], buff[1], - buff[0]) >= NAMELEN) + if (snprintf(key, sizeof(key), "%d.%d.%d.%d", ((unsigned char*)addr)[3], ((unsigned char*)addr)[2], ((unsigned char*)addr)[1], ((unsigned char*)addr)[0]) < 0) return NULL; - if (snprintf(lookup_key, NAMELEN, "%s.origin.asn.cymru.com", key) - >= NAMELEN) + if (snprintf(lookup_key, sizeof(lookup_key), "%s.origin.asn.cymru.com", key) < 0) return NULL; } ```

btw here is more info on how it's built that I should have given in the OP:

CC="clang"
CFLAGS="-O2 -pipe -march=native -mtune=native -Wall -Wextra -Wformat-security -fstack-protector-strong -Qunused-arguments -D_FORTIFY_SOURCE=2"
LDFLAGS="-Wl,-z,relro -Wl,-z,now -Wl,-as-needed"
ELIBC="glibc"

net-analyzer/mtr-9999::gentoo was built with the following:
USE="filecaps ipinfo ipv6 ncurses -gtk -jansson" ABI_X86="(64)"