On shutdown, noticed the error "Stack smashing attack in if_readlist_proc()" as network interfaces are taken down. Also seen the same error running just /sbin/ifconfig on a command line. Most of the system is built with "hardened", ifconfig certainly is (sys-apps/net-tools 1.60-r8). /sbin/ifconfig was built with gcc-3.3.3-r6. What is unusual in my system is that I have used udev to rename the ethernet devices, from eth0/1 to eth_dockw, eth_int and eth_dockh, depending on MAC address. Networks are started by hotplug, not directly by the runlevels; only net.lo is started in the boot runlevel, no other net.* are started in any runlevels. I've renamed "net.eth0" from baselayout to "net.init", and softlinked net.eth* to net.init as appropriate. conf.d/net refers to the new names. Changing the names to ethDW, ethBS and ethDH respectively has stopped the error from being displayed, both on the command line, and starting/stopping interfaces by hand. I have yet to shutdown since making the change to see if it no longer happens on shutdown. However, of course if there's a buffer overrun issue it is still present just not triggered. I suspect that the "get_name" function in lib/interface.c may be the cause, as it overwrites stack data from the caller (if_readlist_proc()) without checking for overrun. Having said that, the buffer "name" should be large enough, although I'm not confident that it is declared sensibly - the "char name[IFNAMSIZ]" is inside a loop which seems strange. <net/if.h> has IFNAMSIZ as 16, as far as I can tell, and hasn't changed since ifconfig was installed (they both have similar dates, from the initial system installation). Reproducible: Always Steps to Reproduce: 1.run /sbin/ifconfig, or shutdown 2. 3. Actual Results: "Stack smashing attack in if_readlist_proc()" reported Expected Results: Not smashed its stack :) # emerge info Portage 2.0.50-r9 (default-x86-2004.0, gcc-3.3.3, glibc-2.3.3.20040420-r0, 2.6.7) ================================================================= System uname: 2.6.7 i686 Mobile Intel(R) Pentium(R) III CPU - M 1200MHz Gentoo Base System version 1.4.16 ccache version 2.3 [enabled] Autoconf: sys-devel/autoconf-2.59-r3 Automake: sys-devel/automake-1.8.3 ACCEPT_KEYWORDS="x86" AUTOCLEAN="yes" CFLAGS="-O2 -march=pentium3 -fomit-frame-pointer -pipe" CHOST="i686-pc-linux-gnu" COMPILER="gcc3" CONFIG_PROTECT="/etc /usr/X11R6/lib/X11/xkb /usr/kde/2/share/config /usr/kde/3.2/share/config /usr/kde/3/share/config /usr/share/config /var/bind /var/qmail/control" CONFIG_PROTECT_MASK="/etc/gconf /etc/terminfo /etc/env.d" CXXFLAGS="-O2 -march=pentium3 -fomit-frame-pointer -pipe" DISTDIR="/usr/portage/distfiles" FEATURES="autoaddcvs ccache sandbox sfperms strict" GENTOO_MIRRORS="http://linux.rz.ruhr-uni-bochum.de/download/gentoo-mirror/ http://ftp.linux.ee/pub/gentoo/distfiles/ http://ftp.easynet.nl/mirror/gentoo/ http://ftp.heanet.ie/pub/gentoo/" MAKEOPTS="-j2" PKGDIR="/usr/portage/packages" PORTAGE_TMPDIR="/var/tmp" PORTDIR="/usr/portage" PORTDIR_OVERLAY="/var/portage" SYNC="rsync://192.168.31.11/gentoo-portage" USE="X acl acpi alsa apm arts avi berkdb cdr crypt cups dhc-fqdn dvd dvdr encode faad foomaticdb gcj gdbm gif gpm gtk gtk2 hardened imlib java jpeg kde kerberos libg++ libwww linguas_de linguas_en_GB linguas_es linguas_fr linguas_it mad mikmod mmx motif mpeg ncurses nls oggvorbis opengl oss pam pcmcia pdflib perl pic pie png pnp python qt quicktime readline samba slang spell sse ssl svga tcltk tcpd tiff truetype trusted unicode usb video_cards_ati x86 xinerama xml2 xmms xv zlib"
What was the name & length of your interface when this happened?
Can you also try and get a core (debug) of this. FEATURES="nostrip static" CFLAGS="-fno-pie -g -fno-omit-frame-pointer" emerge net-tools ulimit -c 0 chpax -zpermsx /sbin/ifconfig paxctl -permsx /sbin/ifconfig [ ! -f /usr/bin/gdb ] && emerge gdb gdb /sbin/ifconfig (gdb) run <with command line options here> # should segfault now. (gdb) bt full (gdb) info registers (gdb) x/8i $pc ------------------------------------------------ This package appears to be maintained upstream by. Phil Blundell <philb@gnu.org> Bernd Eckenfels <net-tools@lina.inka.de>
sorry that's USE=static
Created attachment 36375 [details] gdb output as requested
Created attachment 36376 [details] /proc/net/dev data being parsed by ifconfig at time of crash This might be useful; ifconfig has parsed the first couple of entries in this file fine, but died on the third (with the "long" if name)
Created attachment 36377 [details] gdb results on modified ifconfig with local declaration outside while loop - ultimately "fixes" the smash Something I tried - moved the "char *s, name[IFBUFSIZ]" from inside the while loop to just before it, and the stack smash "moved" to if_readlist_rep (the attachement is the gdb output from this). if_readlist_rep had a similar construct; applied the same change in there and the stack smash disappeared. I don't know enough about gcc/C to understand _why_ it makes a difference; when writing C myself I've always declared local data at the start of the function. As a result, I don't know whether the changes are correct or not, even if they fix the smash.
with ANSI C you're supposed to declare vars at beginning of scope declaring inside of a case scope or if scope is pretty common practice, but i dont think ive ever seen inside of a loop before ... sounds like bad practice to me :)
Created attachment 36378 [details, diff] net-tools.patch see if this makes any difference
Ahhh! Got it. I've just shutdown and rebooted going from home to work, and the problem doesn't occur. I think the problem is where I first thought it would be, in get_name(). From the /proc/net/dev that I attached earlier, the line that triggered the bug starts: eth_dockh:39377042 27571 0 ... get_name() scans past the first ':' looking for <digit>*: - however the "bytes" field has grown far enough to butt right up to the interface name, causing get_name to parse over the bytes field thinking it may be an alias (according to the comments in get_name()). In doing this parse, it happily copies the digits into the "name" buffer, which is 16 bytes. That's overflowed by two digits in the above example, and the name parameter to get_name() is pointing to data on the if_readlist_proc/rep scope stack. Normally it won't happen; with the standard "eth0" as the name, for example, we'd need 13 digits of bytes-count to trigger it instead of 8 - not possible with 32-bit integers. When I changed my device names to ethXX, similarly the problem can't be triggered. The inside-while-loop local data declaration stuff is probably a red herring, although in my opinion it's bad practice, especially ugly inside a loop. It may just move the stack data around, such that instead of corrupting the stack guard, we get corruption on other data that isn't noticed. I don't know what a net alias looks like (never used them) - what is the limit on the number of alias digits, for example? A comment in the header indicates aliases could number in the thousands, so at least 4 digits seems likely. Thus the name buffer limit of 16 (IFNAMSIZ) means the actual name must be 10 bytes or less, leaving one for the ':', four for the alias digits and one for the '\0'. Elsewhere this limit is hard-coded (e.g. in add_interface, when the name is copied to a new interface structure). A brief look at the kernel indicates that it truncates the name string part such that <name>:<alias> is less than IFNAMSIZ in length (net/ipv4/devinet.c, devinet_ioctl() truncates user-supplied name to IFNAMSIZ-1, inetdev_changename() further truncates to try to retain the alias number). So, it looks to me like the kernel will only ever expose names, including :<alias>, of IFNAMSIZ characters including terminator, so /proc/net/dev would meet this. If this is indeed the case, then a fix would be a case of guarding the loops in get_name() such that max IFNAMSIZ characters are examined. A careful patch is necessary to guard the writes to *name in get_name(), I think, but not urgent since obviously few people use long net interface names :) A workaround is to ensure network device names are max 5 characters. Perhaps the following? static char *get_name(char *name, char *p) { char *namestart=name; // record start of name, for guarding while (isspace(*p)) p++; while (*p && ((name-namestart)<(IFNAMSIZ-1))) { // guard if (isspace(*p)) break; if (*p == ':') { /* could be an alias */ char *dot = p, *dotname = name; *name++ = *p++; while (isdigit(*p) && ((name-namestart)<(IFNAMSIZ-1))) // guard *name++ = *p++; if (*p != ':') { /* it wasn't, backup */ p = dot; name = dotname; } if (*p == '\0') return NULL; p++; break; } *name++ = *p++; } *name++ = '\0'; return p; }
Created attachment 36388 [details, diff] Patch as per my comment 9 This patch guards the function get_name() in lib/interface.c against writing past the end of the "name" buffer. The overrun condition should only occur when the "bytes" field of /proc/net/dev is being read on the assumption it may be an alias. A valid alias will always end with a ":" inside IFNAMSIZ characters.
Thought it'd be useful to summarise the status as I see it, in time for North America to wake up :) I can't update the "steps to reproduce" etc; there are some additional conditions to guarantee the error: 1. Cause net device to have a long name - anything over 4 characters can eventually cause the problem, I think. 2. Cause enough traffic on that interface, so that the bytes count "touches" the ':' in /proc/net/dev and the string <interface>:<bytes count> is greater than 15 characters. The longer the name, the quicker this can happen. 3. Run /sbin/ifconfig Cause is an overrun in lib/interface.c function get_name() which overwrites the buffer pointed to by parameter "name" (local data passed from if_readlist_proc() and if_readlist_rep()). Workaround is to use short interface names; 4 character names avoid the problem assuming the bytes count is a 32-bit unsigned integer ("unsigned long"). If aliases are in use, the problem cannot occur as the second ":" will be found by get_name() and the bytes count will not be scanned. Since there is a workaround I think this bug should be classified "minor". Suggested patch attached in comment 10 should fix the issue well, and shows exactly how the overrun occurs. Code in comment 9 is not quite sufficient, as it can still overrun by one byte if aliases are in use - please disregard that code. Discussion in comments 6, 7 and 8 is not pertinent; moving the declarations around did avoid the smash, but didn't really fix it, just moved the smash to something other than the stack guard.
Sent mail upstream requesting they look at this bug.
Created attachment 36449 [details, diff] current version (partial patch) the attached patch is a diff betwenn the 1.60 source and the current cvs head, which will be 1.65 soon. There is a moification and i think the current cvs version will not segfault in your situatons, but i agree that explicitely counting the processed chars is more robust. Perhaps somebody can apply the patch to this version, since I think the version i have has fixed some strange parsing problems with device names. the patch does not update the rest of this file, since it has some major rework, if you want to see all of the changes get the debian diff. Greetings Bernd
Created attachment 36457 [details, diff] patch against debian testing 1.60-10, for Bernd Tried Bernd's patch from cvs head in comment 13; just to confirm that it does work as expected: gentoo-1.60-r8: c1358217 net-tools-1.60 # /sbin/ifconfig.orig eth_dockw ifconfig.orig: stack smashing attack in function if_readlist_proc() Aborted gentoo-1.60-r8 with patch: c1358217 net-tools-1.60 # ./ifconfig eth_dockw eth_dockw Link encap:Ethernet HWaddr 00:0B:DB:23:64:10 inet addr:193.70.211.101 Bcast:193.70.211.255 Mask:255.255.255.0 UP BROADCAST NOTRAILERS RUNNING MULTICAST MTU:1500 Metric:1 RX packets:69771 errors:0 dropped:0 overruns:1 frame:0 TX packets:63237 errors:0 dropped:0 overruns:0 carrier:0 collisions:0 txqueuelen:1000 RX bytes:11844776 (11.2 Mb) TX bytes:4174443 (3.9 Mb) Interrupt:11 Base address:0xec00 It does implicitly assume that the kernel was built with the same value for IFNAMSIZ. Probably this isn't a problem in the real world, I'd guess that anyone changing IFNAMSIZ in /usr/include/net/if.h is going to reap the whirlwind! Having said that, I think it's worth eliminating such assumptions, especially if it's easy to do. This attachment is a patch against the debian '-10' testing diff, which fixes things in a slightly different way, by explicitly finding the start and end offsets of the interface name then using memcpy to write it to the name buffer. It's trivially obvious to see that this is safe. For what it's worth, it's also more efficient than my previous patch as it doesn't need to continually check the number of characters written to name. I haven't tested it with aliases, but it should be ok. I haven't followed up what the rest of the code does if the interface name in /proc/net/dev is not valid; but get_name at least now consistently returns an empty string in name if it can't match the interface properly. My previous diff (and the old code) would leave a truncated version of the name in some circumstances, and the returned pointer value was unclear. I notice in the debian diff that "get_name" is made visible and declared in interface.h - nothing else seems to use it yet, but if it is to be used elsewhere the size of the name buffer ought to be passed to it and the behaviour specified properly for invalid text passed in p.
Kevin, Well thanks for reporting this bug and going out of your way to provide a patch for debian. It unfortunately took me having to apply by hand and making a new diff but thats ok. Patched in the -r9 ~arch Please test.
The patch has a bug - it returns p one byte short of what it should be, causing ifconfig to display interface stats all zeros. Here's a patch against the patch: --- net-tools-1.60-get_name.patch 2004-08-14 00:26:31.029006392 +0200 +++ /usr/portage/sys-apps/net-tools/files/net-tools-1.60-get_name.patch 2004-08-14 00:23:21.438950511 +0200 @@ -42,7 +42,7 @@ + if ((nameend-namestart)<IFNAMSIZ) { + memcpy(name,&p[namestart],nameend-namestart); + name[nameend-namestart]='\0'; -+ p=&p[nameend]; ++ p=&p[nameend]+1; + } else { + /* Interface name too large */ + name[0]='\0'; With the final patch, ifconfig works for me as expected. HTH
Created attachment 37588 [details, diff] Exact patch I'm using, for reference. Sorry for delay in responding; my main machine's psu gave out in the heat (maybe too many xorg builds ;) ), since then I've been on holiday, away from network connections... Dennis is correct. I would prefer to write p = &p[nameend+1]; as a matter of habit avoiding pointer arithmetic where it's not necessary, but the effect is the same. I've attached here, the patch file I have in my local tree, mostly for reference. According to diff it's the same solar's in the actual r9, apart from my version of the Dennis' +1 which I've just added, and indentation which is as per 1.60 here rather than the debian 1.65 stuff I earlier attached.
Updated the get_name patch a little as per comment #16 - 17 no revision bump. Please test =sys-apps/net-tools-1.60-r9
Confirmation - r9 as of comment #18 works fine here; no crash when interface name meets bytes in /proc/net/dev, stats are correct. Tried ifconfig -a, ifconfig -s, /etc/init.d/net.eth* stop/start.
Since ~x86 r9 from portage works, marked as FIXED (am I supposed to do this?)
You could call for a TEST-REQUEST if you wanted to go stable before the default of ~60 days.
reopening bug
arch maintainers please test and mark net-tools-1.60-r9 stable. This is a good house keeping bug and I'd like to get this fix marked stable for all arches before the upcomming portage snapshot and then remove net-tools-1.60-r8 from the tree. - marked net-tools-1.60-r9 stable on x86
Done on PowerPC.
Stable on sparc.
Stable on mips.
Stable on hppa.
Stable on alpha.
amd64 done
stable on ppc64, thanks!
Stable on s390.
arm/ia64 finally done
