{"thread":{"id":"21575","subject":"[PATCH] gitk: Add ability to define an alternate temporary directory","startedAt":"2009-11-11T01:49:08Z","lastAt":"2009-11-12T09:56:20Z","messageCount":9,"participants":["David Aguilar","Sam Vilain","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"127286","messageId":"1257904149-44381-1-git-send-email-davvid@gmail.com","threadId":"21575","inReplyTo":null,"subject":"[PATCH] gitk: Add ability to define an alternate temporary directory","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-11-11T01:49:08Z","receivedAt":"2009-11-11T01:49:08Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"gitk fails to show diffs when browsing a repository for which\nwe have read-only access.  This is due to gitk's assumption\nthat the current directory is always writable.\n\nThis teaches gitk to honor the GITK_TMPDIR environment\nvariable.  This allows users to override the default location\nused for writing temporary files.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n gitk |   10 +++++++---\n 1 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex db5ec54..9139ace 100755\n--- a/gitk\n+++ b/gitk\n@@ -3170,11 +3170,15 @@ proc flist_hl {only} {\n }\n \n proc gitknewtmpdir {} {\n-    global diffnum gitktmpdir gitdir\n+    global diffnum env gitktmpdir gitdir\n \n     if {![info exists gitktmpdir]} {\n-\tset gitktmpdir [file join [file dirname $gitdir] \\\n-\t\t\t    [format \".gitk-tmp.%s\" [pid]]]\n+\tif {[info exists env(GITK_TMPDIR)]} {\n+\t    set tmpdir $env(GITK_TMPDIR)\n+\t} else {\n+\t    set tmpdir [file dirname $gitdir]\n+\t}\n+\tset gitktmpdir [file join $tmpdir [format \".gitk-tmp.%s\" [pid]]]\n \tif {[catch {file mkdir $gitktmpdir} err]} {\n \t    error_popup \"[mc \"Error creating temporary directory %s:\" $gitktmpdir] $err\"\n \t    unset gitktmpdir\n-- \n1.6.5.2.180.gc5b3e\n"},{"id":"127287","messageId":"1257904149-44381-2-git-send-email-davvid@gmail.com","threadId":"21575","inReplyTo":"1257904149-44381-1-git-send-email-davvid@gmail.com","subject":"[PATCH] gitk: Document the $GITK_TMPDIR variable","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-11-11T01:49:09Z","receivedAt":"2009-11-11T01:49:09Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Signed-off-by: David Aguilar <davvid@gmail.com>\n---\n Documentation/gitk.txt |    5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/gitk.txt b/Documentation/gitk.txt\nindex cf465cb..7e184f3 100644\n--- a/Documentation/gitk.txt\n+++ b/Documentation/gitk.txt\n@@ -100,6 +100,11 @@ Files\n Gitk creates the .gitk file in your $HOME directory to store preferences\n such as display options, font, and colors.\n \n+Environment Variables\n+---------------------\n+Gitk honors the $GITK_TMPDIR environment when creating temporary\n+files and directories.\n+\n SEE ALSO\n --------\n 'qgit(1)'::\n-- \n1.6.5.2.180.gc5b3e\n"},{"id":"127288","messageId":"4AFA368F.207@vilain.net","threadId":"21575","inReplyTo":"1257904149-44381-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] gitk: Add ability to define an alternate temporary directory","fromName":"Sam Vilain","fromEmail":"sam@vilain.net","sentAt":"2009-11-11T03:59:11Z","receivedAt":"2009-11-11T03:59:11Z","isPatch":true,"sender":{"key":"sam@vilain.net","avatar":"https://gravatar.com/avatar/8fc840ca854dbf6f7065b4335e3b934951c1dca3b11db688e95e471901f8f4a8?d=mp&s=160"},"body":"David Aguilar wrote:\n> gitk fails to show diffs when browsing a repository for which\n> we have read-only access.  This is due to gitk's assumption\n> that the current directory is always writable.\n>\n> This teaches gitk to honor the GITK_TMPDIR environment\n> variable.  This allows users to override the default location\n> used for writing temporary files.\n>   \n\nIs there a good reason not to use the common TMPDIR or TMP environment\nvariables for this?\n\nSam\n"},{"id":"127291","messageId":"20091111040715.GA44672@gmail.com","threadId":"21575","inReplyTo":"4AFA368F.207@vilain.net","subject":"Re: [PATCH] gitk: Add ability to define an alternate temporary directory","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-11-11T04:07:16Z","receivedAt":"2009-11-11T04:07:16Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Nov 11, 2009 at 04:59:11PM +1300, Sam Vilain wrote:\n> David Aguilar wrote:\n> > gitk fails to show diffs when browsing a repository for which\n> > we have read-only access.  This is due to gitk's assumption\n> > that the current directory is always writable.\n> >\n> > This teaches gitk to honor the GITK_TMPDIR environment\n> > variable.  This allows users to override the default location\n> > used for writing temporary files.\n> >   \n> \n> Is there a good reason not to use the common TMPDIR or TMP environment\n> variables for this?\n\nI, personally, would not be opposed to that.\nThe only reason I chose a different variable was that I didn't\nwant to change the existing behavior (backwards-compat hat on).\n\nSince TMPDIR and TMP are common then we'd be changing the\nbehavior.\n\nThat said, if there's a consensus that the path doesn't matter\nmuch than we could change the behavior.  I was just trying to be\ncareful.\n\n-- \n\t\tDavid\n"},{"id":"127345","messageId":"7vr5s5y8n8.fsf@alter.siamese.dyndns.org","threadId":"21575","inReplyTo":"20091111040715.GA44672@gmail.com","subject":"Re: [PATCH] gitk: Add ability to define an alternate temporary directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-11T04:42:35Z","receivedAt":"2009-11-11T04:42:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> That said, if there's a consensus that the path doesn't matter\n> much than we could change the behavior.  I was just trying to be\n> careful.\n\nIf we are not honoring GITK_TMPDIR and suddenly start paying attention to\nit, that already is a change in behaviour.  Why is it better than paying\nattention to TMPDIR?\n\nBut why does gitk, which is primarily a read-only history browsing tool\n(yes, I know it can do \"checkout\" and stuff), needs to write anything to\nanywhere in the first place?\n"},{"id":"127347","messageId":"20091111164451.GA45475@gmail.com","threadId":"21575","inReplyTo":"7vr5s5y8n8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] gitk: Add ability to define an alternate temporary directory","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-11-11T16:44:53Z","receivedAt":"2009-11-11T16:44:53Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Nov 10, 2009 at 08:42:35PM -0800, Junio C Hamano wrote:\n> David Aguilar <davvid@gmail.com> writes:\n> \n> > That said, if there's a consensus that the path doesn't matter\n> > much than we could change the behavior.  I was just trying to be\n> > careful.\n> \n> If we are not honoring GITK_TMPDIR and suddenly start paying attention to\n> it, that already is a change in behaviour.  Why is it better than paying\n> attention to TMPDIR?\n\nTrue.\n\n> But why does gitk, which is primarily a read-only history browsing tool\n> (yes, I know it can do \"checkout\" and stuff), needs to write anything to\n> anywhere in the first place?\n\nIt happens when you click on a commit, right-click on a file\nand then choose \"external diff\".\n\ngitk writes file@commit and file@commit^ to temporary files\nand diffs them using an external diff tool.\n\nShall I reroll with s/GITK_TMPDIR/TMPDIR/ ?\n\ngitk was written before git-difftool existed.\nA higher impact change would be to change gitk to use\ngit-difftool instead of managing its own temporary files.\n\nIf we're looking for the ideal solution than that is\nprobably it.  The only downside is that gitk would stop\nworking with git versions < 1.6.3.  That doesn't seem like\na real issue, though, because gitk is bundled with git.\n\nThoughts?\n\n-- \n\t\tDavid\n"},{"id":"127422","messageId":"20091112083559.GB17552@coredump.intra.peff.net","threadId":"21575","inReplyTo":"20091111164451.GA45475@gmail.com","subject":"Re: [PATCH] gitk: Add ability to define an alternate temporary directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-12T08:35:59Z","receivedAt":"2009-11-12T08:35:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 11, 2009 at 08:44:53AM -0800, David Aguilar wrote:\n\n> gitk writes file@commit and file@commit^ to temporary files\n> and diffs them using an external diff tool.\n> \n> Shall I reroll with s/GITK_TMPDIR/TMPDIR/ ?\n\ngitk seems to use a very predictable temp filename (\".gitk-tmp.$PID\").\nHave you checked that you are not introducing any security holes by\ncreating that predictable filename in a publicly writable tmpdir?\n\nIt looks like it always tries to \"mkdir\" the file. Does tcl's mkdir\nfunction barf on an existing directory? If so, then I think we may be\nsafe from the naive:\n\n  tmp=.gitk-tmp.`pidof_other_users_gitk`\n  mkdir $tmp\n  ln -s /file/for/other/user/to/destroy /tmp/1\n\nattack. And I think we are not susceptible to races because we fail if\nthe mkdir fails (instead of doing something stupid like stat followed\nby mkdir).\n\nBut it has been a long time since I thought about /tmp security, so I\nmay be forgetting something.\n\n-Peff\n"},{"id":"127428","messageId":"20091112093611.GA47285@gmail.com","threadId":"21575","inReplyTo":"20091112083559.GB17552@coredump.intra.peff.net","subject":"Re: [PATCH] gitk: Add ability to define an alternate temporary directory","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-11-12T09:36:13Z","receivedAt":"2009-11-12T09:36:13Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Nov 12, 2009 at 03:35:59AM -0500, Jeff King wrote:\n> On Wed, Nov 11, 2009 at 08:44:53AM -0800, David Aguilar wrote:\n> \n> > gitk writes file@commit and file@commit^ to temporary files\n> > and diffs them using an external diff tool.\n> > \n> > Shall I reroll with s/GITK_TMPDIR/TMPDIR/ ?\n> \n> gitk seems to use a very predictable temp filename (\".gitk-tmp.$PID\").\n> Have you checked that you are not introducing any security holes by\n> creating that predictable filename in a publicly writable tmpdir?\n> \n> It looks like it always tries to \"mkdir\" the file. Does tcl's mkdir\n> function barf on an existing directory? If so, then I think we may be\n> safe from the naive:\n> \n>   tmp=.gitk-tmp.`pidof_other_users_gitk`\n>   mkdir $tmp\n>   ln -s /file/for/other/user/to/destroy /tmp/1\n> \n> attack. And I think we are not susceptible to races because we fail if\n> the mkdir fails (instead of doing something stupid like stat followed\n> by mkdir).\n> \n> But it has been a long time since I thought about /tmp security, so I\n> may be forgetting something.\n> \n> -Peff\n\nThanks for the review.\nI'm about to reroll with a new subject, \"gitk: Honor TMPDIR...\"\n\nWhen I have more time I can switch gitk over to git-difftool\nwhich I know is /tmp safe.  I only dabble in tcl but the\ndocs say that mkdir does not error out when given\ndirectories that already exist.  It does error out when\ngiven a file.\n\nThe /tmp trick would require them knowing the SHA-1 that\nwe're diffing and symlinking the names to paths they want\nus to destroy.  It seems paranoid to worry about it ;)\nbut since if it's a real concern than I'll try to\nget to the git-difftool rework within the next two week.\nI only dabble in tcl ;)\n\n-- \n\t\tDavid\n"},{"id":"127432","messageId":"20091112095620.GA7847@coredump.intra.peff.net","threadId":"21575","inReplyTo":"20091112093611.GA47285@gmail.com","subject":"Re: [PATCH] gitk: Add ability to define an alternate temporary directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-12T09:56:20Z","receivedAt":"2009-11-12T09:56:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 12, 2009 at 01:36:13AM -0800, David Aguilar wrote:\n\n> When I have more time I can switch gitk over to git-difftool\n> which I know is /tmp safe.  I only dabble in tcl but the\n> docs say that mkdir does not error out when given\n> directories that already exist.  It does error out when\n> given a file.\n\nOK, then I think we would be vulnerable, as I can make a .gitk-tmp.$PID\ndirectory owned by me that your gitk will happily use.\n\n> The /tmp trick would require them knowing the SHA-1 that\n> we're diffing and symlinking the names to paths they want\n> us to destroy.  It seems paranoid to worry about it ;)\n\nBut the SHA-1 is not hard to guess[1], as you have a finite,\neasily-enumerable list of them in your repository. :) One thing that\ndoes make it harder is that gitk actually checks to see if a file is\nalready there before creating it (presumably not for security, but for\nefficiency). Which means I can't just pre-seed a trap and wait for you\nto run gitk; I have to actually race you and create the file between\nyour \"file exists\" check and the eventual \"git show $filename >$output\"\nwhich will hose it.\n\nProbably I can win that race given a sufficient number of attempts, but\nattempts are made at a human pace. So in practice it's probably pretty\nhard to exploit. Still, I'd rather see it done properly on principle.\nThen we _know_ we're not missing some trick, and there's no chance of a\nlater code change increasing an attacker's probability of success.\n\n-Peff\n\n[1] I was also going to suggest a social-engineering attack, like \"hey,\nI screwed up my repository. Can you take a look?\" Then you don't need to\nguess the SHA-1, as you convince the victim to look at a specific one.\nBut that attack is already much, much worse: we respect items in\n.git/config regardless of whether it is owned by the running user. So it\nis not actually safe to \"cd ~other_user/project && git diff\".\n"}]}