Bug 118302 - One more buffer overflow in mc's vfs code
One more buffer overflow in mc's vfs code
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: mc (Show other bugs)
2
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jakub Jelinek
: Security
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2004-03-15 08:30 EST by Andrew V. Samoilov
Modified: 2007-11-30 17:10 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2004-05-19 15:04:30 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Possible buffer overflow in vfs/direntry.c:vfs_s_find_entry_tree() (908 bytes, patch)
2004-03-15 08:34 EST, Andrew V. Samoilov
no flags Details | Diff
Missed "%s" pattern in vfs/fish.c:send_fish_cmd() (1.86 KB, patch)
2004-03-15 11:23 EST, Andrew V. Samoilov
no flags Details | Diff
Numerous readlink() related off-by-ones (4.74 KB, patch)
2004-03-15 11:26 EST, Andrew V. Samoilov
no flags Details | Diff
Unsafe strcpy(), unchecked mc_stat() return value and wrong $ pattern matching in "Find file" (3.69 KB, patch)
2004-03-15 12:42 EST, Andrew V. Samoilov
no flags Details | Diff
view.c off-by-one (1.64 KB, patch)
2004-03-15 12:49 EST, Andrew V. Samoilov
no flags Details | Diff
Rare file corruption on copy/move (782 bytes, patch)
2004-03-15 13:01 EST, Andrew V. Samoilov
no flags Details | Diff
Rare segfault and unwanted command execution in F2/Menu (501 bytes, text/plain)
2004-03-15 13:05 EST, Andrew V. Samoilov
no flags Details
edit/syntax.c off-by-x (3.96 KB, patch)
2004-03-16 04:14 EST, Andrew V. Samoilov
no flags Details | Diff
Two more possible buffer overflows in edit/syntax.c (616 bytes, patch)
2004-03-16 04:22 EST, Andrew V. Samoilov
no flags Details | Diff
Variable misuse and setup_passive() fallback in vfs/ftpfs.c (4.02 KB, patch)
2004-03-16 04:36 EST, Andrew V. Samoilov
no flags Details | Diff
Filter is not applied after command execution (2.64 KB, patch)
2004-03-16 05:19 EST, Andrew V. Samoilov
no flags Details | Diff
Reverse search segfault in built-in viewer if there is no such string (1.47 KB, patch)
2004-03-16 05:26 EST, Andrew V. Samoilov
no flags Details | Diff
My current patch (18.49 KB, patch)
2004-03-16 06:12 EST, Jakub Jelinek
no flags Details | Diff
Memory leaks (2.11 KB, patch)
2004-03-16 06:25 EST, Andrew V. Samoilov
no flags Details | Diff
My current patch (18.49 KB, patch)
2004-03-16 06:35 EST, Jakub Jelinek
no flags Details | Diff
tarfs fixes (895 bytes, patch)
2004-03-16 06:37 EST, Jakub Jelinek
no flags Details | Diff
Unsafe strcat() in editcmd.c:edit_complete_word_cmd() (1.10 KB, patch)
2004-04-02 13:21 EST, Andrew V. Samoilov
no flags Details | Diff
Patch against mc CVS to fix some cpiofs bugs and security issues. (5.17 KB, patch)
2004-04-05 09:34 EDT, Jakub Jelinek
no flags Details | Diff
Patch against CVS mc to fix problems with weirdo chars in rpm filenames and paths (1.69 KB, patch)
2004-04-05 12:14 EDT, Jakub Jelinek
no flags Details | Diff
My current patch against latest mc rpm (73.89 KB, patch)
2004-04-05 12:26 EDT, Jakub Jelinek
no flags Details | Diff
CVS mc patch for debug_out and =+/+= handling (2.64 KB, patch)
2004-04-06 09:14 EDT, Jakub Jelinek
no flags Details | Diff
Patch to fix st_blocks printing on 64-bit platforms against mc CVS (16.12 KB, patch)
2004-04-06 09:36 EDT, Jakub Jelinek
no flags Details | Diff
Updated jumbo patch (79.22 KB, patch)
2004-04-06 09:41 EDT, Jakub Jelinek
no flags Details | Diff
CVS Patch to simplify column title handling (1.18 KB, patch)
2004-04-06 10:16 EDT, Jakub Jelinek
no flags Details | Diff
CVS mc patch to fix temporary file handling and move it all to $TMPDIR/mc-$USER/ (9.62 KB, patch)
2004-04-14 13:31 EDT, Jakub Jelinek
no flags Details | Diff
Fixed CVS Patch to simplify column title handling (instead of 99144) (1.18 KB, patch)
2004-04-14 13:34 EDT, Jakub Jelinek
no flags Details | Diff
CVS patch with tar.c fixes (1.33 KB, patch)
2004-04-14 13:44 EDT, Jakub Jelinek
no flags Details | Diff
mc CVS patch to avoid buffer overflows in search/replace (5.73 KB, patch)
2004-04-16 05:33 EDT, Jakub Jelinek
no flags Details | Diff
Possible double free? (575 bytes, patch)
2004-04-23 15:12 EDT, Andrew V. Samoilov
no flags Details | Diff
extfs.c off-by-one write for empty lines in extfs.ini (1.34 KB, patch)
2004-04-23 17:10 EDT, Andrew V. Samoilov
no flags Details | Diff
Buffer overflow in sfs.c for invalid sfs.ini (1.24 KB, patch)
2004-04-23 17:34 EDT, Andrew V. Samoilov
no flags Details | Diff

  None (edit)
