Bug 118302
Description
Andrew V. Samoilov
2004-03-15 13:30:34 UTC
Created attachment 98532 [details]
Possible buffer overflow in vfs/direntry.c:vfs_s_find_entry_tree()
That -1 in if (t + pseg + sizeof (PATH_SEP_STR) - 1 > &p[sizeof (p)]) is wrong. Otherwise the overflow is still there. Created attachment 98535 [details]
Missed "%s" pattern in vfs/fish.c:send_fish_cmd()
Created attachment 98536 [details]
Numerous readlink() related off-by-ones
Here is a tarball mc without the first patch (s/-1//) segfaults on. begin 644 bad.tar.bz2 M0EIH.3%!629365?;&7P``%G_IOB``$!``?^`8`0(`."IGD```@``@`@`"#`` MS:!D4`:-&AH```E332``````*I(AZ0-#0`R-&GJ?<@:AUX;C$8^T)"RDDES[ M9V@O"!BJ&"L0!!,BXD@T)21$BHQ*BE.KJ73U:0BQ%Y`*AB#%94(Q5+Q2M!PW M.$<%@C16BN(3,UU5L]D35)IV+8IX[(`N80A`BUB]+F:)'D^L$F3)6O%8R)GT 4H9A0H;&A!)`'\7<D4X4)!7VQE\`` ` end BTW< where mc CVS lives ATM? In cvs.gnome.org there is a note that it moved to subversions, but subversions simply times out when trying to connect.. It is on subversions.gnu.org: Anonymous CVS access: cvs -z3 -d:ext:anoncvs.org:/cvsroot/mc co mc Created attachment 98544 [details]
Unsafe strcpy(), unchecked mc_stat() return value and wrong $ pattern matching in "Find file"
Created attachment 98545 [details]
view.c off-by-one
Created attachment 98546 [details]
Rare file corruption on copy/move
* file.c (copy_file_file): Fix data corruption if mc_write() does not write
n_read bytes at once.
Created attachment 98547 [details]
Rare segfault and unwanted command execution in F2/Menu
* user.c (execute_menu_command: Fix rare segmentation violation
and possible unwanted command execution if last line in menu
file contain space(s) only and no trailing newline.
Re: Additional Comment #5 From Jakub Jelinek (jakub) on 2004-03-15 11:52 ------- >Here is a tarball mc without the first patch (s/-1//) segfaults on. Some of -1 are useful there. Created attachment 98562 [details]
edit/syntax.c off-by-x
Created attachment 98563 [details]
Two more possible buffer overflows in edit/syntax.c
Created attachment 98564 [details]
Variable misuse and setup_passive() fallback in vfs/ftpfs.c
2003-07-21 Andrew V. Samoilov <sav.ua>
* ftpfs.c(initconn): Reset variables if setup_passive() fails.
(open_data_connection): Set my_errno to errno before close()
syscall.
2003-06-21 Jindrich Makovicka <makovick.cvut.cz>
* ftpfs.c (command): Fix misuse of the status variable. Don't
cache the value of SUP.sock before reconnect.
Created attachment 98565 [details]
Filter is not applied after command execution
Possible scenario to loss files:
1. Set filter to *.mp3 (F9 - Left|Right - Filter).
2. Hit enter on any .mp3 file. After what filter is not applied, so it can be
other files in Panel.
3. Select all files (keypad +).
4. F8. You are expected to delete only *.mp3, but can be surprised...
Created attachment 98566 [details]
Reverse search segfault in built-in viewer if there is no such string
Created attachment 98567 [details]
My current patch
The http://bugzilla.redhat.com/bugzilla/attachment.cgi?id=98546&action=view patch was incomplete, fixed in my patchset. For AS2.1/RHL9/FC1 errata I'd like to put in just important bugfixes and not enhancements. As for FC2, is 4.6.1 release on the horizon? Concerning the patches, e.g. the readlink routines can use memcpy instead of strncpy and probably any strncpy use should be replaced by something else (either introduce a strlcpy like routine in mc sources, or always use strlen/memcpy). strncpy is really misdesigned function and quite expensive as well. Also, the use of int instead of size_t when dealing with sizes is risky and could be quite easily exploited on 64-bit architectures, consider cding into a bzip2'ed tarball on some ftp site which contains > 2GB long filename. Created attachment 98568 [details]
Memory leaks
Created attachment 98569 [details]
My current patch
Created attachment 98570 [details]
tarfs fixes
I think tarfs should prevent the very long names early. Also, it seems tar.c relies on the filename being terminated in the tarball, which for hostile tarballs might not be true. Re: Additional Comment #19 From Jakub Jelinek (jakub) on 2004-03-16 06:21 > The http://bugzilla.redhat.com/bugzilla/attachment.cgi?id=98546&action=view > patch was incomplete, fixed in my patchset. Yes, of course, just look to our CVS. > As for FC2, is 4.6.1 release on the horizon? No, this is one of the reason I open this bug. > Concerning the patches, e.g. the readlink routines can use memcpy > instead of strncpy and probably any strncpy use should be replaced > by something else (either introduce a strlcpy like routine in mc sources, Already in mc-4.6.1-pre1 > or always use strlen/memcpy). strncpy is really misdesigned function > and quite expensive as well. Also, the use of int instead of size_t > when dealing with sizes is risky and could be quite easily exploited > on 64-bit architectures, consider cding into a bzip2'ed tarball on some ftp site I think this is a pre-historic tail. > which contains > 2GB long filename. It seems you are right. BTW, what about slang fixes, memory leaks, https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=97748&action=view and http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=229773&repeatmerged=no ? Created attachment 99078 [details]
Unsafe strcat() in editcmd.c:edit_complete_word_cmd()
* user.c (extract_line): Add a new parameter (size of the output buffer) to prevent buffer overflow. http://savannah.gnu.org/cgi-bin/viewcvs/mc/mc/src/user.c.diff?r1=1.57&r2=1.58&diff_format=u Created attachment 99110 [details]
Patch against mc CVS to fix some cpiofs bugs and security issues.
#ucpio did not handle hardlinks correctly, which is something that annoyed me
for a long time (e.g. when I tap double enter on gcc rpm, I cannot copy
/usr/bin/gcc out of the archive, since it has hardlinks and mc thinks it has
zero
size. Also, there was a memory leak and hostily crafted cpio archive could
have non-terminated filenames, crippling all kinds of things.
Created attachment 99114 [details]
Patch against CVS mc to fix problems with weirdo chars in rpm filenames and paths
Andrew, your changes to /usr/share/mc/extfs/rpm are certainly not sufficient.
The following patch makes all kinds of weirdo filenames I threw at it work,
including stuff like x\"y\'.rpm, a.rpm\ , b"c'.rpm etc.
Created attachment 99115 [details]
My current patch against latest mc rpm
I hope this patch incorporates all that you have reported and some more things
I have discovered. It also gets rid of most of the strncpy calls in the
source,
replacing them with either g_strlcpy or memcpy as appropriate.
Would you mind if I share this patch with vendor-sec, in case other distros
are willing to do a mc errata as well?
Okay, so for CVE purposes, use CAN-2004-0226 to refer to all the buffer overflow vulnerabilities that are not yet public and were found by Jakub and Andrew. Well, in my uncommited patch I am using g_strndup() instead of your memcpy() in user.c and utilunix.c. Also src/user.c:debug_out() is a whole buffer overflow now. As far as I remember g_strlcpy() is in glib since glib2, so this patch can lead to compilation error for some superstable ;-) distros. SLang related parts are not included. I don't mind patch sharing, but I have some unpleasant experience when my and Pavel patches were included to some distros as their own. g_strndup is better, you're right. I'll deal with debug_out. I'm aware that g_strlcpy is glib2+ only, but AFAIK glibcompat.c in mc CVS deals with this and for our distro I don't need to be that super-portable. SLang changes are not included, since our mc uses shared lib from the slang package, so that would be better tracked as a separate bug against slang. Shall I create patches against CVS for the remaining stuff which is in the jumbo patch and not in CVS (64-bit fixes, tar.c changes, maybe something else as well)? On vendor-sec I'll surely make sure you and Pavel are properly credited. Created attachment 99138 [details]
CVS mc patch for debug_out and =+/+= handling
I don't think debug_out needs to be super efficient, on the other side the
routine should be as short as possible.
=+ and += has been handled incorrectly (seen while testing debug_out).
Created attachment 99139 [details]
Patch to fix st_blocks printing on 64-bit platforms against mc CVS
Created attachment 99140 [details]
Updated jumbo patch
Created attachment 99144 [details]
CVS Patch to simplify column title handling
* src/info.c (info_show_info): Cast myfs_stats.avail to double to fix integer overflow that can cause a negative percent number. http://savannah.gnu.org/cgi-bin/viewcvs/mc/mc/src/info.c.diff?r1=1.19&r2=1.20&diff_format=u Andrew, we gave the patches to other vendor security teams for their review along with the CVE name. We need to choose a date for when these issues will be disclosed publically. I'd like to suggest April 21st, would this be okay with you? Additionally, in order to document the vulnerabilities and get the right number of CVE names assigned, can you confirm that my understanding is correct that we have: - various buffer overflows and one format string vulnerability that are already committed silently to mc CVS (but have not been in an upstream release yet) - various buffer overflows found above that are not yet committed to mc CVS I have no reason to comply about April 21. - various buffer overflows and one format string vulnerability that are already committed silently to mc CVS (but have not been in an upstream release yet) If we will name mc-4.6.1-pre1 as upstream release, there is only format string vulnerability has not been there. But this pre-release has some important regression issues. - various buffer overflows found above that are not yet committed to mc CVS Right Created attachment 99412 [details]
CVS mc patch to fix temporary file handling and move it all to $TMPDIR/mc-$USER/
Pavel/Andrew, if you want me to mail the mc CVS patches in addition to
attaching
here, please let me know.
Created attachment 99413 [details]
Fixed CVS Patch to simplify column title handling (instead of 99144)
Created attachment 99414 [details]
CVS patch with tar.c fixes
Unterminated filenames in hostile tarballs could cause at least a DoS.
BTW, the whole Replace function in mcedit seems to be broken by design. Try e.g.: perl -e 'print "A"; print "B" x 284; print "A\n"' > test mcedit test F4 A(B*)A C%s%s%s%s%s%sC 1,1,1,1,1,1 Alt-R segfault. Or: F4 A%sA x Alt-E endless loop (again, buffer overflow). Created attachment 99474 [details]
mc CVS patch to avoid buffer overflows in search/replace
So as far as CVE names go we have to allocate one name per type of vulnerability; but we can merge the two sets of buffer overflows (public/non-public) because there has not been an release including the public ones yet. So CAN-2004-0226 for buffer overflows in mc (as already stated) CAN-2004-0231 for the temporary file issues CAN-2004-0232 for the format string issue embargo moved to Apr 29th since a number of vendors were not ready in time for yesterday Created attachment 99656 [details]
Possible double free?
Jackub, can you review your code in complete.c and investigate is a double free
possible without this patch?
I don't think this is double free. g_free (path) is called only if the function is about to return NULL, but when it returns NULL, it will be surely called with state == 0 next time and thus reinitialize path. I admit the code is ugly and if I wrote it now and not 9 years ago, I'd wrote it differently. But I think the code maintains the invariant that after it returns NULL, both path and cur_word are freed and cur_word is NULL. And if the function doesn't return NULL, it will be called once again, until it eventually returns NULL. I think the second part of the patch is certainly good (perhaps the if (path) can go, g_free (path) should work even if path == NULL), but the former one could be perhaps replaced by assert if you want. Well, this code abuses statit variables a bit. I preffer to check this condition itself to avoid expensive function call - it's my point like your about strncpy(). Well, can you or Mark visit http://www.securityfocus.com/bid/2016/solution/ to try to close this issue - it seems these people don't trust me and Pavel. Created attachment 99663 [details]
extfs.c off-by-one write for empty lines in extfs.ini
Created attachment 99666 [details]
Buffer overflow in sfs.c for invalid sfs.ini
Possible for empty lines and lines without space or tabulation.
sfs.ini and extfs.ini location can be reset with MC_LIBDIR environment variable
for MC before 4.6.0 and with MC_DATADIR since 4.6.0.
These two bugs are not too dangerous because these files are owned by root.
Case with specially formed environment and {sfs,extfs}.ini is unlikely.
An errata has been issued which should help the problem described in this bug report. This report is therefore being closed with a resolution of ERRATA. For more information on the solution and/or where to find the updated files, please follow the link below. You may reopen this bug report if the solution does not work for you. http://rhn.redhat.com/errata/RHSA-2004-172.html |