[PATCH v3 0/6] Various fixes for v2.45.1 and friends

["Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>]

There have been a couple of issues that were reported about v2.45.1, and in
addition I have noticed some myself:

 * a memory leak in the clone protection logic
 * a missed adjustment in the Makefile that leads to an incorrect templates
   path in v2.39.4, v2.40.2 and v2.41.1 (but not in v2.42.2, ..., v2.45.1)
 * an overzealous core.hooksPath check
 * that Git LFS clone problem where it exits with an error (even if the
   clone often succeeded...)

This patch series is based on maint-2.39 to allow for (relatively) easy
follow-up versions v2.39.5, ..., v2.45.2.

Changes since v2:

 * instead of introducing an escape hatch for the clone protections and
   special-casing Git LFS, drop the clone protections

Changes since v1:

 * simplified adding the SHA-256s corresponding to Git LFS' hooks
 * the core.hooksPath test case now verifies that the config setting was
   configured correctly

Johannes Schindelin (6):
  hook: plug a new memory leak
  init: use the correct path of the templates directory again
  Revert "core.hooksPath: add some protection while cloning"
  tests: verify that `clone -c core.hooksPath=/dev/null` works again
  clone: drop the protections where hooks aren't run
  Revert "Add a helper function to compare file contents"

 Makefile                     |  2 +-
 builtin/clone.c              | 12 +-------
 cache.h                      | 14 ---------
 config.c                     | 13 +-------
 copy.c                       | 58 ------------------------------------
 hook.c                       | 32 --------------------
 t/helper/test-path-utils.c   | 10 -------
 t/t0060-path-utils.sh        | 41 -------------------------
 t/t1350-config-hooks-path.sh |  7 +++++
 t/t1800-hook.sh              | 15 ----------
 t/t5601-clone.sh             | 51 -------------------------------
 11 files changed, 10 insertions(+), 245 deletions(-)


base-commit: 47b6d90e91835082010da926f6a844d4441c57a6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1732%2Fdscho%2Fvarious-fixes-for-v2.45.1-and-friends-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1732/dscho/various-fixes-for-v2.45.1-and-friends-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1732

Range-diff vs v2:

 1:  d4a003bf2ce = 1:  d4a003bf2ce hook: plug a new memory leak
 2:  961dfc35f42 = 2:  961dfc35f42 init: use the correct path of the templates directory again
 3:  57db89a1497 = 3:  57db89a1497 Revert "core.hooksPath: add some protection while cloning"
 4:  cd14042b065 = 4:  cd14042b065 tests: verify that `clone -c core.hooksPath=/dev/null` works again
 5:  b841db8392e < -:  ----------- hook(clone protections): add escape hatch
 6:  5e5128bc232 < -:  ----------- hooks(clone protections): special-case current Git LFS hooks
 7:  bd6d72625f5 ! 5:  0044a355674 hooks(clone protections): simplify templates hooks validation
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    hooks(clone protections): simplify templates hooks validation
     +    clone: drop the protections where hooks aren't run
      
     -    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.
     +    As part of the security bug-fix releases v2.39.4, ..., v2.45.1, I
     +    introduced logic to safeguard `git clone` from running hooks that were
     +    installed _during_ the clone operation.
      
     -    When that logic was introduced, there was no other way to check this
     -    than to add a function to compare files.
     +    The rationale was that Git's CVE-2024-32002, CVE-2021-21300,
     +    CVE-2019-1354, CVE-2019-1353, CVE-2019-1352, and CVE-2019-1349 should
     +    have been low-severity vulnerabilities but were elevated to
     +    critical/high severity by the attack vector that allows a weakness where
     +    files inside `.git/` can be inadvertently written during a `git clone`
     +    to escalate to a Remote Code Execution attack by virtue of installing a
     +    malicious `post-checkout` hook that Git will then run at the end of the
     +    operation without giving the user a chance to see what code is executed.
      
     -    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.
     +    Unfortunately, Git LFS uses a similar strategy to install its own
     +    `post-checkout` hook during a `git clone`; In fact, Git LFS is
     +    installing four separate hooks while running the `smudge` filter.
      
     -    Let's simplify the logic by adding to said list when copying the
     -    templates' hooks.
     +    While this pattern is probably in want of being improved by introducing
     +    better support in Git for Git LFS and other tools wishing to register
     +    hooks to be run at various stages of Git's commands, let's undo the
     +    clone protections to unbreak Git LFS-enabled clones.
      
     -    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.
     +    This reverts commit 8db1e8743c0 (clone: prevent hooks from running
     +    during a clone, 2024-03-28).
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     - ## builtin/init-db.c ##
     -@@
     - #include "exec-cmd.h"
     - #include "parse-options.h"
     - #include "worktree.h"
     -+#include "run-command.h"
     -+#include "hook.h"
     + ## builtin/clone.c ##
     +@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     + 	int err = 0, complete_refs_before_fetch = 1;
     + 	int submodule_progress;
     + 	int filter_submodules = 0;
     +-	const char *template_dir;
     +-	char *template_dir_dup = NULL;
       
     - #ifdef NO_TRUSTABLE_FILEMODE
     - #define TEST_FILEMODE 0
     -@@ builtin/init-db.c: 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/");
     + 	struct transport_ls_refs_options transport_ls_refs_options =
     + 		TRANSPORT_LS_REFS_OPTIONS_INIT;
     +@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     + 		usage_msg_opt(_("You must specify a repository to clone."),
     + 			builtin_clone_usage, builtin_clone_options);
       
     - 	/* Note: if ".git/hooks" file exists in the repository being
     - 	 * re-initialized, /etc/core-git/templates/hooks/update would
     -@@ builtin/init-db.c: static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
     - 			strbuf_release(&lnk);
     +-	xsetenv("GIT_CLONE_PROTECTION_ACTIVE", "true", 0 /* allow user override */);
     +-	template_dir = get_template_dir(option_template);
     +-	if (*template_dir && !is_absolute_path(template_dir))
     +-		template_dir = template_dir_dup =
     +-			absolute_pathdup(template_dir);
     +-	xsetenv("GIT_CLONE_TEMPLATE_DIR", template_dir, 1);
     +-
     + 	if (option_depth || option_since || option_not.nr)
     + 		deepen = 1;
     + 	if (option_single_branch == -1)
     +@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       		}
     - 		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);
     + 	}
     + 
     +-	init_db(git_dir, real_git_dir, template_dir, GIT_HASH_UNKNOWN, NULL,
     ++	init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL,
     + 		INIT_DB_QUIET);
     + 
     + 	if (real_git_dir) {
     +@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     + 	free(unborn_head);
     + 	free(dir);
     + 	free(path);
     +-	free(template_dir_dup);
     + 	UNLEAK(repo);
     + 	junk_mode = JUNK_LEAVE_ALL;
     + 
      
       ## hook.c ##
      @@
     + #include "run-command.h"
       #include "config.h"
     - #include "strmap.h"
       
      -static int identical_to_template_hook(const char *name, const char *path)
      -{
     @@ hook.c
      -	return ret;
      -}
      -
     - static struct strset safe_hook_sha256s = STRSET_INIT;
     - static int safe_hook_sha256s_initialized;
     - 
     -@@ hook.c: 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)
     + const char *find_hook(const char *name)
       {
     - 	struct strset *set = d;
     + 	static struct strbuf path = STRBUF_INIT;
      @@ hook.c: 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"
     -
     - ## hook.h ##
     -@@ hook.h: 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
     -
     - ## setup.c ##
     -@@
     - #include "promisor-remote.h"
     - #include "quote.h"
     - #include "exec-cmd.h"
     -+#include "hook.h"
     +-	if (!git_hooks_path && git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0) &&
     +-	    !identical_to_template_hook(name, path.buf))
     +-		die(_("active `%s` hook found during `git clone`:\n\t%s\n"
     +-		      "For security reasons, this is disallowed by default.\n"
     +-		      "If this is intentional and the hook should actually "
     +-		      "be run, please\nrun the command again with "
     +-		      "`GIT_CLONE_PROTECTION_ACTIVE=false`"),
     +-		    name, path.buf);
     + 	return path.buf;
     + }
       
     - static int inside_git_dir = -1;
     - static int inside_work_tree = -1;
      
       ## t/t5601-clone.sh ##
     -@@ t/t5601-clone.sh: 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
     - 	)
     +@@ t/t5601-clone.sh: test_expect_success 'batch missing blob request does not inadvertently try to fe
     + 	git clone --filter=blob:limit=0 "file://$(pwd)/server" client
       '
       
     +-test_expect_success 'clone with init.templatedir runs hooks' '
     +-	git init tmpl/hooks &&
     +-	write_script tmpl/hooks/post-checkout <<-EOF &&
     +-	echo HOOK-RUN >&2
     +-	echo I was here >hook.run
     +-	EOF
     +-	git -C tmpl/hooks add . &&
     +-	test_tick &&
     +-	git -C tmpl/hooks commit -m post-checkout &&
     +-
     +-	test_when_finished "git config --global --unset init.templateDir || :" &&
     +-	test_when_finished "git config --unset init.templateDir || :" &&
     +-	(
     +-		sane_unset GIT_TEMPLATE_DIR &&
     +-		NO_SET_GIT_TEMPLATE_DIR=t &&
     +-		export NO_SET_GIT_TEMPLATE_DIR &&
     +-
     +-		git -c core.hooksPath="$(pwd)/tmpl/hooks" \
     +-			clone tmpl/hooks hook-run-hookspath 2>err &&
     +-		! grep "active .* hook found" err &&
     +-		test_path_is_file hook-run-hookspath/hook.run &&
     +-
     +-		git -c init.templateDir="$(pwd)/tmpl" \
     +-			clone tmpl/hooks hook-run-config 2>err &&
     +-		! grep "active .* hook found" err &&
     +-		test_path_is_file hook-run-config/hook.run &&
     +-
     +-		git clone --template=tmpl tmpl/hooks hook-run-option 2>err &&
     +-		! grep "active .* hook found" err &&
     +-		test_path_is_file hook-run-option/hook.run &&
     +-
     +-		git config --global init.templateDir "$(pwd)/tmpl" &&
     +-		git clone tmpl/hooks hook-run-global-config 2>err &&
     +-		git config --global --unset init.templateDir &&
     +-		! grep "active .* hook found" err &&
     +-		test_path_is_file hook-run-global-config/hook.run &&
     +-
     +-		# clone ignores local `init.templateDir`; need to create
     +-		# a new repository because we deleted `.git/` in the
     +-		# `setup` test case above
     +-		git init local-clone &&
     +-		cd local-clone &&
     +-
     +-		git config init.templateDir "$(pwd)/../tmpl" &&
     +-		git clone ../tmpl/hooks hook-run-local-config 2>err &&
     +-		git config --unset init.templateDir &&
     +-		! grep "active .* hook found" err &&
     +-		test_path_is_missing hook-run-local-config/hook.run
     +-	)
     +-'
     +-
     + . "$TEST_DIRECTORY"/lib-httpd.sh
     + start_httpd
     + 
 8:  4b0a636d41a = 6:  5c576e889d8 Revert "Add a helper function to compare file contents"

-- 
gitgitgadget