Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!

Bug 73858

Summary: sys-apps/ed produces incorrect output (bug 66400 redux)
Product: Gentoo Linux Reporter: Robin Johnson <robbat2>
Component: [OLD] Core systemAssignee: Gentoo's Team for Core System packages <base-system>
Severity: critical CC: security, ulm
Priority: High    
Version: unspecified   
Hardware: All   
OS: Linux   
Package list:
Runtime testing required: ---
Bug Depends on:    
Bug Blocks: 66400    
Attachments: edtest.cpp

Description Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2004-12-08 18:25:29 UTC
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.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-
Headers:  sys-kernel/linux26-headers-
Libtools: sys-devel/libtool-1.5.2-r7
CFLAGS="-march=athlon-xp -mcpu=athlon-xp -O3 -pipe"
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"
FEATURES="autoaddcvs autoconfig buildpkg ccache confcache cvs digest distcc 
distlocks sandbox sfperms sign userpriv"
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"
Comment 1 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2004-12-08 18:27:53 UTC
Created attachment 45580 [details]

Source code for testing binary.
Comment 2 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2004-12-08 18:28:24 UTC
Created attachment 45581 [details]

Script file to run test case and prove the error.
Comment 3 Thierry Carrez (RETIRED) gentoo-dev 2004-12-22 04:26:40 UTC
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 @@
 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...
Comment 4 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2004-12-23 02:42:56 UTC
The error in your patch is that mkstemp returns a file descriptor for an OPEN file already (eg it runs fopen on the file).

+  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.
Comment 5 SpanKY gentoo-dev 2004-12-23 13:49:11 UTC
-    if ((sfd == -1) || (sfp = fopen (sfn, "w+")) == NULL)
+    if (sfd == -1)

so this patch will fix it ?
Comment 6 Ulrich Müller gentoo-dev 2004-12-27 03:18:25 UTC
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)
Comment 7 SpanKY gentoo-dev 2004-12-27 05:37:23 UTC
true ... robbat2, that patch looks much more sane, does that resolve this for you ?
Comment 8 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2005-01-09 02:50:32 UTC
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.
Comment 9 SpanKY gentoo-dev 2005-01-09 12:31:12 UTC
marked stable ;)