Insecure tempfile creation

Insecure tempfile creation

[Daniel Shahaf]

[moving to -workers from private email]
[this mail contains two mutually-exclusive (conflicting) patches]

Hello.

A few places in the source distribution use predictable temporary
filenames; for example:

 Completion/Unix/Command/_cvs           local d=/tmp/zsh-cvs-work-$$
 Completion/compinstall:                local tmpout=${TMPPREFIX:-/tmp/zsh}compinstall$$
 Functions/Calendar/calendar:           local mycmds="${TMPPREFIX:-/tmp/zsh}.calendar_cmds.$$"
 Functions/Newuser/zsh-newuser-install: local tmpfile=${TMPPREFIX:-/tmp/zsh}-zni-$$
 Functions/Zftp/zfcget:                 local tmpfile=${TMPPREFIX}zfcget$$ rstat tsize
 Functions/Zle/edit-command-line        local tmpfile=${TMPPREFIX:-/tmp/zsh}ecl$$
 Test/ztst.zsh:                         ZTST_in=${TMPPREFIX}.ztst.in.$$

Some of these could be vectors for symlink attacks.  For example, in the
edit-command-line case, a malicious local user could overwrite an
arbitrary file by creating /tmp/zshecl4242 (where 4242 is the pid of an
interactive zsh run by root) as a symlink and waiting for the user
behind pid 4242 to run edit-command-line.  (The attacker would also be
able to see the contents of the edited command line, which is a problem
if it contains passwords.)

(Paraphrasing Bart:) In general, the "standard library" should create
tempfiles in ${TMPPREFIX:-/tmp}, and take care to explicitly protect
(e.g., via umask settings) any files which need to be private.

So, for starters:

diff --git Functions/Zle/edit-command-line Functions/Zle/edit-command-line
index 250cac6..1b1762d 100644
--- Functions/Zle/edit-command-line
+++ Functions/Zle/edit-command-line
@@ -6,12 +6,16 @@
 # will give ksh-like behaviour for that key,
 # except that it will handle multi-line buffers properly.
 
-local tmpfile=${TMPPREFIX:-/tmp/zsh}ecl$$
-
-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
+() {
+  # Use =(:) to create a temporary file with 0600 permissions, since
+  # the command-line may contain passwords.
+  local tmpfile=$1
+  
+  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
+} =(:)

This causes the tempfile to be generated by gettempname(), which wraps
mktemp(), eliminating the predictable filename.

