GNU bug report logs - #15522
gzcmp/gzdiff + gznew shell scripts use temporary files unsafely

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 15522 in the body.
You can then email your comments to 15522 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rich Burridge <rich.burridge <at> oracle.com>
To: bug-gzip <at> gnu.org
Subject: gzcmp/gzdiff + gznew shell scripts use temporary files unsafely
Date: Thu, 03 Oct 2013 17:18:57 -0700

Hi,

We've had a bug reported against the version of gzip that we ship in 
Solaris:

"The gzcmp and gzdiff (same script hardlinked) commands shipped with 
Solaris
write to a file in the world writable directory '/tmp' if both of its
arguments are compressed files. 'set -C' is used to ensure that the file
doesn't already exist when it's being written to (which prevents a
symlink-based attack), but that allows a mild Denial of Service by creating
this file in advance, which would therefore cause gzcmp / gzdiff to abort.

                              set -C
                              trap 'rm -f /tmp/"$F".$$; exit 2' 1 2 13 
15 0
                              gzip -cdfq "$2" > /tmp/"$F".$$ || exit


gznew is similarly impacted:

      tmp=/tmp/zfoo.$$
      set -C
      echo hi > $tmp.1
      echo hi > $tmp.2

While it's arguably unlikely that these issues would ever be exploited,
it is suggested that it would be better for these commands to use mktemp."

Thanks.

Message #8 received at 15522 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Rich Burridge <rich.burridge <at> oracle.com>, 15522 <at> debbugs.gnu.org
Subject: Re: bug#15522: gzcmp/gzdiff + gznew shell scripts use temporary files
 unsafely
Date: Thu, 03 Oct 2013 18:47:28 -0700

Rich Burridge wrote:
> it would be better for these commands to use mktemp

That was done in gzip 1.3.10, released 2006-12-30.
Is this not working for you?  If not, why not?

Message #11 received at 15522 <at> debbugs.gnu.org (full text, mbox):

From: Rich Burridge <rich.burridge <at> oracle.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 15522 <at> debbugs.gnu.org
Subject: Re: bug#15522: gzcmp/gzdiff + gznew shell scripts use temporary files
 unsafely
Date: Thu, 03 Oct 2013 19:37:13 -0700

On 10/03/2013 06:47 PM, Paul Eggert wrote:
> Rich Burridge wrote:
>> it would be better for these commands to use mktemp
> That was done in gzip 1.3.10, released 2006-12-30.
> Is this not working for you?  If not, why not?

I can see mktemp usage in gzexe.in and zdiff.in, but the Solaris bug report
was suggesting the same sort of thing should be done in:

zdiff.in:

128                         else
129                           set -C
130                           tmp=${TMPDIR-/tmp}/$F.$$
131                         fi
132                         gzip -cdfq -- "$2" > "$tmp" || exit 2

and znew.in:

 63 set -C
 64 echo hi > $tmp || exit
 65 if test -z "`(${CPMOD-cpmod} $tmp $tmp) 2>&1`"; then

Sorry, I probably confused things by giving their Solaris g<name> names,
and by stating that gzcmp and gzdiff were hard-linked without actually 
checking
(because that's no longer true in the latest versions of the gzip 
distribution).

Message #16 received at 15522-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Rich Burridge <rich.burridge <at> oracle.com>
Cc: 15522-done <at> debbugs.gnu.org
Subject: Re: bug#15522: gzcmp/gzdiff + gznew shell scripts use temporary files
 unsafely
Date: Thu, 03 Oct 2013 21:14:23 -0700

The zdiff usage of set -C is executed only on older
platforms that lack mktemp, so it shouldn't be a problem.

znew.  What a dinosaur.  It's hardly worth fixing, but I
installed this:

From b3b5611e046b93fb20aa783d6d11d986f33f91f6 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Thu, 3 Oct 2013 21:12:09 -0700
Subject: [PATCH] znew: avoid denial-of-service issue

