File 0011-submodule-reject-submodule.update-command-... of Package git.9802

Overview Repositories Revisions Requests Users Attributes Meta

File 0011-submodule-reject-submodule.update-command-in-.gitmod.patch of Package git.9802

From df906b1fcf4c66354031088027e1aa74cb18ceef Mon Sep 17 00:00:00 2001
From: Jonathan Nieder <jrnieder@gmail.com>
Date: Thu, 5 Dec 2019 01:28:28 -0800
Subject: [PATCH 11/28] 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>
---
 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 db5d47eb19..ac44a1510c 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 3c40f1f1c5..33c1b388e5 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -396,6 +396,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;
@@ -473,8 +480,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 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
 	)
 '
 
-- 
2.24.0

Places

File 0011-submodule-reject-submodule.update-command-in-.gitmod.patch of Package git.9802

Places