[v2,7/8] hooks(clone protections): simplify templates hooks validation

Commit Message

Johannes Schindelin May 18, 2024, 10:32 a.m. UTC

From: Johannes Schindelin <[e-mail]>

When an active hook is encountered during a clone operation, to protect
against Remote Code Execution attack vectors, Git checks whether the
hook was copied over from the templates directory.

When that logic was introduced, there was no other way to check this
than to add a function to compare files.

In the meantime, we've added code to compute the SHA-256 checksum of a
given hook and compare that checksum against a list of known-safe ones.

Let's simplify the logic by adding to said list when copying the
templates' hooks.

We need to be careful to support multi-process operations such as
recursive submodule clones: In such a scenario, the list of SHA-256
checksums that is kept in memory is not enough, we also have to pass the
information down to child processes via `GIT_CONFIG_PARAMETERS`.

Extend the regression test in t5601 to ensure that recursive clones are
handled as expected.

Note: Technically there is no way that the checksums computed while
initializing the submodules' gitdirs can be passed to the process that
performs the checkout: For historical reasons, these operations are
performed in processes spawned in separate loops from the
super-project's `git clone` process. But since the templates from which
the submodules are initialized are the very same as the ones from which
the super-project is initialized, we can get away with using the list of
SHA-256 checksums that is computed when initializing the super-project
and passing that down to the `submodule--helper` processes that perform
the recursive checkout.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 builtin/init-db.c |  7 +++++++
 hook.c            | 43 ++++++++++++++++---------------------------
 hook.h            | 10 ++++++++++
 setup.c           |  1 +
 t/t5601-clone.sh  | 19 +++++++++++++++++++
 5 files changed, 53 insertions(+), 27 deletions(-)

Patch

diff --git a/builtin/init-db.c b/builtin/init-db.c
index a101e7f94c1..64357fdada4 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -10,6 +10,8 @@ 
 #include "exec-cmd.h"
 #include "parse-options.h"
 #include "worktree.h"
+#include "run-command.h"
+#include "hook.h"
 
 #ifdef NO_TRUSTABLE_FILEMODE
 #define TEST_FILEMODE 0
@@ -28,6 +30,7 @@  static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 	size_t path_baselen = path->len;
 	size_t template_baselen = template_path->len;
 	struct dirent *de;
+	int is_hooks_dir = ends_with(template_path->buf, "/hooks/");
 
 	/* Note: if ".git/hooks" file exists in the repository being
 	 * re-initialized, /etc/core-git/templates/hooks/update would
@@ -80,6 +83,10 @@  static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 			strbuf_release(&lnk);
 		}
 		else if (S_ISREG(st_template.st_mode)) {
+			if (is_hooks_dir &&
+			    is_executable(template_path->buf))
+				add_safe_hook(template_path->buf);
+
 			if (copy_file(path->buf, template_path->buf, st_template.st_mode))
 				die_errno(_("cannot copy '%s' to '%s'"),
 					  template_path->buf, path->buf);
diff --git a/hook.c b/hook.c
index f81b13df142..9e762cc9af6 100644
--- a/hook.c
+++ b/hook.c
@@ -4,32 +4,6 @@ 
 #include "config.h"
 #include "strmap.h"
 
-static int identical_to_template_hook(const char *name, const char *path)
-{
-	const char *env = getenv("GIT_CLONE_TEMPLATE_DIR");
-	const char *template_dir = get_template_dir(env && *env ? env : NULL);
-	struct strbuf template_path = STRBUF_INIT;
-	int found_template_hook, ret;
-
-	strbuf_addf(&template_path, "%s/hooks/%s", template_dir, name);
-	found_template_hook = access(template_path.buf, X_OK) >= 0;
-#ifdef STRIP_EXTENSION
-	if (!found_template_hook) {
-		strbuf_addstr(&template_path, STRIP_EXTENSION);
-		found_template_hook = access(template_path.buf, X_OK) >= 0;
-	}
-#endif
-	if (!found_template_hook) {
-		strbuf_release(&template_path);
-		return 0;
-	}
-
-	ret = do_files_match(template_path.buf, path);
-
-	strbuf_release(&template_path);
-	return ret;
-}
-
 static struct strset safe_hook_sha256s = STRSET_INIT;
 static int safe_hook_sha256s_initialized;
 
@@ -60,6 +34,22 @@  static int get_sha256_of_file_contents(const char *path, char *sha256)
 	return 0;
 }
 
+void add_safe_hook(const char *path)
+{
+	char sha256[GIT_SHA256_HEXSZ + 1] = { '\0' };
+
+	if (!get_sha256_of_file_contents(path, sha256)) {
+		char *p;
+
+		strset_add(&safe_hook_sha256s, sha256);
+
+		/* support multi-process operations e.g. recursive clones */
+		p = xstrfmt("safe.hook.sha256=%s", sha256);
+		git_config_push_parameter(p);
+		free(p);
+	}
+}
+
 static int safe_hook_cb(const char *key, const char *value, void *d)
 {
 	struct strset *set = d;
@@ -131,7 +121,6 @@  const char *find_hook(const char *name)
 		return NULL;
 	}
 	if (!git_hooks_path && git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0) &&
-	    !identical_to_template_hook(name, path.buf) &&
 	    !is_hook_safe_during_clone(name, path.buf, sha256))
 		die(_("active `%s` hook found during `git clone`:\n\t%s\n"
 		      "For security reasons, this is disallowed by default.\n"
diff --git a/hook.h b/hook.h
index 4258b13da0d..e2034ee8b23 100644
--- a/hook.h
+++ b/hook.h
@@ -82,4 +82,14 @@  int run_hooks(const char *hook_name);
  * hook. This function behaves like the old run_hook_le() API.
  */
 int run_hooks_l(const char *hook_name, ...);
+
+/**
+ * Mark the contents of the provided path as safe to run during a clone
+ * operation.
+ *
+ * This function is mainly used when copying templates to mark the
+ * just-copied hooks as benign.
+ */
+void add_safe_hook(const char *path);
+
 #endif
diff --git a/setup.c b/setup.c
index c3301f5ab82..7f7538c9bf7 100644
--- a/setup.c
+++ b/setup.c
@@ -7,6 +7,7 @@ 
 #include "promisor-remote.h"
 #include "quote.h"
 #include "exec-cmd.h"
+#include "hook.h"
 
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 20deca0231b..71eaa3d1e14 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -819,6 +819,25 @@  test_expect_success 'clone with init.templatedir runs hooks' '
 		git config --unset init.templateDir &&
 		! grep "active .* hook found" err &&
 		test_path_is_missing hook-run-local-config/hook.run
+	) &&
+
+	test_config_global protocol.file.allow always &&
+	git -C tmpl/hooks submodule add "$(pwd)/tmpl/hooks" sub &&
+	test_tick &&
+	git -C tmpl/hooks add .gitmodules sub &&
+	git -C tmpl/hooks commit -m submodule &&
+
+	(
+		sane_unset GIT_TEMPLATE_DIR &&
+		NO_SET_GIT_TEMPLATE_DIR=t &&
+		export NO_SET_GIT_TEMPLATE_DIR &&
+
+		git -c init.templateDir="$(pwd)/tmpl" \
+			clone --recurse-submodules \
+			tmpl/hooks hook-run-submodule 2>err &&
+		! grep "active .* hook found" err &&
+		test_path_is_file hook-run-submodule/hook.run &&
+		test_path_is_file hook-run-submodule/sub/hook.run
 	)
 '