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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html