Do people find this use of anon functions too obfuscatory?  If so,
perhaps a "mktemp" builtin could be added as a thin wrapper around
gettempname() (on systems that don't have a mktemp command already), and
the code be converted to just:

tarsus,5:src/zsh% git di
diff --git Functions/Zle/edit-command-line Functions/Zle/edit-command-line
index 250cac6..584ae16 100644
--- Functions/Zle/edit-command-line
+++ Functions/Zle/edit-command-line
@@ -6,7 +6,7 @@
 # will give ksh-like behaviour for that key,
 # except that it will handle multi-line buffers properly.
 
-local tmpfile=${TMPPREFIX:-/tmp/zsh}ecl$$
+local tmpfile=`mktemp ${TMPPREFIX:-/tmp/zsh}ecl.XXXXXX`
 
 print -R - "$PREBUFFER$BUFFER" >$tmpfile
 exec </dev/tty

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.

Cheers,

Daniel

Re: Insecure tempfile creation

[Mikael Magnusson]

On Mon, Dec 22, 2014 at 9:36 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> [moving to -workers from private email]
> [this mail contains two mutually-exclusive (conflicting) patches]
>
> Hello.
>
> A few places in the source distribution use predictable temporary
> filenames; for example:
>
>  Completion/Unix/Command/_cvs           local d=/tmp/zsh-cvs-work-$$
>  Completion/compinstall:                local tmpout=${TMPPREFIX:-/tmp/zsh}compinstall$$
>  Functions/Calendar/calendar:           local mycmds="${TMPPREFIX:-/tmp/zsh}.calendar_cmds.$$"
>  Functions/Newuser/zsh-newuser-install: local tmpfile=${TMPPREFIX:-/tmp/zsh}-zni-$$
>  Functions/Zftp/zfcget:                 local tmpfile=${TMPPREFIX}zfcget$$ rstat tsize
>  Functions/Zle/edit-command-line        local tmpfile=${TMPPREFIX:-/tmp/zsh}ecl$$
>  Test/ztst.zsh:                         ZTST_in=${TMPPREFIX}.ztst.in.$$
>
> Some of these could be vectors for symlink attacks.  For example, in the
> edit-command-line case, a malicious local user could overwrite an
> arbitrary file by creating /tmp/zshecl4242 (where 4242 is the pid of an
> interactive zsh run by root) as a symlink and waiting for the user
> behind pid 4242 to run edit-command-line.  (The attacker would also be
> able to see the contents of the edited command line, which is a problem
> if it contains passwords.)
>
> (Paraphrasing Bart:) In general, the "standard library" should create
> tempfiles in ${TMPPREFIX:-/tmp}, and take care to explicitly protect
> (e.g., via umask settings) any files which need to be private.
>
> So, for starters:
>
> diff --git Functions/Zle/edit-command-line Functions/Zle/edit-command-line
> index 250cac6..1b1762d 100644
> --- Functions/Zle/edit-command-line
> +++ Functions/Zle/edit-command-line
> @@ -6,12 +6,16 @@
>  # will give ksh-like behaviour for that key,
>  # except that it will handle multi-line buffers properly.
>
> -local tmpfile=${TMPPREFIX:-/tmp/zsh}ecl$$
> -
> -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
> +() {
> +  # Use =(:) to create a temporary file with 0600 permissions, since
> +  # the command-line may contain passwords.
> +  local tmpfile=$1
> +
> +  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
> +} =(:)
>
> This causes the tempfile to be generated by gettempname(), which wraps
> mktemp(), eliminating the predictable filename.
>
> Do people find this use of anon functions too obfuscatory?  If so,
> perhaps a "mktemp" builtin could be added as a thin wrapper around
> gettempname() (on systems that don't have a mktemp command already), and
> the code be converted to just:

Your first patch is not the best version of that solution;

--- a/Functions/Zle/edit-command-line
+++ b/Functions/Zle/edit-command-line
@@ -6,12 +6,7 @@
 # will give ksh-like behaviour for that key,
 # except that it will handle multi-line buffers properly.

-local tmpfile=${TMPPREFIX:-/tmp/zsh}ecl$$
-
-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
+() {
+  ${=${VISUAL:-${EDITOR:-vi}}} $1
+  BUFFER="$(<$1)"
+} =(print -R "$PREBUFFER$BUFFER")

I don't know what the exec </dev/tty is for, but it can be added back
if it's needed. I've used this patch for two years and never noticed
any problems though.

-- 
Mikael Magnusson

Re: Insecure tempfile creation

[Bart Schaefer]

On Dec 22, 11:01pm, Mikael Magnusson wrote:
}
} Your first patch is not the best version of that solution;
} 
} +() {
} +  ${=${VISUAL:-${EDITOR:-vi}}} $1
} +  BUFFER="$(<$1)"
} +} =(print -R "$PREBUFFER$BUFFER")

Even that isn't the best formulation, =(<<<"$PREBUFFER$BUFFER") would
avoid forking the print.

} I don't know what the exec </dev/tty is for, but it can be added back
} if it's needed. I've used this patch for two years and never noticed
} any problems though.

Widgets run with stdin closed, so if the chosen editor doesn't open the
tty or one of the other std*s directly, it breaks without </dev/tty.

Re: Insecure tempfile creation

[Bart Schaefer]

On Dec 22,  8:36pm, Daniel Shahaf 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?

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

[PATCH] Re: Insecure tempfile creation

[Bart Schaefer]

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

Re: [PATCH] Re: Insecure tempfile creation

[Bart Schaefer]

On Dec 27, 11:44pm, Bart Schaefer wrote:
}
} I suppose =(<<<'') would actually be better, since it won't fork.  Hm.
} 
} This patch does not yet tackle uses of "/tmp" that do not use $TMPPREFIX

Fortunately I didn't find any of the latter except for the previously
identified one in _cvs (_cvs_run).  So the patch below changes the use
of =(:) to =(<<<'') and repairs _cvs_run to create the temp directory 
in a safe (I hope) manner.  Apply on top of 34067.

