Re: [PATCH] gitk: add user-defined custom commands

On Fri, 2026-08-07 at 15:16 -0700, Junio C Hamano wrote:
> "Tim Wiederhake via GitGitGadget" <[email protected]> writes:
> 
> > +    set len [string length $cmd_template]
> > +    for {set i 0} {$i < $len} {incr i} {
> > +        if {[string index $cmd_template $i] eq "%" && $i + 1 <
> > $len} {
> > +            set next [string index $cmd_template [expr {$i + 1}]]
> > +            if {!$blame_computed && ($next eq "b" || $next eq
> > "l")} {
> > +                set blame [get_blame_origin]
> > +                set blame_id [lindex $blame 0]
> > +                set blame_line [lindex $blame 1]
> > +                set blame_computed 1
> > +            }
> > +            switch -- $next {
> > +                "%" { append cmd "%" }
> > +                "i" { append cmd $id }
> > +                "t" { append cmd [lindex $commitinfo($id) 0] }
> > +                "a" { append cmd [lindex $commitinfo($id) 1] }
> > +                "d" { append cmd [lindex $commitinfo($id) 2] }
> > +                "c" { append cmd [lindex $commitinfo($id) 3] }
> > +                "D" { append cmd [lindex $commitinfo($id) 4] }
> > +                "m" { append cmd [lindex $commitinfo($id) 5] }
> > +                "M" { if {[info exists markedid]} { append cmd
> > $markedid } }
> > +                "b" { append cmd $blame_id }
> > +                "f" { append cmd [get_diff_file] }
> > +                "l" { append cmd $blame_line }
> > +                default { append cmd "%" $next }
> > +            }
> > +            incr i
> > +        } else {
> > +            append cmd [string index $cmd_template $i]
> > +        }
> > +    }
> > +
> > +    if {[catch {exec sh -c $cmd 2>@1} output]} {
> 
> What do various members of $commitinfo field have?  I presume that
> title and message are pretty much free text under control of anybody
> who can write to the repository and entice you to run this command,
> so running with "sh -c $cmd" would require $cmd to be quoting the
> payload properly, or you'd be opening yourself to be an arbitrary
> command execution, no?  With template "echo '%t'" you thought you
> are just printing the title but if the title has "title?'; echo no'"
> in
> it, wouldn't cmd end up being 
> 
> 	echo 'title?'; echo no''
> 
> and a more creative type can use something other than "echo no", to
> have a process run under your name and do more interesting things,
> right?
> 
> Note that I no longer speak Tcl (even though I admit I used to), so
> if there is some "magic" that makes use of $cmd in {exec sh -c $cmd}
> safe, the above may be missing the mark by a mile.

You are right, the code is vulnerable to shell injection. The first
version was calling the command directly, so no escaping was necessary.

I added the "sh -c" to facilitate process forking ("&") and simple exit
code manipulation ("command && exit 42"). But by now I honestly am not
sure anymore on what the best solution is: calling the command directly
and have the user write a wrapper script if necessary; or add code to
properly escape all data read from commits and retain the convenience.

Opinions?

Tim