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

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

[David Aguilar]

gitk fails to show diffs when browsing a repository for which
we have read-only access.  This is due to gitk's assumption
that the current directory is always writable.

This teaches gitk to honor the GITK_TMPDIR environment
variable.  This allows users to override the default location
used for writing temporary files.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 gitk |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/gitk b/gitk
index db5ec54..9139ace 100755
--- a/gitk
+++ b/gitk
@@ -3170,11 +3170,15 @@ proc flist_hl {only} {
 }
 
 proc gitknewtmpdir {} {
-    global diffnum gitktmpdir gitdir
+    global diffnum env gitktmpdir gitdir
 
     if {![info exists gitktmpdir]} {
-	set gitktmpdir [file join [file dirname $gitdir] \
-			    [format ".gitk-tmp.%s" [pid]]]
+	if {[info exists env(GITK_TMPDIR)]} {
+	    set tmpdir $env(GITK_TMPDIR)
+	} else {
+	    set tmpdir [file dirname $gitdir]
+	}
+	set gitktmpdir [file join $tmpdir [format ".gitk-tmp.%s" [pid]]]
 	if {[catch {file mkdir $gitktmpdir} err]} {
 	    error_popup "[mc "Error creating temporary directory %s:" $gitktmpdir] $err"
 	    unset gitktmpdir
-- 
1.6.5.2.180.gc5b3e

[PATCH] gitk: Document the $GITK_TMPDIR variable

[David Aguilar]

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Documentation/gitk.txt |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
index cf465cb..7e184f3 100644
--- a/Documentation/gitk.txt
+++ b/Documentation/gitk.txt
@@ -100,6 +100,11 @@ Files
 Gitk creates the .gitk file in your $HOME directory to store preferences
 such as display options, font, and colors.
 
+Environment Variables
+---------------------
+Gitk honors the $GITK_TMPDIR environment when creating temporary
+files and directories.
+
 SEE ALSO
 --------
 'qgit(1)'::
-- 
1.6.5.2.180.gc5b3e

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

[Sam Vilain]

David Aguilar wrote:
> gitk fails to show diffs when browsing a repository for which
> we have read-only access.  This is due to gitk's assumption
> that the current directory is always writable.
>
> This teaches gitk to honor the GITK_TMPDIR environment
> variable.  This allows users to override the default location
> used for writing temporary files.
>   

Is there a good reason not to use the common TMPDIR or TMP environment
variables for this?

Sam

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

[David Aguilar]

On Wed, Nov 11, 2009 at 04:59:11PM +1300, Sam Vilain wrote:
> David Aguilar wrote:
> > gitk fails to show diffs when browsing a repository for which
> > we have read-only access.  This is due to gitk's assumption
> > that the current directory is always writable.
> >
> > This teaches gitk to honor the GITK_TMPDIR environment
> > variable.  This allows users to override the default location
> > used for writing temporary files.
> >   
> 
> Is there a good reason not to use the common TMPDIR or TMP environment
> variables for this?

I, personally, would not be opposed to that.
The only reason I chose a different variable was that I didn't
want to change the existing behavior (backwards-compat hat on).

Since TMPDIR and TMP are common then we'd be changing the
behavior.

That said, if there's a consensus that the path doesn't matter
much than we could change the behavior.  I was just trying to be
careful.

-- 
		David

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> That said, if there's a consensus that the path doesn't matter
> much than we could change the behavior.  I was just trying to be
> careful.

If we are not honoring GITK_TMPDIR and suddenly start paying attention to
it, that already is a change in behaviour.  Why is it better than paying
attention to TMPDIR?

But why does gitk, which is primarily a read-only history browsing tool
(yes, I know it can do "checkout" and stuff), needs to write anything to
anywhere in the first place?

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

[David Aguilar]

On Tue, Nov 10, 2009 at 08:42:35PM -0800, Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
> 
> > That said, if there's a consensus that the path doesn't matter
> > much than we could change the behavior.  I was just trying to be
> > careful.
> 
> If we are not honoring GITK_TMPDIR and suddenly start paying attention to
> it, that already is a change in behaviour.  Why is it better than paying
> attention to TMPDIR?

True.

> But why does gitk, which is primarily a read-only history browsing tool
> (yes, I know it can do "checkout" and stuff), needs to write anything to
> anywhere in the first place?

It happens when you click on a commit, right-click on a file
and then choose "external diff".

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 was written before git-difftool existed.
A higher impact change would be to change gitk to use
git-difftool instead of managing its own temporary files.

If we're looking for the ideal solution than that is
probably it.  The only downside is that gitk would stop
working with git versions < 1.6.3.  That doesn't seem like
a real issue, though, because gitk is bundled with git.

Thoughts?

-- 
		David

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

[Jeff King]

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

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

[David Aguilar]

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

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

[Jeff King]

On Thu, Nov 12, 2009 at 01:36:13AM -0800, David Aguilar wrote:

> 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.

OK, then I think we would be vulnerable, as I can make a .gitk-tmp.$PID
directory owned by me that your gitk will happily use.

> 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 the SHA-1 is not hard to guess[1], as you have a finite,
easily-enumerable list of them in your repository. :) One thing that
does make it harder is that gitk actually checks to see if a file is
already there before creating it (presumably not for security, but for
efficiency). Which means I can't just pre-seed a trap and wait for you
to run gitk; I have to actually race you and create the file between
your "file exists" check and the eventual "git show $filename >$output"
which will hose it.

Probably I can win that race given a sufficient number of attempts, but
attempts are made at a human pace. So in practice it's probably pretty
hard to exploit. Still, I'd rather see it done properly on principle.
Then we _know_ we're not missing some trick, and there's no chance of a
later code change increasing an attacker's probability of success.

-Peff

[1] I was also going to suggest a social-engineering attack, like "hey,
I screwed up my repository. Can you take a look?" Then you don't need to
guess the SHA-1, as you convince the victim to look at a specific one.
But that attack is already much, much worse: we respect items in
.git/config regardless of whether it is owned by the running user. So it
is not actually safe to "cd ~other_user/project && git diff".