Re: Can dependency on /bin/sh be removed?

From: Jeff King <peff@peff.net>
Date: Jul 16, 2024, 19:23 UTC
Message-ID: <20240716192307.GA12536@coredump.intra.peff.net>
In-Reply-To: <20240715235212.GA628996@coredump.intra.peff.net>

On Mon, Jul 15, 2024 at 07:52:12PM -0400, Jeff King wrote:

Show 33 quoted lines

> > 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