<!-- SBZ_reproduce  -->
...

Created attachment 18075 [details]
the patches

Could you please have a look what of this affects us?

Additional info since some fix seems to mix up two arguments:

Date: Wed, 7 Apr 2004 08:53:59 -0400
From: Jakub Jelinek <jakub@redhat.com>
To: vendor-sec@lst.de
Subject: Re: [vendor-sec] Buffer overflows in mc

On Wed, Apr 07, 2004 at 04:24:41PM +0400, Dmitry V. Levin wrote:
> Hi,
> 
> On Tue, Apr 06, 2004 at 04:09:42PM -0400, Jakub Jelinek wrote:
> > We are planning to errata Midnight Commander soon to fix several buffer
> > overflows and one format string vulnerability.
> [...]
> > In addition to this the patch includes some bugfixes as well.
> 
> And some bugs, too; in the screen.c hunk,
> 
> >                  attrset (MARKED_COLOR);
> >                  spaces = (format->field_len - header_len) / 2;
> >                  extra  = (format->field_len - header_len) % 2;
> > -  printw ("%*s%-s%*s", spaces, "",
> > -   txt, spaces+extra, "");
> > +  printw ("%*s%.*s%*s", spaces, "",
> > +  format->title, header_len, spaces+extra, "");
> >    width -= 2 * spaces + extra + header_len;
> >        } else {
> >    attrset (NORMAL_COLOR);
> 
> you can see that 4th and 5th arguments of the printw call are mixed up.

Oops, sorry.  In our package we have a UTF-8 patch which collides
in this area and I've apparently made a bug when backporting the
patch to vanilla 4.6.0.
Of course it should have been
+      header_len, format->title, spaces+extra, "");

       Jakub
_______________________________________________

Yes, we are affected. 
Applying these patches to mc form 9.1 will need some work as we have quite a 
lot of patches for UTF-8 and other. 
In older dstributions should applying this patch make less problems. 

Thank you.

I have ported the patch to all versions. 
Can you please attach patchinfo files? 

Created attachment 18349 [details]
new patch from vendor-sec

There was a new patch ball sent to vendor-sec. Basically
I think they handle the tmp-stuff different they said
and fix one hunk I already wrote down in some comment above.
Is it possible to use this instead of the old fix? Sorry.

The tmp handling from this patch can't be ported to SLES7 and SLES7-PPC, 
because it was completely rewritten between 4.5.54 and 4.5.55. 
The old mc versions use tempnam, thus are probably vulnerable too. 

I think the tmp handling within the C-files are not the
important part (find.c). Might be the SLES7 src does not contain
the vulnerable code there?
The changes for the tmp files basically
protect from overflows AFAIS. Only the changes to the script files
affect tmp-race issues.

mc 4.5.54 uses tempnam 
mc 4.5.55 uses it's own implementation of mkstemp (mc_mkstemp), which creates  
	  files directly in /tmp 
mc 4.6.0  creates a subdirectory in /tmp and mc_mkstemp creates files in this      
          subdirectory. 
 
The second patch modifies the virtual filesystem scripts to use this 
subdirectory. (the directory name is exported via MC_TMPDIR). 
 
Creating the subdirectory can be easily ported to 4.5.55 as it affects only 
the function mc_mkstemp. 
 
Using mkstemp in 4.5.54 would require many changes in the code. Here I don't 
know how to best solve it. 
 
The buffer overflows are fixed already in the first patch. There are not 
problems with it. 
 

Hm, no idea. Should we use the old patch?
If not it seems someone has to port back the tmp-code for SLES7.
Thomas, Roman?

The virtual filesystem scripts can create subdirectories in /tmp independently  
of mc. 
The only remaining question is whether we want to fix tempnam usage. 
 

That would be desireable, yes. Either by open(O_CREAT|O_EXCL, ...) and 
fdopen(), or by mkstemp(3) and again subsequent fdopen().

OK. It uses O_CREAT|O_EXCL everywhere except one place. I can fix it. 
This code is supposed to be safe, OK? 
 
    tmp = tempnam (NULL, "mclocalcopy"); 
    fdout = open (tmp, O_CREAT|O_WRONLY|O_TRUNC|O_EXCL, 0600); 
    if (fdout == -1) 
        goto fail; 
 
 
For scripts I can use something like this: 
 
 mchafs_copyout () 
 { 
-    cd /tmp 
+    TMPDIR=`mktemp -d /tmp/mctmpdir-uha.XXXXXX` || exit 1 
+    cd $TMPDIR 
     eval $HA xy $1 $2 >/dev/null 2>&1 
     mv -f $2 $3 
+    rm -rf $TMPDIR 
 } 
  

The code snippet above: O_CREAT|O_EXCL|O_WRONLY should be enough - it can't
truncate a file if it fails already if the file exists before the open().

What the script patch is concerned: I don't know right now in which context
this function is called, but the eval with variables should be taken seriously
(quoting, meta characters). What the creation of the temp directory is concerned,
I would think it is perfect, yes. :-)

I have done the patches. 
Can you please create patchinfo files? 

packages are submitted. 

submitted patchinfo files: 
 
thomas@bragg:~/LPPF> cp patchinfo*.mc /work/src/done/PATCHINFO/ 
`patchinfo-box.mc' -> `/work/src/done/PATCHINFO/patchinfo-box.mc' 
`patchinfo.mc' -> `/work/src/done/PATCHINFO/patchinfo.mc' 

The patch for 8.0, 8.1 and SLES8 contained implicit declaration of  
mc_tmpdir. It crashed on 64 bit architectures due this bug. 
I submitted fixed packages. 
Can you please resubmit patchinfo? 

Thank you.. 
 
thomas@bragg:~/LPPF> cp patchinfo.mc patchinfo-box.mc /work/src/done/
PATCHINFO/ 
`patchinfo.mc' -> `/work/src/done/PATCHINFO/patchinfo.mc' 
`patchinfo-box.mc' -> `/work/src/done/PATCHINFO/patchinfo-box.mc 

Created attachment 18754 [details]
patchinfo-box.mc

Created attachment 18755 [details]
patchinfo.mc

rejected old mc packages... 
 
thomas@bragg:~/LPPF> reject-patchinfo  432dab346d98eab9b34660ce2aa6460e 
"buggy, new one follows" 
thomas@bragg:~/LPPF> reject-patchinfo 18728354d964b9ae8157899485cb3a0a "buggy, 
new one follows" 

Have the correct packages been approved? then this case can be closed :)

yes! 

from advisory:
CVE-2004-0226
CVE-2004-0231
CVE-2004-0232

CVE-2004-0232: CVSS v2 Base Score: 5.0 (AV:N/AC:L/Au:N/C:N/I:N/A:P)