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

From: David Aguilar <davvid@gmail.com>
Date: Nov 12, 2009, 09:36 UTC
Message-ID: <20091112093611.GA47285@gmail.com>
In-Reply-To: <20091112083559.GB17552@coredump.intra.peff.net>

On Thu, Nov 12, 2009 at 03:35:59AM -0500, Jeff King wrote:

Show 27 quoted lines

> 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