Description Andrew V. Samoilov 2004-03-15 08:30:34 EST
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5)
Gecko/20031007

Description of problem:
It's possible to be new security issue.
There is no buffer limit check in vfs/direntry.c:vfs_s_find_entry_tree().

Version-Release number of selected component (if applicable):
mc-4.6.0-8.4

How reproducible:
Always

Steps to Reproduce:
Open a specially constructed vfs archive.

Additional info: patch will be attached as soon as bugzilla allow it.
Comment 1 Andrew V. Samoilov 2004-03-15 08:34:13 EST
Created attachment 98532 [details]
Possible buffer overflow in vfs/direntry.c:vfs_s_find_entry_tree()
Comment 2 Jakub Jelinek 2004-03-15 10:02:20 EST
That -1 in if (t + pseg + sizeof (PATH_SEP_STR) - 1 > &p[sizeof (p)])
is wrong.  Otherwise the overflow is still there.
Comment 3 Andrew V. Samoilov 2004-03-15 11:23:45 EST
Created attachment 98535 [details]
Missed "%s" pattern in vfs/fish.c:send_fish_cmd()
Comment 4 Andrew V. Samoilov 2004-03-15 11:26:41 EST
Created attachment 98536 [details]
Numerous readlink() related off-by-ones
Comment 5 Jakub Jelinek 2004-03-15 11:52:44 EST
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
Comment 6 Jakub Jelinek 2004-03-15 12:13:34 EST
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..
Comment 7 Andrew V. Samoilov 2004-03-15 12:33:12 EST
It is on subversions.gnu.org:

Anonymous CVS access:

cvs -z3 -d:ext:anoncvs@savannah.gnu.org:/cvsroot/mc co mc



Comment 8 Andrew V. Samoilov 2004-03-15 12:42:32 EST
Created attachment 98544 [details]
Unsafe strcpy(), unchecked mc_stat() return value and wrong $ pattern matching in "Find file"
Comment 9 Andrew V. Samoilov 2004-03-15 12:49:37 EST
Created attachment 98545 [details]
view.c off-by-one
Comment 10 Andrew V. Samoilov 2004-03-15 13:01:48 EST
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.
Comment 11 Andrew V. Samoilov 2004-03-15 13:05:34 EST
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.
Comment 12 Andrew V. Samoilov 2004-03-15 13:10:13 EST
Re: Additional Comment #5 From Jakub Jelinek (jakub@redhat.com)  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.
Comment 13 Andrew V. Samoilov 2004-03-16 04:14:06 EST
Created attachment 98562 [details]
edit/syntax.c off-by-x
Comment 14 Andrew V. Samoilov 2004-03-16 04:22:10 EST
Created attachment 98563 [details]
Two more possible buffer overflows in edit/syntax.c
Comment 15 Andrew V. Samoilov 2004-03-16 04:36:12 EST
Created attachment 98564 [details]
Variable misuse and setup_passive() fallback in vfs/ftpfs.c

2003-07-21  Andrew V. Samoilov	<sav@bcs.zp.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@kmlinux.fjfi.cvut.cz>

  * ftpfs.c (command): Fix misuse of the status variable.  Don't
  cache the value of SUP.sock before reconnect.
