Re: [PATCH v8 9/9] for-each-repo: with bad config, don't conflate <path> and <cmd>

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Tue, Mar 28, 2023 at 04:04:28PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Fix a logic error in 4950b2a2b5c (for-each-repo: run subcommands on
> configured repos, 2020-09-11). Due to assuming that elements returned
> from the repo_config_get_value_multi() call wouldn't be "NULL" we'd
> conflate the <path> and <command> part of the argument list when
> running commands.
> 
> As noted in the preceding commit the fix is to move to a safer
> "*_string_multi()" version of the *_multi() API. This change is
> separated from the rest because those all segfaulted. In this change
> we ended up with different behavior.
> 
> When using the "--config=<config>" form we take each element of the
> list as a path to a repository. E.g. with a configuration like:
> 
> 	[repo] list = /some/repo
> 
> We would, with this command:
> 
> 	git for-each-repo --config=repo.list status builtin
> 
> Run a "git status" in /some/repo, as:
> 
> 	git -C /some/repo status builtin
> 
> I.e. ask "status" to report on the "builtin" directory. But since a
> configuration such as this would result in a "struct string_list *"
> with one element, whose "string" member is "NULL":
> 
> 	[repo] list
> 
> We would, when constructing our command-line in
> "builtin/for-each-repo.c"...
> 
> 	strvec_pushl(&child.args, "-C", path, NULL);
> 	for (i = 0; i < argc; i++)
> 		strvec_push(&child.args, argv[i]);
> 
> ...have that "path" be "NULL", and as strvec_pushl() stops when it
> sees NULL we'd end with the first "argv" element as the argument to
> the "-C" option, e.g.:
> 
> 	git -C status builtin
> 
> I.e. we'd run the command "builtin" in the "status" directory.
> 
> In another context this might be an interesting security
> vulnerability, but I think that this amounts to a nothingburger on
> that front.
> 
> A hypothetical attacker would need to be able to write config for the
> victim to run, if they're able to do that there's more interesting
> attack vectors. See the "safe.directory" facility added in
> 8d1a7448206 (setup.c: create `safe.bareRepository`, 2022-07-14).
> 
> An even more unlikely possibility would be an attacker able to
> generate the config used for "for-each-repo --config=<key>", but
> nothing else (e.g. an automated system producing that list).
> 
> Even in that case the attack vector is limited to the user running
> commands whose name matches a directory that's interesting to the
> attacker (e.g. a "log" directory in a repository). The second
> argument (if any) of the command is likely to make git die without
> doing anything interesting (e.g. "-p" to "log", there being no "-p"
> built-in command to run).
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/for-each-repo.c  |  2 +-
>  t/t0068-for-each-repo.sh | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
> index 224164addb3..ce8f7a99086 100644
> --- a/builtin/for-each-repo.c
> +++ b/builtin/for-each-repo.c
> @@ -46,7 +46,7 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
>  	if (!config_key)
>  		die(_("missing --config=<config>"));
>  
> -	err = repo_config_get_value_multi(the_repository, config_key, &values);
> +	err = repo_config_get_string_multi(the_repository, config_key, &values);
>  	if (err < 0)
>  		usage_msg_optf(_("got bad config --config=%s"),
>  			       for_each_repo_usage, options, config_key);
> diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh
> index 6b51e00da0e..4b90b74d5d5 100755
> --- a/t/t0068-for-each-repo.sh
> +++ b/t/t0068-for-each-repo.sh
> @@ -46,4 +46,17 @@ test_expect_success 'error on bad config keys' '
>  	test_expect_code 129 git for-each-repo --config="'\''.b"
>  '
>  
> +test_expect_success 'error on NULL value for config keys' '
> +	cat >>.git/config <<-\EOF &&
> +	[empty]
> +		key
> +	EOF
> +	cat >expect <<-\EOF &&
> +	error: missing value for '\''empty.key'\''
> +	EOF
> +	test_expect_code 129 git for-each-repo --config=empty.key 2>actual.raw &&
> +	grep ^error actual.raw >actual &&
> +	test_cmp expect actual
> +'

In this case the full error message looks like this:

  $ ./git -c empty.key for-each-repo --config=empty.key
  error: missing value for 'empty.key'
  fatal: got bad config --config=empty.key

  usage: git for-each-repo --config=<config> [--] <arguments>

      --config <config>     config key storing a list of repository paths

Having both an "error:" and a "fatal:" message seems redundant.


On a related note, according to the usage shown above (and the
synopsis in the man page), 'git for-each-repo' expects mandatory
<arguments>, but this doesn't seem to be enforced, and invoking it
without any arguments results in the usage of the main git command:

  $ ./git -c empty.key=. for-each-repo --config=empty.key
  usage: git [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]
             [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
             [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]
             [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
             [--config-env=<name>=<envvar>] <command> [<args>]
  
  These are common Git commands used in various situations:
  
  start a working area (see also: git help tutorial)
  [...]

This is misleading, because without any hints as to what was wrong I
thought that the problem is with the options of the main git command,
not with the (lack of) arguments of the 'for-each-repo' command.