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@xxxxxxxxx>
> ---
> 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.
