| From 8dca1221839cf6054fb05d63d2049869600ed672 Mon Sep 17 00:00:00 2001 |
| From: Jonathan Nieder <jrnieder@gmail.com> |
| Date: Thu, 5 Dec 2019 01:28:28 -0800 |
| Subject: submodule: reject submodule.update = !command in .gitmodules |
| |
| Since ac1fbbda2013 (submodule: do not copy unknown update mode from |
| .gitmodules, 2013-12-02), Git has been careful to avoid copying |
| |
| [submodule "foo"] |
| update = !run an arbitrary scary command |
| |
| from .gitmodules to a repository's local config, copying in the |
| setting 'update = none' instead. The gitmodules(5) manpage documents |
| the intention: |
| |
| The !command form is intentionally ignored here for security |
| reasons |
| |
| Unfortunately, starting with v2.20.0-rc0 (which integrated ee69b2a9 |
| (submodule--helper: introduce new update-module-mode helper, |
| 2018-08-13, first released in v2.20.0-rc0)), there are scenarios where |
| we *don't* ignore it: if the config store contains no |
| submodule.foo.update setting, the submodule-config API falls back to |
| reading .gitmodules and the repository-supplied !command gets run |
| after all. |
| |
| This was part of a general change over time in submodule support to |
| read more directly from .gitmodules, since unlike .git/config it |
| allows a project to change values between branches and over time |
| (while still allowing .git/config to override things). But it was |
| never intended to apply to this kind of dangerous configuration. |
| |
| The behavior change was not advertised in ee69b2a9's commit message |
| and was missed in review. |
| |
| Let's take the opportunity to make the protection more robust, even in |
| Git versions that are technically not affected: instead of quietly |
| converting 'update = !command' to 'update = none', noisily treat it as |
| an error. Allowing the setting but treating it as meaning something |
| else was just confusing; users are better served by seeing the error |
| sooner. Forbidding the construct makes the semantics simpler and |
| means we can check for it in fsck (in a separate patch). |
| |
| As a result, the submodule-config API cannot read this value from |
| .gitmodules under any circumstance, and we can declare with confidence |
| |
| For security reasons, the '!command' form is not accepted |
| here. |
| |
| Reported-by: Joern Schneeweisz <jschneeweisz@gitlab.com> |
| Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> |
| Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> |
| (cherry picked from commit e904deb89d9a9669a76a426182506a084d3f6308) |
| Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> |
| --- |
| Documentation/gitmodules.txt | 5 ++--- |
| submodule-config.c | 12 ++++++++++-- |
| t/t7406-submodule-update.sh | 14 ++++++++------ |
| 3 files changed, 20 insertions(+), 11 deletions(-) |
| |
| diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt |
| index 312b6f9259..164995d1af 100644 |
| --- a/Documentation/gitmodules.txt |
| +++ b/Documentation/gitmodules.txt |
| @@ -44,9 +44,8 @@ submodule.<name>.update:: |
| submodule init` to initialize the configuration variable of |
| the same name. Allowed values here are 'checkout', 'rebase', |
| 'merge' or 'none'. See description of 'update' command in |
| - linkgit:git-submodule[1] for their meaning. Note that the |
| - '!command' form is intentionally ignored here for security |
| - reasons. |
| + linkgit:git-submodule[1] for their meaning. For security |
| + reasons, the '!command' form is not accepted here. |
| |
| submodule.<name>.branch:: |
| A remote branch name for tracking updates in the upstream submodule. |
| diff --git a/submodule-config.c b/submodule-config.c |
| index 52702c62d9..8c00855ed8 100644 |
| --- a/submodule-config.c |
| +++ b/submodule-config.c |
| @@ -398,6 +398,13 @@ struct parse_config_parameter { |
| int overwrite; |
| }; |
| |
| +/* |
| + * Parse a config item from .gitmodules. |
| + * |
| + * This does not handle submodule-related configuration from the main |
| + * config store (.git/config, etc). Callers are responsible for |
| + * checking for overrides in the main config store when appropriate. |
| + */ |
| static int parse_config(const char *var, const char *value, void *data) |
| { |
| struct parse_config_parameter *me = data; |
| @@ -475,8 +482,9 @@ static int parse_config(const char *var, const char *value, void *data) |
| warn_multiple_config(me->treeish_name, submodule->name, |
| "update"); |
| else if (parse_submodule_update_strategy(value, |
| - &submodule->update_strategy) < 0) |
| - die(_("invalid value for %s"), var); |
| + &submodule->update_strategy) < 0 || |
| + submodule->update_strategy.type == SM_UPDATE_COMMAND) |
| + die(_("invalid value for %s"), var); |
| } else if (!strcmp(item.buf, "shallow")) { |
| if (!me->overwrite && submodule->recommend_shallow != -1) |
| warn_multiple_config(me->treeish_name, submodule->name, |
| diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh |
| index e87164aa8f..778352e313 100755 |
| --- a/t/t7406-submodule-update.sh |
| +++ b/t/t7406-submodule-update.sh |
| @@ -407,12 +407,12 @@ test_expect_success 'submodule update - command in .git/config' ' |
| ) |
| ' |
| |
| -test_expect_success 'submodule update - command in .gitmodules is ignored' ' |
| +test_expect_success 'submodule update - command in .gitmodules is rejected' ' |
| test_when_finished "git -C super reset --hard HEAD^" && |
| git -C super config -f .gitmodules submodule.submodule.update "!false" && |
| git -C super commit -a -m "add command to .gitmodules file" && |
| git -C super/submodule reset --hard $submodulesha1^ && |
| - git -C super submodule update submodule |
| + test_must_fail git -C super submodule update submodule |
| ' |
| |
| cat << EOF >expect |
| @@ -481,6 +481,9 @@ test_expect_success 'recursive submodule update - command in .git/config catches |
| ' |
| |
| test_expect_success 'submodule init does not copy command into .git/config' ' |
| + test_when_finished "git -C super update-index --force-remove submodule1" && |
| + test_when_finished git config -f super/.gitmodules \ |
| + --remove-section submodule.submodule1 && |
| (cd super && |
| git ls-files -s submodule >out && |
| H=$(cut -d" " -f2 out) && |
| @@ -489,10 +492,9 @@ test_expect_success 'submodule init does not copy command into .git/config' ' |
| git config -f .gitmodules submodule.submodule1.path submodule1 && |
| git config -f .gitmodules submodule.submodule1.url ../submodule && |
| git config -f .gitmodules submodule.submodule1.update !false && |
| - git submodule init submodule1 && |
| - echo "none" >expect && |
| - git config submodule.submodule1.update >actual && |
| - test_cmp expect actual |
| + test_must_fail git submodule init submodule1 && |
| + test_expect_code 1 git config submodule.submodule1.update >actual && |
| + test_must_be_empty actual |
| ) |
| ' |
| |
| -- |
| 2.24.0.393.g34dc348eaf |
| |