| Message ID | 20250331151213.274691-1-mlevedahl@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | gitk - override $PATH search only on Windows | expand |
Am 31.03.25 um 17:12 schrieb Mark Levedahl: > Commit 4cbe9e0e2 was written to address problems that result from Tcl's > documented behavior on Windows where the current working directory and a > number of Windows system directories are automatically prepended to > $PATH when searching for executables [1]. This basic Windows behavior > has resulted in more than one CVE against git for Windows: > CVE-2023-23618, CVE-2022-41953 are listed on the git for Windows github > website for the Tcl components of git (gitk, git-gui). > > 4cbe9e0e2 is intended to restrict the search to looking only in > directories given in $PATH and in the given order, which is exactly the > Tcl behavior documented to exist on non-Windows platforms [1]. Thus, > this change could have been written to affect only Windows, leaving > other platforms alone. > > However, 4cbe9e0e2 implements the override for all platforms. and > includes specialized code for Cygwin, copied copied from git-gui prior > to commit 6d2f9d90 on https://github.com/j6t/git-gui.git), so targets a I can't find 6d2f9d90 anywhere. Do you have a URL? > long retired Cygwin port of the Windows Tcl/Tk using Windows pathnames. > Since 2012, Cygwin uses a Unix/X11 port requiring Unix pathnames, > meaning 4cbe9e0e2 is incompatible. The patch also induces an infinite > recursion as _which now invokes the exec wrapper that invokes _which. > As this is part of git v2.49.0, gitk on Cygwin is broken in that > release. > > Rather than fix the unnecessary override code for Cygwin, let's just > limit the override of exec/open to Windows, leaving all other platforms > using their native exec/open as they did prior to 4cbe9e0e2. This patch > wraps the override code in an "if {[is_Windows]} { ... }" block while > removing the non-Windows code added in 4cbe9e0e2. > > [1] see https://www.tcl-lang.org/man/tcl8.6/TclCmd/exec.htm Thanks, nicely summarized. > > Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> > --- > gitk | 146 ++++++++++++++++++++++++----------------------------------- > 1 file changed, 58 insertions(+), 88 deletions(-) > > diff --git a/gitk b/gitk > index bc9efa1..a101b07 100755 > --- a/gitk > +++ b/gitk > @@ -13,13 +13,6 @@ package require Tk > ## > ## Enabling platform-specific code paths > > -proc is_MacOSX {} { > - if {[tk windowingsystem] eq {aqua}} { > - return 1 > - } > - return 0 > -} > - > proc is_Windows {} { > if {$::tcl_platform(platform) eq {windows}} { > return 1 > @@ -27,36 +20,16 @@ proc is_Windows {} { > return 0 > } > > -set _iscygwin {} > -proc is_Cygwin {} { > - global _iscygwin > - if {$_iscygwin eq {}} { > - if {[string match "CYGWIN_*" $::tcl_platform(os)]} { > - set _iscygwin 1 > - } else { > - set _iscygwin 0 > - } > - } > - return $_iscygwin > -} > - > ###################################################################### > ## > ## PATH lookup > > -set _search_path {} > -proc _which {what args} { > - global env _search_exe _search_path > - > - if {$_search_path eq {}} { > - if {[is_Cygwin] && [regexp {^(/|\.:)} $env(PATH)]} { > - set _search_path [split [exec cygpath \ > - --windows \ > - --path \ > - --absolute \ > - $env(PATH)] {;}] > - set _search_exe .exe > - } elseif {[is_Windows]} { > +if {[is_Windows]} { > + set _search_path {} > + proc _which {what args} { > + global env _search_exe _search_path > + > + if {$_search_path eq {}} { > set gitguidir [file dirname [info script]] > regsub -all ";" $gitguidir "\\;" gitguidir > set env(PATH) "$gitguidir;$env(PATH)" > @@ -65,81 +38,78 @@ proc _which {what args} { > set _search_path [lsearch -all -inline -not -exact \ > $_search_path ""] > set _search_exe .exe > - } else { > - set _search_path [split $env(PATH) :] > - set _search_exe {} > } > - } > > - if {[is_Windows] && [lsearch -exact $args -script] >= 0} { > - set suffix {} > - } else { > - set suffix $_search_exe > - } > + if {[lsearch -exact $args -script] >= 0} { > + set suffix {} > + } else { > + set suffix $_search_exe > + } Now that this code is only about Windows, _search_exe is always ".exe". It would be great if we could remove it as well. > > - foreach p $_search_path { > - set p [file join $p $what$suffix] > - if {[file exists $p]} { > - return [file normalize $p] > + foreach p $_search_path { > + set p [file join $p $what$suffix] > + if {[file exists $p]} { > + return [file normalize $p] > + } > } > + return {} > } > - return {} > -} > > -proc sanitize_command_line {command_line from_index} { > - set i $from_index > - while {$i < [llength $command_line]} { > - set cmd [lindex $command_line $i] > - if {[file pathtype $cmd] ne "absolute"} { > - set fullpath [_which $cmd] > - if {$fullpath eq ""} { > - throw {NOT-FOUND} "$cmd not found in PATH" > + proc sanitize_command_line {command_line from_index} { > + set i $from_index > + while {$i < [llength $command_line]} { > + set cmd [lindex $command_line $i] > + if {[file pathtype $cmd] ne "absolute"} { > + set fullpath [_which $cmd] > + if {$fullpath eq ""} { > + throw {NOT-FOUND} "$cmd not found in PATH" > + } > + lset command_line $i $fullpath > } > - lset command_line $i $fullpath > - } > > - # handle piped commands, e.g. `exec A | B` > - for {incr i} {$i < [llength $command_line]} {incr i} { > - if {[lindex $command_line $i] eq "|"} { > - incr i > - break > + # handle piped commands, e.g. `exec A | B` > + for {incr i} {$i < [llength $command_line]} {incr i} { > + if {[lindex $command_line $i] eq "|"} { > + incr i > + break > + } > } > } > + return $command_line > } > - return $command_line > -} > > -# Override `exec` to avoid unsafe PATH lookup > + # Override `exec` to avoid unsafe PATH lookup > > -rename exec real_exec > + rename exec real_exec > > -proc exec {args} { > - # skip options > - for {set i 0} {$i < [llength $args]} {incr i} { > - set arg [lindex $args $i] > - if {$arg eq "--"} { > - incr i > - break > - } > - if {[string range $arg 0 0] ne "-"} { > - break > + proc exec {args} { > + # skip options > + for {set i 0} {$i < [llength $args]} {incr i} { > + set arg [lindex $args $i] > + if {$arg eq "--"} { > + incr i > + break > + } > + if {[string range $arg 0 0] ne "-"} { > + break > + } > } > + set args [sanitize_command_line $args $i] > + uplevel 1 real_exec $args > } > - set args [sanitize_command_line $args $i] > - uplevel 1 real_exec $args > -} > > -# Override `open` to avoid unsafe PATH lookup > + # Override `open` to avoid unsafe PATH lookup > > -rename open real_open > + rename open real_open > > -proc open {args} { > - set arg0 [lindex $args 0] > - if {[string range $arg0 0 0] eq "|"} { > - set command_line [string trim [string range $arg0 1 end]] > - lset args 0 "| [sanitize_command_line $command_line 0]" > + proc open {args} { > + set arg0 [lindex $args 0] > + if {[string range $arg0 0 0] eq "|"} { > + set command_line [string trim [string range $arg0 1 end]] > + lset args 0 "| [sanitize_command_line $command_line 0]" > + } > + uplevel 1 real_open $args > } > - uplevel 1 real_open $args > } > > # End of safe PATH lookup stuff -- Hannes
On 3/31/25 1:12 PM, Johannes Sixt wrote: > Am 31.03.25 um 17:12 schrieb Mark Levedahl: >> Commit 4cbe9e0e2 was written to address problems that result from Tcl's >> documented behavior on Windows where the current working directory and a >> number of Windows system directories are automatically prepended to >> $PATH when searching for executables [1]. This basic Windows behavior >> has resulted in more than one CVE against git for Windows: >> CVE-2023-23618, CVE-2022-41953 are listed on the git for Windows github >> website for the Tcl components of git (gitk, git-gui). >> >> 4cbe9e0e2 is intended to restrict the search to looking only in >> directories given in $PATH and in the given order, which is exactly the >> Tcl behavior documented to exist on non-Windows platforms [1]. Thus, >> this change could have been written to affect only Windows, leaving >> other platforms alone. >> >> However, 4cbe9e0e2 implements the override for all platforms. and >> includes specialized code for Cygwin, copied copied from git-gui prior >> to commit 6d2f9d90 on https://github.com/j6t/git-gui.git), so targets a > I can't find 6d2f9d90 anywhere. Do you have a URL? Sorry about that (bad copy / paste). Should be 7145c654 https://github.com/j6t/git-gui/commit/7145c654fffecd1f3d4a2b8bf05755ce262903e8 > Now that this code is only about Windows, _search_exe is always ".exe". > It would be great if we could remove it as well. > Will do for v2. Mark
diff --git a/gitk b/gitk index bc9efa1..a101b07 100755 --- a/gitk +++ b/gitk @@ -13,13 +13,6 @@ package require Tk ## ## Enabling platform-specific code paths -proc is_MacOSX {} { - if {[tk windowingsystem] eq {aqua}} { - return 1 - } - return 0 -} - proc is_Windows {} { if {$::tcl_platform(platform) eq {windows}} { return 1 @@ -27,36 +20,16 @@ proc is_Windows {} { return 0 } -set _iscygwin {} -proc is_Cygwin {} { - global _iscygwin - if {$_iscygwin eq {}} { - if {[string match "CYGWIN_*" $::tcl_platform(os)]} { - set _iscygwin 1 - } else { - set _iscygwin 0 - } - } - return $_iscygwin -} - ###################################################################### ## ## PATH lookup -set _search_path {} -proc _which {what args} { - global env _search_exe _search_path - - if {$_search_path eq {}} { - if {[is_Cygwin] && [regexp {^(/|\.:)} $env(PATH)]} { - set _search_path [split [exec cygpath \ - --windows \ - --path \ - --absolute \ - $env(PATH)] {;}] - set _search_exe .exe - } elseif {[is_Windows]} { +if {[is_Windows]} { + set _search_path {} + proc _which {what args} { + global env _search_exe _search_path + + if {$_search_path eq {}} { set gitguidir [file dirname [info script]] regsub -all ";" $gitguidir "\\;" gitguidir set env(PATH) "$gitguidir;$env(PATH)" @@ -65,81 +38,78 @@ proc _which {what args} { set _search_path [lsearch -all -inline -not -exact \ $_search_path ""] set _search_exe .exe - } else { - set _search_path [split $env(PATH) :] - set _search_exe {} } - } - if {[is_Windows] && [lsearch -exact $args -script] >= 0} { - set suffix {} - } else { - set suffix $_search_exe - } + if {[lsearch -exact $args -script] >= 0} { + set suffix {} + } else { + set suffix $_search_exe + } - foreach p $_search_path { - set p [file join $p $what$suffix] - if {[file exists $p]} { - return [file normalize $p] + foreach p $_search_path { + set p [file join $p $what$suffix] + if {[file exists $p]} { + return [file normalize $p] + } } + return {} } - return {} -} -proc sanitize_command_line {command_line from_index} { - set i $from_index - while {$i < [llength $command_line]} { - set cmd [lindex $command_line $i] - if {[file pathtype $cmd] ne "absolute"} { - set fullpath [_which $cmd] - if {$fullpath eq ""} { - throw {NOT-FOUND} "$cmd not found in PATH" + proc sanitize_command_line {command_line from_index} { + set i $from_index + while {$i < [llength $command_line]} { + set cmd [lindex $command_line $i] + if {[file pathtype $cmd] ne "absolute"} { + set fullpath [_which $cmd] + if {$fullpath eq ""} { + throw {NOT-FOUND} "$cmd not found in PATH" + } + lset command_line $i $fullpath } - lset command_line $i $fullpath - } - # handle piped commands, e.g. `exec A | B` - for {incr i} {$i < [llength $command_line]} {incr i} { - if {[lindex $command_line $i] eq "|"} { - incr i - break + # handle piped commands, e.g. `exec A | B` + for {incr i} {$i < [llength $command_line]} {incr i} { + if {[lindex $command_line $i] eq "|"} { + incr i + break + } } } + return $command_line } - return $command_line -} -# Override `exec` to avoid unsafe PATH lookup + # Override `exec` to avoid unsafe PATH lookup -rename exec real_exec + rename exec real_exec -proc exec {args} { - # skip options - for {set i 0} {$i < [llength $args]} {incr i} { - set arg [lindex $args $i] - if {$arg eq "--"} { - incr i - break - } - if {[string range $arg 0 0] ne "-"} { - break + proc exec {args} { + # skip options + for {set i 0} {$i < [llength $args]} {incr i} { + set arg [lindex $args $i] + if {$arg eq "--"} { + incr i + break + } + if {[string range $arg 0 0] ne "-"} { + break + } } + set args [sanitize_command_line $args $i] + uplevel 1 real_exec $args } - set args [sanitize_command_line $args $i] - uplevel 1 real_exec $args -} -# Override `open` to avoid unsafe PATH lookup + # Override `open` to avoid unsafe PATH lookup -rename open real_open + rename open real_open -proc open {args} { - set arg0 [lindex $args 0] - if {[string range $arg0 0 0] eq "|"} { - set command_line [string trim [string range $arg0 1 end]] - lset args 0 "| [sanitize_command_line $command_line 0]" + proc open {args} { + set arg0 [lindex $args 0] + if {[string range $arg0 0 0] eq "|"} { + set command_line [string trim [string range $arg0 1 end]] + lset args 0 "| [sanitize_command_line $command_line 0]" + } + uplevel 1 real_open $args } - uplevel 1 real_open $args } # End of safe PATH lookup stuff
Commit 4cbe9e0e2 was written to address problems that result from Tcl's documented behavior on Windows where the current working directory and a number of Windows system directories are automatically prepended to $PATH when searching for executables [1]. This basic Windows behavior has resulted in more than one CVE against git for Windows: CVE-2023-23618, CVE-2022-41953 are listed on the git for Windows github website for the Tcl components of git (gitk, git-gui). 4cbe9e0e2 is intended to restrict the search to looking only in directories given in $PATH and in the given order, which is exactly the Tcl behavior documented to exist on non-Windows platforms [1]. Thus, this change could have been written to affect only Windows, leaving other platforms alone. However, 4cbe9e0e2 implements the override for all platforms. and includes specialized code for Cygwin, copied copied from git-gui prior to commit 6d2f9d90 on https://github.com/j6t/git-gui.git), so targets a long retired Cygwin port of the Windows Tcl/Tk using Windows pathnames. Since 2012, Cygwin uses a Unix/X11 port requiring Unix pathnames, meaning 4cbe9e0e2 is incompatible. The patch also induces an infinite recursion as _which now invokes the exec wrapper that invokes _which. As this is part of git v2.49.0, gitk on Cygwin is broken in that release. Rather than fix the unnecessary override code for Cygwin, let's just limit the override of exec/open to Windows, leaving all other platforms using their native exec/open as they did prior to 4cbe9e0e2. This patch wraps the override code in an "if {[is_Windows]} { ... }" block while removing the non-Windows code added in 4cbe9e0e2. [1] see https://www.tcl-lang.org/man/tcl8.6/TclCmd/exec.htm Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> --- gitk | 146 ++++++++++++++++++++++++----------------------------------- 1 file changed, 58 insertions(+), 88 deletions(-)