Reported by Rich Burridge in <http://bugs.gnu.org/15522>.
* znew.in: Rewrite to avoid the need for a temporary file in /tmp.
That way, we avoid the need for set -C
and worrying about denial of service.
Use touch -r and chmod --reference rather than cpmod.
Assume cp -p works, as it's now universal.
Quote 'echo' args better, while we're at it.
(warn, tmp, cpmod, cpmodarg): Remove.
(GZIP): Unset, so that we needn't test for gzip extension.
(ext): Now always '.gz'.
* znew.1: Document the change of implementation assumptions.
---
 znew.1  | 19 +++++++++++++------
 znew.in | 51 ++++++++++++++-------------------------------------
 2 files changed, 27 insertions(+), 43 deletions(-)

diff --git a/znew.1 b/znew.1
index dcdf84f..2a7e5e1 100644
--- a/znew.1
+++ b/znew.1
@@ -32,9 +32,16 @@ Keep a .Z file when it is smaller than the .gz file; implies
 .SH "SEE ALSO"
 gzip(1), zmore(1), zdiff(1), zgrep(1), zforce(1), gzexe(1), compress(1)
 .SH BUGS
-.I Znew
-does not maintain the time stamp with the -P option if
-.I cpmod(1)
-is not available and
-.I touch(1)
-does not support the -r option.
+If the
+.B \-P
+option is used,
+.I znew
+does not maintain the time stamp if
+.IR touch (1)
+does not support the
+.B \-r
+option, and does not maintain permissions if
+.IR chmod (1)
+does not support the
+.B \-\-reference
+option.
diff --git a/znew.in b/znew.in
index 9bd3ce9..d16311a 100644
--- a/znew.in
+++ b/znew.in
@@ -58,33 +58,9 @@ new=0
 block=1024
 # block is the disk block size (best guess, need not be exact)
 
-warn="(does not preserve modes and timestamp)"
-tmp=${TMPDIR-/tmp}/zfoo.$$
-set -C
-echo hi > $tmp || exit
-if test -z "`(${CPMOD-cpmod} $tmp $tmp) 2>&1`"; then
-  cpmod=${CPMOD-cpmod}
-  warn=""
-fi
-
-if test -z "$cpmod" && ${TOUCH-touch} -r $tmp $tmp 2>/dev/null; then
-  cpmod="${TOUCH-touch}"
-  cpmodarg="-r"
-  warn="(does not preserve file modes)"
-fi
-
-# check if GZIP env. variable uses -S or --suffix
-gzip -q $tmp
-ext=`echo $tmp* | sed "s|$tmp||"`
-rm -f $tmp*
-if test -z "$ext"; then
-  echo znew: error determining gzip extension
-  exit 1
-fi
-if test "$ext" = ".Z"; then
-  echo znew: cannot use .Z as gzip extension.
-  exit 1
-fi
+# Beware -s or --suffix in $GZIP.
+unset GZIP
+ext=.gz
 
 for arg
 do
@@ -116,26 +92,27 @@ if test -n "$opt"; then
 fi
 
 for i do
-  n=`echo $i | sed 's/.Z$//'`
+  n=`echo "$i" | sed 's/.Z$//'`
   if test ! -f "$n.Z" ; then
-    echo $n.Z not found
+    echo "$n.Z not found"
     res=1; continue
   fi
   test $keep -eq 1 && old=`wc -c < "$n.Z"`
   if test $pipe -eq 1; then
     if gzip -d < "$n.Z" | gzip $opt > "$n$ext"; then
       # Copy file attributes from old file to new one, if possible.
-      test -n "$cpmod" && $cpmod $cpmodarg "$n.Z" "$n$ext" 2> /dev/null
+      touch -r"$n.Z" -- "$n$ext" 2>/dev/null
+      chmod --reference="$n.Z" -- "$n$ext" 2>/dev/null
     else
-      echo error while recompressing $n.Z
+      echo "error while recompressing $n.Z"
       res=1; continue
     fi
   else
     if test $check -eq 1; then
-      if cp -p "$n.Z" "$n.$$" 2> /dev/null || cp "$n.Z" "$n.$$"; then
+      if cp -p "$n.Z" "$n.$$"; then
         :
       else
-        echo cannot backup "$n.Z"
+        echo "cannot backup $n.Z"
         res=1; continue
       fi
     fi
