Re: PATCH: [for consideration] TMPSUFFIX

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Bart Schaefer wrote on Mon, Sep 26, 2016 at 09:19:22 -0700:
> On Sep 26,  7:25am, Daniel Shahaf wrote:
> } Subject: Re: PATCH: [for consideration] TMPSUFFIX
> }
> } This looks like a race condition: if another process creates the
> } filename that mktemp() returned before the open(O_EXCL) call [which will
> } therefore fail], zsh will remove that filename even though zsh didn't
> } create it.
> 
> If another process creates the filename that mktemp() returned, we have
> a much larger problem -- the whole point is that it should be impossible
> for another process to create that name first.
> 

That is not impossible; it is only improbable.  That's why O_EXCL is
passed to the open() call that receives the name returned by mktemp().

It would be impossible if the code used mkstemp().

> } [This could trigger through a deliberate attack or through
> } a race condition between two zsh instances calling =(:) concurrently.]
> 
> It definitely CANNOT occur because of two zsh instances using =().  I

In the time window between mktemp() returning and open() called,
a second zsh process could call mktemp() and be returned the same value
that was returned in the first process, couldn't it?

I think it's more likely that a name of the form ${TMPPREFIX}XXXXXX
would be created by another zsh process that has the same $TMPPREFIX
setting than by an attacker.

> won't assert the attack is 100% impossible, but there's nothing more
> we can do about that than we already have.
> 

Yes, there is: we should stop calling addfilelist(nam) if open(nam)
returns negative.

(Sidebar: that mktemp() call is the only warning in my build; it'd be
nice to get rid of it while we're here.)

> So mostly we're concerned with $TMPDIR being unwritable, or full file
> system, etc. -- but of course failure for any other reason including
> attacker-duplicated file name would get swept up if we change the way
> failure is handled.
> 

Agreed, all failure reasons can be handled the same way.

> } I don't get "the caller will create the file".  [...]
> } (The caller can call getoutputfile() again if he wishes.)
> 
> The SHELL SCRIPT caller, not the C code caller.  That is, as currently
> coded, =(...) will substitute a file name whether or not it actually got
> created.  In a case like
> 
>     () { ... do something with $1 ... } =(contents)
> 
> the name in $1 could be used in output redirections, etc.  As written,
> exec.c seems intended to allow this.
> 

I checked 'blame' to find the original motivation for that, but the
lines in question predate the import to git.

> } How is it helpful to return a filename, which may or may not exist and
> } certainly doesn't contain the output of 'cmd'? It seems to me erroring
> } out would be better.
> 
> It'd have to be an error on the same order as "bad substitution" so the
> whole command context fails.  Hence bringing it up for discussion.

I don't know what's best here.

I'm quite sure that if open() fails, we mustn't pass the filename we
tried to code that expects to receive a newly-created filename, since
that exposes us to a symlink attack.

Using ERRFLAG_ERROR risks aborting too much code (39154 being a recent
example).

A middle way would be to force the simple command that =(:) was part to
return 127.  When =(:) is used a an argument to an anonymous function,
that would prevent the function's body from being executed with an
invalid filename argument, which is good... but nobody checks the exit
code of such anonymous functions, so code _after_ the anonymous function
might break because it assumes the function was executed.

Maybe there's some simple solution that I'm overlooking.  (And with any
luck, it will appear in my -workers@ mailbox in the near future ;-).)

Cheers,

Daniel