{"thread":{"id":"36913","subject":"[PATCH] gitk: use mktemp -d to avoid predictable temporary directories","startedAt":"2014-06-13T21:43:48Z","lastAt":"2014-06-19T02:54:07Z","messageCount":9,"participants":["David Aguilar","Paul Mackerras","Pat Thoyts","brian m. carlson","Thomas Braun","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"244177","messageId":"1402695828-91537-1-git-send-email-davvid@gmail.com","threadId":"36913","inReplyTo":null,"subject":"[PATCH] gitk: use mktemp -d to avoid predictable temporary directories","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-06-13T21:43:48Z","receivedAt":"2014-06-13T21:43:48Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"gitk uses a predictable \".gitk-tmp.$PID\" pattern when generating\na temporary directory.\n\nUse \"mktemp -d .gitk-tmp.XXXXXX\" to harden gitk against someone\nseeding /tmp with files matching the pid pattern.\n\nSigned-off-by: David Aguilar \n---\nThis issue was brought up during the first review of the previous patch\nback in 2009.\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/132609/focus=132748\n\nThis is really [PATCH 2/2] and should be applied on top of my previous\ngitk patch.\n\n gitk | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/gitk b/gitk\nindex 82293dd..dd2ff63 100755\n--- a/gitk\n+++ b/gitk\n@@ -3502,7 +3502,8 @@ proc gitknewtmpdir {} {\n \t} else {\n \t set tmpdir $gitdir\n \t}\n-\tset gitktmpdir [file join $tmpdir [format \".gitk-tmp.%s\" [pid]]]\n+\tset gitktmpformat [file join $tmpdir \".gitk-tmp.XXXXXX\"]\n+\tset gitktmpdir [exec mktemp -d $gitktmpformat]\n \tif {[catch {file mkdir $gitktmpdir} err]} {\n \t error_popup \"[mc \"Error creating temporary directory %s:\" $gitktmpdir] $err\"\n \t unset gitktmpdir\n-- \n2.0.0.257.g75cc6c6\n"},{"id":"244200","messageId":"20140615045752.GF21978@iris.ozlabs.ibm.com","threadId":"36913","inReplyTo":"1402695828-91537-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2014-06-15T04:57:52Z","receivedAt":"2014-06-15T04:57:52Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"On Fri, Jun 13, 2014 at 02:43:48PM -0700, David Aguilar wrote:\n> gitk uses a predictable \".gitk-tmp.$PID\" pattern when generating\n> a temporary directory.\n> \n> Use \"mktemp -d .gitk-tmp.XXXXXX\" to harden gitk against someone\n> seeding /tmp with files matching the pid pattern.\n> \n> Signed-off-by: David Aguilar \n\nThanks, applied.\n\nPaul.\n"},{"id":"244206","messageId":"87k38ir4p0.fsf@red.patthoyts.tk","threadId":"36913","inReplyTo":"1402695828-91537-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories","fromName":"Pat Thoyts","fromEmail":"patthoyts@users.sourceforge.net","sentAt":"2014-06-15T07:51:23Z","receivedAt":"2014-06-15T07:51:23Z","isPatch":true,"sender":{"key":"patthoyts@users.sourceforge.net","avatar":"https://avatars.githubusercontent.com/u/30739?v=4"},"body":"David Aguilar writes:\n\n>gitk uses a predictable \".gitk-tmp.$PID\" pattern when generating\n>a temporary directory.\n>\n>Use \"mktemp -d .gitk-tmp.XXXXXX\" to harden gitk against someone\n>seeding /tmp with files matching the pid pattern.\n>\n>Signed-off-by: David Aguilar \n>---\n>This issue was brought up during the first review of the previous patch\n>back in 2009.\n>\n>http://thread.gmane.org/gmane.comp.version-control.git/132609/focus=132748\n>\n>This is really [PATCH 2/2] and should be applied on top of my previous\n>gitk patch.\n>\n> gitk | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n>diff --git a/gitk b/gitk\n>index 82293dd..dd2ff63 100755\n>--- a/gitk\n>+++ b/gitk\n>@@ -3502,7 +3502,8 @@ proc gitknewtmpdir {} {\n> \t} else {\n> \t set tmpdir $gitdir\n> \t}\n>-\tset gitktmpdir [file join $tmpdir [format \".gitk-tmp.%s\" [pid]]]\n>+\tset gitktmpformat [file join $tmpdir \".gitk-tmp.XXXXXX\"]\n>+\tset gitktmpdir [exec mktemp -d $gitktmpformat]\n> \tif {[catch {file mkdir $gitktmpdir} err]} {\n> \t error_popup \"[mc \"Error creating temporary directory %s:\" $gitktmpdir] $err\"\n> \t unset gitktmpdir\n\nThis is a problem on Windows where we will not have mktemp. In Tcl 8.6\nthe file command acquired a \"file tempfile\" command to help with this\nkind of issue (https://www.tcl.tk/man/tcl8.6/TclCmd/file.htm#M39) but\nfor older versions we should probably stick with the existing pattern at\nleast on Windows.\n\n-- \nPat Thoyts http://www.patthoyts.tk/\nPGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD\n"},{"id":"244215","messageId":"20140615163227.GE368384@vauxhall.crustytoothpaste.net","threadId":"36913","inReplyTo":"87k38ir4p0.fsf@red.patthoyts.tk","subject":"Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-06-15T16:32:27Z","receivedAt":"2014-06-15T16:32:27Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Jun 15, 2014 at 08:51:23AM +0100, Pat Thoyts wrote:\n> David Aguilar writes:\n> >--- a/gitk\n> >+++ b/gitk\n> >@@ -3502,7 +3502,8 @@ proc gitknewtmpdir {} {\n> > \t} else {\n> > \t set tmpdir $gitdir\n> > \t}\n> >-\tset gitktmpdir [file join $tmpdir [format \".gitk-tmp.%s\" [pid]]]\n> >+\tset gitktmpformat [file join $tmpdir \".gitk-tmp.XXXXXX\"]\n> >+\tset gitktmpdir [exec mktemp -d $gitktmpformat]\n> > \tif {[catch {file mkdir $gitktmpdir} err]} {\n> > \t error_popup \"[mc \"Error creating temporary directory %s:\" $gitktmpdir] $err\"\n> > \t unset gitktmpdir\n> \n> This is a problem on Windows where we will not have mktemp. In Tcl 8.6\n> the file command acquired a \"file tempfile\" command to help with this\n> kind of issue (https://www.tcl.tk/man/tcl8.6/TclCmd/file.htm#M39) but\n> for older versions we should probably stick with the existing pattern at\n> least on Windows.\n\nThe existing pattern is a security bug on Unix systems. MITRE (CWE-377)\ntells me that it is a vulnerability on Windows as well, so you'd\nprobably want to come up with a better solution than the existing\npattern.\n\nYou also probably want to request a CVE for this, which the Red Hat and\nDebian security teams can do for you if you like. Distributions will\nlikely want to issue security advisories for this.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"244233","messageId":"20140615214928.GA619@gmail.com","threadId":"36913","inReplyTo":"20140615163227.GE368384@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-06-15T21:49:29Z","receivedAt":"2014-06-15T21:49:29Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Jun 15, 2014 at 04:32:27PM +0000, brian m. carlson wrote:\n> On Sun, Jun 15, 2014 at 08:51:23AM +0100, Pat Thoyts wrote:\n> > David Aguilar writes:\n> > >--- a/gitk\n> > >+++ b/gitk\n> > >@@ -3502,7 +3502,8 @@ proc gitknewtmpdir {} {\n> > > \t} else {\n> > > \t set tmpdir $gitdir\n> > > \t}\n> > >-\tset gitktmpdir [file join $tmpdir [format \".gitk-tmp.%s\" [pid]]]\n> > >+\tset gitktmpformat [file join $tmpdir \".gitk-tmp.XXXXXX\"]\n> > >+\tset gitktmpdir [exec mktemp -d $gitktmpformat]\n> > > \tif {[catch {file mkdir $gitktmpdir} err]} {\n> > > \t error_popup \"[mc \"Error creating temporary directory %s:\" $gitktmpdir] $err\"\n> > > \t unset gitktmpdir\n> > \n> > This is a problem on Windows where we will not have mktemp. In Tcl 8.6\n> > the file command acquired a \"file tempfile\" command to help with this\n> > kind of issue (https://www.tcl.tk/man/tcl8.6/TclCmd/file.htm#M39) but\n> > for older versions we should probably stick with the existing pattern at\n> > least on Windows.\n> \n> The existing pattern is a security bug on Unix systems. MITRE (CWE-377)\n> tells me that it is a vulnerability on Windows as well, so you'd\n> probably want to come up with a better solution than the existing\n> pattern.\n> \n> You also probably want to request a CVE for this, which the Red Hat and\n> Debian security teams can do for you if you like. Distributions will\n> likely want to issue security advisories for this.\n\nI don't think this requires a CVE since it's basically plugging a hole\nthat my previous patch introduced by making gitk honor the TMPDIR\nvariable; it hasn't strictly been in any release yet.\n\nDoes Git on Windows use a modern tcl?\nI checked, and my (old) existing msysgit installation had tcl\n8.5, so I unfortunately using \"file tempname\" won't help there.\n\nHmm.. I guess what I could do is keep the old behavior (having gitk ignore TMPDIR)\non Windows and only use the new code path on non-Windows.\n\nThat seems like it'd be the simplest implementation (no need to check versions)\nand the least harmful to existing users (avoids a tcl upgrade or mkdtemp installation\nfor Windows users).\n-- \nDavid\n"},{"id":"244234","messageId":"20140615221632.GH368384@vauxhall.crustytoothpaste.net","threadId":"36913","inReplyTo":"20140615214928.GA619@gmail.com","subject":"Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-06-15T22:16:32Z","receivedAt":"2014-06-15T22:16:32Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Jun 15, 2014 at 02:49:29PM -0700, David Aguilar wrote:\n> I don't think this requires a CVE since it's basically plugging a hole\n> that my previous patch introduced by making gitk honor the TMPDIR\n> variable; it hasn't strictly been in any release yet.\n\nYeah, that's not needed, then. I didn't notice it was the immediately\nprevious patch. My bad.\n\n> Hmm.. I guess what I could do is keep the old behavior (having gitk\n> ignore TMPDIR) on Windows and only use the new code path on\n> non-Windows.\n> \n> That seems like it'd be the simplest implementation (no need to check\n> versions) and the least harmful to existing users (avoids a tcl\n> upgrade or mkdtemp installation for Windows users).\n\nYeah, that would be the safest bet. Maybe a comment to that effect\nwould be appropriate, so that when Tcl gets upgraded, that change can be\nremoved.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"244245","messageId":"539ED793.7050409@virtuell-zuhause.de","threadId":"36913","inReplyTo":"87k38ir4p0.fsf@red.patthoyts.tk","subject":"Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2014-06-16T11:40:03Z","receivedAt":"2014-06-16T11:40:03Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"Am 15.06.2014 09:51, schrieb Pat Thoyts:\n> David Aguilar writes:\n> \n>> gitk uses a predictable \".gitk-tmp.$PID\" pattern when generating\n>> a temporary directory.\n>>\n>> Use \"mktemp -d .gitk-tmp.XXXXXX\" to harden gitk against someone\n>> seeding /tmp with files matching the pid pattern.\n>>\n>> Signed-off-by: David Aguilar \n>> ---\n>> This issue was brought up during the first review of the previous patch\n>> back in 2009.\n>>\n>> http://thread.gmane.org/gmane.comp.version-control.git/132609/focus=132748\n>>\n>> This is really [PATCH 2/2] and should be applied on top of my previous\n>> gitk patch.\n>>\n>> gitk | 3 ++-\n>> 1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/gitk b/gitk\n>> index 82293dd..dd2ff63 100755\n>> --- a/gitk\n>> +++ b/gitk\n>> @@ -3502,7 +3502,8 @@ proc gitknewtmpdir {} {\n>> \t} else {\n>> \t set tmpdir $gitdir\n>> \t}\n>> -\tset gitktmpdir [file join $tmpdir [format \".gitk-tmp.%s\" [pid]]]\n>> +\tset gitktmpformat [file join $tmpdir \".gitk-tmp.XXXXXX\"]\n>> +\tset gitktmpdir [exec mktemp -d $gitktmpformat]\n>> \tif {[catch {file mkdir $gitktmpdir} err]} {\n>> \t error_popup \"[mc \"Error creating temporary directory %s:\" $gitktmpdir] $err\"\n>> \t unset gitktmpdir\n> \n> This is a problem on Windows where we will not have mktemp. In Tcl 8.6\n> the file command acquired a \"file tempfile\" command to help with this\n> kind of issue (https://www.tcl.tk/man/tcl8.6/TclCmd/file.htm#M39) but\n> for older versions we should probably stick with the existing pattern at\n> least on Windows.\n\nWe could of course add mktemp from http://www.mktemp.org to msysgit.\nI can do that if required.\n\nIn mingwgitDevEnv we already have the the need for mktemp, and a msys\npackage, so this is also not a problem.\n"},{"id":"244325","messageId":"xmqqtx7kra5x.fsf@gitster.dls.corp.google.com","threadId":"36913","inReplyTo":"20140615214928.GA619@gmail.com","subject":"Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-16T18:17:46Z","receivedAt":"2014-06-16T18:17:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Hmm.. I guess what I could do is keep the old behavior (having gitk ignore TMPDIR)\n> on Windows and only use the new code path on non-Windows.\n\nOr perhaps attempt to create, catch error and then retry the old way?\n\nHopefully Windows folks do not have to worry about forgetting to\nupdate the codepath when they update their tcl/wish if you did it\nthat way, no?\n\n>\n> That seems like it'd be the simplest implementation (no need to check versions)\n> and the least harmful to existing users (avoids a tcl upgrade or mkdtemp installation\n> for Windows users).\n"},{"id":"244615","messageId":"20140619025406.GA8660@gmail.com","threadId":"36913","inReplyTo":"xmqqtx7kra5x.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-06-19T02:54:07Z","receivedAt":"2014-06-19T02:54:07Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Jun 16, 2014 at 11:17:46AM -0700, Junio C Hamano wrote:\n> David Aguilar writes:\n> \n> > Hmm.. I guess what I could do is keep the old behavior (having gitk ignore TMPDIR)\n> > on Windows and only use the new code path on non-Windows.\n> \n> Or perhaps attempt to create, catch error and then retry the old way?\n> \n> Hopefully Windows folks do not have to worry about forgetting to\n> update the codepath when they update their tcl/wish if you did it\n> that way, no?\n\nTrue, that would be the safest. I just submitted a new replacement patch\nfor these two patches.\n\nThanks,\n-- \nDavid\n"}]}