{"thread":{"id":"57845","subject":"[PATCH v2] setup: tighten ownership checks post CVE-2022-24765","startedAt":"2022-05-05T00:50:40Z","lastAt":"2022-05-05T13:59:02Z","messageCount":5,"participants":["Carlo Marcelo Arenas Belón","Phillip Wood","Derrick Stolee"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"454845","messageId":"20220505005009.27789-1-carenas@gmail.com","threadId":"57845","inReplyTo":"20220504184401.17438-1-carenas@gmail.com","subject":"[PATCH v2] setup: tighten ownership checks post CVE-2022-24765","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-05-05T00:50:09Z","receivedAt":"2022-05-05T00:50:40Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"8959555cee7 (setup_git_directory(): add an owner check for the top-level\ndirectory, 2022-03-02), adds a function to check for ownership of\nrepositories using a directory that is representative of it (its workdir)\nand ways to add it to an exception list if needed, but that check breaks\nwhen the ownership of the workdir is not the same than the ownership of\ndirectory where the configuration and other relevant files reside.\n\nAn attacker could create a git repository in a directory that he has write\naccess to but is owned by the victim, and therefore workaround the fix that\nwas introduced with CVE-2022-24765 to attack them, like in the following\nscenario which could result in privilege escalation if root then runs a git\ncommand in that directory or any of its sub directories:\n\n  $ git -C /tmp init\n\nTo avoid that, extend the ensure_valid_ownership function to be able to\ncheck for ownership of both the worktree and the gitdir, and use that for\nnon bare repositories.\n\nReported-by: Hanno Böck <hanno@hboeck.de>\nHelped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\nChanges since RFC\n* remove debug code from ensure_valid_ownership since is no longer needed\n* replace convoluted logic in setup_git_directory_gently_1 with Junio's\n* improve tests (AGAIN, not considered production and only for convenience)\n* hopefully improved commit message and spelling.\n\nThe changes in setup.c should be sufficient to cover for all known issues, but\nhas been only lightly tested and mostly in *NIX, so more changes might be\nneeded to cover Windows. Specially the use of \"/\" to reconstruct the gitdir\nbased on the previously cut workdir might be problematic if not covered by\nits compat code.\n\nThe code for setup_git_directory_gently_1 is inefficient (as pointed by\ndscho) and could be improved by instead reusing the buffer before it is cut\nby the setlen, but if doing so, then a copy of the full gitdir will be\nneeded, so a solution for that is not provided.\n\nIn the same line, we already know before getting into the condition, if we\nare coming from a gitfile or not, so the is_absolute_path(gitdirenv) could\nbe optimized away, like it was done in the RFC with an incorrectly named\nis_bare boolean, but that change hasn't been implemented as the cost of the\ncurrent implementation is unknown and feels like premature optimization.\n\nSlightly off-topic and maybe more of an ADMINISTRATIVE, but had added the\nReported-by for the guy that came with the last report, not sure what the\nright procedure is, and might be better if kept as a note, but be careful\nof git send-email to avoid leaks, at least until we have a final version.\n\n setup.c                        | 28 +++++++++++++++++++++++-----\n t/t0034-root-safe-directory.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 5 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex aad9ace0af9..0fae2d71a3c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1054,14 +1054,21 @@ static int safe_directory_cb(const char *key, const char *value, void *d)\n \treturn 0;\n }\n \n-static int ensure_valid_ownership(const char *path)\n+static int ensure_valid_ownership(const char *worktree, const char *gitdir)\n {\n-\tstruct safe_directory_data data = { .path = path };\n+\tstruct safe_directory_data data = { .path = worktree };\n+\tconst char *check_path;\n+\n+\tif (gitdir)\n+\t\tcheck_path = gitdir;\n+\telse\n+\t\tcheck_path = worktree;\n \n \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n-\t    is_path_owned_by_current_user(path))\n+\t    is_path_owned_by_current_user(check_path))\n \t\treturn 1;\n \n+\tdata.is_safe = 0; /* ensure we are initialized and secure by default */\n \tread_very_early_config(safe_directory_cb, &data);\n \n \treturn data.is_safe;\n@@ -1166,14 +1173,25 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t}\n \t\tstrbuf_setlen(dir, offset);\n \t\tif (gitdirenv) {\n-\t\t\tif (!ensure_valid_ownership(dir->buf))\n+\t\t\tconst char *gitdir_to_check = gitdirenv;\n+\t\t\tstruct strbuf gdbuf = STRBUF_INIT;\n+\t\t\tint ret;\n+\n+\t\t\tif (!is_absolute_path(gitdirenv)) {\n+\t\t\t\tstrbuf_addf(&gdbuf, \"%s/%s\", dir->buf,\n+\t\t\t\t\t\tgitdirenv);\n+\t\t\t\tgitdir_to_check = gdbuf.buf;\n+\t\t\t}\n+\t\t\tret = ensure_valid_ownership(dir->buf, gitdir_to_check);\n+\t\t\tstrbuf_release(&gdbuf);\n+\t\t\tif (!ret)\n \t\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\n \t\t\tstrbuf_addstr(gitdir, gitdirenv);\n \t\t\treturn GIT_DIR_DISCOVERED;\n \t\t}\n \n \t\tif (is_git_directory(dir->buf)) {\n-\t\t\tif (!ensure_valid_ownership(dir->buf))\n+\t\t\tif (!ensure_valid_ownership(NULL, dir->buf))\n \t\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\n \t\t\tstrbuf_addstr(gitdir, \".\");\n \t\t\treturn GIT_DIR_BARE;\ndiff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh\nindex a68e1d7602b..a3ddebb009a 100755\n--- a/t/t0034-root-safe-directory.sh\n+++ b/t/t0034-root-safe-directory.sh\n@@ -47,6 +47,35 @@ test_expect_success SUDO 'sudo git status as original owner' '\n \t)\n '\n \n+test_expect_success SUDO 'unsecure worktree with non bare repository' '\n+\tsudo rm -rf root &&\n+\tsudo mkdir -p root/t &&\n+\tsudo chmod 1777 root/t &&\n+\t(\n+\t\tcd root/t &&\n+\t\tgit init &&\n+\t\tgit status &&\n+\t\tsudo git status &&\n+\t\trun_with_sudo <<-END\n+\t\t\tunset SUDO_UID &&\n+\t\t\t! git status\n+\t\tEND\n+\t)\n+'\n+\n+test_expect_success SUDO 'non bare repository using a gitfile' '\n+\tsudo rm -rf root &&\n+\tmkdir -p root/w &&\n+\tmkdir -p root/e &&\n+\t(\n+\t\tcd root/w &&\n+\t\tgit init --separate-git-dir ../e &&\n+\t\tgit status &&\n+\t\tsudo chown -R root ../e &&\n+\t\ttest_must_fail git status\n+\t)\n+'\n+\n # this destroys the test environment used above\n test_expect_success SUDO 'cleanup regression' '\n \tsudo rm -rf root\n-- \n2.36.0.352.g0cd7feaf86f\n\n"},{"id":"454849","messageId":"41ec8c09-f31f-46ce-d6ec-4b6fdd78228a@gmail.com","threadId":"57845","inReplyTo":"20220505005009.27789-1-carenas@gmail.com","subject":"Re: [PATCH v2] setup: tighten ownership checks post CVE-2022-24765","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-05-05T09:40:54Z","receivedAt":"2022-05-05T09:41:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Carlo\n\nOn 05/05/2022 01:50, Carlo Marcelo Arenas Belón wrote:\n> 8959555cee7 (setup_git_directory(): add an owner check for the top-level\n> directory, 2022-03-02), adds a function to check for ownership of\n> repositories using a directory that is representative of it (its workdir)\n> and ways to add it to an exception list if needed, but that check breaks\n> when the ownership of the workdir is not the same than the ownership of\n> directory where the configuration and other relevant files reside.\n> \n> An attacker could create a git repository in a directory that he has write\n> access to but is owned by the victim, and therefore workaround the fix that\n> was introduced with CVE-2022-24765 to attack them, like in the following\n> scenario which could result in privilege escalation if root then runs a git\n> command in that directory or any of its sub directories:\n> \n>    $ git -C /tmp init\n> \n> To avoid that, extend the ensure_valid_ownership function to be able to\n> check for ownership of both the worktree and the gitdir, and use that for\n> non bare repositories.\n\nLooking at the code below it now only ever checks the ownership of the \ngitdir, it no longer checks the ownership of the worktree. I haven't \nreally thought through what happens if I cd into a worktree added by an \nattacker to a repository that I own which has extentions.worktreeConfig \nset. My initial thought is that if they can add a worktree then they can \nprobably edit the repository config anyway but I wonder if an attacker \ncan set GIT_COMMON_DIR to a directory where they have write permission \nto add a worktree to a repository where they don't have write permission.\n\n> [...]\n> diff --git a/setup.c b/setup.c\n> index aad9ace0af9..0fae2d71a3c 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1054,14 +1054,21 @@ static int safe_directory_cb(const char *key, const char *value, void *d)\n>   \treturn 0;\n>   }\n>   \n> -static int ensure_valid_ownership(const char *path)\n> +static int ensure_valid_ownership(const char *worktree, const char *gitdir)\n>   {\n> -\tstruct safe_directory_data data = { .path = path };\n> +\tstruct safe_directory_data data = { .path = worktree };\n\nWe keep checking the worktree path against safe.directory - good.\n\n> +\tconst char *check_path;\n> +\n> +\tif (gitdir)\n> +\t\tcheck_path = gitdir;\n> +\telse\n> +\t\tcheck_path = worktree;\n\nWe check either the gitdir or worktree but always call this function \nwith a non-NULL gitdir so in fact always check that. This code could be \nremoved.\n\n>   \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n> -\t    is_path_owned_by_current_user(path))\n> +\t    is_path_owned_by_current_user(check_path))\n\nPreviously we checked the ownership of the worktree but now we check the \nownership of the gitdir instead to handle the \"git -C /tmp init\" case.\n\n>   \t\treturn 1;\n>   \n> +\tdata.is_safe = 0; /* ensure we are initialized and secure by default */\n>   \tread_very_early_config(safe_directory_cb, &data);\n>   \n>   \treturn data.is_safe;\n> @@ -1166,14 +1173,25 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n>   \t\t}\n>   \t\tstrbuf_setlen(dir, offset);\n>   \t\tif (gitdirenv) {\n> -\t\t\tif (!ensure_valid_ownership(dir->buf))\n> +\t\t\tconst char *gitdir_to_check = gitdirenv;\n> +\t\t\tstruct strbuf gdbuf = STRBUF_INIT;\n> +\t\t\tint ret;\n> +\n> +\t\t\tif (!is_absolute_path(gitdirenv)) {\n> +\t\t\t\tstrbuf_addf(&gdbuf, \"%s/%s\", dir->buf,\n> +\t\t\t\t\t\tgitdirenv);\n> +\t\t\t\tgitdir_to_check = gdbuf.buf;\n> +\t\t\t}\n> +\t\t\tret = ensure_valid_ownership(dir->buf, gitdir_to_check);\n> +\t\t\tstrbuf_release(&gdbuf);\n> +\t\t\tif (!ret)\n>   \t\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\n>   \t\t\tstrbuf_addstr(gitdir, gitdirenv);\n>   \t\t\treturn GIT_DIR_DISCOVERED;\n>   \t\t}\n>   \n>   \t\tif (is_git_directory(dir->buf)) {\n> -\t\t\tif (!ensure_valid_ownership(dir->buf))\n> +\t\t\tif (!ensure_valid_ownership(NULL, dir->buf))\n\nPreviously we checked bare repositories against safe.directory now we no \nlonger do that as worktree is NULL. If this is intentional it should be \nflagged up in the commit message.\n\nBest Wishes\n\nPhillip\n\n>   \t\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\n>   \t\t\tstrbuf_addstr(gitdir, \".\");\n>   \t\t\treturn GIT_DIR_BARE;\n> diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh\n> index a68e1d7602b..a3ddebb009a 100755\n> --- a/t/t0034-root-safe-directory.sh\n> +++ b/t/t0034-root-safe-directory.sh\n> @@ -47,6 +47,35 @@ test_expect_success SUDO 'sudo git status as original owner' '\n>   \t)\n>   '\n>   \n> +test_expect_success SUDO 'unsecure worktree with non bare repository' '\n> +\tsudo rm -rf root &&\n> +\tsudo mkdir -p root/t &&\n> +\tsudo chmod 1777 root/t &&\n> +\t(\n> +\t\tcd root/t &&\n> +\t\tgit init &&\n> +\t\tgit status &&\n> +\t\tsudo git status &&\n> +\t\trun_with_sudo <<-END\n> +\t\t\tunset SUDO_UID &&\n> +\t\t\t! git status\n> +\t\tEND\n> +\t)\n> +'\n> +\n> +test_expect_success SUDO 'non bare repository using a gitfile' '\n> +\tsudo rm -rf root &&\n> +\tmkdir -p root/w &&\n> +\tmkdir -p root/e &&\n> +\t(\n> +\t\tcd root/w &&\n> +\t\tgit init --separate-git-dir ../e &&\n> +\t\tgit status &&\n> +\t\tsudo chown -R root ../e &&\n> +\t\ttest_must_fail git status\n> +\t)\n> +'\n> +\n>   # this destroys the test environment used above\n>   test_expect_success SUDO 'cleanup regression' '\n>   \tsudo rm -rf root\n\n"},{"id":"454853","messageId":"5d7ace7c-17d3-591d-6cca-ba5223449609@gmail.com","threadId":"57845","inReplyTo":"41ec8c09-f31f-46ce-d6ec-4b6fdd78228a@gmail.com","subject":"Re: [PATCH v2] setup: tighten ownership checks post CVE-2022-24765","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-05-05T12:04:21Z","receivedAt":"2022-05-05T12:04:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 05/05/2022 10:40, Phillip Wood wrote:\n> [...]\n>> To avoid that, extend the ensure_valid_ownership function to be able to\n>> check for ownership of both the worktree and the gitdir, and use that for\n>> non bare repositories.\n> \n> Looking at the code below it now only ever checks the ownership of the \n> gitdir, it no longer checks the ownership of the worktree. I haven't \n> really thought through what happens if I cd into a worktree added by an \n> attacker to a repository that I own which has extentions.worktreeConfig \n> set. My initial thought is that if they can add a worktree then they can \n> probably edit the repository config anyway but I wonder if an attacker \n> can set GIT_COMMON_DIR to a directory where they have write permission \n> to add a worktree to a repository where they don't have write permission.\n\nThinking about this some more, I don't think setting GIT_COMMON_DIR \nwhile running \"git worktree add\" will help an attacker as the worktree's \ngitdir is created under the main gitdir. I've had a bit of a think and \nI've not been able to come up with a senario where GIT_DIR and \nGIT_COMMON_DIR have different owners that is exploitable but it might be \nworth someone else checking I've not missed something.\n\nBest Wishes\n\nPhillip\n"},{"id":"454855","messageId":"0f9aaada-8e72-38b9-e58a-9b26ed7bc780@github.com","threadId":"57845","inReplyTo":"20220505005009.27789-1-carenas@gmail.com","subject":"Re: [PATCH v2] setup: tighten ownership checks post CVE-2022-24765","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-05T13:14:45Z","receivedAt":"2022-05-05T13:14:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/4/2022 8:50 PM, Carlo Marcelo Arenas Belón wrote:\n> 8959555cee7 (setup_git_directory(): add an owner check for the top-level\n> directory, 2022-03-02), adds a function to check for ownership of\n> repositories using a directory that is representative of it (its workdir)\n> and ways to add it to an exception list if needed, but that check breaks\n> when the ownership of the workdir is not the same than the ownership of\n> directory where the configuration and other relevant files reside.\n> \n> An attacker could create a git repository in a directory that he has write\n> access to but is owned by the victim, and therefore workaround the fix that\n> was introduced with CVE-2022-24765 to attack them,\n\nIt's worth noting that this requires having the current user owning a\ndirectory, but allowing other users to write into it.\n\n> [...] like in the following\n> scenario which could result in privilege escalation if root then runs a git\n> command in that directory or any of its sub directories:\n> \n>   $ git -C /tmp init\n\nThis /tmp example is an example of why that is actually common, and not\njust a \"user misconfigured their machine\" issue.\n\n> To avoid that, extend the ensure_valid_ownership function to be able to\n> check for ownership of both the worktree and the gitdir, and use that for\n> non bare repositories.\n\nYou mention extending the check here, but...\n\n> -static int ensure_valid_ownership(const char *path)\n> +static int ensure_valid_ownership(const char *worktree, const char *gitdir)\n>  {\n> -\tstruct safe_directory_data data = { .path = path };\n> +\tstruct safe_directory_data data = { .path = worktree };\n> +\tconst char *check_path;\n> +\n> +\tif (gitdir)\n> +\t\tcheck_path = gitdir;\n> +\telse\n> +\t\tcheck_path = worktree;\n\n...this makes it appear like you are choosing to check only _one_ of the\ndirectories, not _both_, which is what I would expect for a strengthening\nof our security checks.\n\n>  \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n> -\t    is_path_owned_by_current_user(path))\n> +\t    is_path_owned_by_current_user(check_path))\n>  \t\treturn 1;\n\nIndeed, we are checking only one of these directories for ownership.\n\nI think you should remove check_path and instead do the following:\n\n\tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n\t    is_path_owned_by_current_user(worktree) &&\n\t    (!gitdir || is_path_owned_by_current_user(gitdir)))\n \t\treturn 1;\n\nThanks,\n-Stolee\n"},{"id":"454857","messageId":"281017cb-0078-0635-8a3b-fed430b5439a@github.com","threadId":"57845","inReplyTo":"0f9aaada-8e72-38b9-e58a-9b26ed7bc780@github.com","subject":"Re: [PATCH v2] setup: tighten ownership checks post CVE-2022-24765","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-05T13:58:57Z","receivedAt":"2022-05-05T13:59:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/5/2022 9:14 AM, Derrick Stolee wrote:\n> On 5/4/2022 8:50 PM, Carlo Marcelo Arenas Belón wrote:\n>> -static int ensure_valid_ownership(const char *path)\n>> +static int ensure_valid_ownership(const char *worktree, const char *gitdir)\n>>  {\n>> -\tstruct safe_directory_data data = { .path = path };\n>> +\tstruct safe_directory_data data = { .path = worktree };\n\nThis also seems a bit backwards to me. I think bare repos will have a NULL\nworktree, but all repos will have a gitdir. I think what we really want\nis this:\n\n\t.path = worktree ? worktree : gitdir\n\nAnd that might affect the callers of this method allowing the worktree to\nbe NULL.\n\n> I think you should remove check_path and instead do the following:\n> \n> \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n> \t    is_path_owned_by_current_user(worktree) &&\n> \t    (!gitdir || is_path_owned_by_current_user(gitdir)))\n>  \t\treturn 1;\n\nBut that changes my logic here to instead be\n\n \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n\t    is_path_owned_by_current_user(gitdir) &&\n\t    (!worktree|| is_path_owned_by_current_user(worktree)))\n  \t\treturn 1;\n\nThanks,\n-Stolee\n"}]}