Re: [PATCH] gitk: Add ability to define an alternate temporary directory

[David Aguilar <davvid@gmail.com>]

On Thu, Nov 12, 2009 at 03:35:59AM -0500, Jeff King wrote:
> On Wed, Nov 11, 2009 at 08:44:53AM -0800, David Aguilar wrote:
> 
> > gitk writes file@commit and file@commit^ to temporary files
> > and diffs them using an external diff tool.
> > 
> > Shall I reroll with s/GITK_TMPDIR/TMPDIR/ ?
> 
> gitk seems to use a very predictable temp filename (".gitk-tmp.$PID").
> Have you checked that you are not introducing any security holes by
> creating that predictable filename in a publicly writable tmpdir?
> 
> It looks like it always tries to "mkdir" the file. Does tcl's mkdir
> function barf on an existing directory? If so, then I think we may be
> safe from the naive:
> 
>   tmp=.gitk-tmp.`pidof_other_users_gitk`
>   mkdir $tmp
>   ln -s /file/for/other/user/to/destroy /tmp/1
> 
> attack. And I think we are not susceptible to races because we fail if
> the mkdir fails (instead of doing something stupid like stat followed
> by mkdir).
> 
> But it has been a long time since I thought about /tmp security, so I
> may be forgetting something.
> 
> -Peff

Thanks for the review.
I'm about to reroll with a new subject, "gitk: Honor TMPDIR..."

When I have more time I can switch gitk over to git-difftool
which I know is /tmp safe.  I only dabble in tcl but the
docs say that mkdir does not error out when given
directories that already exist.  It does error out when
given a file.

The /tmp trick would require them knowing the SHA-1 that
we're diffing and symlinking the names to paths they want
us to destroy.  It seems paranoid to worry about it ;)
but since if it's a real concern than I'll try to
get to the git-difftool rework within the next two week.
I only dabble in tcl ;)

-- 
		David