gitk: sanitize 'exec' arguments: simple cases
Tcl 'exec' assigns special meaning to its argument when they begin with redirection, pipe or background operator. There are a number of invocations of 'exec' which construct arguments that are taken from the Git repository or a user input. However, when file names or ref names are taken from the repository, it is possible to find names with have these special forms. They must not be interpreted by 'exec' lest it redirects input or output, or attempts to build a pipeline using a command name controlled by the repository. Introduce a helper function that identifies such arguments and prepends "./" to force such a name to be regarded as a relative file name. Convert those 'exec' calls where the arguments can simply be packed into a list. Signed-off-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Johannes Sixt committed
Mar 17, 2025 at 22:59 UTC
9f0d1c2f7d2543cd2549cbc26e142fd199f18b45
1 file changed
+48
-18
gitk
+48
-18
@@ -10,7 +10,35 @@ exec wish "$0" -- "$@"
10
package require Tk
11
12
13
-# Wrap open to sanitize arguments
13
+# Wrap exec/open to sanitize arguments
14
+
15
+# unsafe arguments begin with redirections or the pipe or background operators
16
+proc is_arg_unsafe {arg} {
17
+ regexp {^([<|>&]|2>)} $arg
18
+}
19
+
20
+proc make_arg_safe {arg} {
21
+ if {[is_arg_unsafe $arg]} {
22
+ set arg [file join . $arg]
23
+ }
24
+ return $arg
25
+}
26
+
27
+proc make_arglist_safe {arglist} {
28
+ set res {}
29
+ foreach arg $arglist {
30
+ lappend res [make_arg_safe $arg]
31
+ }
32
+ return $res
33
+}
34
+
35
+# executes one command
36
+# no redirections or pipelines are possible
37
+# cmd is a list that specifies the command and its arguments
38
+# calls `exec` and returns its value
39
+proc safe_exec {cmd} {
40
+ eval exec [make_arglist_safe $cmd]
41
+}
42
43
proc safe_open_file {filename flags} {
44
# a file name starting with "|" would attempt to run a process
@@ -22,6 +50,8 @@ proc safe_open_file {filename flags} {
50
open $filename $flags
51
}
52
53
+# End exec/open wrappers
54
+
55
proc hasworktree {} {
56
return [expr {[exec git rev-parse --is-bare-repository] == "false" &&
57
[exec git rev-parse --is-inside-git-dir] == "false"}]
@@ -387,7 +417,7 @@ proc start_rev_list {view} {
417
set args $viewargs($view)
418
if {$viewargscmd($view) ne {}} {
419
if {[catch {
390
- set str [exec sh -c $viewargscmd($view)]
420
+ set str [safe_exec [list sh -c $viewargscmd($view)]]
421
} err]} {
422
error_popup "[mc "Error executing --argscmd command:"] $err"
423
return 0
@@ -459,9 +489,9 @@ proc stop_instance {inst} {
489
set pid [pid $fd]
490
491
if {$::tcl_platform(platform) eq {windows}} {
462
- exec taskkill /pid $pid
492
+ safe_exec [list taskkill /pid $pid]
493
} else {
464
- exec kill $pid
494
+ safe_exec [list kill $pid]
495
}
496
}
497
catch {close $fd}
@@ -1540,8 +1570,8 @@ proc getcommitlines {fd inst view updating} {
1570
# and if we already know about it, using the rewritten
1571
# parent as a substitute parent for $id's children.
1572
if {![catch {
1543
- set rwid [exec git rev-list --first-parent --max-count=1 \
1544
- $id -- $vfilelimit($view)]
1573
+ set rwid [safe_exec [list git rev-list --first-parent --max-count=1 \
1574
+ $id -- $vfilelimit($view)]]
1575
}]} {
1576
if {$rwid ne {} && [info exists varcid($view,$rwid)]} {
1577
# use $rwid in place of $id
@@ -1845,7 +1875,7 @@ proc readrefs {} {
1875
set selectheadid {}
1876
if {$selecthead ne {}} {
1877
catch {
1848
- set selectheadid [exec git rev-parse --verify $selecthead]
1878
+ set selectheadid [safe_exec [list git rev-parse --verify $selecthead]]
1879
}
1880
}
1881
}
@@ -3603,7 +3633,7 @@ proc gitknewtmpdir {} {
3633
set tmpdir $gitdir
3634
}
3635
set gitktmpformat [file join $tmpdir ".gitk-tmp.XXXXXX"]
3606
- if {[catch {set gitktmpdir [exec mktemp -d $gitktmpformat]}]} {
3636
+ if {[catch {set gitktmpdir [safe_exec [list mktemp -d $gitktmpformat]]}]} {
3637
set gitktmpdir [file join $gitdir [format ".gitk-tmp.%s" [pid]]]
3638
}
3639
if {[catch {file mkdir $gitktmpdir} err]} {
@@ -8774,7 +8804,7 @@ proc gotocommit {} {
8804
set id [lindex $matches 0]
8805
}
8806
} else {
8777
- if {[catch {set id [exec git rev-parse --verify $sha1string]}]} {
8807
+ if {[catch {set id [safe_exec [list git rev-parse --verify $sha1string]]}]} {
8808
error_popup [mc "Revision %s is not known" $sha1string]
8809
return
8810
}
@@ -9394,9 +9424,9 @@ proc domktag {} {
9424
}
9425
if {[catch {
9426
if {$msg != {}} {
9397
- exec git tag -a -m $msg $tag $id
9427
+ safe_exec [list git tag -a -m $msg $tag $id]
9428
} else {
9399
- exec git tag $tag $id
9429
+ safe_exec [list git tag $tag $id]
9430
}
9431
} err]} {
9432
error_popup "[mc "Error creating tag:"] $err" $mktagtop
@@ -9723,7 +9753,7 @@ proc cherrypick {} {
9753
update
9754
# Unfortunately git-cherry-pick writes stuff to stderr even when
9755
# no error occurs, and exec takes that as an indication of error...
9726
- if {[catch {exec sh -c "git cherry-pick -r $rowmenuid 2>&1"} err]} {
9756
+ if {[catch {safe_exec [list sh -c "git cherry-pick -r $rowmenuid 2>&1"]} err]} {
9757
notbusy cherrypick
9758
if {[regexp -line \
9759
{Entry '(.*)' (would be overwritten by merge|not uptodate)} \
@@ -9785,7 +9815,7 @@ proc revert {} {
9815
nowbusy revert [mc "Reverting"]
9816
update
9817
9788
- if [catch {exec git revert --no-edit $rowmenuid} err] {
9818
+ if [catch {safe_exec [list git revert --no-edit $rowmenuid]} err] {
9819
notbusy revert
9820
if [regexp {files would be overwritten by merge:(\n(( |\t)+[^\n]+\n)+)}\
9821
$err match files] {
@@ -10018,7 +10048,7 @@ proc rmbranch {} {
10048
}
10049
nowbusy rmbranch
10050
update
10021
- if {[catch {exec git branch -D $head} err]} {
10051
+ if {[catch {safe_exec [list git branch -D $head]} err]} {
10052
notbusy rmbranch
10053
error_popup $err
10054
return
@@ -11336,7 +11366,7 @@ proc add_tag_ctext {tag} {
11366
11367
if {![info exists cached_tagcontent($tag)]} {
11368
catch {
11339
- set cached_tagcontent($tag) [exec git cat-file -p $tag]
11369
+ set cached_tagcontent($tag) [safe_exec [list git cat-file -p $tag]]
11370
}
11371
}
11372
$ctext insert end "[mc "Tag"]: $tag\n" bold
@@ -12222,7 +12252,7 @@ proc gitattr {path attr default} {
12252
set r $path_attr_cache($attr,$path)
12253
} else {
12254
set r "unspecified"
12225
- if {![catch {set line [exec git check-attr $attr -- $path]}]} {
12255
+ if {![catch {set line [safe_exec [list git check-attr $attr -- $path]]}]} {
12256
regexp "(.*): $attr: (.*)" $line m f r
12257
}
12258
set path_attr_cache($attr,$path) $r
@@ -12302,11 +12332,11 @@ if {[catch {package require Tk 8.4} err]} {
12332
12333
# on OSX bring the current Wish process window to front
12334
if {[tk windowingsystem] eq "aqua"} {
12305
- exec osascript -e [format {
12335
+ safe_exec [list osascript -e [format {
12336
tell application "System Events"
12337
set frontmost of processes whose unix id is %d to true
12338
end tell
12309
- } [pid] ]
12339
+ } [pid] ]]
12340
}
12341
12342
# Unset GIT_TRACE var if set