[PATCH] gitk: use mktemp -d to avoid predictable temporary directories

[PATCH] gitk: use mktemp -d to avoid predictable temporary directories

[David Aguilar]

gitk uses a predictable ".gitk-tmp.$PID" pattern when generating
a temporary directory.

Use "mktemp -d .gitk-tmp.XXXXXX" to harden gitk against someone
seeding /tmp with files matching the pid pattern.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
This issue was brought up during the first review of the previous patch
back in 2009.

http://thread.gmane.org/gmane.comp.version-control.git/132609/focus=132748

This is really [PATCH 2/2] and should be applied on top of my previous
gitk patch.

 gitk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gitk b/gitk
index 82293dd..dd2ff63 100755
--- a/gitk
+++ b/gitk
@@ -3502,7 +3502,8 @@ proc gitknewtmpdir {} {
 	} else {
 	    set tmpdir $gitdir
 	}
-	set gitktmpdir [file join $tmpdir [format ".gitk-tmp.%s" [pid]]]
+	set gitktmpformat [file join $tmpdir ".gitk-tmp.XXXXXX"]
+	set gitktmpdir [exec mktemp -d $gitktmpformat]
 	if {[catch {file mkdir $gitktmpdir} err]} {
 	    error_popup "[mc "Error creating temporary directory %s:" $gitktmpdir] $err"
 	    unset gitktmpdir
-- 
2.0.0.257.g75cc6c6

Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories

[Paul Mackerras]

On Fri, Jun 13, 2014 at 02:43:48PM -0700, David Aguilar wrote:
> gitk uses a predictable ".gitk-tmp.$PID" pattern when generating
> a temporary directory.
> 
> Use "mktemp -d .gitk-tmp.XXXXXX" to harden gitk against someone
> seeding /tmp with files matching the pid pattern.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>

Thanks, applied.

Paul.

Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories

[Pat Thoyts]

David Aguilar <davvid@gmail.com> writes:

>gitk uses a predictable ".gitk-tmp.$PID" pattern when generating
>a temporary directory.
>
>Use "mktemp -d .gitk-tmp.XXXXXX" to harden gitk against someone
>seeding /tmp with files matching the pid pattern.
>
>Signed-off-by: David Aguilar <davvid@gmail.com>
>---
>This issue was brought up during the first review of the previous patch
>back in 2009.
>
>http://thread.gmane.org/gmane.comp.version-control.git/132609/focus=132748
>
>This is really [PATCH 2/2] and should be applied on top of my previous
>gitk patch.
>
> gitk | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/gitk b/gitk
>index 82293dd..dd2ff63 100755
>--- a/gitk
>+++ b/gitk
>@@ -3502,7 +3502,8 @@ proc gitknewtmpdir {} {
> 	} else {
> 	    set tmpdir $gitdir
> 	}
>-	set gitktmpdir [file join $tmpdir [format ".gitk-tmp.%s" [pid]]]
>+	set gitktmpformat [file join $tmpdir ".gitk-tmp.XXXXXX"]
>+	set gitktmpdir [exec mktemp -d $gitktmpformat]
> 	if {[catch {file mkdir $gitktmpdir} err]} {
> 	    error_popup "[mc "Error creating temporary directory %s:" $gitktmpdir] $err"
> 	    unset gitktmpdir

This is a problem on Windows where we will not have mktemp. In Tcl 8.6
the file command acquired a "file tempfile" command to help with this
kind of issue (https://www.tcl.tk/man/tcl8.6/TclCmd/file.htm#M39) but
for older versions we should probably stick with the existing pattern at
least on Windows.

-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD

Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

On Sun, Jun 15, 2014 at 08:51:23AM +0100, Pat Thoyts wrote:
> David Aguilar <davvid@gmail.com> writes:
> >--- a/gitk
> >+++ b/gitk
> >@@ -3502,7 +3502,8 @@ proc gitknewtmpdir {} {
> > 	} else {
> > 	    set tmpdir $gitdir
> > 	}
> >-	set gitktmpdir [file join $tmpdir [format ".gitk-tmp.%s" [pid]]]
> >+	set gitktmpformat [file join $tmpdir ".gitk-tmp.XXXXXX"]
> >+	set gitktmpdir [exec mktemp -d $gitktmpformat]
> > 	if {[catch {file mkdir $gitktmpdir} err]} {
> > 	    error_popup "[mc "Error creating temporary directory %s:" $gitktmpdir] $err"
> > 	    unset gitktmpdir
> 
> This is a problem on Windows where we will not have mktemp. In Tcl 8.6
> the file command acquired a "file tempfile" command to help with this
> kind of issue (https://www.tcl.tk/man/tcl8.6/TclCmd/file.htm#M39) but
> for older versions we should probably stick with the existing pattern at
> least on Windows.

The existing pattern is a security bug on Unix systems. MITRE (CWE-377)
tells me that it is a vulnerability on Windows as well, so you'd
probably want to come up with a better solution than the existing
pattern.

You also probably want to request a CVE for this, which the Red Hat and
Debian security teams can do for you if you like.  Distributions will
likely want to issue security advisories for this.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories

[David Aguilar]

On Sun, Jun 15, 2014 at 04:32:27PM +0000, brian m. carlson wrote:
> On Sun, Jun 15, 2014 at 08:51:23AM +0100, Pat Thoyts wrote:
> > David Aguilar <davvid@gmail.com> writes:
> > >--- a/gitk
> > >+++ b/gitk
> > >@@ -3502,7 +3502,8 @@ proc gitknewtmpdir {} {
> > > 	} else {
> > > 	    set tmpdir $gitdir
> > > 	}
> > >-	set gitktmpdir [file join $tmpdir [format ".gitk-tmp.%s" [pid]]]
> > >+	set gitktmpformat [file join $tmpdir ".gitk-tmp.XXXXXX"]
> > >+	set gitktmpdir [exec mktemp -d $gitktmpformat]
> > > 	if {[catch {file mkdir $gitktmpdir} err]} {
> > > 	    error_popup "[mc "Error creating temporary directory %s:" $gitktmpdir] $err"
> > > 	    unset gitktmpdir
> > 
> > This is a problem on Windows where we will not have mktemp. In Tcl 8.6
> > the file command acquired a "file tempfile" command to help with this
> > kind of issue (https://www.tcl.tk/man/tcl8.6/TclCmd/file.htm#M39) but
> > for older versions we should probably stick with the existing pattern at
> > least on Windows.
> 
> The existing pattern is a security bug on Unix systems. MITRE (CWE-377)
> tells me that it is a vulnerability on Windows as well, so you'd
> probably want to come up with a better solution than the existing
> pattern.
> 
> You also probably want to request a CVE for this, which the Red Hat and
> Debian security teams can do for you if you like.  Distributions will
> likely want to issue security advisories for this.

I don't think this requires a CVE since it's basically plugging a hole
that my previous patch introduced by making gitk honor the TMPDIR
variable; it hasn't strictly been in any release yet.

Does Git on Windows use a modern tcl?
I checked, and my (old) existing msysgit installation had tcl
8.5, so I unfortunately using "file tempname" won't help there.

Hmm.. I guess what I could do is keep the old behavior (having gitk ignore TMPDIR)
on Windows and only use the new code path on non-Windows.

That seems like it'd be the simplest implementation (no need to check versions)
and the least harmful to existing users (avoids a tcl upgrade or mkdtemp installation
for Windows users).
-- 
David

Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

On Sun, Jun 15, 2014 at 02:49:29PM -0700, David Aguilar wrote:
> I don't think this requires a CVE since it's basically plugging a hole
> that my previous patch introduced by making gitk honor the TMPDIR
> variable; it hasn't strictly been in any release yet.

Yeah, that's not needed, then.  I didn't notice it was the immediately
previous patch.  My bad.

> Hmm.. I guess what I could do is keep the old behavior (having gitk
> ignore TMPDIR) on Windows and only use the new code path on
> non-Windows.
> 
> That seems like it'd be the simplest implementation (no need to check
> versions) and the least harmful to existing users (avoids a tcl
> upgrade or mkdtemp installation for Windows users).

Yeah, that would be the safest bet.  Maybe a comment to that effect
would be appropriate, so that when Tcl gets upgraded, that change can be
removed.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Hmm.. I guess what I could do is keep the old behavior (having gitk ignore TMPDIR)
> on Windows and only use the new code path on non-Windows.

Or perhaps attempt to create, catch error and then retry the old way?

Hopefully Windows folks do not have to worry about forgetting to
update the codepath when they update their tcl/wish if you did it
that way, no?

>
> That seems like it'd be the simplest implementation (no need to check versions)
> and the least harmful to existing users (avoids a tcl upgrade or mkdtemp installation
> for Windows users).

Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories

[David Aguilar]

On Mon, Jun 16, 2014 at 11:17:46AM -0700, Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
> 
> > Hmm.. I guess what I could do is keep the old behavior (having gitk ignore TMPDIR)
> > on Windows and only use the new code path on non-Windows.
> 
> Or perhaps attempt to create, catch error and then retry the old way?
> 
> Hopefully Windows folks do not have to worry about forgetting to
> update the codepath when they update their tcl/wish if you did it
> that way, no?

True, that would be the safest. I just submitted a new replacement patch
for these two patches.

Thanks,
-- 
David

Re: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories

[Thomas Braun]

Am 15.06.2014 09:51, schrieb Pat Thoyts:
> David Aguilar <davvid@gmail.com> writes:
> 
>> gitk uses a predictable ".gitk-tmp.$PID" pattern when generating
>> a temporary directory.
>>
>> Use "mktemp -d .gitk-tmp.XXXXXX" to harden gitk against someone
>> seeding /tmp with files matching the pid pattern.
>>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>> ---
>> This issue was brought up during the first review of the previous patch
>> back in 2009.
>>
>> http://thread.gmane.org/gmane.comp.version-control.git/132609/focus=132748
>>
>> This is really [PATCH 2/2] and should be applied on top of my previous
>> gitk patch.
>>
>> gitk | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/gitk b/gitk
>> index 82293dd..dd2ff63 100755
>> --- a/gitk
>> +++ b/gitk
>> @@ -3502,7 +3502,8 @@ proc gitknewtmpdir {} {
>> 	} else {
>> 	    set tmpdir $gitdir
>> 	}
>> -	set gitktmpdir [file join $tmpdir [format ".gitk-tmp.%s" [pid]]]
>> +	set gitktmpformat [file join $tmpdir ".gitk-tmp.XXXXXX"]
>> +	set gitktmpdir [exec mktemp -d $gitktmpformat]
>> 	if {[catch {file mkdir $gitktmpdir} err]} {
>> 	    error_popup "[mc "Error creating temporary directory %s:" $gitktmpdir] $err"
>> 	    unset gitktmpdir
> 
> This is a problem on Windows where we will not have mktemp. In Tcl 8.6
> the file command acquired a "file tempfile" command to help with this
> kind of issue (https://www.tcl.tk/man/tcl8.6/TclCmd/file.htm#M39) but
> for older versions we should probably stick with the existing pattern at
> least on Windows.

We could of course add mktemp from http://www.mktemp.org to msysgit.
I can do that if required.

In mingwgitDevEnv we already have the the need for mktemp, and a msys
package, so this is also not a problem.