threads / patch / 21575

patchgitk: Add ability to define an alternate temporary directory

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

## tl;dr

9 messages between Nov 11, 2009 and Nov 12, 2009. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

David Aguilar· Nov 11, 2009, 01:49 UTC · lore

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(-)
Show changes to gitk +7 −3
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
David Aguilar· Nov 11, 2009, 01:49 UTC · re: David Aguilar · lore

[PATCH] gitk: Document the $GITK_TMPDIR variable

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Documentation/gitk.txt |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
Show changes to Documentation/gitk.txt +5 −0
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
Sam Vilain· Nov 11, 2009, 03:59 UTC · re: David Aguilar · lore

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

David Aguilar wrote:
Show 8 quoted lines
> 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
David Aguilar· Nov 11, 2009, 04:07 UTC · re: Sam Vilain · lore

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

On Wed, Nov 11, 2009 at 04:59:11PM +1300, Sam Vilain wrote:
Show 12 quoted lines
> 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
Junio C Hamano· Nov 11, 2009, 04:42 UTC · re: David Aguilar · lore

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

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?

David Aguilar· Nov 11, 2009, 16:44 UTC · re: Junio C Hamano · lore

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

On Tue, Nov 10, 2009 at 08:42:35PM -0800, Junio C Hamano wrote:
Show 9 quoted lines
> 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
Jeff King· Nov 12, 2009, 08:35 UTC · re: David Aguilar · lore

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

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
David Aguilar· Nov 12, 2009, 09:36 UTC · re: Jeff King · lore

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

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
Jeff King· Nov 12, 2009, 09:56 UTC · re: David Aguilar · lore

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

On Thu, Nov 12, 2009 at 01:36:13AM -0800, David Aguilar wrote:
Show 5 quoted lines
> 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".

← back to recent threads