From f4aa8c8bb11dae6e769cd930565173808cbb69c8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 10 Apr 2024 14:39:37 +0200 Subject: fetch/clone: detect dubious ownership of local repositories When cloning from somebody else's repositories, it is possible that, say, the `upload-pack` command is overridden in the repository that is about to be cloned, which would then be run in the user's context who started the clone. To remind the user that this is a potentially unsafe operation, let's extend the ownership checks we have already established for regular gitdir discovery to extend also to local repositories that are about to be cloned. This protection extends also to file:// URLs. The fixes in this commit address CVE-2024-32004. Note: This commit does not touch the `fetch`/`clone` code directly, but instead the function used implicitly by both: `enter_repo()`. This function is also used by `git receive-pack` (i.e. pushes), by `git upload-archive`, by `git daemon` and by `git http-backend`. In setups that want to serve repositories owned by different users than the account running the service, this will require `safe.*` settings to be configured accordingly. Also note: there are tiny time windows where a time-of-check-time-of-use ("TOCTOU") race is possible. The real solution to those would be to work with `fstat()` and `openat()`. However, the latter function is not available on Windows (and would have to be emulated with rather expensive low-level `NtCreateFile()` calls), and the changes would be quite extensive, for my taste too extensive for the little gain given that embargoed releases need to pay extra attention to avoid introducing inadvertent bugs. Signed-off-by: Johannes Schindelin --- setup.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) (limited to 'setup.c') diff --git a/setup.c b/setup.c index cefd5f63c4..9d401ae4c8 100644 --- a/setup.c +++ b/setup.c @@ -1165,6 +1165,27 @@ static int ensure_valid_ownership(const char *gitfile, return data.is_safe; } +void die_upon_dubious_ownership(const char *gitfile, const char *worktree, + const char *gitdir) +{ + struct strbuf report = STRBUF_INIT, quoted = STRBUF_INIT; + const char *path; + + if (ensure_valid_ownership(gitfile, worktree, gitdir, &report)) + return; + + strbuf_complete(&report, '\n'); + path = gitfile ? gitfile : gitdir; + sq_quote_buf_pretty("ed, path); + + die(_("detected dubious ownership in repository at '%s'\n" + "%s" + "To add an exception for this directory, call:\n" + "\n" + "\tgit config --global --add safe.directory %s"), + path, report.buf, quoted.buf); +} + static int allowed_bare_repo_cb(const char *key, const char *value, void *d) { enum allowed_bare_repo *allowed_bare_repo = d; -- cgit v1.2.3 From df93e407f0618e4a8265ac619dc7f4c7005155bc Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 29 Mar 2024 11:45:01 +0100 Subject: init: refactor the template directory discovery into its own function We will need to call this function from `hook.c` to be able to prevent hooks from running that were written as part of a `clone` but did not originate from the template directory. Signed-off-by: Johannes Schindelin --- builtin/init-db.c | 22 ++++------------------ cache.h | 1 + setup.c | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 18 deletions(-) (limited to 'setup.c') diff --git a/builtin/init-db.c b/builtin/init-db.c index dcaaf102ea..a101e7f94c 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -11,10 +11,6 @@ #include "parse-options.h" #include "worktree.h" -#ifndef DEFAULT_GIT_TEMPLATE_DIR -#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates" -#endif - #ifdef NO_TRUSTABLE_FILEMODE #define TEST_FILEMODE 0 #else @@ -93,8 +89,9 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path, } } -static void copy_templates(const char *template_dir, const char *init_template_dir) +static void copy_templates(const char *option_template) { + const char *template_dir = get_template_dir(option_template); struct strbuf path = STRBUF_INIT; struct strbuf template_path = STRBUF_INIT; size_t template_len; @@ -103,16 +100,8 @@ static void copy_templates(const char *template_dir, const char *init_template_d DIR *dir; char *to_free = NULL; - if (!template_dir) - template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT); - if (!template_dir) - template_dir = init_template_dir; - if (!template_dir) - template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR); - if (!template_dir[0]) { - free(to_free); + if (!template_dir || !*template_dir) return; - } strbuf_addstr(&template_path, template_dir); strbuf_complete(&template_path, '/'); @@ -200,7 +189,6 @@ static int create_default_files(const char *template_path, int reinit; int filemode; struct strbuf err = STRBUF_INIT; - const char *init_template_dir = NULL; const char *work_tree = get_git_work_tree(); /* @@ -212,9 +200,7 @@ static int create_default_files(const char *template_path, * values (since we've just potentially changed what's available on * disk). */ - git_config_get_pathname("init.templatedir", &init_template_dir); - copy_templates(template_path, init_template_dir); - free((char *)init_template_dir); + copy_templates(template_path); git_config_clear(); reset_shared_repository(); git_config(git_default_config, NULL); diff --git a/cache.h b/cache.h index a46a3e4b6b..8c5fb1e1ba 100644 --- a/cache.h +++ b/cache.h @@ -656,6 +656,7 @@ int path_inside_repo(const char *prefix, const char *path); #define INIT_DB_QUIET 0x0001 #define INIT_DB_EXIST_OK 0x0002 +const char *get_template_dir(const char *option_template); int init_db(const char *git_dir, const char *real_git_dir, const char *template_dir, int hash_algo, const char *initial_branch, unsigned int flags); diff --git a/setup.c b/setup.c index 9d401ae4c8..e6e749ec4b 100644 --- a/setup.c +++ b/setup.c @@ -6,6 +6,7 @@ #include "chdir-notify.h" #include "promisor-remote.h" #include "quote.h" +#include "exec-cmd.h" static int inside_git_dir = -1; static int inside_work_tree = -1; @@ -1720,3 +1721,34 @@ int daemonize(void) return 0; #endif } + +#ifndef DEFAULT_GIT_TEMPLATE_DIR +#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates" +#endif + +const char *get_template_dir(const char *option_template) +{ + const char *template_dir = option_template; + + if (!template_dir) + template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT); + if (!template_dir) { + static const char *init_template_dir; + static int initialized; + + if (!initialized) { + git_config_get_pathname("init.templatedir", + &init_template_dir); + initialized = 1; + } + template_dir = init_template_dir; + } + if (!template_dir) { + static char *dir; + + if (!dir) + dir = system_path(DEFAULT_GIT_TEMPLATE_DIR); + template_dir = dir; + } + return template_dir; +} -- cgit v1.2.3 From 4412a04fe6f7e632269a6668a4f367230ca2c0e0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 29 Mar 2024 13:15:32 +0100 Subject: init.templateDir: consider this config setting protected The ability to configuring the template directory is a delicate feature: It allows defining hooks that will be run e.g. during a `git clone` operation, such as the `post-checkout` hook. As such, it is of utmost importance that Git would not allow that config setting to be changed during a `git clone` by mistake, allowing an attacker a chance for a Remote Code Execution, allowing attackers to run arbitrary code on unsuspecting users' machines. As a defense-in-depth measure, to prevent minor vulnerabilities in the `git clone` code from ballooning into higher-serverity attack vectors, let's make this a protected setting just like `safe.directory` and friends, i.e. ignore any `init.templateDir` entries from any local config. Note: This does not change the behavior of any recursive clone (modulo bugs), as the local repository config is not even supposed to be written while cloning the superproject, except in one scenario: If a config template is configured that sets the template directory. This might be done because `git clone --recurse-submodules --template=` does not pass that template directory on to the submodules' initialization. Another scenario where this commit changes behavior is where repositories are _not_ cloned recursively, and then some (intentional, benign) automation configures the template directory to be used before initializing the submodules. So the caveat is that this could theoretically break existing processes. In both scenarios, there is a way out, though: configuring the template directory via the environment variable `GIT_TEMPLATE_DIR`. This change in behavior is a trade-off between security and backwards-compatibility that is struck in favor of security. Signed-off-by: Johannes Schindelin --- setup.c | 37 ++++++++++++++++++++++++++++++------- t/t7400-submodule-basic.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 7 deletions(-) (limited to 'setup.c') diff --git a/setup.c b/setup.c index e6e749ec4b..c3301f5ab8 100644 --- a/setup.c +++ b/setup.c @@ -1726,6 +1726,31 @@ int daemonize(void) #define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates" #endif +struct template_dir_cb_data { + char *path; + int initialized; +}; + +static int template_dir_cb(const char *key, const char *value, void *d) +{ + struct template_dir_cb_data *data = d; + + if (strcmp(key, "init.templatedir")) + return 0; + + if (!value) { + data->path = NULL; + } else { + char *path = NULL; + + FREE_AND_NULL(data->path); + if (!git_config_pathname((const char **)&path, key, value)) + data->path = path ? path : xstrdup(value); + } + + return 0; +} + const char *get_template_dir(const char *option_template) { const char *template_dir = option_template; @@ -1733,15 +1758,13 @@ const char *get_template_dir(const char *option_template) if (!template_dir) template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT); if (!template_dir) { - static const char *init_template_dir; - static int initialized; + static struct template_dir_cb_data data; - if (!initialized) { - git_config_get_pathname("init.templatedir", - &init_template_dir); - initialized = 1; + if (!data.initialized) { + git_protected_config(template_dir_cb, &data); + data.initialized = 1; } - template_dir = init_template_dir; + template_dir = data.path; } if (!template_dir) { static char *dir; diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index eae6a46ef3..3e8cf9b885 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -1436,4 +1436,35 @@ test_expect_success 'recursive clone respects -q' ' test_must_be_empty actual ' +test_expect_success '`submodule init` and `init.templateDir`' ' + mkdir -p tmpl/hooks && + write_script tmpl/hooks/post-checkout <<-EOF && + echo HOOK-RUN >&2 + echo I was here >hook.run + exit 1 + EOF + + test_config init.templateDir "$(pwd)/tmpl" && + test_when_finished \ + "git config --global --unset init.templateDir || true" && + ( + sane_unset GIT_TEMPLATE_DIR && + NO_SET_GIT_TEMPLATE_DIR=t && + export NO_SET_GIT_TEMPLATE_DIR && + + git config --global init.templateDir "$(pwd)/tmpl" && + test_must_fail git submodule \ + add "$submodurl" sub-global 2>err && + git config --global --unset init.templateDir && + grep HOOK-RUN err && + test_path_is_file sub-global/hook.run && + + git config init.templateDir "$(pwd)/tmpl" && + git submodule add "$submodurl" sub-local 2>err && + git config --unset init.templateDir && + ! grep HOOK-RUN err && + test_path_is_missing sub-local/hook.run + ) +' + test_done -- cgit v1.2.3