| Message ID | pull.1732.v3.git.1716236526.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Various fixes for v2.45.1 and friends | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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 It is debatable if we are ripping out clone "protection" or a new restriction on executing hooks before the end of clone that has backfired. In any case, I just compared the result of applying these patches to v2.39.4 with the result of reverting the following out of v2.39.4: 584de0b4 (Add a helper function to compare file contents, 2024-03-30) 8db1e874 (clone: prevent hooks from running during a clone, 2024-03-28) 20f3588e (core.hooksPath: add some protection while cloning, 2024-03-30) and the differences was exactly as I expected. A Makefile fix and a new test added to t1350 are the extra in the series, but otherwise the patches are essentially reversion of these three steps. Very nicely done. Thanks for a quick turnaround. Will take further look.
Junio C Hamano <gitster@pobox.com> writes: > In any case, I just compared the result of applying these patches to > v2.39.4 with the result of reverting the following out of v2.39.4: > > 584de0b4 (Add a helper function to compare file contents, 2024-03-30) > 8db1e874 (clone: prevent hooks from running during a clone, 2024-03-28) > 20f3588e (core.hooksPath: add some protection while cloning, 2024-03-30) > > and the differences was exactly as I expected. A Makefile fix and a > new test added to t1350 are the extra in the series, but otherwise > the patches are essentially reversion of these three steps. Very > nicely done. > > Thanks for a quick turnaround. Will take further look. I completed merge-up exercise and compared the result with your "tentative" cascade from maint-2.39 to maint-2.45 tracks. The differences came from pointed cherry-picks (like 'ci: avoid bare "gcc" for osx-gcc job') looked minimal and sensible. I wonder what the best way to do a public review of this kind of history, though. $ git log --oneline --graph maint-2.45..dscho/tentative/maint-2.45 * aeddcb0275 Git 2.45.2 * 65f0d62523 Sync with 2.44.2 * 9953011fcd Git 2.44.2 * f78818b645 Sync with 2.43.5 * 0aeca2f80b Git 2.43.5 * 0cc3782b1a Sync with 2.42.3 * 33efa2ad1a Git 2.42.3 * 30195eb2b6 Sync with 2.41.2 * 5215e4e368 Git 2.41.2 * 9d6788fd73 Sync with 2.40.3 * 4bf5d57da6 Git 2.40.3 * 9f7a956be5 Sync with 2.39.5 * b9a96c4e5d Git 2.39.5 All of the above (and the one below) are merging up, resolving conflicts, and updating release notes and GIT-VERSION-GEN. * b674c6f66c Merge branch 'js/fix-v2.39.4-regressions' into maint-2.39 |\ | * 5c576e889d Revert "Add a helper function to compare file contents" | * 0044a35567 clone: drop the protections where hooks aren't run | * cd14042b06 tests: verify that `clone -c core.hooksPath=/dev/null` works again | * 57db89a149 Revert "core.hooksPath: add some protection while cloning" | * 961dfc35f4 init: use the correct path of the templates directory again | * d4a003bf2c hook: plug a new memory leak The above 6 patches all appeared on the list in this "v3" thread. * 883ca51e0a Merge branch 'jk/ci-macos-gcc13-fix' into 'maint-2.39' This is a merge of the following three patches to maint-2.39 * d4543be3f2 ci: stop installing "gcc-13" for osx-gcc * 2aef8020d2 ci: avoid bare "gcc" for osx-gcc job * f3e5bdfebc ci: drop mention of BREW_INSTALL_PACKAGES variable These three patches were taken from jk/ci-macos-gcc13-fix that was forked from v2.45.0 and rebased them on top of v2.39.4. The bottom one seems to have been adjusted for the older contexts, which during subsequent merging-up has been adjusted back again for the more recent contexts (e.g., we used to use $HOME/bin but use $P4_PATH for Perforce these days, and such differences in the base version appear in the context for "BREW_INSTALL_PACKAGES" change). So, in short, I didn't see anything unexpected to see in these branches. The "ci" fixes were already reviewed elsewhere (even though there are slight deviations), so if people are OK with the 6 patches in this thread, I would say we are good to go. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > So, in short, I didn't see anything unexpected to see in these > branches. The "ci" fixes were already reviewed elsewhere (even > though there are slight deviations), so if people are OK with the 6 > patches in this thread, I would say we are good to go. Maybe I spoke a bit too early, and we may need to redo (not necessarily revert) f4aa8c8b as well.
On 2024-05-20 at 20:21:59, Johannes Schindelin via GitGitGadget wrote: > 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. I looked at this series and seems fine. I tested it with the latest HEAD of Git LFS and it seems to function as expected. I appreciate the prompt fixes. (My apologies for not getting back to this sooner. I took a long weekend for the Victoria Day holiday and was busy riding rollercoasters instead of reading the list.)
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > (My apologies for not getting back to this sooner. I took a long > weekend for the Victoria Day holiday and was busy riding rollercoasters > instead of reading the list.) That's fine. We all must have fun sometimes ;-)
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > I looked at this series and seems fine. I tested it with the latest > HEAD of Git LFS and it seems to function as expected. I appreciate the > prompt fixes. I've merged the 11 patches (the 12-patch series without the last step) https://lore.kernel.org/git/xmqqed9u95l0.fsf@gitster.g/ to 'seen'. These 11 patches contains Dscho's 6 patches but also contains backports Dscho had on his "tentative" topic branch, plus two additional backports to help tests pass. Additionally what is in 'seen' obviously does not update GIT-VERSION-GEN or add new release notes files. Other than that, it is the same topic, only done in a bit more open way to make it easier to apply further polish. Testing is appreciated. Hopefully they can be merged down to 'next' and then 'master' soonish, but will be updated if a new issue is discovered. After then, we'll do a release engineering to merge them to all the maintenance tracks affected by the recent security updates and we can go from there. Hopefully, as not critical vulnerability fix but a "layered defence" on top, we probably do not have to merge down whatever additional follow-up changes beyond the latest maintenance track (i.e. maint-2.45). Thanks.
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"