traviscross / mtr

Official repository for mtr, a network diagnostic tool
http://www.bitwizard.nl/mtr/
GNU General Public License v2.0
2.71k stars 342 forks source link

Ubuntu 24.04 - MTR 0.95 #508

Open bturnbough81 opened 5 months ago

bturnbough81 commented 5 months ago

Running an MTR report against a general internet target produces the following output:

mtr -r -n 4.2.2.2 Start: 2024-06-27T11:05:42-0500 buffer overflow detected : terminated Aborted (core dumped)

When I run the same command on a Ubuntu 20.04 (mtr version 0.93) the issue does not occur

arthurlutz commented 4 months ago

Same here

yvs2014 commented 4 months ago

As far as I can see it's fixed in current with https://github.com/traviscross/mtr/commit/5908af4c19188cb17b62f23368b6ef462831a0cb

rewolff commented 4 months ago

Thanks for pointing that out, Yves. Looking at the patch... If we DO overrun that buffer, it is too small for what we're using it for. Would it be an idea to increase the buffer size say by a factor of 2?

And if we're overflowing, we're still in trouble: snprintf will not terminate the buffer when overflow happens. Thus us using that buffer afterwards is "dangerous". A buffer[sizeof(buffer)-1] = 0; at the end would help for that. (if you do this with an "if necessary", modern processors are likely slower than just "doing it").

yvs2014 commented 4 months ago

I suppose it depends on optimization level at building, if so, a result will depend on compiler. For example this part of code below is not triggered overflow with 'gcc -O0', and triggered glibc overflow detection if compiled with 'gcc -O2' (at least on my PC with current gcc version). In this case extra space with "BSIZE 200" will be optimized out in the same way as with "BSIZE 100".

#include <stdio.h>
#include <string.h>

#define BSIZE 100

int main() {
  char buf[BSIZE];
  snprintf(buf + 33, BSIZE, "few");
  printf("%s\n", buf + 33);
  return 0;
}

upd: space optimization like that (supposing) had effects in report-mode with previous versions, and I think already fixed with 'snprintf(buf +n, buf_size -n, ...)' in https://github.com/traviscross/mtr/commit/5908af4c19188cb17b62f23368b6ef462831a0cb , but I can mistake in this discourse

rewolff commented 4 months ago

OK.... I investigated some more. Modern ubuntu is using "string_fortified" by default.

Your statement about "optimized out" is not true. The space allocated is as defined: 100 bytes. However, __snprintf_chk@PLT ends up being called and somewhere around there, the "buffer overflow" is triggered.

At first I thought that snprintf was behaving IMHO against "tradition" and filling the whole "n" bytes of buffer. But that doesn't happen: snprintf copies 4 bytes as expected, and leaves byte 38...100 alone. But snprintf_chk seems to detect the possible buffer overflow and terminates.

OK. So my suspicion that we were overflowing the buffer is incorrect. It is just that snprint detects a POSSIBLE buffer overflow, and terminates being a bit overly cautious.

On the other hand, such a hard "stance" of the settings mean that developers get notified about possbile overflows and code gets fixed.

sanderjo commented 4 months ago

N=10:

On Ubuntu 24.04 with the Ubuntu provided mtr: it seems intermittent, as in: not always a core dump.

sander@witje: $ mtr -nrc2 1.1.1.1
Start: 2024-07-14T14:18:45+0200
*** buffer overflow detected ***: terminated
Aborted (core dumped)

sander@witje: $ mtr -nrc2 1.1.1.1
Start: 2024-07-14T14:19:52+0200
*** buffer overflow detected ***: terminated
Aborted (core dumped)

I compiled the github version of mtr, and so far no core dumps.

