Zsh Mailing List Archive
Messages sorted by: Reverse Date, Date, Thread, Author

[PATCH] Re: Insecure tempfile creation

On Dec 27, 10:30pm, Bart Schaefer wrote:
}
} } Of course, once we decide what to do with edit-command-line, it would be
} } good to go through all the other instances and fix them too.
} 
} Question related to this:  Is there some reason the following does NOT
} work?  Does zftp fail in the subshell?

I found at least one other place where $(zftp ...) was being used, so I
presume this is OK.  However, somebody (mystical chanting) should check
my work in Functions/Zftp/.

In the patch below, cases where the use of the temporary file was easily
localized or the name didn't matter, I used anonymous functions with an
argument created by =(:).  Where the name seemed to matter or the usage
was scattered, I applied a trick that occurred to me when looking at
Functions/Calendar/calendar, to wit:

	mv -f =(:) tmpfilename

The file is created safely and with mode 0600 by =(:) and then renamed
to the desired name, which will replace any symlink or other file that
has been left in in the way.  I then made sure that redirects either
used ">>" (knowing the file is empty) or ">|" to avoid issues with
noclobber.  I think a few files in Zftp may need "emulate zsh" but I
did not attempt to fix that.

I suppose =(<<<'') would actually be better, since it won't fork.  Hm.

In compinstall I noticed that a second tempfile was created by adding
the digit "2" to a file name already ending in a PID.  Since this could
collide with the PID of a different shell, I changed it to append "x".

This patch does not yet tackle uses of "/tmp" that do not use $TMPPREFIX,
nor did I attempt to retrofit zsh-newuser-install, which uses a common
prefix name built on $TMPPREFIX for several different files.

