zsh/files module and insecure tempfile creation

zsh/files module and insecure tempfile creation

[Bart Schaefer]

I'm trying to work through the "attack vector" for the mv -f trick.
The situation is that a script wants to create a plain empty file
with a known name, let's say "/tmp/zsh12345".

If the attacker creates his own plain file named /tmp/zsh12345 then the
"mv -f" will clobber it, so that's not at issue.  Therefore an attack
is possible if the attacker can create a directory (or symlink to a
directory) named /tmp/zsh12345 and writable by the zsh process,
because then mv will put the empty plain file inside that directory.

Next the attacker must be able to swap the directory or symlink with
a symlink to his own target file.  Presumably if he could create it
in the first place, he can swap it.  So we do have an attack that can
clobber any file writable by the zsh user.  Anybody disagree?

Additionally for compinstall and calendar, the files so created are
later read back into the shell as commands, so if the attacker waits
for the right moment to do the swap he can run arbitrary commands as
the user.  This is a serious enough issue that those functions should
simply fail if there is no way to create the tempfile securely.

Anyone want to suggest additional verbosity in those cases?  Here is
the minimal patch.  Is it sufficient for zfget_match to return 1, or
does it also need to mess with $reply?  (And is $reply really declared
local in the correct place in zfget_match?  I didn't change that.)


diff --git a/Completion/Base/Widget/_complete_debug b/Completion/Base/Widget/_complete_debug
index 50fc809..ba3d2b4 100644
--- a/Completion/Base/Widget/_complete_debug
+++ b/Completion/Base/Widget/_complete_debug
@@ -9,7 +9,8 @@ local pager w="${(qq)words}"
 integer debug_fd=-1
 {
   if [[ -t 2 ]]; then
-    mv -f =(<<<'') $tmp &&
+    zmodload -m -F zsh/files b:zf_ln 2>/dev/null &&
+    zf_ln -fn =(<<<'') $tmp &&
     exec {debug_fd}>&2 2>| $tmp
   fi
 
diff --git a/Completion/compinstall b/Completion/compinstall
index ae94993..2f99d27 100644
--- a/Completion/compinstall
+++ b/Completion/compinstall
@@ -3,6 +3,8 @@
 emulate -L zsh
 setopt extendedglob
 
+zmodload -m -F zsh/files b:zf_ln || return 1
+
 local key
 local compcontext=-default-
 
@@ -1958,8 +1960,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
+zf_ln -fn =(<<<'') $tmpout &&	# safe tempfile creation
+zf_ln -fn =(<<<'') ${tmpout}x || return 1
 
 #
 # Assemble the complete set of lines to
diff --git a/Functions/Calendar/calendar b/Functions/Calendar/calendar
index 39fc431..0d651dc 100644
--- a/Functions/Calendar/calendar
+++ b/Functions/Calendar/calendar
@@ -12,6 +12,7 @@ local -A reply
 
 zmodload -i zsh/datetime || return 1
 zmodload -i zsh/zutil || return 1
+zmodload -m -F zsh/files b:zf_ln || return 1
 
 autoload -Uz calendar_{add,parse,read,scandate,show,lockfiles}
 
@@ -254,7 +255,7 @@ if (( verbose )); then
 fi
 
 local mycmds="${TMPPREFIX:-/tmp/zsh}.calendar_cmds.$$"
-mv -f =(<<<'') $mycmds
+zf_ln -fn =(<<<'') $mycmds || return 1
 
 # start of subshell for OS file locking
 (
diff --git a/Functions/Zftp/zfget_match b/Functions/Zftp/zfget_match
index 3ba06c4..3f2bbf3 100644
--- a/Functions/Zftp/zfget_match
+++ b/Functions/Zftp/zfget_match
@@ -1,6 +1,7 @@
 # function zfget_match {
 
 emulate -L zsh
+zmodload -m -F zsh/files b:zf_ln || return 1
 
 # the zfcd hack:  this may not be necessary here
 if [[ $1 == $HOME || $1 == $HOME/* ]]; then
@@ -10,8 +11,8 @@ fi
 if [[ $ZFTP_SYSTEM == UNIX* && $1 == */* ]]; then
   setopt localoptions clobber
   local tmpf=${TMPPREFIX}zfgm$$
-  mv -f =(<<<'') $tmpf
-	
+  zf_ln -fn =(<<<'') $tmpf || return 1
+
   if [[ -n $WIDGET ]]; then
     local dir=${1:h}
     [[ $dir = */ ]] || dir="$dir/"

-- 
Barton E. Schaefer

Re: zsh/files module and insecure tempfile creation

[Peter Stephenson]

On Fri, 09 Jan 2015 22:31:50 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> +zmodload -m -F zsh/files b:zf_ln || return 1

It's harmless, but you don't need the -m here: there's no pattern to
match.

pws

Re: zsh/files module and insecure tempfile creation

[Bart Schaefer]

On Jan 10,  7:58pm, Peter Stephenson wrote:
}
} > +zmodload -m -F zsh/files b:zf_ln || return 1
} 
} It's harmless, but you don't need the -m here: there's no pattern to
} match.

Ugh.  Duh.  Didn't edit enough out of the example I modified.

Re: zsh/files module and insecure tempfile creation

[Daniel Shahaf]

Bart Schaefer wrote on Fri, Jan 09, 2015 at 22:31:50 -0800:
> I'm trying to work through the "attack vector" for the mv -f trick.
> The situation is that a script wants to create a plain empty file
> with a known name, let's say "/tmp/zsh12345".
> 
> If the attacker creates his own plain file named /tmp/zsh12345 then the
> "mv -f" will clobber it, so that's not at issue.  Therefore an attack
> is possible if the attacker can create a directory (or symlink to a
> directory) named /tmp/zsh12345 and writable by the zsh process,
> because then mv will put the empty plain file inside that directory.
> 
> Next the attacker must be able to swap the directory or symlink with
> a symlink to his own target file.  Presumably if he could create it
> in the first place, he can swap it.  So we do have an attack that can
> clobber any file writable by the zsh user.  Anybody disagree?

On FreeBSD systems, 'cat /path/to/dir' works (prints something and
returns 0).  Therefore, even if the (symlink-to-)dir is not replaced by
a file, unexpected effects might be caused when some code attempts to
cat the tempfile (which is in fact a directory) — whether in order to
run it as shell commands or in order to use it as data.

Sorry, but I haven't audited the patch to see what exactly this could
cause in each instance.  (No time.)

Daniel

Re: zsh/files module and insecure tempfile creation

[Bart Schaefer]

On Jan 12,  5:43am, Daniel Shahaf wrote:
} Subject: Re: zsh/files module and insecure tempfile creation
}
} Bart Schaefer wrote on Fri, Jan 09, 2015 at 22:31:50 -0800:
} > Next the attacker must be able to swap the directory or symlink with
} > a symlink to his own target file.
} 
} On FreeBSD systems, 'cat /path/to/dir' works (prints something and
} returns 0).

That is also avoided, I think, by using zf_ln -fn to be sure the target
is removed and replaced by zsh's temp file.  I think we're OK there after
this latest patch pass.