Re: Insecure tempfile creation

[Mikael Magnusson <mikachu@xxxxxxxxx>]

On Mon, Dec 22, 2014 at 9:36 PM, Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> 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