@@ -143,7 +120,7 @@ for i do
       :
     else
       test $check -eq 1 && mv "$n.$$" "$n.Z"
-      echo error while uncompressing $n.Z
+      echo "error while uncompressing $n.Z"
       res=1; continue
     fi
     if gzip $opt "$n"; then
@@ -151,10 +128,10 @@ for i do
     else
       if test $check -eq 1; then
         mv "$n.$$" "$n.Z" && rm -f "$n"
-        echo error while recompressing $n
+        echo "error while recompressing $n"
       else
         # compress $n  (might be dangerous if disk full)
-        echo error while recompressing $n, left uncompressed
+        echo "error while recompressing $n, left uncompressed"
       fi
       res=1; continue
     fi
@@ -175,7 +152,7 @@ for i do
     else
       test $pipe -eq 0 && mv "$n.$$" "$n.Z"
       rm -f "$n$ext"
-      echo error while testing $n$ext, $n.Z unchanged
+      echo "error while testing $n$ext, $n.Z unchanged"
       res=1; continue
     fi
   elif test $pipe -eq 1; then
-- 
1.8.3.1

Message #19 received at 15522-done <at> debbugs.gnu.org (full text, mbox):

From: Rich Burridge <rich.burridge <at> oracle.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 15522-done <at> debbugs.gnu.org
Subject: Re: bug#15522: gzcmp/gzdiff + gznew shell scripts use temporary files
 unsafely
Date: Thu, 03 Oct 2013 21:16:40 -0700

On 10/03/2013 09:14 PM, Paul Eggert wrote:
> The zdiff usage of set -C is executed only on older
> platforms that lack mktemp, so it shouldn't be a problem.

Okay.

>
> znew.  What a dinosaur.  It's hardly worth fixing, but I
> installed this:

Excellent. We'll use a similar patch against the version we current have.

Thanks.

Message #22 received at 15522 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Rich Burridge <rich.burridge <at> oracle.com>
Cc: 15522 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#15522: gzcmp/gzdiff + gznew shell scripts use temporary files
 unsafely
Date: Thu, 3 Oct 2013 21:25:06 -0700

On Thu, Oct 3, 2013 at 7:37 PM, Rich Burridge <rich.burridge <at> oracle.com> wrote:
...
> Sorry, I probably confused things by giving their Solaris g<name> names,
> and by stating that gzcmp and gzdiff were hard-linked without actually
> checking
> (because that's no longer true in the latest versions of the gzip
> distribution).

tags 15522 notabug
close 15522
thanks

Thanks for the report.
Since that problem was fixed long ago, I'm marking this ticket as "done".
If I've misunderstood, please let us know and I'll reopen it.

Message #25 received at 15522 <at> debbugs.gnu.org (full text, mbox):

From: Rich Burridge <rich.burridge <at> oracle.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 15522 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#15522: gzcmp/gzdiff + gznew shell scripts use temporary files
 unsafely
Date: Thu, 03 Oct 2013 21:27:36 -0700

On 10/03/2013 09:25 PM, Jim Meyering wrote:
> On Thu, Oct 3, 2013 at 7:37 PM, Rich Burridge <rich.burridge <at> oracle.com> wrote:
> ...
>> Sorry, I probably confused things by giving their Solaris g<name> names,
>> and by stating that gzcmp and gzdiff were hard-linked without actually
>> checking
>> (because that's no longer true in the latest versions of the gzip
>> distribution).
> tags 15522 notabug
> close 15522
> thanks
>
> Thanks for the report.
> Since that problem was fixed long ago, I'm marking this ticket as "done".
> If I've misunderstood, please let us know and I'll reopen it.

Well I guess I'm confused now. If Paul has just generated a patch to
change things, how can you consider this to not be a bug?

Message #28 received at 15522 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Rich Burridge <rich.burridge <at> oracle.com>
Cc: 15522 <15522 <at> debbugs.gnu.org>, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#15522: gzcmp/gzdiff + gznew shell scripts use temporary files
 unsafely
Date: Fri, 4 Oct 2013 06:58:16 -0700

Sorry.  The notabug tag is inaccurate.  I'll remove it.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.