| Message ID | 20220428033544.68188-2-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix `sudo make install` regression in maint | expand |
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > In a previous patch, the behaviour of git was changed so it will be able > to find the "effective uid" that is required when git was invoked with > sudo to root, for example: > > $ sudo make install > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > Documentation/config/safe.txt | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt > index 6d764fe0ccf..67f8ef5d766 100644 > --- a/Documentation/config/safe.txt > +++ b/Documentation/config/safe.txt > @@ -26,3 +26,11 @@ directory was listed in the `safe.directory` list. If `safe.directory=*` > is set in system config and you want to re-enable this protection, then > initialize your list with an empty value before listing the repositories > that you deem safe. > ++ > +When git tries to check for ownership of git repositories it will obviously Comma before "it will obviously". > +use the user that is being used to run git itself, but if git is running > +as root, it will first check if it might had been started through `sudo`, > +and if that is the case, will use the user id that invoked sudo instead. This raises a design question. In a repository is owned by root, shouldn't "sudo git describe" work? IOW, I am wondering if the "instead" at the end of the description is what we want, or if we want to check both the original user and "root". There is not much point in protecting against a malicious repository a repository that is owned by "root"---an attacker that can create such a repository and futz with its config or hooks can attack you more directly, without social engineering you into using git on such a repository. So from that point of view, it may be reasonable to say that we can trust repositories owned by - euid (both when euid is root and euid is not root) - SUDO_UID (when euid is root) I think. And even if we adopt such a tweak, ... > +If that is not what you would prefer and want git to instead only trust > +repositories that are owned by root, then you should remove the `SUDO_UID` > +variable from root's environment. ... this is still true. I guess the necessary tweak to the code, if we were to take that suggestion, would be quite small. We are happy when euid owns the path (regardless of who euid is), and in addition, only when euid is root, we check with SUDO_UID, too. git-compat-util.h | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git c/git-compat-util.h w/git-compat-util.h index dfdd3e4f81..18660553b3 100644 --- c/git-compat-util.h +++ w/git-compat-util.h @@ -401,13 +401,13 @@ static inline int git_offset_1st_component(const char *path) #endif /* - * this helper function overrides a ROOT_UID with the one provided by - * an environment variable, do not use unless the original user is - * root + * The environment variable (e.g. SUDO_UID) gives an integer; + * is it the same as the given uid_t id? */ -static inline void extract_id_from_env(const char *env, uid_t *id) +static inline int id_from_env_matches(const char *env, uid_t id) { const char *real_uid = getenv(env); + int matches = 0; /* discard any empty values */ if (real_uid && *real_uid) { @@ -417,11 +417,12 @@ static inline void extract_id_from_env(const char *env, uid_t *id) errno = 0; env_id = strtoul(real_uid, &endptr, 10); - if (!errno && !*endptr && env_id <= (uid_t)-1) - *id = env_id; - + if (!errno && !*endptr && env_id <= (uid_t)-1 && + (uid_t)env_id == id) + matches = 1; errno = saved_errno; } + return matches; } static inline int is_path_owned_by_current_uid(const char *path) @@ -433,10 +434,11 @@ static inline int is_path_owned_by_current_uid(const char *path) return 0; euid = geteuid(); + if (st.st_uid == euid) + return 1; if (euid == ROOT_UID) - extract_id_from_env("SUDO_UID", &euid); - - return st.st_uid == euid; + return id_from_env_matches("SUDO_UID", st.st_uid); + return 0; } #define is_path_owned_by_current_user is_path_owned_by_current_uid
On Wed, Apr 27, 2022 at 10:17 PM Junio C Hamano <gitster@pobox.com> wrote: > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt > > index 6d764fe0ccf..67f8ef5d766 100644 > > --- a/Documentation/config/safe.txt > > +++ b/Documentation/config/safe.txt > > @@ -26,3 +26,11 @@ directory was listed in the `safe.directory` list. If `safe.directory=*` > > is set in system config and you want to re-enable this protection, then > > initialize your list with an empty value before listing the repositories > > that you deem safe. > > ++ > > +When git tries to check for ownership of git repositories it will obviously > > Comma before "it will obviously". Obviously my whole paragraph could be improved further, do you want a reroll with this fix, or would instead fixup locally? > > +use the user that is being used to run git itself, but if git is running "use the user that is being used", is something my high school grammar teacher would label as a "cacophony", so feel free to provide an alternative as well there. > > +as root, it will first check if it might had been started through `sudo`, > > +and if that is the case, will use the user id that invoked sudo instead. > > This raises a design question. In a repository is owned by root, > shouldn't "sudo git describe" work? IOW, I am wondering if the > "instead" at the end of the description is what we want, or if we > want to check both the original user and "root". I think it makes sense to have both, and your implementation below seems like a good way to do it but it might not be as safe as it seems, since sometimes directories owned by root might be also world writable and therefore not necessarily safe. This is obviously not directly related to this code, but the original issue as reported in Windows for the original CVE could be traced back to the default policy to allow any authenticated user to write in the root directory. Carlo
Carlo Arenas <carenas@gmail.com> writes: > On Wed, Apr 27, 2022 at 10:17 PM Junio C Hamano <gitster@pobox.com> wrote: >> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: >> > diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt >> > index 6d764fe0ccf..67f8ef5d766 100644 >> > --- a/Documentation/config/safe.txt >> > +++ b/Documentation/config/safe.txt >> > @@ -26,3 +26,11 @@ directory was listed in the `safe.directory` list. If `safe.directory=*` >> > is set in system config and you want to re-enable this protection, then >> > initialize your list with an empty value before listing the repositories >> > that you deem safe. >> > ++ >> > +When git tries to check for ownership of git repositories it will obviously >> >> Comma before "it will obviously". > > Obviously my whole paragraph could be improved further, do you want > a reroll with this fix, or would instead fixup locally? I think the patches (including the previous one) are still fluid and expect them to be reworked; local fix-ups would be a bit premature and leads to waste. At least not yet. >> This raises a design question. In a repository is owned by root, >> shouldn't "sudo git describe" work? IOW, I am wondering if the >> "instead" at the end of the description is what we want, or if we >> want to check both the original user and "root". > > I think it makes sense to have both, and your implementation below > seems like a good way to do it but it might not be as safe as it > seems, since sometimes directories owned by root might be also world > writable and therefore not necessarily safe. I am not quite following you; that logic applies to directories owned by euid (not necessarily root). As we are loosening to make "sudo" usable again, the use case to visit root-owned repository as root via "sudo" is worth discussing, if not worth immediately supporting, I would think. I do not think it is absolutely needed as there is an easy workaround (see below). Assuming we will go without "same euid, whether it is root or not, plus SUDO_UID when run as root", here a test addition, updated from the one I gave you in the review of [2/2] test_expect_success SUDO 'in root owned repository' ' mkdir root/p sudo chown root root/p && sudo git init root/p && # owned by other person (root), do I see it as a repository? ( cd root/p && test_must_fail git status ) && # owned by root, can I access it under sudo? ( cd root/p && test_must_fail sudo git status ) && # workaround #1, giving GIT_DIR explicitly ( cd root/p && sudo " GIT_DIR=.git GIT_WORK_TREE=. git status " ) && # workaround #2, discarding SUDO_UID ( cd root/p && sudo " unset SUDO_UID; git status " ) '
diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt index 6d764fe0ccf..67f8ef5d766 100644 --- a/Documentation/config/safe.txt +++ b/Documentation/config/safe.txt @@ -26,3 +26,11 @@ directory was listed in the `safe.directory` list. If `safe.directory=*` is set in system config and you want to re-enable this protection, then initialize your list with an empty value before listing the repositories that you deem safe. ++ +When git tries to check for ownership of git repositories it will obviously +use the user that is being used to run git itself, but if git is running +as root, it will first check if it might had been started through `sudo`, +and if that is the case, will use the user id that invoked sudo instead. +If that is not what you would prefer and want git to instead only trust +repositories that are owned by root, then you should remove the `SUDO_UID` +variable from root's environment.
In a previous patch, the behaviour of git was changed so it will be able to find the "effective uid" that is required when git was invoked with sudo to root, for example: $ sudo make install Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- Documentation/config/safe.txt | 8 ++++++++ 1 file changed, 8 insertions(+)