diff --git a/Completion/Base/Widget/_complete_debug b/Completion/Base/Widget/_complete_debug
index 00f600e..50fc809 100644
--- a/Completion/Base/Widget/_complete_debug
+++ b/Completion/Base/Widget/_complete_debug
@@ -9,7 +9,7 @@ local pager w="${(qq)words}"
 integer debug_fd=-1
 {
   if [[ -t 2 ]]; then
-    mv -f =(:) $tmp &&
+    mv -f =(<<<'') $tmp &&
     exec {debug_fd}>&2 2>| $tmp
   fi
 
diff --git a/Completion/Unix/Command/_cvs b/Completion/Unix/Command/_cvs
index 3c06e04..31997ec 100644
--- a/Completion/Unix/Command/_cvs
+++ b/Completion/Unix/Command/_cvs
@@ -704,15 +704,18 @@ _cvs_sub_modules() {
 _cvs_run() {
   local cvsroot="$1" dir="$2"
   shift 2
-  local d=/tmp/zsh-cvs-work-$$
-  mkdir $d >&/dev/null
-  cd $d
-  mkdir CVS >&/dev/null
+  local d=${TMPPREFIX:-/tmp/zsh}-cvs-work-$$
+  rm -rf $d
+  mkdir $d &&
+  (
+  chmod 0700 $d &&
+  builtin cd -q $d &&
+  mkdir CVS >&/dev/null || return 1
   print -r - "$cvsroot" > CVS/Root
   print "$dir" > CVS/Repository
   print D > CVS/Entries
   CVS_IGNORE_REMOTE_ROOT= cvs "$@"
-  cd $OLDPWD
+  )
   rm -rf $d
 }
 
diff --git a/Completion/compinstall b/Completion/compinstall
index 7d34ee4..ae94993 100644
--- a/Completion/compinstall
+++ b/Completion/compinstall
@@ -1958,8 +1958,8 @@ if [[ -z $ifile || -d $ifile ]] ||
 fi
 
 local tmpout=${TMPPREFIX:-/tmp/zsh}compinstall$$
-mv -f =(:) $tmpout &&	# safe tempfile creation
-mv -f =(:) ${tmpout}x || return 1
+mv -f =(<<<'') $tmpout &&	# safe tempfile creation
+mv -f =(<<<'') ${tmpout}x || return 1
 
 #
 # Assemble the complete set of lines to
diff --git a/Functions/Calendar/calendar b/Functions/Calendar/calendar
index 08c4250..39fc431 100644
--- a/Functions/Calendar/calendar
+++ b/Functions/Calendar/calendar
@@ -254,7 +254,7 @@ if (( verbose )); then
 fi
 
 local mycmds="${TMPPREFIX:-/tmp/zsh}.calendar_cmds.$$"
-mv -f =(:) $mycmds
+mv -f =(<<<'') $mycmds
 
 # start of subshell for OS file locking
 (
diff --git a/Functions/Zftp/zfcd_match b/Functions/Zftp/zfcd_match
index 2c809c2..9159f49 100644
--- a/Functions/Zftp/zfcd_match
+++ b/Functions/Zftp/zfcd_match
@@ -29,7 +29,7 @@ if [[ $ZFTP_SYSTEM = UNIX* ]]; then
 #  () {
 #    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 4359801..569ee9d 100644
--- a/Functions/Zftp/zfcget
+++ b/Functions/Zftp/zfcget
@@ -43,7 +43,7 @@ for remlist in $*; do
 	  zftp remote $rem >|$1
 	  rstat=$?
 	  remst=($(<$1))
-	} =(: temporary file)
+	} =(<<<'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 2cf8fe2..eafecde 100644
--- a/Functions/Zftp/zfcput
+++ b/Functions/Zftp/zfcput
@@ -43,7 +43,7 @@ for loc in $*; do
       zftp remote $rem >|$1
       rstat=$?
       remst=($(<$1))
-    } =(: temporary file)
+    } =(<<<'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/zfget_match b/Functions/Zftp/zfget_match
index c2871fa..3ba06c4 100644
--- a/Functions/Zftp/zfget_match
+++ b/Functions/Zftp/zfget_match
@@ -10,7 +10,7 @@ fi
 if [[ $ZFTP_SYSTEM == UNIX* && $1 == */* ]]; then
   setopt localoptions clobber
   local tmpf=${TMPPREFIX}zfgm$$
-  mv -f =(:) $tmpf
+  mv -f =(<<<'') $tmpf
 	
   if [[ -n $WIDGET ]]; then
     local dir=${1:h}
diff --git a/Functions/Zftp/zfrglob b/Functions/Zftp/zfrglob
index 5015be7..677b85f 100644
--- a/Functions/Zftp/zfrglob
+++ b/Functions/Zftp/zfrglob
@@ -38,7 +38,7 @@ if [[ $zfrglob != '' ]]; then
   () {
     zftp ls "$pat" >|$1 2>/dev/null
     eval "$1=(\$(<\$1))"
-  } =(: temporary file)
+  } =(<<<'temporary file')
 else
   if [[ $ZFTP_SYSTEM = UNIX* && $pat = */* ]]; then
     # not the current directory and we know how to handle paths
@@ -52,7 +52,7 @@ else
     () {
       zftp ls "$dir" 2>/dev/null >|$1
       files=($(<$1))
-    } =(: temporary file)
+    } =(<<<'temporary file')
     files=(${files:t})
   else
     # we just have to do an ls and hope that's right