diff --git a/Completion/Base/Widget/_complete_debug b/Completion/Base/Widget/_complete_debug
index 6044863..00f600e 100644
--- a/Completion/Base/Widget/_complete_debug
+++ b/Completion/Base/Widget/_complete_debug
@@ -9,6 +9,7 @@ local pager w="${(qq)words}"
 integer debug_fd=-1
 {
   if [[ -t 2 ]]; then
+    mv -f =(:) $tmp &&
     exec {debug_fd}>&2 2>| $tmp
   fi
 
diff --git a/Completion/compinstall b/Completion/compinstall
index c04543c..7d34ee4 100644
--- a/Completion/compinstall
+++ b/Completion/compinstall
@@ -1958,11 +1958,14 @@ if [[ -z $ifile || -d $ifile ]] ||
 fi
 
 local tmpout=${TMPPREFIX:-/tmp/zsh}compinstall$$
+mv -f =(:) $tmpout &&	# safe tempfile creation
+mv -f =(:) ${tmpout}x || return 1
+
 #
 # Assemble the complete set of lines to
 # insert.
 #
-__ci_output >$tmpout
+__ci_output >>$tmpout
 
 if [[ -n $ifile ]]; then
   if [[ $ifile != *(zshrc|zlogin|zshenv) ]]; then 
@@ -1984,15 +1987,15 @@ future use."
   fi
   if { { grep "$endline" $ifile >/dev/null 2>&1 &&
          sed -e "/^[ 	]*$endline/r $tmpout
-/^[ 	]*$startline/,/^[ 	]*$endline/d" $ifile >${tmpout}2 } || 
-        { cp $ifile ${tmpout}2 && cat $tmpout >>${tmpout}2 } } &&
-  cp ${tmpout}2 $ifile && rm -f ${tmpout}2; then
+/^[ 	]*$startline/,/^[ 	]*$endline/d" $ifile >>${tmpout}x } || 
+        { cp $ifile ${tmpout}x && cat $tmpout >>${tmpout}x } } &&
+  cp ${tmpout}x $ifile && rm -f ${tmpout}x; then
     print "\nSuccessfully added compinstall lines to $ifile."
     rm -f $tmpout
   else
     print "\nFailure adding lines to $ifile.  Lines left in \`$tmpout'"
   fi
-  rm -f ${tmpout}2
+  rm -f ${tmpout}x
 elif read -q key'?Print them to stdout instead ([y]es, [n]o)? '; then
   cat $tmpout
   rm -f $tmpout
diff --git a/Functions/Calendar/calendar b/Functions/Calendar/calendar
index 00f5998..08c4250 100644
--- a/Functions/Calendar/calendar
+++ b/Functions/Calendar/calendar
@@ -254,8 +254,7 @@ if (( verbose )); then
 fi
 
 local mycmds="${TMPPREFIX:-/tmp/zsh}.calendar_cmds.$$"
-touch $mycmds
-chmod 600 $mycmds
+mv -f =(:) $mycmds
 
 # start of subshell for OS file locking
 (
diff --git a/Functions/Zftp/zfcd_match b/Functions/Zftp/zfcd_match
index 95de4c5..2c809c2 100644
--- a/Functions/Zftp/zfcd_match
+++ b/Functions/Zftp/zfcd_match
@@ -12,7 +12,6 @@ fi
 local ZFTP_VERBOSE=45
 # should we redirect 2>/dev/null or let the user see it?
 
-local tmpf=${TMPPREFIX}zfcm$$
 local -a match mbegin mend
 
 if [[ $ZFTP_SYSTEM = UNIX* ]]; then
@@ -27,9 +26,10 @@ if [[ $ZFTP_SYSTEM = UNIX* ]]; then
   # If we're using -F, we get away with using a directory
   # to list, but not a glob.  Don't ask me why.
   reply=(${${(M)${(f)"$(zftp ls -lF $dir)"}:#d*}/(#b)*[[:space:]](*)\//$match[1]})
-#  zftp ls -LF $dir >$tmpf
-#  reply=($(awk '/\/$/ { print substr($1, 1, length($1)-1) }' $tmpf))
-#  rm -f $tmpf
+#  () {
+#    zftp ls -LF $dir >|$1
+#    reply=($(awk '/\/$/ { print substr($1, 1, length($1)-1) }' $1))
+#  } =(:)
   [[ -n $dir && $dir != */ ]] && dir="$dir/"
   if [[ -n $WIDGET ]]; then
     _wanted directories expl 'remote directory' \
diff --git a/Functions/Zftp/zfcget b/Functions/Zftp/zfcget
index 476a730..4359801 100644
--- a/Functions/Zftp/zfcget
+++ b/Functions/Zftp/zfcget
@@ -14,7 +14,7 @@ emulate -L zsh
 
 [[ $curcontext = :zf* ]] || local curcontext=:zfcget
 local loc rem stat=0 opt opt_G opt_t remlist locst remst
-local tmpfile=${TMPPREFIX}zfcget$$ rstat tsize
+local rstat tsize
 
 while getopts :Gt opt; do
   [[ $opt = '?' ]] && print "zfcget: bad option: -$OPTARG" && return 1
@@ -39,10 +39,11 @@ for remlist in $*; do
       else
 	# Compare the sizes.
 	locst=($(zftp local $loc))
-	zftp remote $rem >$tmpfile
-	rstat=$?
-	remst=($(<$tmpfile))
-	rm -f $tmpfile
+	() {
+	  zftp remote $rem >|$1
+	  rstat=$?
+	  remst=($(<$1))
+	} =(: temporary file)
 	if [[ $rstat = 2 ]]; then
 	  print "Server does not support SIZE command.\n" \
 	  "Assuming you know what you're doing..." 2>&1
diff --git a/Functions/Zftp/zfcput b/Functions/Zftp/zfcput
index 85141b6..2cf8fe2 100644
--- a/Functions/Zftp/zfcput
+++ b/Functions/Zftp/zfcput
@@ -14,7 +14,6 @@ emulate -L zsh
 
 [[ $curcontext = :zf* ]] || local curcontext=:zfcput
 local loc rem stat=0 locst remst offs tailtype
-local tmpfile=${TMPPREFIX}zfcget$$ rstat
 
 # find how tail works.  this is intensely annoying, since it's completely
 # standard in C.  od's no use, since we can only skip whole blocks.
@@ -40,10 +39,11 @@ for loc in $*; do
   else
     # Compare the sizes.
     locst=($(zftp local $loc))
-    zftp remote $rem >$tmpfile
-    rstat=$?
-    remst=($(<$tmpfile))
-    rm -f $tmpfile
+    () {
+      zftp remote $rem >|$1
+      rstat=$?
+      remst=($(<$1))
+    } =(: temporary file)
     if [[ $rstat = 2 ]]; then
       print "Server does not support remote status commands.\n" \
       "You will have to find out the size by hand and use zftp append." 2>&1
diff --git a/Functions/Zftp/zffcache b/Functions/Zftp/zffcache
index 48afdcb..b609c21 100644
--- a/Functions/Zftp/zffcache
+++ b/Functions/Zftp/zffcache
@@ -19,8 +19,5 @@ fi
 if [[ $1 = -d ]]; then
   unset $fcache_name
 elif (( ${(P)#fcache_name} == 0 )); then
-  local tmpf=${TMPPREFIX}zffcache$$
-  zftp ls >$tmpf
-  eval "$fcache_name=(\${(f)\"\$(<\$tmpf)\"})"
-  rm -f $tmpf
+  eval "$fcache_name=(\${(f)\"\$(zftp ls)\"})"
 fi
diff --git a/Functions/Zftp/zfget_match b/Functions/Zftp/zfget_match
index 1d90bea..c2871fa 100644
--- a/Functions/Zftp/zfget_match
+++ b/Functions/Zftp/zfget_match
@@ -7,22 +7,22 @@ if [[ $1 == $HOME || $1 == $HOME/* ]]; then
   1="~${1#$HOME}"
 fi
 
-local tmpf=${TMPPREFIX}zfgm$$
-
 if [[ $ZFTP_SYSTEM == UNIX* && $1 == */* ]]; then
+  setopt localoptions clobber
+  local tmpf=${TMPPREFIX}zfgm$$
+  mv -f =(:) $tmpf
+	
   if [[ -n $WIDGET ]]; then
     local dir=${1:h}
     [[ $dir = */ ]] || dir="$dir/"
     zftp ls -LF $dir >$tmpf
     local reply
     reply=(${${${(f)"$(<$tmpf)"}##$dir}%\*})
-    rm -f $tmpf
     _wanted files expl 'remote file' compadd -P $dir - $reply
   else
     # On the first argument to ls, we usually get away with a glob.
     zftp ls "$1*$2" >$tmpf
     reply=($(<$tmpf))
-    rm -f $tmpf
   fi
 else
   local fcache_name
diff --git a/Functions/Zftp/zfrglob b/Functions/Zftp/zfrglob
index 1fb8d76..5015be7 100644
--- a/Functions/Zftp/zfrglob
+++ b/Functions/Zftp/zfrglob
@@ -33,12 +33,12 @@ if [[ $pat != *[][*?]* &&
   ( -n $zfrglob || $pat != *[(|)#^]* ) ]]; then
   return 0
 fi
-local tmpf=${TMPPREFIX}zfrglob$$
 
 if [[ $zfrglob != '' ]]; then
-  zftp ls "$pat" >$tmpf 2>/dev/null
-  eval "$1=(\$(<\$tmpf))"
-  rm -f $tmpf
+  () {
+    zftp ls "$pat" >|$1 2>/dev/null
+    eval "$1=(\$(<\$1))"
+  } =(: temporary file)
 else
   if [[ $ZFTP_SYSTEM = UNIX* && $pat = */* ]]; then
     # not the current directory and we know how to handle paths
@@ -49,10 +49,11 @@ else
       dir=/
     fi
     nondir=${pat##*/}
-    zftp ls "$dir" 2>/dev/null >$tmpf
-    files=($(<$tmpf))
+    () {
+      zftp ls "$dir" 2>/dev/null >|$1
+      files=($(<$1))
+    } =(: temporary file)
     files=(${files:t})
-    rm -f $tmpf
   else
     # we just have to do an ls and hope that's right
     local fcache_name
diff --git a/Functions/Zftp/zftransfer b/Functions/Zftp/zftransfer
index c70bf72..432e2f5 100644
--- a/Functions/Zftp/zftransfer
+++ b/Functions/Zftp/zftransfer
@@ -43,10 +43,11 @@ zfautocheck || return 1
 local style
 zstyle -s ':zftp:zftransfer' progress style
 if [[ -n $style && $style != none ]]; then
-  local ZFTP_TSIZE array tmpfile=${TMPPREFIX}zft$$
-  zftp remote $file1 >$tmpfile 2>/dev/null
-  array=($(<$tmpfile))
-  rm -f $tmpfile
+  local ZFTP_TSIZE array
+  () {
+    zftp remote $file1 >|$1 2>/dev/null
+    array=($(<$1))
+  } =(: temporary file)
   [[ $#array -eq 2 ]] && ZFTP_TSIZE=$array[1]
 fi
 
diff --git a/Functions/Zftp/zftype b/Functions/Zftp/zftype
index 0cdf7e2..81f95de 100644
--- a/Functions/Zftp/zftype
+++ b/Functions/Zftp/zftype
@@ -1,13 +1,11 @@
 # function zftype {
-local type zftmp=${TMPPREFIX}zftype$$
+local type
 [[ $curcontext = :zf* ]] || local curcontext=:zftype
 
 zfautocheck -d
 
 if (( $# == 0 )); then
-  zftp type >$zftmp
-  type=$(<$zftmp)
-  rm -f $zftmp
+  type=$(zftp type)
   if [[ $type = I ]]; then
     print "Current type is image (binary)"
     return 0
diff --git a/Functions/Zftp/zfuget b/Functions/Zftp/zfuget
index c1033c9..7bdaedc 100644
--- a/Functions/Zftp/zfuget
+++ b/Functions/Zftp/zfuget
@@ -26,7 +26,7 @@
 emulate -L zsh
 
 [[ $curcontext = :zf* ]] || local curcontext=:zfuget
-local loc rem locstats remstats doit tmpfile=${TMPPREFIX}zfuget$$
+local loc rem locstats remstats doit
 local rstat remlist opt opt_v opt_s opt_G opt_t
 integer stat do_close
 
@@ -66,12 +66,13 @@ for remlist in $*; do
       doit=y
       remstats=()
       if [[ -f $loc ]]; then
-	zftp local $loc >$tmpfile
-	locstats=($(<$tmpfile))
-	zftp remote $rem >$tmpfile
-	rstat=$?
-	remstats=($(<$tmpfile))
-	rm -f $tmpfile
+	() {
+	  zftp local $loc >|$1
+	  locstats=($(<$1))
+	  zftp remote $rem >|$1
+	  rstat=$?
+	  remstats=($(<$1))
+	} =(: temporary file)
 	if [[ $rstat = 2 ]]; then
 	  print "Server does not implement full command set required." 1>&2
 	  return 1
diff --git a/Functions/Zftp/zfuput b/Functions/Zftp/zfuput
index 4e0e42d..24a3559 100644
--- a/Functions/Zftp/zfuput
+++ b/Functions/Zftp/zfuput
@@ -12,7 +12,7 @@
 emulate -L zsh
 
 [[ $curcontext = :zf* ]] || local curcontext=:zfuput
-local loc rem locstats remstats doit tmpfile=${TMPPREFIX}zfuput$$
+local loc rem locstats remstats doit
 local rstat opt opt_v opt_s
 integer stat do_close
 
@@ -52,12 +52,13 @@ for rem in $*; do
     stat=1
     continue
   fi
-  zftp local $loc >$tmpfile
-  locstats=($(<$tmpfile))
-  zftp remote $rem >$tmpfile
-  rstat=$?
-  remstats=($(<$tmpfile))
-  rm -f $tmpfile
+  () {
+    zftp local $loc >|$1
+    locstats=($(<$1))
+    zftp remote $rem >|$1
+    rstat=$?
+    remstats=($(<$1))
+  } =(: temporary file)
   if [[ $rstat = 2 ]]; then
     print "Server does not implement full command set required." 1>&2
     return 1
diff --git a/Functions/Zle/edit-command-line b/Functions/Zle/edit-command-line
index 250cac6..100af96 100644
--- a/Functions/Zle/edit-command-line
+++ b/Functions/Zle/edit-command-line
@@ -6,12 +6,10 @@
 # will give ksh-like behaviour for that key,
 # except that it will handle multi-line buffers properly.
 
-local tmpfile=${TMPPREFIX:-/tmp/zsh}ecl$$
+() {
+  exec </dev/tty
+  ${=${VISUAL:-${EDITOR:-vi}}} $1
+  print -Rz - "$(<$1)" 
+} =(<<<"$PREBUFFER$BUFFER")
 
-print -R - "$PREBUFFER$BUFFER" >$tmpfile
-exec </dev/tty
-${=${VISUAL:-${EDITOR:-vi}}} $tmpfile
-print -Rz - "$(<$tmpfile)" 
-
-command rm -f $tmpfile
 zle send-break		# Force reload from the buffer stack

-- 
Barton E. Schaefer
Messages sorted by: Reverse Date, Date, Thread, Author