From f3e5bdfebc6be6e56700c3988d726d6c85d50660 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 May 2024 12:23:05 -0400 Subject: [PATCH 01/14] ci: drop mention of BREW_INSTALL_PACKAGES variable The last user of this variable went away in 4a6e4b9602 (CI: remove Travis CI support, 2021-11-23), so it's doing nothing except making it more confusing to find out which packages _are_ installed. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ci/install-dependencies.sh | 2 -- 1 file changed, 2 deletions(-) diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 4f407530d30575..33039d516b8d17 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -34,8 +34,6 @@ macos-*) export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1 # Uncomment this if you want to run perf tests: # brew install gnu-time - test -z "$BREW_INSTALL_PACKAGES" || - brew install $BREW_INSTALL_PACKAGES brew link --force gettext mkdir -p $HOME/bin ( From 2aef8020d2ac6115d424c8db265d9f6d9e475bf0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 May 2024 12:24:15 -0400 Subject: [PATCH 02/14] ci: avoid bare "gcc" for osx-gcc job On macOS, a bare "gcc" (without a version) will invoke a wrapper for clang, not actual gcc. Even when gcc is installed via homebrew, that only provides version-specific links in /usr/local/bin (like "gcc-13"), and never a version-agnostic "gcc" wrapper. As far as I can tell, this has been the case for a long time, and this osx-gcc job has largely been doing nothing. We can point it at "gcc-13", which will pick up the homebrew-installed version. The fix here is specific to the github workflow file, as the gitlab one does not have a matching job. It's a little unfortunate that we cannot just ask for the latest version of gcc which homebrew provides, but as far as I can tell there is no easy alias (you'd have to find the highest number gcc-* in /usr/local/bin yourself). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 2dc0221f7f587b..583e7cd5f077ee 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -264,7 +264,7 @@ jobs: cc: clang pool: macos-13 - jobname: osx-gcc - cc: gcc + cc: gcc-13 cc_package: gcc-13 pool: macos-13 - jobname: linux-gcc-default From d4543be3f2941a6a00c0d5d33ee14475164aa259 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 May 2024 12:25:44 -0400 Subject: [PATCH 03/14] ci: stop installing "gcc-13" for osx-gcc Our osx-gcc job explicitly asks to install gcc-13. But since the GitHub runner image already comes with gcc-13 installed, this is mostly doing nothing (or in some cases it may install an incremental update over the runner image). But worse, it recently started causing errors like: ==> Fetching gcc@13 ==> Downloading https://ghcr.io/v2/homebrew/core/gcc/13/blobs/sha256:fb2403d97e2ce67eb441b54557cfb61980830f3ba26d4c5a1fe5ecd0c9730d1a ==> Pouring gcc@13--13.2.0.ventura.bottle.tar.gz Error: The `brew link` step did not complete successfully The formula built, but is not symlinked into /usr/local Could not symlink bin/c++-13 Target /usr/local/bin/c++-13 is a symlink belonging to gcc. You can unlink it: brew unlink gcc which cause the whole CI job to bail. I didn't track down the root cause, but I suspect it may be related to homebrew recently switching the "gcc" default to gcc-14. And it may even be fixed when a new runner image is released. But if we don't need to run brew at all, it's one less thing for us to worry about. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- .github/workflows/main.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 583e7cd5f077ee..76e3f1e76894cb 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -265,7 +265,6 @@ jobs: pool: macos-13 - jobname: osx-gcc cc: gcc-13 - cc_package: gcc-13 pool: macos-13 - jobname: linux-gcc-default cc: gcc From d4a003bf2ceafcc6d47d01d21b7faff48c9e85aa Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 14 May 2024 14:53:31 +0200 Subject: [PATCH 04/14] hook: plug a new memory leak In 8db1e8743c0 (clone: prevent hooks from running during a clone, 2024-03-28), I introduced an inadvertent memory leak that was unfortunately not caught before v2.45.1 was released. Here is a fix. Signed-off-by: Johannes Schindelin --- hook.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hook.c b/hook.c index 632b537b9933ad..fc974cee1d842f 100644 --- a/hook.c +++ b/hook.c @@ -18,8 +18,10 @@ static int identical_to_template_hook(const char *name, const char *path) found_template_hook = access(template_path.buf, X_OK) >= 0; } #endif - if (!found_template_hook) + if (!found_template_hook) { + strbuf_release(&template_path); return 0; + } ret = do_files_match(template_path.buf, path); From 961dfc35f426388d660cca4e92f43e169819886a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 16 May 2024 13:59:32 +0200 Subject: [PATCH 05/14] init: use the correct path of the templates directory again In df93e407f06 (init: refactor the template directory discovery into its own function, 2024-03-29), I refactored the way the templates directory is discovered. The refactoring was faithful, but missed a reference in the `Makefile` where the `DEFAULT_GIT_TEMPLATE_DIR` constant is defined. As a consequence, Git v2.45.1 and friends will always use the hard-coded path `/usr/share/git-core/templates`. Let's fix that by defining the `DEFAULT_GIT_TEMPLATE_DIR` when building `setup.o`, where that constant is actually used. Signed-off-by: Johannes Schindelin --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 093829ae2832aa..4b1502ba2c6960 100644 --- a/Makefile +++ b/Makefile @@ -2751,7 +2751,7 @@ exec-cmd.sp exec-cmd.s exec-cmd.o: EXTRA_CPPFLAGS = \ '-DFALLBACK_RUNTIME_PREFIX="$(prefix_SQ)"' builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX -builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \ +setup.sp setup.s setup.o: EXTRA_CPPFLAGS = \ -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"' config.sp config.s config.o: GIT-PREFIX From 57db89a14977bdff01f8f82cb4d6f85cc49d4b55 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 17 May 2024 15:12:16 +0200 Subject: [PATCH 06/14] Revert "core.hooksPath: add some protection while cloning" This defense-in-depth was intended to protect the clone operation against future escalations where bugs in `git clone` would allow attackers to write arbitrary files in the `.git/` directory would allow for Remote Code Execution attacks via maliciously-placed hooks. However, it turns out that the `core.hooksPath` protection has unintentional side effects so severe that they do not justify the benefit of the protections. For example, it has been reported in https://lore.kernel.org/git/FAFA34CB-9732-4A0A-87FB-BDB272E6AEE8@alchemists.io/ that the following invocation, which is intended to make `git clone` safer, is itself broken by that protective measure: git clone --config core.hooksPath=/dev/null Since it turns out that the benefit does not justify the cost, let's revert 20f3588efc6 (core.hooksPath: add some protection while cloning, 2024-03-30). Signed-off-by: Johannes Schindelin --- config.c | 13 +------------ t/t1800-hook.sh | 15 --------------- 2 files changed, 1 insertion(+), 27 deletions(-) diff --git a/config.c b/config.c index 85b37f2ee09d0a..8c1c4071f0d1a7 100644 --- a/config.c +++ b/config.c @@ -1525,19 +1525,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb) if (!strcmp(var, "core.attributesfile")) return git_config_pathname(&git_attributes_file, var, value); - if (!strcmp(var, "core.hookspath")) { - if (current_config_scope() == CONFIG_SCOPE_LOCAL && - git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0)) - die(_("active `core.hooksPath` found in the local " - "repository config:\n\t%s\nFor security " - "reasons, this is disallowed by default.\nIf " - "this is intentional and the hook should " - "actually be run, please\nrun the command " - "again with " - "`GIT_CLONE_PROTECTION_ACTIVE=false`"), - value); + if (!strcmp(var, "core.hookspath")) return git_config_pathname(&git_hooks_path, var, value); - } if (!strcmp(var, "core.bare")) { is_bare_repository_cfg = git_config_bool(var, value); diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 7ee12e6f48afab..2ef3579fa7c23d 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -177,19 +177,4 @@ test_expect_success 'git hook run a hook with a bad shebang' ' test_cmp expect actual ' -test_expect_success 'clone protections' ' - test_config core.hooksPath "$(pwd)/my-hooks" && - mkdir -p my-hooks && - write_script my-hooks/test-hook <<-\EOF && - echo Hook ran $1 - EOF - - git hook run test-hook 2>err && - grep "Hook ran" err && - test_must_fail env GIT_CLONE_PROTECTION_ACTIVE=true \ - git hook run test-hook 2>err && - grep "active .core.hooksPath" err && - ! grep "Hook ran" err -' - test_done From cd14042b065e57f132c27d09005242fc500439e4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 16 May 2024 14:14:58 +0200 Subject: [PATCH 07/14] tests: verify that `clone -c core.hooksPath=/dev/null` works again As part of the protections added in Git v2.45.1 and friends, repository-local `core.hooksPath` settings are no longer allowed, as a defense-in-depth mechanism to prevent future Git vulnerabilities to raise to critical level if those vulnerabilities inadvertently allow the repository-local config to be written. What the added protection did not anticipate is that such a repository-local `core.hooksPath` can not only be used to point to maliciously-placed scripts in the current worktree, but also to _prevent_ hooks from being called altogether. We just reverted the `core.hooksPath` protections, based on the Git maintainer's recommendation in https://lore.kernel.org/git/xmqq4jaxvm8z.fsf@gitster.g/ to address this concern as well as related ones. Let's make sure that we won't regress while trying to protect the clone operation further. Reported-by: Brooke Kuhlmann Signed-off-by: Johannes Schindelin --- t/t1350-config-hooks-path.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh index f6dc83e2aabf69..45a04929170640 100755 --- a/t/t1350-config-hooks-path.sh +++ b/t/t1350-config-hooks-path.sh @@ -41,4 +41,11 @@ test_expect_success 'git rev-parse --git-path hooks' ' test .git/custom-hooks/abc = "$(cat actual)" ' +test_expect_success 'core.hooksPath=/dev/null' ' + git clone -c core.hooksPath=/dev/null . no-templates && + value="$(git -C no-templates config --local core.hooksPath)" && + # The Bash used by Git for Windows rewrites `/dev/null` to `nul` + { test /dev/null = "$value" || test nul = "$value"; } +' + test_done From b841db8392ebd924d1893829a7e5e22240f1e9cf Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 14 May 2024 12:39:20 +0200 Subject: [PATCH 08/14] hook(clone protections): add escape hatch As defense-in-depth measures, v2.39.4 and friends leading up to v2.45.1 introduced code that detects when hooks have been installed during a `git clone`, which is indicative of a common attack vector with critical severity that allows Remote Code Execution. There are legitimate use cases for such behavior, though, for example when those hooks stem from Git's own templates, which system administrators are at liberty to modify to enforce, say, commit message conventions. The git clone protections specifically add exceptions to allow for that. Another legitimate use case that has been identified too late to be handled in these security bug-fix versions is Git LFS: It behaves somewhat similar to common attack vectors by writing a few hooks while running the `smudge` filter during a regular clone, which means that Git has no chance to know that the hooks are benign and e.g. the `post-checkout` hook can be safely executed as part of the clone operation. To help Git LFS, and other tools behaving similarly (if there are any), let's add a new, multi-valued `safe.hook.sha256` config setting. Like the already-existing `safe.*` settings, it is ignored in repository-local configs, and it is interpreted as a list of SHA-256 checksums of hooks' contents that are safe to execute during a clone operation. Future Git LFS versions will need to write those entries at the same time they install the `smudge`/`clean` filters. Signed-off-by: Johannes Schindelin --- Documentation/config/safe.txt | 6 ++++ hook.c | 66 ++++++++++++++++++++++++++++++++--- t/t1800-hook.sh | 15 ++++++++ 3 files changed, 82 insertions(+), 5 deletions(-) diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt index bde7f31459b981..69ee845be89e4a 100644 --- a/Documentation/config/safe.txt +++ b/Documentation/config/safe.txt @@ -59,3 +59,9 @@ which id the original user has. If that is not what you would prefer and want git to only trust repositories that are owned by root instead, then you can remove the `SUDO_UID` variable from root's environment before invoking git. + +safe.hook.sha256:: + The value is the SHA-256 of hooks that are considered to be safe + to run during a clone operation. ++ +Multiple values can be added via `git config --global --add`. diff --git a/hook.c b/hook.c index fc974cee1d842f..a2479738451fba 100644 --- a/hook.c +++ b/hook.c @@ -2,6 +2,7 @@ #include "hook.h" #include "run-command.h" #include "config.h" +#include "strmap.h" static int identical_to_template_hook(const char *name, const char *path) { @@ -29,11 +30,65 @@ static int identical_to_template_hook(const char *name, const char *path) return ret; } +static struct strset safe_hook_sha256s = STRSET_INIT; +static int safe_hook_sha256s_initialized; + +static int get_sha256_of_file_contents(const char *path, char *sha256) +{ + struct strbuf sb = STRBUF_INIT; + int fd; + ssize_t res; + + git_hash_ctx ctx; + const struct git_hash_algo *algo = &hash_algos[GIT_HASH_SHA256]; + unsigned char hash[GIT_MAX_RAWSZ]; + + if ((fd = open(path, O_RDONLY)) < 0) + return -1; + res = strbuf_read(&sb, fd, 400); + close(fd); + if (res < 0) + return -1; + + algo->init_fn(&ctx); + algo->update_fn(&ctx, sb.buf, sb.len); + strbuf_release(&sb); + algo->final_fn(hash, &ctx); + + hash_to_hex_algop_r(sha256, hash, algo); + + return 0; +} + +static int safe_hook_cb(const char *key, const char *value, void *d) +{ + struct strset *set = d; + + if (value && !strcmp(key, "safe.hook.sha256")) + strset_add(set, value); + + return 0; +} + +static int is_hook_safe_during_clone(const char *name, const char *path, char *sha256) +{ + if (get_sha256_of_file_contents(path, sha256) < 0) + return 0; + + if (!safe_hook_sha256s_initialized) { + safe_hook_sha256s_initialized = 1; + git_protected_config(safe_hook_cb, &safe_hook_sha256s); + } + + return strset_contains(&safe_hook_sha256s, sha256); +} + const char *find_hook(const char *name) { static struct strbuf path = STRBUF_INIT; int found_hook; + char sha256[GIT_SHA256_HEXSZ + 1] = { '\0' }; strbuf_reset(&path); strbuf_git_path(&path, "hooks/%s", name); @@ -65,13 +120,14 @@ 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)) + !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" - "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); + "If this is intentional and the hook is safe to run, " + "please run the following command and try again:\n\n" + " git config --global --add safe.hook.sha256 %s"), + name, path.buf, sha256); return path.buf; } diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 2ef3579fa7c23d..0f74c9154d0590 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -177,4 +177,19 @@ test_expect_success 'git hook run a hook with a bad shebang' ' test_cmp expect actual ' +test_expect_success '`safe.hook.sha256` and clone protections' ' + git init safe-hook && + write_script safe-hook/.git/hooks/pre-push <<-\EOF && + echo "called hook" >safe-hook.log + EOF + + test_must_fail env GIT_CLONE_PROTECTION_ACTIVE=true \ + git -C safe-hook hook run pre-push 2>err && + cmd="$(grep "git config --global --add safe.hook.sha256 [0-9a-f]" err)" && + eval "$cmd" && + GIT_CLONE_PROTECTION_ACTIVE=true \ + git -C safe-hook hook run pre-push && + test "called hook" = "$(cat safe-hook/safe-hook.log)" +' + test_done From 5e5128bc232fbb822925efde20395484354492b6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 17 May 2024 22:58:29 +0200 Subject: [PATCH 09/14] hooks(clone protections): special-case current Git LFS hooks A notable regression in v2.45.1 and friends (all the way down to v2.39.4) has been that Git LFS-enabled clones error out with a message indicating that the `post-checkout` hook has been tampered with while cloning, and as a safety measure it is not executed. A generic fix for benign third-party applications wishing to write hooks during clone operations has been implemented in the parent of this commit: said applications are expected to add `safe.hook.sha256` values to a protected config. However, the current version of Git LFS, v3.5.1, cannot be adapted retroactively; Therefore, let's just hard-code the SHA-256 values for this version. That way, Git LFS usage will no longer be broken, and the next Git LFS version can be taught to add those `safe.hook.sha256` entries. Signed-off-by: Johannes Schindelin --- hook.c | 11 +++++++++++ t/t1800-hook.sh | 20 ++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/hook.c b/hook.c index a2479738451fba..f81b13df142f1a 100644 --- a/hook.c +++ b/hook.c @@ -77,6 +77,17 @@ static int is_hook_safe_during_clone(const char *name, const char *path, char *s if (!safe_hook_sha256s_initialized) { safe_hook_sha256s_initialized = 1; + + /* Hard-code known-safe values for Git LFS v3.4.0..v3.5.1 */ + /* pre-push */ + strset_add(&safe_hook_sha256s, "df5417b2daa3aa144c19681d1e997df7ebfe144fb7e3e05138bd80ae998008e4"); + /* post-checkout */ + strset_add(&safe_hook_sha256s, "791471b4ff472aab844a4fceaa48bbb0a12193616f971e8e940625498b4938a6"); + /* post-commit */ + strset_add(&safe_hook_sha256s, "21e961572bb3f43a5f2fbafc1cc764d86046cc2e5f0bbecebfe9684a0b73b664"); + /* post-merge */ + strset_add(&safe_hook_sha256s, "75da0da66a803b4b030ad50801ba57062c6196105eb1d2251590d100edb9390b"); + git_protected_config(safe_hook_cb, &safe_hook_sha256s); } diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 0f74c9154d0590..af66999aff3d40 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -192,4 +192,24 @@ test_expect_success '`safe.hook.sha256` and clone protections' ' test "called hook" = "$(cat safe-hook/safe-hook.log)" ' +write_lfs_pre_push_hook () { + write_script "$1" <<-\EOF + command -v git-lfs >/dev/null 2>&1 || { echo >&2 "\nThis repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks').\n"; exit 2; } + git lfs pre-push "$@" + EOF +} + +test_expect_success 'Git LFS special-handling in clone protections' ' + git init lfs-hooks && + write_lfs_pre_push_hook lfs-hooks/.git/hooks/pre-push && + write_script git-lfs <<-\EOF && + echo "called $*" >fake-git-lfs.log + EOF + + PATH="$PWD:$PATH" GIT_CLONE_PROTECTION_ACTIVE=true \ + git -C lfs-hooks hook run pre-push && + test_write_lines "called pre-push" >expect && + test_cmp lfs-hooks/fake-git-lfs.log expect +' + test_done From bd6d72625f53e5983256da0ed7fe1a6431b3cfdf Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 17 May 2024 23:48:41 +0200 Subject: [PATCH 10/14] hooks(clone protections): simplify templates hooks validation 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 --- 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(-) diff --git a/builtin/init-db.c b/builtin/init-db.c index a101e7f94c119b..64357fdada4126 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 f81b13df142f1a..9e762cc9af6371 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 4258b13da0d7c3..e2034ee8b233a9 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 c3301f5ab824b5..7f7538c9bf7688 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 20deca0231b8bf..71eaa3d1e14068 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 ) ' From 4b0a636d41a52513d57380589147a808840b4ad8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 18 May 2024 00:03:40 +0200 Subject: [PATCH 11/14] Revert "Add a helper function to compare file contents" Now that during a `git clone`, the hooks' contents are no longer compared to the templates' files', the caller for which the `do_files_match()` function was introduced is gone, and therefore this function can be retired, too. This reverts commit 584de0b4c23 (Add a helper function to compare file contents, 2024-03-30). Signed-off-by: Johannes Schindelin --- cache.h | 14 --------- copy.c | 58 -------------------------------------- t/helper/test-path-utils.c | 10 ------- t/t0060-path-utils.sh | 41 --------------------------- 4 files changed, 123 deletions(-) diff --git a/cache.h b/cache.h index 16b34799bfd48c..8c5fb1e1ba14f6 100644 --- a/cache.h +++ b/cache.h @@ -1785,20 +1785,6 @@ int copy_fd(int ifd, int ofd); int copy_file(const char *dst, const char *src, int mode); int copy_file_with_time(const char *dst, const char *src, int mode); -/* - * Compare the file mode and contents of two given files. - * - * If both files are actually symbolic links, the function returns 1 if the link - * targets are identical or 0 if they are not. - * - * If any of the two files cannot be accessed or in case of read failures, this - * function returns 0. - * - * If the file modes and contents are identical, the function returns 1, - * otherwise it returns 0. - */ -int do_files_match(const char *path1, const char *path2); - void write_or_die(int fd, const void *buf, size_t count); void fsync_or_die(int fd, const char *); int fsync_component(enum fsync_component component, int fd); diff --git a/copy.c b/copy.c index 8492f6fc8319af..4de6a110f0912d 100644 --- a/copy.c +++ b/copy.c @@ -65,61 +65,3 @@ int copy_file_with_time(const char *dst, const char *src, int mode) return copy_times(dst, src); return status; } - -static int do_symlinks_match(const char *path1, const char *path2) -{ - struct strbuf buf1 = STRBUF_INIT, buf2 = STRBUF_INIT; - int ret = 0; - - if (!strbuf_readlink(&buf1, path1, 0) && - !strbuf_readlink(&buf2, path2, 0)) - ret = !strcmp(buf1.buf, buf2.buf); - - strbuf_release(&buf1); - strbuf_release(&buf2); - return ret; -} - -int do_files_match(const char *path1, const char *path2) -{ - struct stat st1, st2; - int fd1 = -1, fd2 = -1, ret = 1; - char buf1[8192], buf2[8192]; - - if ((fd1 = open_nofollow(path1, O_RDONLY)) < 0 || - fstat(fd1, &st1) || !S_ISREG(st1.st_mode)) { - if (fd1 < 0 && errno == ELOOP) - /* maybe this is a symbolic link? */ - return do_symlinks_match(path1, path2); - ret = 0; - } else if ((fd2 = open_nofollow(path2, O_RDONLY)) < 0 || - fstat(fd2, &st2) || !S_ISREG(st2.st_mode)) { - ret = 0; - } - - if (ret) - /* to match, neither must be executable, or both */ - ret = !(st1.st_mode & 0111) == !(st2.st_mode & 0111); - - if (ret) - ret = st1.st_size == st2.st_size; - - while (ret) { - ssize_t len1 = read_in_full(fd1, buf1, sizeof(buf1)); - ssize_t len2 = read_in_full(fd2, buf2, sizeof(buf2)); - - if (len1 < 0 || len2 < 0 || len1 != len2) - ret = 0; /* read error or different file size */ - else if (!len1) /* len2 is also 0; hit EOF on both */ - break; /* ret is still true */ - else - ret = !memcmp(buf1, buf2, len1); - } - - if (fd1 >= 0) - close(fd1); - if (fd2 >= 0) - close(fd2); - - return ret; -} diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c index 0e0de2180767d9..f69709d674fe18 100644 --- a/t/helper/test-path-utils.c +++ b/t/helper/test-path-utils.c @@ -495,16 +495,6 @@ int cmd__path_utils(int argc, const char **argv) return !!res; } - if (argc == 4 && !strcmp(argv[1], "do_files_match")) { - int ret = do_files_match(argv[2], argv[3]); - - if (ret) - printf("equal\n"); - else - printf("different\n"); - return !ret; - } - fprintf(stderr, "%s: unknown function name: %s\n", argv[0], argv[1] ? argv[1] : "(there was none)"); return 1; diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 73d0e1a7f1060c..68e29c904a62c9 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -560,45 +560,4 @@ test_expect_success !VALGRIND,RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' test_cmp expect actual ' -test_expect_success 'do_files_match()' ' - test_seq 0 10 >0-10.txt && - test_seq -1 10 >-1-10.txt && - test_seq 1 10 >1-10.txt && - test_seq 1 9 >1-9.txt && - test_seq 0 8 >0-8.txt && - - test-tool path-utils do_files_match 0-10.txt 0-10.txt >out && - - assert_fails() { - test_must_fail \ - test-tool path-utils do_files_match "$1" "$2" >out && - grep different out - } && - - assert_fails 0-8.txt 1-9.txt && - assert_fails -1-10.txt 0-10.txt && - assert_fails 1-10.txt 1-9.txt && - assert_fails 1-10.txt .git && - assert_fails does-not-exist 1-10.txt && - - if test_have_prereq FILEMODE - then - cp 0-10.txt 0-10.x && - chmod a+x 0-10.x && - assert_fails 0-10.txt 0-10.x - fi && - - if test_have_prereq SYMLINKS - then - ln -sf 0-10.txt symlink && - ln -s 0-10.txt another-symlink && - ln -s over-the-ocean yet-another-symlink && - ln -s "$PWD/0-10.txt" absolute-symlink && - assert_fails 0-10.txt symlink && - test-tool path-utils do_files_match symlink another-symlink && - assert_fails symlink yet-another-symlink && - assert_fails symlink absolute-symlink - fi -' - test_done From c0eacef1b84620e16b4218acb7478224eb942d63 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 18 May 2024 11:35:31 +0200 Subject: [PATCH 12/14] Git 2.39.5 Signed-off-by: Johannes Schindelin --- Documentation/RelNotes/2.39.5.txt | 26 ++++++++++++++++++++++++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.39.5.txt diff --git a/Documentation/RelNotes/2.39.5.txt b/Documentation/RelNotes/2.39.5.txt new file mode 100644 index 00000000000000..519ba4ef3f97c7 --- /dev/null +++ b/Documentation/RelNotes/2.39.5.txt @@ -0,0 +1,26 @@ +Git 2.39.5 Release Notes +======================== + +Relative to Git 2.39.5, this release has fixes for regressions that +were introduced in 2.39.4, most notably the error message shown when +cloning Git LFS-enabled repositories. It also contains a fix for the +`osx-gcc` CI job. + +Fixes since Git 2.39.4 +---------------------- + + * Hooks that are intentionally installed _during_ a clone operation + (like Git LFS does in its `smudge` filter) can now be marked as + intentional by appending `safe.hook.sha256` values to a protected + config. + + The hooks installed by the current Git LFS version (3.5.1) are + hard-coded as being safe. + + * The `core.hooksPath` setting is allowed in repository-local + configs again; The benefits of making it protected were + outweighed by the cost. + + * Fix a memory leak. + + * CI fix. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index c124ef0b41f2a8..b58a5e2d3010ac 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.39.4 +DEF_VER=v2.39.5 LF=' ' diff --git a/RelNotes b/RelNotes index 34ee2940000ef4..eda51229b82d04 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.39.4.txt \ No newline at end of file +Documentation/RelNotes/2.39.5.txt \ No newline at end of file From bf52f2cc851069e14f1c8bcadb3e1c9fa8bf51cc Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 18 May 2024 13:20:01 +0200 Subject: [PATCH 13/14] Git 2.40.3 Signed-off-by: Johannes Schindelin --- Documentation/RelNotes/2.40.3.txt | 6 ++++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.40.3.txt diff --git a/Documentation/RelNotes/2.40.3.txt b/Documentation/RelNotes/2.40.3.txt new file mode 100644 index 00000000000000..adc9641db3ad48 --- /dev/null +++ b/Documentation/RelNotes/2.40.3.txt @@ -0,0 +1,6 @@ +Git v2.40.3 Release Notes +========================= + +This release merges up the regression bug fixes in v2.39.5, +most notably the bug where cloning Git LFS-enabled repositories +failed; see the release notes for that version for details. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index fc9c36a511cd74..b345e89cbef095 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.40.2 +DEF_VER=v2.40.3 LF=' ' diff --git a/RelNotes b/RelNotes index 6d30158d7de8a2..afb47e28e0b1fe 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.40.2.txt \ No newline at end of file +Documentation/RelNotes/2.40.3.txt \ No newline at end of file From e5be6e1c851788656ce1c4183cacc7548f1ed4a2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 18 May 2024 13:22:43 +0200 Subject: [PATCH 14/14] Git 2.41.2 Signed-off-by: Johannes Schindelin --- Documentation/RelNotes/2.41.2.txt | 7 +++++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.41.2.txt diff --git a/Documentation/RelNotes/2.41.2.txt b/Documentation/RelNotes/2.41.2.txt new file mode 100644 index 00000000000000..2aa3db6a42ee12 --- /dev/null +++ b/Documentation/RelNotes/2.41.2.txt @@ -0,0 +1,7 @@ +Git v2.41.2 Release Notes +========================= + +This release merges up the regression bug fixes in v2.39.5 and +v2.40.3, most notably the bug where cloning Git LFS-enabled +repositories failed; see the release notes for these versions +for details. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index a142c55c859b76..990ca9c64376be 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.41.1 +DEF_VER=v2.41.2 LF=' ' diff --git a/RelNotes b/RelNotes index 41c25a6bc10cb8..04f6c713106c09 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.41.1.txt \ No newline at end of file +Documentation/RelNotes/2.41.2.txt \ No newline at end of file