diff --git a/Functions/Zftp/zftransfer b/Functions/Zftp/zftransfer
index 432e2f5..c97ae46 100644
--- a/Functions/Zftp/zftransfer
+++ b/Functions/Zftp/zftransfer
@@ -47,7 +47,7 @@ if [[ -n $style && $style != none ]]; then
   () {
     zftp remote $file1 >|$1 2>/dev/null
     array=($(<$1))
-  } =(: temporary file)
+  } =(<<<'temporary file')
   [[ $#array -eq 2 ]] && ZFTP_TSIZE=$array[1]
 fi
 
diff --git a/Functions/Zftp/zfuget b/Functions/Zftp/zfuget
index 7bdaedc..2850975 100644
--- a/Functions/Zftp/zfuget
+++ b/Functions/Zftp/zfuget
@@ -72,7 +72,7 @@ for remlist in $*; do
 	  zftp remote $rem >|$1
 	  rstat=$?
 	  remstats=($(<$1))
-	} =(: temporary file)
+	} =(<<<'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 24a3559..f4e6a0f 100644
--- a/Functions/Zftp/zfuput
+++ b/Functions/Zftp/zfuput
@@ -58,7 +58,7 @@ for rem in $*; do
     zftp remote $rem >|$1
     rstat=$?
     remstats=($(<$1))
-  } =(: temporary file)
+  } =(<<<'temporary file')
   if [[ $rstat = 2 ]]; then
     print "Server does not implement full command set required." 1>&2
     return 1

Re: [PATCH] Re: Insecure tempfile creation

[Daniel Shahaf]

Bart Schaefer wrote on Sun, Dec 28, 2014 at 00:41:01 -0800:
> On Dec 27, 11:44pm, Bart Schaefer wrote:
> }
> } I suppose =(<<<'') would actually be better, since it won't fork.  Hm.
> } 
> } This patch does not yet tackle uses of "/tmp" that do not use $TMPPREFIX
> 
> Fortunately I didn't find any of the latter except for the previously
> identified one in _cvs (_cvs_run).  So the patch below changes the use
> of =(:) to =(<<<'') and repairs _cvs_run to create the temp directory 
> in a safe (I hope) manner.  Apply on top of 34067.
> 

First of all, thanks for picking this up.  I'd meant to get back to this
thread early January, but I'm happy to have been beaten to it :-)

Your patches look good to me, including the rmdir, but except for:

> -	} =(: temporary file)
> +	} =(<<<'temporary file')

I assume =(<<<'') was the intention.

Thanks again,

Daniel

Re: [PATCH] Re: Insecure tempfile creation

[Bart Schaefer]

On Dec 29, 12:49am, Daniel Shahaf wrote:
}
} Your patches look good to me, including the rmdir

