| Message ID | 20220503065442.95699-3-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix `sudo make install` regression in maint | expand |
Hi Carlo, On Mon, 2 May 2022, Carlo Marcelo Arenas Belón wrote: > bdc77d1d685 (Add a function to determine whether a path is owned by the > current user, 2022-03-02) checks for the effective uid of the running > process using geteuid() but didn't account for cases where that user was > root (because git was invoked through sudo or a compatible tool) and the > original uid that repository trusted for its config was no longer known, > therefore failing the following otherwise safe call: > > guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty > [sudo] password for guy: > fatal: unsafe repository ('/home/guy/Software/uncrustify' is owned by someone else) > > Attempt to detect those cases by using the environment variables that > those tools create to keep track of the original user id, and do the > ownership check using that instead. > > This assumes the environment the user is running on after going > privileged can't be tampered with, and also adds code to restrict that > the new behavior only applies if running as root, therefore keeping the > most common case, which runs unprivileged, from changing, but because of > that, it will miss cases where sudo (or an equivalent) was used to change > to another unprivileged user or where the equivalent tool used to raise > privileges didn't track the original id in a sudo compatible way. Hmm. I do realize that this is a quite common scenario, and I wish we would not need to rush for a fix here: Otherwise we could carefully design an "untrusted" mode in which Git errors out on spawning user-specified commands and on writing files (and avoids refreshing the index to avoid having to write a file), but runs normally if none of that is needed. > diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt > index 6d764fe0ccf..ee558ced8c7 100644 > --- a/Documentation/config/safe.txt > +++ b/Documentation/config/safe.txt > @@ -26,3 +26,12 @@ 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 do so with the uid of the user that is running git itself, > +but if git is running as root, it will check first if it might have > +been started through `sudo`, and if that is the case, will instead > +use the uid of the user that did so. > +If that is not what you would prefer and want git to only trust > +repositories that are owned by root instead, then you should remove > +the `SUDO_UID` variable from root's environment. > diff --git a/git-compat-util.h b/git-compat-util.h > index 63ba89dd31d..dfdd3e4f81a 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -393,12 +393,50 @@ static inline int git_offset_1st_component(const char *path) > #endif > > #ifndef is_path_owned_by_current_user > + > +#ifdef __TANDEM > +#define ROOT_UID 65535 > +#else > +#define ROOT_UID 0 > +#endif I do wonder whether we have to play this kind of fragile game. Why not simply respect `SUDO_UID` if it is set? It's not like we expect attackers to have control over the environment and could set this malicously. > + > +/* > + * this helper function overrides a ROOT_UID with the one provided by > + * an environment variable, do not use unless the original user is > + * root > + */ > +static inline void extract_id_from_env(const char *env, uid_t *id) > +{ > + const char *real_uid = getenv(env); > + > + /* discard any empty values */ > + if (real_uid && *real_uid) { > + char *endptr; > + unsigned long env_id; > + int saved_errno = errno; > + > + errno = 0; > + env_id = strtoul(real_uid, &endptr, 10); > + if (!errno && !*endptr && env_id <= (uid_t)-1) We should not look at `errno` here unless the return value of `strtoul()` indicates that there might have been an error (i.e. when it is `ULONG_MAX`). Likewise, we need to either initialize `endptr` or only look at it when `strtoul()` succeeded. We could side-step all of this, of course, if we simply did this: euid = getuid(); if (euid == ROOT_UID) euid = git_env_ulong("SUDO_UID", euid); > + *id = env_id; > + > + errno = saved_errno; > + } > +} > + > static inline int is_path_owned_by_current_uid(const char *path) > { > struct stat st; > + uid_t euid; > + > if (lstat(path, &st)) > return 0; > - return st.st_uid == geteuid(); > + > + euid = geteuid(); > + if (euid == ROOT_UID) > + extract_id_from_env("SUDO_UID", &euid); > + > + return st.st_uid == euid; Since this code is not even compiled on Windows, I believe we need to adjust the documentation accordingly ("On systems other than Windows, where `sudo` is available, ..."). > } > > #define is_path_owned_by_current_user is_path_owned_by_current_uid > diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh > index 6dac7a05cfd..dd659aed4e1 100755 > --- a/t/t0034-root-safe-directory.sh > +++ b/t/t0034-root-safe-directory.sh > @@ -32,7 +32,7 @@ test_expect_success SUDO 'setup' ' > ) > ' > > -test_expect_failure SUDO 'sudo git status as original owner' ' > +test_expect_success SUDO 'sudo git status as original owner' ' > ( > cd root/r && > git status && > -- > 2.36.0.352.g0cd7feaf86f Again, thank you for working on this! Dscho
Hi Dscho On 05/05/2022 15:01, Johannes Schindelin wrote: > [...] >> + >> +/* >> + * this helper function overrides a ROOT_UID with the one provided by >> + * an environment variable, do not use unless the original user is >> + * root >> + */ >> +static inline void extract_id_from_env(const char *env, uid_t *id) >> +{ >> + const char *real_uid = getenv(env); >> + >> + /* discard any empty values */ >> + if (real_uid && *real_uid) { >> + char *endptr; >> + unsigned long env_id; >> + int saved_errno = errno; >> + >> + errno = 0; >> + env_id = strtoul(real_uid, &endptr, 10); >> + if (!errno && !*endptr && env_id <= (uid_t)-1) > > We should not look at `errno` here unless the return value of `strtoul()` > indicates that there might have been an error (i.e. when it is > `ULONG_MAX`). > > Likewise, we need to either initialize `endptr` or only look at it when > `strtoul()` succeeded. I don't think we need to do either of those, and indeed the function you suggest below does not do them. The standard guarantees that endptr is always set and there is no harm in unconditionally checking errno. > We could side-step all of this, of course, if we simply did this: > > euid = getuid(); > if (euid == ROOT_UID) > euid = git_env_ulong("SUDO_UID", euid); That's a nice suggestion, I didn't know that function existed. It means we would die() if we could not parse SUDO_UID which I think is reasonable (we'd also accept a units suffix an the uid) Best Wishes Phillip
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > We could side-step all of this, of course, if we simply did this: > > euid = getuid(); > if (euid == ROOT_UID) > euid = git_env_ulong("SUDO_UID", euid); Yes, that is not "side-stepping" at all. It is "we already have a function that knows how to use strto*l() correctly" ;-) Very good suggestion. Thanks.
On Thu, May 5, 2022 at 7:32 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 05/05/2022 15:01, Johannes Schindelin wrote: > > [...] > >> + > >> +/* > >> + * this helper function overrides a ROOT_UID with the one provided by > >> + * an environment variable, do not use unless the original user is > >> + * root > >> + */ > >> +static inline void extract_id_from_env(const char *env, uid_t *id) > >> +{ > >> + const char *real_uid = getenv(env); > >> + > >> + /* discard any empty values */ > >> + if (real_uid && *real_uid) { > >> + char *endptr; > >> + unsigned long env_id; > >> + int saved_errno = errno; > >> + > >> + errno = 0; > >> + env_id = strtoul(real_uid, &endptr, 10); > >> + if (!errno && !*endptr && env_id <= (uid_t)-1) > > > > We should not look at `errno` here unless the return value of `strtoul()` > > indicates that there might have been an error (i.e. when it is > > `ULONG_MAX`). > > > Likewise, we need to either initialize `endptr` or only look at it when > > `strtoul()` succeeded. > > I don't think we need to do either of those, and indeed the function you > suggest below does not do them. The standard guarantees that endptr is > always set and there is no harm in unconditionally checking errno. I think the point dscho was trying to make is that while you are correct that the standard guarantees those two things and implementation might decide to not do them, we obviously support systems that are not POSIX. The irony is that my first confusing attempt to implement this did that before by explicitly ignoring errno and instead relying in the "expected overflow/underflow" values to detect the cases we care for (which is valid UID numbers that are most likely to be uint32_t) and which was the same thing we got from my original (but hated) atoi. > > We could side-step all of this, of course, if we simply did this: > > > > euid = getuid(); > > if (euid == ROOT_UID) > > euid = git_env_ulong("SUDO_UID", euid); > > That's a nice suggestion, I didn't know that function existed. It means > we would die() if we could not parse SUDO_UID which I think is > reasonable (we'd also accept a units suffix an the uid) which is also why we can't use it, any possibly bogus or suspicious value we get from SUDO_UID MUST be ignored. Carlo
Carlo Arenas <carenas@gmail.com> writes: > which is also why we can't use it, any possibly bogus or suspicious > value we get from SUDO_UID MUST be ignored. I do not think I agree. If we have strange value in SUDO_UID, it would be much better and safer to err on the safe side. Instead of ignoring, in the situation where we care about the value we read from SUDO_UID (i.e. when euid==0), we should die loudly when it has a strange value.
On Thu, May 5, 2022 at 7:01 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > On Mon, 2 May 2022, Carlo Marcelo Arenas Belón wrote: > > bdc77d1d685 (Add a function to determine whether a path is owned by the > > current user, 2022-03-02) checks for the effective uid of the running > > process using geteuid() but didn't account for cases where that user was > > root (because git was invoked through sudo or a compatible tool) and the > > original uid that repository trusted for its config was no longer known, > > therefore failing the following otherwise safe call: > > > > guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty > > [sudo] password for guy: > > fatal: unsafe repository ('/home/guy/Software/uncrustify' is owned by someone else) > > > > Attempt to detect those cases by using the environment variables that > > those tools create to keep track of the original user id, and do the > > ownership check using that instead. > > > > This assumes the environment the user is running on after going > > privileged can't be tampered with, and also adds code to restrict that > > the new behavior only applies if running as root, therefore keeping the > > most common case, which runs unprivileged, from changing, but because of > > that, it will miss cases where sudo (or an equivalent) was used to change > > to another unprivileged user or where the equivalent tool used to raise > > privileges didn't track the original id in a sudo compatible way. > > Hmm. I do realize that this is a quite common scenario, and I wish we > would not need to rush for a fix here: not sure what are you referring by "this", and I read the whole snip just in case, but assuming is about the last paragraph * sudo between unprivileged users is still safe because we only look if we are running as root, my comment doesn't imply a regression there, but just that the "feature" wouldn't work for them. * doas is a common tool that is used sometimes as a sudo alternative and I can see there might be even a version of it that would probably provide a SUDO_UID for compatibility, once word goes out of how useful that is for working with git, but until then only sudo is supported. > Otherwise we could carefully design > an "untrusted" mode in which Git errors out on spawning user-specified > commands and on writing files (and avoids refreshing the index to avoid > having to write a file), but runs normally if none of that is needed. This seems like a useful feature to have, and would definitely make this solution irrelevant, but this one is already implemented and I don't see yet why there is concern, probably until "this" could be clarified. > > diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt > > index 6d764fe0ccf..ee558ced8c7 100644 > > --- a/Documentation/config/safe.txt > > +++ b/Documentation/config/safe.txt > > @@ -26,3 +26,12 @@ 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 do so with the uid of the user that is running git itself, > > +but if git is running as root, it will check first if it might have > > +been started through `sudo`, and if that is the case, will instead > > +use the uid of the user that did so. > > +If that is not what you would prefer and want git to only trust > > +repositories that are owned by root instead, then you should remove > > +the `SUDO_UID` variable from root's environment. > > diff --git a/git-compat-util.h b/git-compat-util.h > > index 63ba89dd31d..dfdd3e4f81a 100644 > > --- a/git-compat-util.h > > +++ b/git-compat-util.h > > @@ -393,12 +393,50 @@ static inline int git_offset_1st_component(const char *path) > > #endif > > > > #ifndef is_path_owned_by_current_user > > + > > +#ifdef __TANDEM > > +#define ROOT_UID 65535 > > +#else > > +#define ROOT_UID 0 > > +#endif > > I do wonder whether we have to play this kind of fragile game. Why not > simply respect `SUDO_UID` if it is set? It's not like we expect attackers > to have control over the environment and could set this maliciously. The problem is that it indeed would lower the bar on how this feature might weaken the current protections. Getting an environment variable set "maliciously" is not that hard with some social engineering, so making sure only root would have that escape hatch, and knowing that there is a way to infer from the environment what the relevant user is, is a powerful way to solve this regression without making the protections weaker. My original implementation did not use SUDO_UID but the owner of the pty as that is harder to fake, and therefore safer even for non root users, but it makes it much more intrusive for the same reason. If your concern is the introduced regression where `sudo git` will no longer work without adding a safe.directory exception or removing SUDO_UID from the environment (or another of the workaround documented in the test case), I would actually argue that it instead increases the security of the original solution by implementing a mechanism that user can use to communicate their intention and that would prefer the lower privilege by default. Still I am considering it as a "known bug" which will hopefully be fixed soon, since Junio already provided the code to improve this one so that a root user can access both repositories owned by them and by their SUDO_UID. Carlo
On Fri, May 6, 2022 at 1:00 PM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Arenas <carenas@gmail.com> writes: > > > which is also why we can't use it, any possibly bogus or suspicious > > value we get from SUDO_UID MUST be ignored. > > I do not think I agree. If we have a strange value in SUDO_UID, it > would be much better and safer to err on the safe side. ignoring it is the safe side; for example if we replace the current function with the proposed one then some user lucky enough to have access to the latest linux supercomputer that has been patched to have a 64-bit uid_t (because who makes 32-bit supercomputers nowadays) would get root[1] access by simply faking his SUDO_UID to be UINT_MAX + 1. We will also honour probably SUDO_UID=0M as root instead of the current action which is to ignore that nonsense and most likely die by telling the pranker that he still can't run `git status` on that root owned repository he got access to even after he managed to get sudo to generate that as a SUDO_UID. > Instead of ignoring, in the situation where we care about the value > we read from SUDO_UID (i.e. when euid==0), we should die loudly when > it has a strange value. that is fair, but then it would then make this feature into a denial of service attack target ;) The current implementation instead keeps git running under the UID it was started as, which should be root if it gets to use this code under the current implementation. I am still open to changing it if you would rather let git be the last line of defense, I just think that the current implementation of ignoring it is more user friendly and better at punking would be attackers. Carlo [1] https://lwn.net/Articles/727490/
Carlo Arenas <carenas@gmail.com> writes: > On Fri, May 6, 2022 at 1:00 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Carlo Arenas <carenas@gmail.com> writes: >> >> > which is also why we can't use it, any possibly bogus or suspicious >> > value we get from SUDO_UID MUST be ignored. >> >> I do not think I agree. If we have a strange value in SUDO_UID, it >> would be much better and safer to err on the safe side. > > ignoring it is the safe side; for example if we replace the current > function with the proposed one then some user lucky enough to have > access to the latest linux supercomputer that has been patched to have > a 64-bit uid_t (because who makes 32-bit supercomputers nowadays) > would get root[1] access by simply faking his SUDO_UID to be UINT_MAX > + 1. Since we do not pay attention to SUDO_UID unless euid is root, anybody who can attack by faking SUDO_UID to affect what Git does can already become root on the box. So such an attacker would already have root access without our help, or they would not. In any case, if we notice that SUDO_UID is not a valid number and die(), we deny the access anyway, so there is no need to write more code to ignore.
On May 6, 2022 4:23 PM, Carlo Arenas wrote: >On Fri, May 6, 2022 at 1:00 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Carlo Arenas <carenas@gmail.com> writes: >> >> > which is also why we can't use it, any possibly bogus or suspicious >> > value we get from SUDO_UID MUST be ignored. >> >> I do not think I agree. If we have a strange value in SUDO_UID, it >> would be much better and safer to err on the safe side. > >ignoring it is the safe side; for example if we replace the current function with the >proposed one then some user lucky enough to have access to the latest linux >supercomputer that has been patched to have a 64-bit uid_t (because who makes >32-bit supercomputers nowadays) would get root[1] access by simply faking his >SUDO_UID to be UINT_MAX >+ 1. > >We will also honour probably SUDO_UID=0M as root instead of the current action >which is to ignore that nonsense and most likely die by telling the pranker that he >still can't run `git status` on that root owned repository he got access to even after >he managed to get sudo to generate that as a SUDO_UID. > >> Instead of ignoring, in the situation where we care about the value we >> read from SUDO_UID (i.e. when euid==0), we should die loudly when it >> has a strange value. > >that is fair, but then it would then make this feature into a denial of service attack >target ;) > >The current implementation instead keeps git running under the UID it was >started as, which should be root if it gets to use this code under the current >implementation. > >I am still open to changing it if you would rather let git be the last line of defense, I >just think that the current implementation of ignoring it is more user friendly and >better at punking would be attackers. Please keep in mind the uid_t == 65535 on __TANDEM. uid_t == 0 actually means "not logged in". Thanks, Randall
On Fri, May 6, 2022 at 2:00 PM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Arenas <carenas@gmail.com> writes: > > > On Fri, May 6, 2022 at 1:00 PM Junio C Hamano <gitster@pobox.com> wrote: > >> > >> Carlo Arenas <carenas@gmail.com> writes: > >> > >> > which is also why we can't use it, any possibly bogus or suspicious > >> > value we get from SUDO_UID MUST be ignored. > >> > >> I do not think I agree. If we have a strange value in SUDO_UID, it > >> would be much better and safer to err on the safe side. > > > > ignoring it is the safe side; for example if we replace the current > > function with the proposed one then some user lucky enough to have > > access to the latest linux supercomputer that has been patched to have > > a 64-bit uid_t (because who makes 32-bit supercomputers nowadays) > > would get root[1] access by simply faking his SUDO_UID to be UINT_MAX > > + 1. > > Since we do not pay attention to SUDO_UID unless euid is root, > anybody who can attack by faking SUDO_UID to affect what Git does > can already become root on the box. Normally yes, but they just might be too clever and found instead a way to fool sudo into generating a bogus SUDO_UID with the hopes that whoever implemented this forgot to check for bogus values and used atoi() and therefore grants them uid = 0 for whatever nefarious objective they might have. While we can't protect against a buggy sudo, we can't do the next best thing and that is to NOT let a would-be attacker still get their 5 min of fame by reporting a vulnerability in git, because they just made it crash through sudo. Especially when the impact of just ignoring them and going our merry way is that (with the current code) we will use the euid of the user that started sudo and "do the right thing" (tm). > In any case, if we notice that SUDO_UID is not a valid number and > die(), we deny the access anyway, so there is no need to write more > code to ignore. not sure which additional code you are referring to here, but currently the only code to ignore is embedded in the one that is needed to read the value, so it is not additional, except maybe for the (uid_t)-1 check that was discussed before[1] and that is what protects us from overflows when sizeof(long) > sizeof(uid_t) && UID_T_MIN >= 0, which are likely most of the systems we are expected to run on. if you are referring to the whole function that could be replaced by using instead git_env_ulong() as dscho suggested, additional code would be needed anyway to make sure that we truncate it appropriately as what we really need is a git_env_uint32() which I am really glad to add if you would rather go that way, but is definitely going to require more code than what we have right now. Note also that we will need to add a flag to it, so it doesn't try to be helpful and allow SUDO_UID=0M to sneak in. Carlo [1] https://lore.kernel.org/git/CAPUEsphFb-=BcV-mxS5RZpJQ8UVq23ni0Lo8tQ4J3TP04B4KQg@mail.gmail.com/
diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt index 6d764fe0ccf..ee558ced8c7 100644 --- a/Documentation/config/safe.txt +++ b/Documentation/config/safe.txt @@ -26,3 +26,12 @@ 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 do so with the uid of the user that is running git itself, +but if git is running as root, it will check first if it might have +been started through `sudo`, and if that is the case, will instead +use the uid of the user that did so. +If that is not what you would prefer and want git to only trust +repositories that are owned by root instead, then you should remove +the `SUDO_UID` variable from root's environment. diff --git a/git-compat-util.h b/git-compat-util.h index 63ba89dd31d..dfdd3e4f81a 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -393,12 +393,50 @@ static inline int git_offset_1st_component(const char *path) #endif #ifndef is_path_owned_by_current_user + +#ifdef __TANDEM +#define ROOT_UID 65535 +#else +#define ROOT_UID 0 +#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 + */ +static inline void extract_id_from_env(const char *env, uid_t *id) +{ + const char *real_uid = getenv(env); + + /* discard any empty values */ + if (real_uid && *real_uid) { + char *endptr; + unsigned long env_id; + int saved_errno = errno; + + errno = 0; + env_id = strtoul(real_uid, &endptr, 10); + if (!errno && !*endptr && env_id <= (uid_t)-1) + *id = env_id; + + errno = saved_errno; + } +} + static inline int is_path_owned_by_current_uid(const char *path) { struct stat st; + uid_t euid; + if (lstat(path, &st)) return 0; - return st.st_uid == geteuid(); + + euid = geteuid(); + if (euid == ROOT_UID) + extract_id_from_env("SUDO_UID", &euid); + + return st.st_uid == euid; } #define is_path_owned_by_current_user is_path_owned_by_current_uid diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh index 6dac7a05cfd..dd659aed4e1 100755 --- a/t/t0034-root-safe-directory.sh +++ b/t/t0034-root-safe-directory.sh @@ -32,7 +32,7 @@ test_expect_success SUDO 'setup' ' ) ' -test_expect_failure SUDO 'sudo git status as original owner' ' +test_expect_success SUDO 'sudo git status as original owner' ' ( cd root/r && git status &&
bdc77d1d685 (Add a function to determine whether a path is owned by the current user, 2022-03-02) checks for the effective uid of the running process using geteuid() but didn't account for cases where that user was root (because git was invoked through sudo or a compatible tool) and the original uid that repository trusted for its config was no longer known, therefore failing the following otherwise safe call: guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty [sudo] password for guy: fatal: unsafe repository ('/home/guy/Software/uncrustify' is owned by someone else) Attempt to detect those cases by using the environment variables that those tools create to keep track of the original user id, and do the ownership check using that instead. This assumes the environment the user is running on after going privileged can't be tampered with, and also adds code to restrict that the new behavior only applies if running as root, therefore keeping the most common case, which runs unprivileged, from changing, but because of that, it will miss cases where sudo (or an equivalent) was used to change to another unprivileged user or where the equivalent tool used to raise privileges didn't track the original id in a sudo compatible way. Reported-by: Guy Maurel <guy.j@maurel.de> Helped-by: SZEDER Gábor <szeder.dev@gmail.com> Helped-by: Randall Becker <rsbecker@nexbridge.com> Helped-by: Phillip Wood <phillip.wood123@gmail.com> Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- Documentation/config/safe.txt | 9 ++++++++ git-compat-util.h | 40 +++++++++++++++++++++++++++++++++- t/t0034-root-safe-directory.sh | 2 +- 3 files changed, 49 insertions(+), 2 deletions(-)