From e904deb89d9a9669a76a426182506a084d3f6308 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder 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 Signed-off-by: Jonathan Nieder Signed-off-by: Johannes Schindelin --- t/t7406-submodule-update.sh | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 't/t7406-submodule-update.sh') diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index 6f083c4d68..779932457a 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -406,12 +406,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 @@ -480,6 +480,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 && H=$(git ls-files -s submodule | cut -d" " -f2) && mkdir submodule1 && @@ -487,10 +490,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 ) ' -- cgit v1.2.3 From bb92255ebe6bccd76227e023d6d0bc997e318ad0 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Thu, 5 Dec 2019 01:30:43 -0800 Subject: fsck: reject submodule.update = !command in .gitmodules This allows hosting providers to detect whether they are being used to attack users using malicious 'update = !command' settings in .gitmodules. Since ac1fbbda2013 (submodule: do not copy unknown update mode from .gitmodules, 2013-12-02), in normal cases such settings have been treated as 'update = none', so forbidding them should not produce any collateral damage to legitimate uses. A quick search does not reveal any repositories making use of this construct, either. Reported-by: Joern Schneeweisz Signed-off-by: Jonathan Nieder Signed-off-by: Johannes Schindelin --- t/t7406-submodule-update.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 't/t7406-submodule-update.sh') diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index 779932457a..ceb5eed6e1 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -414,6 +414,20 @@ test_expect_success 'submodule update - command in .gitmodules is rejected' ' test_must_fail git -C super submodule update submodule ' +test_expect_success 'fsck detects command in .gitmodules' ' + git init command-in-gitmodules && + ( + cd command-in-gitmodules && + git submodule add ../submodule submodule && + test_commit adding-submodule && + + git config -f .gitmodules submodule.submodule.update "!false" && + git add .gitmodules && + test_commit configuring-update && + test_must_fail git fsck + ) +' + cat << EOF >expect Execution of 'false $submodulesha1' failed in submodule path 'submodule' EOF -- cgit v1.2.3