I avoided using "mkdir -m 0700" in favor of the chmod but then found some
other places where mkdir is passed the -m option.  So maybe that should
be tweaked.

} but except for:
} 
} > -	} =(: temporary file)
} > +	} =(<<<'temporary file')
} 
} I assume =(<<<'') was the intention.

I meant to say something about that but forgot.

The places where I left that immediately use >|$1 to clobber the file,
so it doesn't matter if the file starts out empty; I hoped it could be
a clue to the reader what was going on.

Re: [PATCH] Re: Insecure tempfile creation

[Daniel Shahaf]

Bart Schaefer wrote on Sun, Dec 28, 2014 at 20:01:42 -0800:
> On Dec 29, 12:49am, Daniel Shahaf wrote:
> }
> } Your patches look good to me, including the rmdir
> 
> I avoided using "mkdir -m 0700" in favor of the chmod but then found some
> other places where mkdir is passed the -m option.  So maybe that should
> be tweaked.

Coming back to this, it has occurred to me that

	mv -f =(:) ${TMPPREFIX:-/tmp/zsh}foo$$

wouldn't perform an atomic rename (as intended) if /tmp/zshfoo$$ is a
directory or symlink-to-directory.  So hypothetically an attacker might
be able to create a file named `basename =(:)` in a directory of his
choice owned by the victim.

I realize this is more far-fetched than the previous scenario.  Do we
consider this a problem that should be fixed?

Cheers,

Daniel

Re: [PATCH] Re: Insecure tempfile creation

[Bart Schaefer]

On Wed, Jan 7, 2015 at 2:03 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Coming back to this, it has occurred to me that
>
>         mv -f =(:) ${TMPPREFIX:-/tmp/zsh}foo$$
>
> wouldn't perform an atomic rename (as intended) if /tmp/zshfoo$$ is a
> directory or symlink-to-directory.  So hypothetically an attacker might
> be able to create a file named `basename =(:)` in a directory of his
> choice owned by the victim.

Hmm.  Yup, we need "ln -Fh" instead of "mv -f".  Are the -F and -h
options of "ln" fairly standard?

Re: [PATCH] Re: Insecure tempfile creation

[Danek Duvall]

On Wed, Jan 07, 2015 at 10:22:20PM -0800, Bart Schaefer wrote:

> On Wed, Jan 7, 2015 at 2:03 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Coming back to this, it has occurred to me that
> >
> >         mv -f =(:) ${TMPPREFIX:-/tmp/zsh}foo$$
> >
> > wouldn't perform an atomic rename (as intended) if /tmp/zshfoo$$ is a
> > directory or symlink-to-directory.  So hypothetically an attacker might
> > be able to create a file named `basename =(:)` in a directory of his
> > choice owned by the victim.
> 
> Hmm.  Yup, we need "ln -Fh" instead of "mv -f".  Are the -F and -h
> options of "ln" fairly standard?

Neither exists on Solaris ln.  GNU coreutils ln doesn't seem to have -h,
either.  And -F just seems like a bad idea, supported or not.

What about mktemp?  The above construction is pretty weird, anyway.  If an
external command isn't desired, then mktemp seems like a reasonable thing
to make builtin.

Danek

Re: [PATCH] Re: Insecure tempfile creation

[Bart Schaefer]

On Jan 7, 10:48pm, Danek Duvall wrote:
} Subject: Re: [PATCH] Re: Insecure tempfile creation
}
} On Wed, Jan 07, 2015 at 10:22:20PM -0800, Bart Schaefer wrote:
} 
} > On Wed, Jan 7, 2015 at 2:03 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
} > > Coming back to this, it has occurred to me that
} > >
} > >         mv -f =(:) ${TMPPREFIX:-/tmp/zsh}foo$$
} > >
} > 
} > Hmm.  Yup, we need "ln -Fh" instead of "mv -f".  Are the -F and -h
} > options of "ln" fairly standard?
} 
} Neither exists on Solaris ln.  GNU coreutils ln doesn't seem to have -h,
} either.  And -F just seems like a bad idea, supported or not.

-F on MacOS (where I was reading the manual) is like -f in coreutils,
not like -F in coreutils (sigh).  And -h is --no-dereference.  

} What about mktemp?

That doesn't help; it's exactly the same as =(:) for this purpose.  The
"mv" trick above is used where we need to create a file with a specific
name -- if we did not need a specific name, we could just use the name
created by =(:) directly.