sander@witje:~/git/mtr$ ./mtr -nrc2 1.1.1.1 
Start: 2024-07-14T14:18:26+0200
HOST: witje                       Loss%   Snt   Last   Avg  Best  Wrst StDev
  1.|-- 192.168.1.254              0.0%     2    2.2   2.2   2.2   2.2   0.0
  2.|-- 145.90.81.254              0.0%     2    2.6   2.4   2.1   2.6   0.4
  3.|-- 145.97.193.109             0.0%     2    2.8   2.4   2.1   2.8   0.5
  4.|-- 145.145.4.190              0.0%     2    2.4   2.4   2.3   2.4   0.1
  5.|-- ???                       100.0     2    0.0   0.0   0.0   0.0   0.0
  6.|-- 145.145.166.48             0.0%     2    4.2   4.2   4.2   4.2   0.0
  7.|-- 145.145.166.49             0.0%     2    4.8   5.5   4.8   6.2   1.0
  8.|-- 141.101.65.97              0.0%     2    5.8   4.8   3.8   5.8   1.4
  9.|-- 1.1.1.1                    0.0%     2    5.7   5.0   4.2   5.7   1.1
sander@witje:~/git/mtr$ ./mtr -nrc2 1.1.1.1
Start: 2024-07-14T14:18:35+0200
HOST: witje                       Loss%   Snt   Last   Avg  Best  Wrst StDev
  1.|-- 192.168.1.254              0.0%     2    2.2   2.5   2.2   2.7   0.4
  2.|-- 145.90.81.254              0.0%     2    2.8   5.4   2.8   8.0   3.7
  3.|-- 145.97.193.109             0.0%     2    3.0   4.5   3.0   6.0   2.1
  4.|-- 145.145.4.190              0.0%     2    4.5   5.0   4.5   5.6   0.8
  5.|-- ???                       100.0     2    0.0   0.0   0.0   0.0   0.0
  6.|-- 145.145.166.48             0.0%     2   27.6  18.6   9.6  27.6  12.7
  7.|-- 145.145.166.49             0.0%     2    4.9   4.4   3.8   4.9   0.8
  8.|-- 141.101.65.97              0.0%     2   30.4  17.0   3.6  30.4  19.0
  9.|-- 1.1.1.1                    0.0%     2    4.1   4.2   4.1   4.2   0.1
collinanderson commented 3 months ago

I'm getting this abort consistently when using -r. What's the next step to get this fixed in Ubuntu? Ubuntu needs to apply the - len 5908af4c19188cb17b62f23368b6ef462831a0cb patch?

rewolff commented 3 months ago

Yup. that should work. (assuming you're referencing the right patch).

collinanderson commented 3 months ago

Perfect. Looks like there's already an open Ubuntu bug here. Thanks!

https://bugs.launchpad.net/ubuntu/+source/mtr/+bug/2071890

mrngm commented 1 month ago

Cross-referencing this Arch Linux bug report for mtr as well.

sanderjo commented 1 month ago

Easy workaround on Ubuntu: https://www.reddit.com/r/Ubuntu/comments/1e2ddx4/comment/lpzwpra/

collinanderson commented 2 weeks ago

Update: Ubuntu 24.04 finally released a fix (0.95-1.1ubuntu0.1) in noble-updates about 24 hours ago. https://bugs.launchpad.net/ubuntu/+source/mtr/+bug/2069401

I think this github issue can now be closed.

sanderjo commented 2 weeks ago

I can confirm:

Setting up mtr (0.95-1.1ubuntu0.1) ...
Processing triggers for man-db (2.12.0-4build2) ...

and then:

sander@zwarte:~$ mtr -nrc2 1.1.1.1
Start: 2024-11-07T12:17:12+0100
HOST: zwarte                      Loss%   Snt   Last   Avg  Best  Wrst StDev
  1.|-- 192.168.1.250              0.0%     2   29.5  16.1   2.7  29.5  18.9
  2.|-- ???                       100.0     2    0.0   0.0   0.0   0.0   0.0
  3.|-- 100.64.0.1                50.0%     2   23.0  23.0  23.0  23.0   0.0
  4.|-- 100.64.0.2                 0.0%     2   15.3  10.4   5.4  15.3   7.0
  5.|-- ???                       100.0     2    0.0   0.0   0.0   0.0   0.0
  6.|-- 80.249.211.140             0.0%     2   50.8  71.8  50.8  92.8  29.7
  7.|-- 141.101.65.107             0.0%     2    6.5   8.2   6.5   9.8   2.3
  8.|-- 1.1.1.1                    0.0%     2   11.4   9.6   7.9  11.4   2.5
rkochar commented 1 week ago

I have the problem on mtr-0.95-5-x86_64. Kernel: Linux 6.1.111-1-MANJARO