[PATCH v3 0/6] Various fixes for v2.45.1 and friends
- From
Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
- Date
- May 20, 2024, 20:21 UTC
- Message-ID
- <pull.1732.v3.git.1716236526.gitgitgadget@gmail.com>
- In-Reply-To
- <pull.1732.v2.git.1716028366.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