Fortunately, we have the zsh/files module which provides a buitin "ln"
with well-defined semantics.  Hopefully that's good enough.

Re: [PATCH] Re: Insecure tempfile creation

[Daniel Shahaf]

Bart Schaefer wrote on Thu, Jan 08, 2015 at 00:08:21 -0800:
> On Jan 7, 10:48pm, Danek Duvall wrote:
> } Subject: Re: [PATCH] Re: Insecure tempfile creation
> }
> } On Wed, Jan 07, 2015 at 10:22:20PM -0800, Bart Schaefer wrote:
> } 
> } > On Wed, Jan 7, 2015 at 2:03 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> } > > Coming back to this, it has occurred to me that
> } > >
> } > >         mv -f =(:) ${TMPPREFIX:-/tmp/zsh}foo$$
> } > >
> } > 
> } > Hmm.  Yup, we need "ln -Fh" instead of "mv -f".  Are the -F and -h
> } > options of "ln" fairly standard?
> } 
> } Neither exists on Solaris ln.  GNU coreutils ln doesn't seem to have -h,
> } either.  And -F just seems like a bad idea, supported or not.
> 
> -F on MacOS (where I was reading the manual) is like -f in coreutils,
> not like -F in coreutils (sigh).  And -h is --no-dereference.  
> 

So, for clarity, the flags you proposed mean "overwrite destination if
existing" and "if destination is symlink, don't dereference it".

> Fortunately, we have the zsh/files module which provides a buitin "ln"
> with well-defined semantics.  Hopefully that's good enough.

Another option: add a builtin that wraps the rename(2) syscall, and
then use:

    zrename =(<<<'') ${TMPPREFIX:-/tmp/zsh}foo$$

Re: [PATCH] Re: Insecure tempfile creation

[Peter Stephenson]

On Thu, 8 Jan 2015 00:08:21 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Fortunately, we have the zsh/files module which provides a buitin "ln"
> with well-defined semantics.  Hopefully that's good enough.

It's a little bit tangential, but it's always bothered me that the only
option we have for module builtins of this kind is to import the into
the command namespace under the standard name, trashing the use of the
system-standard utility your code may elsewhere depend on That is, you
can use "command ln" if you need to, but the point is in the majority of
existing code you would never have bothered to do that.

We made special arrangements for (z)stat but that really doesn't scale
well.

Apart from (z)stat, most of the builtins that look like standard utilies
are only there for special cases, e.g. for some reason you can't get to
the file system where they live, in which case there's no real problem.
But for uses like this there potentially is.

pws

Re: [PATCH] Re: Insecure tempfile creation

[Ray Andrews]

On 01/08/2015 06:24 AM, Peter Stephenson wrote:
> On Thu, 8 Jan 2015 00:08:21 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
>> Fortunately, we have the zsh/files module which provides a buitin "ln"
>> with well-defined semantics.  Hopefully that's good enough.
> It's a little bit tangential, but it's always bothered me that the only
> option we have for module builtins of this kind is to import the into
> the command namespace under the standard name, trashing the use of the
> system-standard utility your code may elsewhere depend on That is, you
> can use "command ln" if you need to, but the point is in the majority of
> existing code you would never have bothered to do that.
>
> We made special arrangements for (z)stat but that really doesn't scale
> well.
>
> Apart from (z)stat, most of the builtins that look like standard utilies
> are only there for special cases, e.g. for some reason you can't get to
> the file system where they live, in which case there's no real problem.
> But for uses like this there potentially is.

What an interesting thought.  Reading, as I just did about some 'other' 
'ln' I: '$ whence ln' and what I get is '/bin/ln' ... and I shrug my 
shoulders and move on.
So are you saying that zsh has an internal 'ln'?  If so how can I learn 
about it?
I know we have uniquely named zsh functions, but I have no idea about any
that share standard names. man zshbuiltins doesn't mention any 'ln'. What
am I missing?

Re: [PATCH] Re: Insecure tempfile creation

[Peter Stephenson]

