On Mon, Jul 15, 2024 at 07:52:12PM -0400, Jeff King wrote:
> > We may be able to teach credential.c:credential_do() not to paste
> > the operation verb to the command line so early. Instead you could
> > teach the function to send the command line and operation verb
> > separately down to run_credential_helper() though. That way, we
> > might be able to avoid the shell in this particular case. That is,
> > if we can
> >
> > * Have start_command() -> prepare_cmd() -> prepare_shell_cmd()
> > codepath to take the usual route _without_ the operation verb
> > tucked to the command line, we would get cmd->args.v[] that does
> > not rely on the shell;
> >
> > * Then before the prepared command is executed, if we can somehow
> > _append_ to cmd->args.v[] the operation verb (after all, that
> > wants to become the argv[1] to the spawned command) before
> > start_command() exec's it
> >
> > then we are done.
>
> Yes, I think this is reasonable. You'd also perhaps want to have it set
> child->git_cmd as appropriate (though really, I do not think that does
> anything except stick "git" into child.args[0], so we could just do that
> ourselves).
>
> I'm actually a little surprised it was not written this way in the first
> place. In the non-!, non-absolute-path case we are pasting together a
> string that will be passed to the shell, and it includes the "helper"
> argument without further quoting. I don't think you could smuggle a
> semicolon into there (due to our protocol restrictions), but it does
> seem like a possible shell injection route.
>
> I think it probably goes all the way back to my abca927dbe (introduce
> credentials API, 2011-12-10).
Ah, having tried to refactor it, I see now why it is written as it is.
Even for a regular helper without "!", it is important that we construct
a string and pass it to the shell, since it is legal (and even
encouraged) to do things like:
[credential]
helper = cache --socket=/path/to/socket --timeout=123
Arguably we could have gotten away with word-splitting ourselves,
sticking the result in child_process.args, and avoided the shell. But
the use of the shell is documented in gitcredentials(7):
helper
The name of an external credential helper, and any associated
options. If the helper name is not an absolute path, then the string
git credential- is prepended. The resulting string is executed by
the shell (so, for example, setting this to foo --option=bar will
execute git credential-foo --option=bar via the shell. See the
manual of specific helpers for examples of their use.
So users may be depending on that to do "--socket=$HOME/.foo", or even
more exotic shell constructs.
Again, it's possible that we could detect that no shell metacharacters
are in play and do the word-splitting ourselves. But at that point I
think it should go into run-command's prepare_shell_cmd(). That is, I I
think it could take space out of the list of metachars that force us to
invoke the shell, and do the word-splitting there. But not having
thought very hard about it, there are probably corner cases where that
optimization is detectable by the user (presumably unusual IFS, but
maybe more?).
-Peff
