Insecure tempfile creation

[Daniel Shahaf <d.s@daniel.shahaf.name>]

[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