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

From: Jeff King <peff@peff.net>
Date: Nov 12, 2009, 08:35 UTC
Message-ID: <20091112083559.GB17552@coredump.intra.peff.net>
In-Reply-To: <20091111164451.GA45475@gmail.com>

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