clang fortify kills netcat |
||||||
Issue descriptionCHROMEOS_RELEASE_BUILDER_PATH=reef-release/R63-9930.0.0 + self-built net-analyzer/netcat Run netcat on reef, and from another host, try to connect and receive a few bytes. # nc -lp 1800 *** buffer overflow detected ***: nc terminated; report to <http://crbug.com/new> Killed This goes away if I add -D_CLANG_FORTIFY_DISABLE to the cflags. Seems to be 2 problems: 1. netcat somehow doesn't like the fortify checks (or vice versa) and 2. I'm getting SIGKILL'ed, so I can't even catch this in a debugger Bug 389360 claims to have fixed problem 2 before. Did we regress? I can't easily progress on 1 cuz I'm a lame programmer who needs his debugger ;) Well, I can just add -D_CLANG_FORTIFY_DISABLE to the bash env scripts, like we do with several other packages.
,
Oct 13 2017
Yes, could easily be a real issue. But it's prohibitively difficult for the casual observer to debug when I'm getting SIGKILLed. I'm only using netcat for debugging (have to manually install). Lakitu seems to be the board that actually wanted netcat, so you'd have to ask them if they've noticed or care. Andreyu? If the SIGKILL is a regression, that means we're not getting crashdumps from such issues either, and so problem 2 would probably be higher than P3. I can file a separate bug if that's deemed correct.
,
Oct 13 2017
Looking at issue 1 now. :) ( Issue 2 is also interesting, since I remember being able to look at other binaries under gdb when I got FORTIFY errors before)
,
Oct 13 2017
Make sure to build with US=-crypt, to avoid a missing dep. I'm preparing a CL to do that, so overlays besides lakitu can build netcat :)
,
Oct 13 2017
Sorry, USE=-crypt not US= :)
,
Oct 13 2017
does the issue go away when crypt support is disabled ? fortify failures shouldn't be SIGKILL-ing ... we patched glibc so it should be dumping core so we can get crash reports like normal
,
Oct 13 2017
> does the issue go away when crypt support is disabled ? Nope. localhost ~ # nc -lp 1800 *** buffer overflow detected ***: nc terminated; report to <http://crbug.com/new> Killed > fortify failures shouldn't be SIGKILL-ing ... we patched glibc so it should be dumping core so we can get crash reports like normal Dunno what to say, other than... localhost ~ # gdb nc GNU gdb (Chromium OS 7.11.20170503 vanilla) 7.11 Copyright (C) 2016 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-cros-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <http://crbug.com/new>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from nc...(no debugging symbols found)...done. (gdb) run -lp 1800 Starting program: /usr/bin/nc -lp 1800 *** buffer overflow detected ***: nc terminated; report to <http://crbug.com/new> Program terminated with signal SIGKILL, Killed. The program no longer exists. (gdb)
,
Oct 13 2017
FWIW, I'm seeing SIGKILLs if I use gdb-reef in my chroot, and SIGABRTs if I just use vanilla gdb. Smells like it deserves a separate bug to me. In any case, the original issue is caused by `strncpy (poop->addrs[0], inet_ntoa (iaddr), sizeof (poop->addrs));`. Since inet_ntoa returns a nicely-formatted IPv4 address, it can't overflow a 24-byte buffer (poop->addrs is a char[8][24]), so there's no reason to actually be concerned about the scary "*** buffer overflow terminated ***" message. The intent seems to be to never overwrite anything after &poop->addrs[0][24] in the strncpy anyway, so I'd imagine that the ideal fix is to turn `sizeof (poop->addrs)` into `sizeof (poop->addrs[0])`. In reality, netcat's last release was a decade ago... As for why this behavior is just cropping up now: clang returns 24 for __builtin_object_size(poop->addrs[0], 1). GCC returns 8*24. Since the machinery for strncpy doesn't check to see if an actual overflow will happen, the complaint is just because sizeof (poop->addrs) (== 8 * 24) > __builtin_object_size(poop->addrs[0], 1) (== 24). Playing around more, gcc reports 24 for __builtin_object_size(&poop->addrs[0][0], 1), so I'm not actually sure how intentional the result of 8*24 from GCC is. Feel free to turn FORTIFY off locally while I figure out the best way to go at this. Since we compile nc with -DGAPING_SECURITY_HOLE already, -D_CLANG_FORTIFY_DISABLE will probably look safe in comparison. ;)
,
Oct 13 2017
feel free to send me a patch ... i have commit access to the upstream netcat repo on sourceforge don't read too much into GAPING_SECURITY_HOLE ... that simply controls the existence of the exec (-e) option rather than being an actual gaping security hole
,
Oct 13 2017
Will do shortly; thanks. > don't read too much into GAPING_SECURITY_HOLE ... that simply controls the existence of the exec (-e) option rather than being an actual gaping security hole Yeah, I just thought it was funny to see scrolling across my screen. :)
,
Oct 20 2017
Filed issue 776972 about the SIGKILLs.
,
Jan 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/portage-stable/+/c8b5aaf9a61d168b4623d207c6306b4809d3b195 commit c8b5aaf9a61d168b4623d207c6306b4809d3b195 Author: George Burgess IV <gbiv@chromium.org> Date: Mon Jan 22 21:13:31 2018 netcat: upgraded package to upstream Upgraded net-analyzer/netcat to version 110.20180111 on amd64, arm Tried passing oak+elm to the tool for aarch64, but it considered them to be arm. BUG= chromium:774573 TEST=used reef's netcat; no FORTIFY crash observed. emerges on all of reef, oak, veyron_jaq, daisy, elm Change-Id: I8dd4dba42172d3696083373d9917de978ab8f84b Reviewed-on: https://chromium-review.googlesource.com/877842 Commit-Ready: George Burgess <gbiv@chromium.org> Tested-by: George Burgess <gbiv@chromium.org> Reviewed-by: Mike Frysinger <vapier@chromium.org> [modify] https://crrev.com/c8b5aaf9a61d168b4623d207c6306b4809d3b195/net-analyzer/netcat/metadata.xml [add] https://crrev.com/c8b5aaf9a61d168b4623d207c6306b4809d3b195/metadata/md5-cache/net-analyzer/netcat-110.20180111 [delete] https://crrev.com/b00c9149d1878edfd911dadf2e99b4fced9fa337/metadata/md5-cache/net-analyzer/netcat-110-r9 [delete] https://crrev.com/b00c9149d1878edfd911dadf2e99b4fced9fa337/net-analyzer/netcat/netcat-110-r9.ebuild [modify] https://crrev.com/c8b5aaf9a61d168b4623d207c6306b4809d3b195/net-analyzer/netcat/Manifest [add] https://crrev.com/c8b5aaf9a61d168b4623d207c6306b4809d3b195/net-analyzer/netcat/netcat-110.20180111.ebuild
,
Jan 22 2018
,
Feb 6 2018
Hello, We'd like to merge this bugfix to netcat. This should be very low-risk, since the delta in actual code is one line: https://sourceforge.net/p/nc110/code/25/, and I've heard 0 shouting since this was submitted. The cherry-pick is available at https://chromium-review.googlesource.com/c/chromiumos/overlays/portage-stable/+/903269 Thank you.
,
Feb 6 2018
Didn't realize the b/ bug from said cherry-pick already had merge-request added. Dropping the tag from here...
,
Feb 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/portage-stable/+/6505549ac10878b487c95e83ca8e6f097a5ae83d commit 6505549ac10878b487c95e83ca8e6f097a5ae83d Author: George Burgess IV <gbiv@chromium.org> Date: Wed Feb 07 02:21:52 2018 netcat: upgraded package to upstream Upgraded net-analyzer/netcat to version 110.20180111 on amd64, arm Tried passing oak+elm to the tool for aarch64, but it considered them to be arm. BUG= chromium:774573 ,b:72841386 TEST=used reef's netcat; no FORTIFY crash observed. emerges on all of reef, oak, veyron_jaq, daisy, elm Change-Id: I8dd4dba42172d3696083373d9917de978ab8f84b Reviewed-on: https://chromium-review.googlesource.com/877842 Commit-Ready: George Burgess <gbiv@chromium.org> Tested-by: George Burgess <gbiv@chromium.org> Reviewed-by: Mike Frysinger <vapier@chromium.org> (cherry picked from commit c8b5aaf9a61d168b4623d207c6306b4809d3b195) Reviewed-on: https://chromium-review.googlesource.com/903269 Commit-Queue: Daniel Wang <wonderfly@google.com> Tested-by: Daniel Wang <wonderfly@google.com> [modify] https://crrev.com/6505549ac10878b487c95e83ca8e6f097a5ae83d/net-analyzer/netcat/metadata.xml [add] https://crrev.com/6505549ac10878b487c95e83ca8e6f097a5ae83d/metadata/md5-cache/net-analyzer/netcat-110.20180111 [delete] https://crrev.com/f027b5e1fd8a07055e2b6e2725e0b722f4da1d83/metadata/md5-cache/net-analyzer/netcat-110-r9 [delete] https://crrev.com/f027b5e1fd8a07055e2b6e2725e0b722f4da1d83/net-analyzer/netcat/netcat-110-r9.ebuild [modify] https://crrev.com/6505549ac10878b487c95e83ca8e6f097a5ae83d/net-analyzer/netcat/Manifest [add] https://crrev.com/6505549ac10878b487c95e83ca8e6f097a5ae83d/net-analyzer/netcat/netcat-110.20180111.ebuild |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by llozano@chromium.org
, Oct 13 2017