Bugzilla Bug 73858

Since the security fix in bug #66400 was applied to ed, it no longer produces a
correct output file if the stderr file description is closed. Instead it
produces a corrupted output file.

If the patch from #66400 is NOT used, then the bug does not occur.

Reproducible: Always
Steps to Reproduce:
see attached test script files.
Actual Results:  
if stderr is closed when ed runs, the output file is corrupted when the 'write' 
command is run in ed.

Expected Results:  
The output file should not be corrupted.

Portage 2.0.51-r3 (!/usr/portage/profiles/default-linux/x86/2004.3, gcc-3.3.5, 
glibc-2.3.4.20041102-r0, 2.6.7-mm4 i686)
=================================================================
System uname: 2.6.7-mm4 i686 AMD Athlon(tm) XP 2400+
Gentoo Base System version 1.6.6
distcc 2.18 i686-pc-linux-gnu (protocols 1 and 2) (default port 3632) [enabled]
ccache version 2.3 [enabled]
Autoconf: sys-devel/autoconf-2.59-r5
Automake: sys-devel/automake-1.8.5-r1
Binutils: sys-devel/binutils-2.15.92.0.2-r1
Headers:  sys-kernel/linux26-headers-2.6.8.1-r1
Libtools: sys-devel/libtool-1.5.2-r7
ACCEPT_KEYWORDS="x86 ~x86"
AUTOCLEAN="yes"
CFLAGS="-march=athlon-xp -mcpu=athlon-xp -O3 -pipe"
CHOST="i686-pc-linux-gnu"
COMPILER=""
CONFIG_PROTECT="/etc /usr/X11R6/lib/X11/xkb /usr/kde/2/share/config
/usr/kde/3/s
hare/config /usr/share/config /usr/share/texmf/dvipdfm/config/
/usr/share/texmf/
dvips/config/ /usr/share/texmf/tex/generic/config/
/usr/share/texmf/tex/platex/c
onfig/ /usr/share/texmf/xdvi/ /var/bind /var/qmail/alias /var/qmail/control
/var
/vpopmail/domains /var/vpopmail/etc"
CONFIG_PROTECT_MASK="/etc/gconf /etc/terminfo /etc/env.d"
CXXFLAGS="-march=athlon-xp -mcpu=athlon-xp -O3 -pipe"
DISTDIR="/usr/gentoo-distfiles"
FEATURES="autoaddcvs autoconfig buildpkg ccache confcache cvs digest distcc 
distlocks sandbox sfperms sign userpriv"
GENTOO_MIRRORS="http://gentoo.ccccom.com http://gentoo.seren.com/gentoo"
MAKEOPTS="-j1"
PKGDIR="/usr/gentoo-packages"
PORTAGE_TMPDIR="/var/tmp"
PORTDIR="/usr/gentoo-cvs/gentoo-x86"
PORTDIR_OVERLAY="/usr/local/portage"
SYNC=""
USE="3dnow aalib acl acpi alsa amd apache2 apm berkdb bitmap-fonts cdr cgi 
clearpasswd crypt cscope cups curl divx4linux f77 fam foomaticdb fortran gd 
gdbm geoip gif imagemagick imap innodb ipalias ipv6 java jikes jpeg junit 
libwww mad maildir mcal md5sum mikmod mmx mpeg multitarget mysql ncurses 
offensive pam pcap pdflib perl pic plotutils png pnp ppds python qmail readline 
samba scanner slp snmp socks5 spell sqlite sse ssl tetex tiff truetype type1 
ungif usb userlocales v4l v4l2 x86 xml xml2 xvid zlib"

Created an attachment (id=45580) [edit]
edtest.cpp

Source code for testing binary.

Created an attachment (id=45581) [edit]
dotest.sh

Script file to run test case and prove the error.

Here is the patch (from LFS) that was applied by base-system to fix the
vulnerability :

============================================================
--- ed-0.2/buf.c        Sat Nov 19 04:37:59 1994
+++ ed-0.2-2/buf.c      Tue May 28 18:38:23 2002
@@ -200,13 +200,13 @@
 int
 open_sbuf ()
 {
-  char *mktemp ();
-  int u;
+  int u, sfd;

   isbinary = newline_added = 0;
   u = umask(077);
   strcpy (sfn, "/tmp/ed.XXXXXX");
-  if (mktemp (sfn) == NULL || (sfp = fopen (sfn, "w+")) == NULL)
+  sfd = mkstemp(sfn);
+    if ((sfd == -1) || (sfp = fopen (sfn, "w+")) == NULL)
     {
       fprintf (stderr, "%s: %s\n", sfn, strerror (errno));
       sprintf (errmsg, "Cannot open temp file");
=============================================================

I fail to see where it introduces a regression, since apparently it just
switched a mktemp for a mkstemp. Maybe someone more literate on those issues
will find where it fails...

The error in your patch is that mkstemp returns a file descriptor for an OPEN
file already (eg it runs fopen on the file).

so:
+  sfd = mkstemp(sfn);
+    if ((sfd == -1) || (sfp = fopen (sfn, "w+")) == NULL)

if mkstemp returns a good value, and then "sfp = fopen (sfn, "w+")" is run,
opening the file again.

-    if ((sfd == -1) || (sfp = fopen (sfn, "w+")) == NULL)
+    if (sfd == -1)

so this patch will fix it ?

This wouldn't work, since not a file descriptor, but a FILE pointer is needed.
However, I believe the patch from LFS is incorrect. Not fopen should be called,
but fdopen on the descriptor:

-    if ((sfd == -1) || (sfp = fopen (sfn, "w+")) == NULL)
+    if ((sfd == -1) || (sfp = fdopen (sfd, "w+")) == NULL)

true ... robbat2, that patch looks much more sane, does that resolve this for
you ?

Ok, your patch change there to use fdopen fixes the problem.
I've put it in the tree as r5, but arches need to stabilize.
The changed patch should probably be sent upstream.

marked stable ;)