| Message ID | 20220426183105.99779-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] git-compat-util: avoid failing dir ownership checks if running priviledged | expand |
On 4/26/2022 2:31 PM, 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 uid was > root (because git was invoked through sudo or doas) and the effetive id s/effetive/effective > that repositiry trusted for its config was different, therefore failing s/repositiry/repository > the following common 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 those instead. > > This assumes the environment the user is running with after going > priviledged can't be tampered with, and also does the check only for s/priviledged/privileged > root to keep the most common case less complicated, but as a side effect > will miss cases where sudo (or an equivalent) was used to change to > another unpriviledged user. s/unpriviledged/unpriviledged > Sent as an RFC as it has been only lightly tested and because some of > the assumptions I have to make, made me unconfortable. > Ex, in order to make the atoi() calls safe, I was originally doing > is_digit(), but that would require this function to move further down > to work. > > It is also now big enough that would make sense for it to move into > its own compat file and outside for git-compat-util.h, but if that is > done we might not keep the "root uid is not always 0" bits that seem > useful to have for the future. > > getent() is not thread safe, so it might be worth to use an alternative > but that would require a bigger change. These points make sense. It would be worth taking our time and doing some refactoring in advance of this functional change. > IMHO it should have a test added, but not sure where it would fit. I wonder how we could test a multi-user scenario. The tests I added in e47363e5a (t0033: add tests for safe.directory, 2022-04-13) purposefully avoid the user-id functionality. > Original discussion in : > > https://lore.kernel.org/git/4ef9287b-6260-9538-7c89-cffb611520ee@maurel.de/ I agree that the idea behind this change is a good one. The escalation of privilege isn't a huge concern when the "real" user is the same. The only way to trick the root user here is to set an environment variable, in which case they might as well modify PATH and be done with it. > + euid = geteuid(); > + if (euid == ROOT_UID) { > + /* we might have raised our priviledges with sudo or doas */ Similar spelling error here. > + const char *real_uid = getenv("SUDO_UID"); > + if (real_uid && *real_uid) > + euid = atoi(real_uid); > + else { > + real_uid = getenv("DOAS_UID"); > + if (real_uid && *real_uid) > + euid = atoi(real_uid); > + } I imagine that something else could be added here to help Windows users who have elevated to administrator privileges. It will use a completely different mechanism, though, if needed at all. We can delay that for now. Thanks, -Stolee
Derrick Stolee <derrickstolee@github.com> writes: >> Original discussion in : >> >> https://lore.kernel.org/git/4ef9287b-6260-9538-7c89-cffb611520ee@maurel.de/ > > I agree that the idea behind this change is a good one. The escalation > of privilege isn't a huge concern when the "real" user is the same. > The only way to trick the root user here is to set an environment > variable, in which case they might as well modify PATH and be done with > it. How much do we really want to trust SUDO_UID or DOSA_UID are telling the truth, though? >> + euid = geteuid(); >> + if (euid == ROOT_UID) { >> + /* we might have raised our priviledges with sudo or doas */ > > Similar spelling error here. > >> + const char *real_uid = getenv("SUDO_UID"); >> + if (real_uid && *real_uid) >> + euid = atoi(real_uid); >> + else { >> + real_uid = getenv("DOAS_UID"); >> + if (real_uid && *real_uid) >> + euid = atoi(real_uid); >> + } > > I imagine that something else could be added here to help Windows > users who have elevated to administrator privileges. It will use a > completely different mechanism, though, if needed at all. We can > delay that for now. > > Thanks, > -Stolee
On April 26, 2022 3:56 PM, Junio C Hamano wrote: >Subject: Re: [RFC PATCH] git-compat-util: avoid failing dir ownership checks if >running priviledged > >Derrick Stolee <derrickstolee@github.com> writes: > >>> Original discussion in : >>> >>> >>> https://lore.kernel.org/git/4ef9287b-6260-9538-7c89-cffb611520ee@maur >>> el.de/ >> >> I agree that the idea behind this change is a good one. The escalation >> of privilege isn't a huge concern when the "real" user is the same. >> The only way to trick the root user here is to set an environment >> variable, in which case they might as well modify PATH and be done >> with it. > >How much do we really want to trust SUDO_UID or DOSA_UID are telling the >truth, though? > >>> + euid = geteuid(); >>> + if (euid == ROOT_UID) { >>> + /* we might have raised our priviledges with sudo or doas */ >> >> Similar spelling error here. >> >>> + const char *real_uid = getenv("SUDO_UID"); >>> + if (real_uid && *real_uid) >>> + euid = atoi(real_uid); >>> + else { >>> + real_uid = getenv("DOAS_UID"); >>> + if (real_uid && *real_uid) >>> + euid = atoi(real_uid); This should be strtol() instead of atoi(). Putting garbage into DOAS_UID might end up causing some unwanted effects since atoi() could then return 0 or some partial value. The result should also be checked for sanity and the end pointer should point to a '\0'. My team has effectively banned the use of atoi() in new code and is migrating to strtol() or strtoll() as code is touched. >>> + } >> >> I imagine that something else could be added here to help Windows >> users who have elevated to administrator privileges. It will use a >> completely different mechanism, though, if needed at all. We can delay >> that for now. >> >> Thanks, >> -Stolee
On Tue, Apr 26, 2022 at 12:56 PM Junio C Hamano <gitster@pobox.com> wrote: > > How much do we really want to trust SUDO_UID or DOSA_UID are telling > the truth, though? IMHO since we are only trusting this if the EUID is root it would require that the root account was compromised already or running in a tampered environment. for the absolutely paranoid we could trace back the process tree to make sure the current session was indeed created by that tool, but if we are going that way I think that trusting the ownership of the pty as was proposed[1] originally would be simpler and is indeed how other tools (like who) deal with that problem. The advantage of trusting these variables is that we can keep the more common case simpler and avoid the reported regression. Carlo [1] https://lore.kernel.org/git/20220425084003.nf267feurpqyvmsd@carlos-mbp.lan/
On Tue, Apr 26, 2022 at 12:48 PM Derrick Stolee <derrickstolee@github.com> wrote: > These points make sense. It would be worth taking our time and > doing some refactoring in advance of this functional change. Problem with taking our time or making it a bigger change is that it is currently broken in the last maint versions so it would be ideal to backport quickly there as a "regression". > I wonder how we could test a multi-user scenario. The tests I > added in e47363e5a (t0033: add tests for safe.directory, 2022-04-13) > purposefully avoid the user-id functionality. It would need to be a CI only test (where passwordless sudo is allowed) > I imagine that something else could be added here to help Windows > users who have elevated to administrator privileges. It will use a > completely different mechanism, though, if needed at all. We can > delay that for now. Note this function is *NIX only, there is another one for Windows where a similar fix (if needed) could be implemented Carlo
On Tue, Apr 26, 2022 at 1:10 PM <rsbecker@nexbridge.com> wrote: > > Putting garbage into DOAS_UID might end up causing some unwanted effects Since it was the root user who put garbage there, we will have to trust it was not unwanted. My proposal to use is_digit() was to make sure we didn't get garbage from the getenv() call (ex: "") that would confuse the logic, but if there is some sudo version that is saving the uid as "ThisIsGarbage" then that is a bug better handled somewhere else. Agree with you that using strtol is better, but the added checks and logic make it more complicated and go against the assumption made in the commit message that the environment CAN'T be tampered with. Carlo
Carlo Arenas <carenas@gmail.com> writes: > Agree with you that using strtol is better, but the added checks > and logic make it more complicated and go against the assumption > made in the commit message that the environment CAN'T be tampered > with. I think we require strto*l() here, to relieve us from worrying about "is int large enough?" question.
On 4/26/2022 3:48 PM, Derrick Stolee wrote: > On 4/26/2022 2:31 PM, Carlo Marcelo Arenas Belón wrote: >> + const char *real_uid = getenv("SUDO_UID"); >> + if (real_uid && *real_uid) >> + euid = atoi(real_uid); >> + else { >> + real_uid = getenv("DOAS_UID"); >> + if (real_uid && *real_uid) >> + euid = atoi(real_uid); >> + } > > I imagine that something else could be added here to help Windows > users who have elevated to administrator privileges. It will use a > completely different mechanism, though, if needed at all. We can > delay that for now. Just chiming in to say that the way we do this check on Windows, there is no difference between a user and the same user with elevated privileges (the "Security Identifier (SID)" is the same in both cases). Thanks, -Stolee
diff --git a/git-compat-util.h b/git-compat-util.h index 58fd813bd01..2ed97b47979 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -437,12 +437,34 @@ 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 + 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) { + /* we might have raised our priviledges with sudo or doas */ + const char *real_uid = getenv("SUDO_UID"); + if (real_uid && *real_uid) + euid = atoi(real_uid); + else { + real_uid = getenv("DOAS_UID"); + if (real_uid && *real_uid) + euid = atoi(real_uid); + } + } + return st.st_uid == euid; } #define is_path_owned_by_current_user is_path_owned_by_current_uid
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 uid was root (because git was invoked through sudo or doas) and the effetive id that repositiry trusted for its config was different, therefore failing the following common 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 those instead. This assumes the environment the user is running with after going priviledged can't be tampered with, and also does the check only for root to keep the most common case less complicated, but as a side effect will miss cases where sudo (or an equivalent) was used to change to another unpriviledged user. Reported-by: Guy Maurel <guy.j@maurel.de> Helped-by: SZEDER Gábor <szeder.dev@gmail.com> Helped-by: Randall Becker <rsbecker@nexbridge.com> Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- Sent as an RFC as it has been only lightly tested and because some of the assumptions I have to make, made me unconfortable. Ex, in order to make the atoi() calls safe, I was originally doing is_digit(), but that would require this function to move further down to work. It is also now big enough that would make sense for it to move into its own compat file and outside for git-compat-util.h, but if that is done we might not keep the "root uid is not always 0" bits that seem useful to have for the future. getent() is not thread safe, so it might be worth to use an alternative but that would require a bigger change. IMHO it should have a test added, but not sure where it would fit. Original discussion in : https://lore.kernel.org/git/4ef9287b-6260-9538-7c89-cffb611520ee@maurel.de/ git-compat-util.h | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)