Comment 16 Andrew V. Samoilov 2004-03-16 05:19:57 EST
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...
Comment 17 Andrew V. Samoilov 2004-03-16 05:26:48 EST
Created attachment 98566 [details]
Reverse search segfault in built-in viewer if there is no such string
Comment 18 Jakub Jelinek 2004-03-16 06:12:51 EST
Created attachment 98567 [details]
My current patch
Comment 19 Jakub Jelinek 2004-03-16 06:21:54 EST
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.
Comment 20 Andrew V. Samoilov 2004-03-16 06:25:42 EST
Created attachment 98568 [details]
Memory leaks
Comment 21 Jakub Jelinek 2004-03-16 06:35:41 EST
Created attachment 98569 [details]
My current patch
Comment 22 Jakub Jelinek 2004-03-16 06:37:37 EST
Created attachment 98570 [details]
tarfs fixes
Comment 23 Jakub Jelinek 2004-03-16 06:39:10 EST
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.
Comment 24 Andrew V. Samoilov 2004-03-16 07:30:44 EST
Re: Additional Comment #19 From Jakub Jelinek (jakub@redhat.com)  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 ?
Comment 25 Andrew V. Samoilov 2004-04-02 13:21:46 EST
Created attachment 99078 [details]
Unsafe strcat() in editcmd.c:edit_complete_word_cmd()
Comment 26 Andrew V. Samoilov 2004-04-02 13:30:46 EST
* 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
Comment 27 Jakub Jelinek 2004-04-05 09:34:31 EDT
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.
Comment 28 Jakub Jelinek 2004-04-05 12:14:44 EDT
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.
Comment 29 Jakub Jelinek 2004-04-05 12:26:56 EDT
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?
Comment 30 Mark J. Cox (Product Security) 2004-04-06 06:11:08 EDT
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.
Comment 31 Andrew V. Samoilov 2004-04-06 07:22:18 EDT
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.
Comment 32 Jakub Jelinek 2004-04-06 07:54:24 EDT
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.
Comment 33 Jakub Jelinek 2004-04-06 09:14:20 EDT
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).
Comment 34 Jakub Jelinek 2004-04-06 09:36:20 EDT
Created attachment 99139 [details]
Patch to fix st_blocks printing on 64-bit platforms against mc CVS
Comment 35 Jakub Jelinek 2004-04-06 09:41:28 EDT
Created attachment 99140 [details]
Updated jumbo patch
Comment 36 Jakub Jelinek 2004-04-06 10:16:16 EDT
Created attachment 99144 [details]
CVS Patch to simplify column title handling
Comment 37 Andrew V. Samoilov 2004-04-07 13:30:57 EDT
* 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
Comment 38 Mark J. Cox (Product Security) 2004-04-08 08:03:28 EDT
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
Comment 39 Andrew V. Samoilov 2004-04-08 14:52:02 EDT
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
Comment 40 Jakub Jelinek 2004-04-14 13:31:37 EDT
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.
Comment 41 Jakub Jelinek 2004-04-14 13:34:55 EDT
Created attachment 99413 [details]
Fixed CVS Patch to simplify column title handling (instead of 99144)
Comment 42 Jakub Jelinek 2004-04-14 13:44:31 EDT
Created attachment 99414 [details]
CVS patch with tar.c fixes

Unterminated filenames in hostile tarballs could cause at least a DoS.
Comment 43 Jakub Jelinek 2004-04-16 03:51:05 EDT
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).
Comment 44 Jakub Jelinek 2004-04-16 05:33:40 EDT
Created attachment 99474 [details]
mc CVS patch to avoid buffer overflows in search/replace
Comment 45 Mark J. Cox (Product Security) 2004-04-16 11:26:26 EDT
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

Comment 46 Mark J. Cox (Product Security) 2004-04-22 06:34:03 EDT
embargo moved to Apr 29th since a number of vendors were not ready in
time for yesterday
Comment 47 Andrew V. Samoilov 2004-04-23 15:12:35 EDT
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?
Comment 48 Jakub Jelinek 2004-04-23 15:46:41 EDT
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.
Comment 49 Andrew V. Samoilov 2004-04-23 16:07:47 EDT
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.
Comment 50 Andrew V. Samoilov 2004-04-23 17:10:05 EDT
Created attachment 99663 [details]
extfs.c off-by-one write for empty lines in extfs.ini
Comment 51 Andrew V. Samoilov 2004-04-23 17:34:06 EDT
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.
Comment 52 Mark J. Cox (Product Security) 2004-05-19 15:04:31 EDT
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

Note You need to log in before you can comment on or make changes to this bug.