On Thu, 8 Jan 2015 08:35:47 -0800
Ray Andrews <rayandrews@eastlink.ca> wrote:
> 'ln' I: '$ whence ln' and what I get is '/bin/ln' ... and I shrug my 
> shoulders and move on.
> So are you saying that zsh has an internal 'ln'?  If so how can I learn 
> about it?

It's in the zsh/files module.  See "man zshmodules".  This is more of
a zsh-users question.

pws

Re: [PATCH] Re: Insecure tempfile creation

[Mikael Magnusson]

On Thu, Jan 8, 2015 at 3:24 PM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Thu, 8 Jan 2015 00:08:21 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
>> Fortunately, we have the zsh/files module which provides a buitin "ln"
>> with well-defined semantics.  Hopefully that's good enough.
>
> It's a little bit tangential, but it's always bothered me that the only
> option we have for module builtins of this kind is to import the into
> the command namespace under the standard name, trashing the use of the
> system-standard utility your code may elsewhere depend on That is, you
> can use "command ln" if you need to, but the point is in the majority of
> existing code you would never have bothered to do that.
>
> We made special arrangements for (z)stat but that really doesn't scale
> well.
>
> Apart from (z)stat, most of the builtins that look like standard utilies
> are only there for special cases, e.g. for some reason you can't get to
> the file system where they live, in which case there's no real problem.
> But for uses like this there potentially is.

Actually all the builtins foo in zsh/files are also available as zf_foo.

-- 
Mikael Magnusson

Re: [PATCH] Re: Insecure tempfile creation

[Peter Stephenson]

On Fri, 9 Jan 2015 03:51:01 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> On Thu, Jan 8, 2015 at 3:24 PM, Peter Stephenson
> <p.stephenson@samsung.com> wrote:
> > It's a little bit tangential, but it's always bothered me that the only
> > option we have for module builtins of this kind is to import the into
> > the command namespace under the standard name, trashing the use of the
> > system-standard utility your code may elsewhere depend on That is, you
> > can use "command ln" if you need to, but the point is in the majority of
> > existing code you would never have bothered to do that.
> >
> > We made special arrangements for (z)stat but that really doesn't scale
> > well.
> >
> > Apart from (z)stat, most of the builtins that look like standard utilies
> > are only there for special cases, e.g. for some reason you can't get to
> > the file system where they live, in which case there's no real problem.
> > But for uses like this there potentially is.
> 
> Actually all the builtins foo in zsh/files are also available as zf_foo.

You're right, that's another, different, special case.  But it's the one
that probably matters the most in practice.

pws

Re: [PATCH] Re: Insecure tempfile creation

[Peter Stephenson]

On Fri, 9 Jan 2015 09:02:38 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Fri, 9 Jan 2015 03:51:01 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
> > Actually all the builtins foo in zsh/files are also available as zf_foo.
> 
> You're right, that's another, different, special case.  But it's the one
> that probably matters the most in practice.

Hey, I made this work!  Well, I assume it was me...

% zmodload -m -F zsh/files b:zf_\*
% zmodload -lF zsh/files
-b:chgrp
-b:chown
-b:ln
-b:mkdir
-b:mv
-b:rm
-b:rmdir
-b:sync
+b:zf_chgrp
+b:zf_chown
+b:zf_ln
+b:zf_mkdir
+b:zf_mv
+b:zf_rm
+b:zf_rmdir
+b:zf_sync

I feel quite smart, now.

Probably worth mentioning in the zsh/files doc.

pws

Re: [PATCH] Re: Insecure tempfile creation

[Peter Stephenson]

On Fri, 9 Jan 2015 12:51:05 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> Probably worth mentioning in the zsh/files doc.

diff --git a/Doc/Zsh/mod_files.yo b/Doc/Zsh/mod_files.yo
index 5dbdae7..90e9884 100644
--- a/Doc/Zsh/mod_files.yo
+++ b/Doc/Zsh/mod_files.yo
@@ -10,7 +10,10 @@ all features now required by relevant standards committees.
 
 For all commands, a variant beginning tt(zf_) is also available and loaded
 automatically.  Using the features capability of zmodload will let you load
-only those names you want.
+only those names you want.  Note that it's possible to load only the
+builtins with zsh-specific names using the following command:
+
+example(zmodload -m -F zsh/files b:zf_\*)
 
 The commands loaded by default are:
 
pws