Bug 118302

Summary: One more buffer overflow in mc's vfs code
Product: [Fedora] Fedora Reporter: Andrew V. Samoilov <samoilov>
Component: mcAssignee: Jakub Jelinek <jakub>
Status: CLOSED ERRATA QA Contact:
Severity: medium Docs Contact:
Priority: medium    
Version: 2CC: mitr, mjc, plroskin
Target Milestone: ---Keywords: Security
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2004-05-19 19:04:30 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Possible buffer overflow in vfs/direntry.c:vfs_s_find_entry_tree()
none
Missed "%s" pattern in vfs/fish.c:send_fish_cmd()
none
Numerous readlink() related off-by-ones
none
Unsafe strcpy(), unchecked mc_stat() return value and wrong $ pattern matching in "Find file"
none
view.c off-by-one
none
Rare file corruption on copy/move
none
Rare segfault and unwanted command execution in F2/Menu
none
edit/syntax.c off-by-x
none
Two more possible buffer overflows in edit/syntax.c
none
Variable misuse and setup_passive() fallback in vfs/ftpfs.c
none
Filter is not applied after command execution
none
Reverse search segfault in built-in viewer if there is no such string
none
My current patch
none
Memory leaks
none
My current patch
none
tarfs fixes
none
Unsafe strcat() in editcmd.c:edit_complete_word_cmd()
none
Patch against mc CVS to fix some cpiofs bugs and security issues.
none
Patch against CVS mc to fix problems with weirdo chars in rpm filenames and paths
none
My current patch against latest mc rpm
none
CVS mc patch for debug_out and =+/+= handling
none
Patch to fix st_blocks printing on 64-bit platforms against mc CVS
none
Updated jumbo patch
none
CVS Patch to simplify column title handling
none
CVS mc patch to fix temporary file handling and move it all to $TMPDIR/mc-$USER/
none
Fixed CVS Patch to simplify column title handling (instead of 99144)
none
CVS patch with tar.c fixes
none
mc CVS patch to avoid buffer overflows in search/replace
none
Possible double free?
none
extfs.c off-by-one write for empty lines in extfs.ini
none
Buffer overflow in sfs.c for invalid sfs.ini none

Description Andrew V. Samoilov 2004-03-15 13:30:34 UTC
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 13:34:13 UTC
Created attachment 98532 [details]
Possible buffer overflow in vfs/direntry.c:vfs_s_find_entry_tree()

Comment 2 Jakub Jelinek 2004-03-15 15:02:20 UTC
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 16:23:45 UTC
Created attachment 98535 [details]
Missed "%s" pattern in vfs/fish.c:send_fish_cmd()

Comment 4 Andrew V. Samoilov 2004-03-15 16:26:41 UTC
Created attachment 98536 [details]
Numerous readlink() related off-by-ones

Comment 5 Jakub Jelinek 2004-03-15 16:52:44 UTC
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 17:13:34 UTC
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 17:33:12 UTC
It is on subversions.gnu.org:

Anonymous CVS access:

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





Comment 8 Andrew V. Samoilov 2004-03-15 17:42:32 UTC
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 17:49:37 UTC
Created attachment 98545 [details]
view.c off-by-one

Comment 10 Andrew V. Samoilov 2004-03-15 18:01:48 UTC
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 18:05:34 UTC
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 18:10:13 UTC
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.

Comment 13 Andrew V. Samoilov 2004-03-16 09:14:06 UTC
Created attachment 98562 [details]
edit/syntax.c off-by-x

Comment 14 Andrew V. Samoilov 2004-03-16 09:22:10 UTC
Created attachment 98563 [details]
Two more possible buffer overflows in edit/syntax.c

Comment 15 Andrew V. Samoilov 2004-03-16 09:36:12 UTC
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.

Comment 16 Andrew V. Samoilov 2004-03-16 10:19:57 UTC
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 10:26:48 UTC
Created attachment 98566 [details]
Reverse search segfault in built-in viewer if there is no such string

Comment 18 Jakub Jelinek 2004-03-16 11:12:51 UTC
Created attachment 98567 [details]
My current patch

Comment 19 Jakub Jelinek 2004-03-16 11:21:54 UTC
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 11:25:42 UTC
Created attachment 98568 [details]
Memory leaks

Comment 21 Jakub Jelinek 2004-03-16 11:35:41 UTC
Created attachment 98569 [details]
My current patch

Comment 22 Jakub Jelinek 2004-03-16 11:37:37 UTC
Created attachment 98570 [details]
tarfs fixes

Comment 23 Jakub Jelinek 2004-03-16 11:39:10 UTC
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 12:30:44 UTC
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 ?


Comment 25 Andrew V. Samoilov 2004-04-02 18:21:46 UTC
Created attachment 99078 [details]
Unsafe strcat() in editcmd.c:edit_complete_word_cmd()

Comment 26 Andrew V. Samoilov 2004-04-02 18:30:46 UTC
* 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 13:34:31 UTC
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 16:14:44 UTC
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 16:26:56 UTC
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 2004-04-06 10:11:08 UTC
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 11:22:18 UTC
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 11:54:24 UTC
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 13:14:20 UTC
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 13:36:20 UTC
Created attachment 99139 [details]
Patch to fix st_blocks printing on 64-bit platforms against mc CVS

Comment 35 Jakub Jelinek 2004-04-06 13:41:28 UTC
Created attachment 99140 [details]
Updated jumbo patch

Comment 36 Jakub Jelinek 2004-04-06 14:16:16 UTC
Created attachment 99144 [details]
CVS Patch to simplify column title handling

Comment 37 Andrew V. Samoilov 2004-04-07 17:30:57 UTC
* 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 2004-04-08 12:03:28 UTC
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 18:52:02 UTC
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 17:31:37 UTC
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 17:34:55 UTC
Created attachment 99413 [details]
Fixed CVS Patch to simplify column title handling (instead of 99144)

Comment 42 Jakub Jelinek 2004-04-14 17:44:31 UTC
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 07:51:05 UTC
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 09:33:40 UTC
Created attachment 99474 [details]
mc CVS patch to avoid buffer overflows in search/replace

Comment 45 Mark J. Cox 2004-04-16 15:26:26 UTC
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 2004-04-22 10:34:03 UTC
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 19:12:35 UTC
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 19:46:41 UTC
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 20:07:47 UTC
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 21:10:05 UTC
Created attachment 99663 [details]
extfs.c off-by-one write for empty lines in extfs.ini

Comment 51 Andrew V. Samoilov 2004-04-23 21:34:06 UTC
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 2004-05-19 19:04:31 UTC
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