a problem with git describe

a problem with git describe

[Guy Maurel]

Hello!

I am using git version 2.25.1

6-8 weeks ago I don't get any problem.
Now I get :
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)

What is to do?

Thanks for any help
guy
-- 
Guy Maurel
Sebastian-Fischer-Weg 13
89077 Ulm/ Germany

Re: a problem with git describe

[Philip Oakley]

Hi Guy,

On 23/04/2022 10:12, Guy Maurel wrote:
> Hello!
>
> I am using git version 2.25.1
>
> 6-8 weeks ago I don't get any problem.
> Now I get :
> 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)
>
> What is to do?
>
There was a security warning noted in recent maintenance releases, see
https://lore.kernel.org/git/20220414003839.1616296-2-gitster@pobox.com/
where users could be fooled to access unsafe repositories that they
didn't own.

You may have received a maintenance update that makes this check now the
default.

Could you check what `git version` reports? 

There has also been added an escape hatch of allowing "*" for the
permitted safe directories. but do check the updated manuals, and the
git mailing list archive (update the search in the above link).
--
Philip

Re: a problem with git describe

[Junio C Hamano]

Philip Oakley <philipoakley@iee.email> writes:

>> guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty
> ...
> There has also been added an escape hatch of allowing "*" for the
> permitted safe directories. but do check the updated manuals, and the
> git mailing list archive (update the search in the above link).

In this particular case, I do not think '*' is needed, but you need
to be careful here.  Whose configuration are you suggesting to add
such an entry?  Yourself?  ~root/.gitconfig?

I wonder if we should loosen "the same owner" check somewhat to
cover this situation better.  I expect people also run the
installation in repositories they own with "sudo make install",
and complaining "euid does not own that repository" when it is
merely because they are running as root (and their real identity
is still in ruid) feels a bit too strict to be useful.

Dscho, what do you think?

Re: a problem with git describe

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Philip Oakley <philipoakley@iee.email> writes:
>
>>> guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty
>> ...
>> There has also been added an escape hatch of allowing "*" for the
>> permitted safe directories. but do check the updated manuals, and the
>> git mailing list archive (update the search in the above link).
>
> In this particular case, I do not think '*' is needed, but you need
> to be careful here.  Whose configuration are you suggesting to add
> such an entry?  Yourself?  ~root/.gitconfig?
>
> I wonder if we should loosen "the same owner" check somewhat to
> cover this situation better.  I expect people also run the
> installation in repositories they own with "sudo make install",
> and complaining "euid does not own that repository" when it is
> merely because they are running as root (and their real identity
> is still in ruid) feels a bit too strict to be useful.

Actually, not quite.  when "git" runs in "sudo git", the real
identity has long lost, so the below would not help.  Sigh.

 git-compat-util.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git i/git-compat-util.h w/git-compat-util.h
index 63ba89dd31..90dc1b17cd 100644
--- i/git-compat-util.h
+++ w/git-compat-util.h
@@ -398,7 +398,7 @@ static inline int is_path_owned_by_current_uid(const char *path)
 	struct stat st;
 	if (lstat(path, &st))
 		return 0;
-	return st.st_uid == geteuid();
+	return st.st_uid == geteuid() || st.st_uid == getuid();
 }
 
 #define is_path_owned_by_current_user is_path_owned_by_current_uid

Re: a problem with git describe

[Carlo Marcelo Arenas Belón]

On Sat, Apr 23, 2022 at 04:44:57PM -0700, Junio C Hamano wrote:
> 
> Actually, not quite.  when "git" runs in "sudo git", the real
> identity has long lost

Right, but in this specific case, the terminal is still a good indication
of who the user is, so the following would work.

diff --git a/git-compat-util.h b/git-compat-util.h
index 58fd813bd01..5d5d91688ee 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -442,6 +442,11 @@ static inline int is_path_owned_by_current_uid(const char *path)
 	struct stat st;
 	if (lstat(path, &st))
 		return 0;
+	if (isatty(1)) {
+		struct stat ttyst;
+		if (!stat(ttyname(1), &ttyst))
+			return st.st_uid == ttyst.st_uid;
+	}
 	return st.st_uid == geteuid();
 }
 
It obviously needs more polishing and portability work, though, and I don't
like that it makes the general case more complicated, so maybe would be better
to only do it running as root?

At that point, though you might as well excempt root from this check

Carlo

Re: a problem with git describe

[SZEDER Gábor]

On Sun, Apr 24, 2022 at 07:01:08PM -0700, Carlo Marcelo Arenas Belón wrote:
> On Sat, Apr 23, 2022 at 04:44:57PM -0700, Junio C Hamano wrote:
> > 
> > Actually, not quite.  when "git" runs in "sudo git", the real
> > identity has long lost
> 
> Right, but in this specific case, the terminal is still a good indication
> of who the user is, so the following would work.
> 
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 58fd813bd01..5d5d91688ee 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -442,6 +442,11 @@ static inline int is_path_owned_by_current_uid(const char *path)
>  	struct stat st;
>  	if (lstat(path, &st))
>  		return 0;
> +	if (isatty(1)) {
> +		struct stat ttyst;
> +		if (!stat(ttyname(1), &ttyst))
> +			return st.st_uid == ttyst.st_uid;
> +	}
>  	return st.st_uid == geteuid();
>  }

Our 'GIT-VERSION-GEN' runs 'var=$(git describe ...)', so standard
output is not a terminal during 'sudo make install'

> It obviously needs more polishing and portability work, though, and I don't
> like that it makes the general case more complicated, so maybe would be better
> to only do it running as root?
> 
> At that point, though you might as well excempt root from this check
> 
> Carlo

Re: a problem with git describe

[Carlo Marcelo Arenas Belón]

On Mon, Apr 25, 2022 at 07:05:49AM +0200, SZEDER Gábor wrote:
> On Sun, Apr 24, 2022 at 07:01:08PM -0700, Carlo Marcelo Arenas Belón wrote:
> > On Sat, Apr 23, 2022 at 04:44:57PM -0700, Junio C Hamano wrote:
> > > 
> > > Actually, not quite.  when "git" runs in "sudo git", the real
> > > identity has long lost
> > 
> > Right, but in this specific case, the terminal is still a good indication
> > of who the user is, so the following would work.
> > 
> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index 58fd813bd01..5d5d91688ee 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -442,6 +442,11 @@ static inline int is_path_owned_by_current_uid(const char *path)
> >  	struct stat st;
> >  	if (lstat(path, &st))
> >  		return 0;
> > +	if (isatty(1)) {
> > +		struct stat ttyst;
> > +		if (!stat(ttyname(1), &ttyst))
> > +			return st.st_uid == ttyst.st_uid;
> > +	}
> >  	return st.st_uid == geteuid();
> >  }
> 
> Our 'GIT-VERSION-GEN' runs 'var=$(git describe ...)', so standard
> output is not a terminal during 'sudo make install'

didn't realize the orginal report was coming from `sudo make install`
in git's own repository, but even that worked for me.

when I meant the original case I went with an artificial test where
I run :

  $ mkdir -p r/t
  $ sudo chown root r
  $ cd r/t
  $ git init
  $ sudo git status

and I get no error, unlike the original version of the code.

of course, part of the "extra work needed" is to figure out if stdout
is the best place to look at, I chose it just because that is the
unoficial way to signal in our code that we are running interactively.

note that usually unless a pipe is involved or nohup is used, the tty
is still attached to your process.

Carlo

Re: a problem with git describe

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> At that point, though you might as well excempt root from this check

But "root" or any higher-valued account is what needs this kind of
protection the most, no?  The protection is *not* about people
knowingly accessing their own repository via "root", but those who
are in a directory without being aware of its repository-ness.  You
prepare /var/tmp/.git/ and let others do things that they would do
in /var/tmp/ throw-away directories normally and get affected by
what you leave in /var/tmp/.git/config file.  If "root" is among
these "others", that sounds like you caught a bigger fish X-<.

Re: a problem with git describe

[Carlo Marcelo Arenas Belón]

On Sun, Apr 24, 2022 at 11:39:27PM -0700, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> 
> > At that point, though you might as well excempt root from this check
> 
> But "root" or any higher-valued account is what needs this kind of
> protection the most, no?

correct, and I didn't meant to excempt root from the protection, but
from the check that requires that the config file ownership matches.

if the config file is owned by root, we already lost, regardless of what
uid git is running as.

FWIW the proposed change doesn't weaken the current protection, it just
allows a git process that is running as root (through sudo) to figure
out what real user was the terminal session running as, so it wouldn't
incorrectly triggeer an error.

> The protection is *not* about people
> knowingly accessing their own repository via "root",

nope, but the regression that was described by the original post is

Carlo

Re: a problem with git describe

[Carlo Marcelo Arenas Belón]

On Mon, Apr 25, 2022 at 12:02:45AM -0700, Carlo Marcelo Arenas Belón wrote:
> On Sun, Apr 24, 2022 at 11:39:27PM -0700, Junio C Hamano wrote:
> > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> > 
> > > At that point, though you might as well excempt root from this check
> > 
> > But "root" or any higher-valued account is what needs this kind of
> > protection the most, no?
> 
> correct, and I didn't meant to excempt root from the protection, but
> from the check that requires that the config file ownership matches.
> 
> if the config file is owned by root, we already lost, regardless of what
> uid git is running as.

apologies for my confusing english, hopefully this C is clearer

diff --git a/git-compat-util.h b/git-compat-util.h
index 58fd813bd01..6a385be7d1d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -440,9 +440,19 @@ static inline int git_offset_1st_component(const char *path)
 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 && st.st_uid && isatty(0)) {
+		struct stat ttyst;
+		if (!stat(ttyname(0), &ttyst))
+			euid = ttyst.st_uid;
+	}
+
+	return st.st_uid == euid;
 }
 
 #define is_path_owned_by_current_user is_path_owned_by_current_uid

it uses stdin instead not to fall in the issue that was raised by
Gábor, but I am affraid that it might need to check all stdnandles for
a valid tty to be safe, and it looking even more complex.

Carlo

Re: a problem with git describe

[Guy Maurel]

Hello!

It might be important for your debugging work.
I introduce a new file /root/.gitconfig
-rw-r--r--  1 root root   57 Apr 25 17:03 .gitconfig
with
[safe]
         directory = /home/guy/Software/uncrustify

And the call works pretty well:
uncrustify/build $ sudo ~/Software/Git/2.36.0/tar-gz/git-2.36.0/git-describe
uncrustify-0.74.0-315-ga3466c92

Thanks for helping!
guy
Am 25.04.22 um 10:40 schrieb Carlo Marcelo Arenas Belón:
> On Mon, Apr 25, 2022 at 12:02:45AM -0700, Carlo Marcelo Arenas Belón wrote:
>> On Sun, Apr 24, 2022 at 11:39:27PM -0700, Junio C Hamano wrote:
>>> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
>>>
>>>> At that point, though you might as well excempt root from this check
>>>
>>> But "root" or any higher-valued account is what needs this kind of
>>> protection the most, no?
>>
>> correct, and I didn't meant to excempt root from the protection, but
>> from the check that requires that the config file ownership matches.
>>
>> if the config file is owned by root, we already lost, regardless of what
>> uid git is running as.
> 
> apologies for my confusing english, hopefully this C is clearer
> 
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 58fd813bd01..6a385be7d1d 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -440,9 +440,19 @@ static inline int git_offset_1st_component(const char *path)
>   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 && st.st_uid && isatty(0)) {
> +		struct stat ttyst;
> +		if (!stat(ttyname(0), &ttyst))
> +			euid = ttyst.st_uid;
> +	}
> +
> +	return st.st_uid == euid;
>   }
>   
>   #define is_path_owned_by_current_user is_path_owned_by_current_uid
> 
> it uses stdin instead not to fall in the issue that was raised by
> Gábor, but I am affraid that it might need to check all stdnandles for
> a valid tty to be safe, and it looking even more complex.
> 
> Carlo

-- 
Guy Maurel
Sebastian-Fischer-Weg 13
89077 Ulm

Re: a problem with git describe

[Taylor Blau]

On Sat, Apr 23, 2022 at 09:09:59AM -0700, Junio C Hamano wrote:
> Philip Oakley <philipoakley@iee.email> writes:
>
> >> guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty
> > ...
> > There has also been added an escape hatch of allowing "*" for the
> > permitted safe directories. but do check the updated manuals, and the
> > git mailing list archive (update the search in the above link).
>
> In this particular case, I do not think '*' is needed, but you need
> to be careful here.  Whose configuration are you suggesting to add
> such an entry?  Yourself?  ~root/.gitconfig?
>
> I wonder if we should loosen "the same owner" check somewhat to
> cover this situation better.  I expect people also run the
> installation in repositories they own with "sudo make install",
> and complaining "euid does not own that repository" when it is
> merely because they are running as root (and their real identity
> is still in ruid) feels a bit too strict to be useful.

I was thinking about this today and realized that my original train of
thought along the lines of "ignore the new safety check when the current
user has higher permissions than the user who owns the repository we're
working in" was misguided.

What about loosening the check in a different way? Instead of causing
Git to abort early, what if we:

  - skipped reading the repository's configuration and hooks (unless
    safe.directory includes $CWD)
  - emit a warning (which goes away when safe.directory contains $CWD)
  - otherwise continue executing as normal

That would unbreak the case of $(git describe ...) in our Makefile in
this instance, without opening ourselves up to execution-via-config.

Though I think we'd have to be slightly more careful than that, since
we definitely _do_ want to read repository format extensions.

I'm hesitant to recommend reading some parts of the configuration
without others, though this is slightly different than that. Instead of
saying "read every entry of config except core.editor, core.pager,
core.alternateRefsCommand, core.fsmonitor" and so on, this says "when
operating in a repository not owned by the current user (or the
repository is in our global safe.directory list) only read repository
format extensions, but ignore everything else in config and hooks".

> Dscho, what do you think?

I'd be curious to hear what Johannes thinks of your original mail as
well as my own.

Thanks,
Taylor

Re: a problem with git describe

[Johannes Schindelin]

Hi Junio,

On Sat, 23 Apr 2022, Junio C Hamano wrote:

> Philip Oakley <philipoakley@iee.email> writes:
>
> >> guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty
> > ...
> > There has also been added an escape hatch of allowing "*" for the
> > permitted safe directories. but do check the updated manuals, and the
> > git mailing list archive (update the search in the above link).
>
> In this particular case, I do not think '*' is needed, but you need
> to be careful here.  Whose configuration are you suggesting to add
> such an entry?  Yourself?  ~root/.gitconfig?

One relatively simple work-around might be to call `sudo chown root .`
before running `sudo make install`, but of course this would require the
`rm -rf` to be run via `sudo`, too.

> I wonder if we should loosen "the same owner" check somewhat to
> cover this situation better.  I expect people also run the
> installation in repositories they own with "sudo make install",
> and complaining "euid does not own that repository" when it is
> merely because they are running as root (and their real identity
> is still in ruid) feels a bit too strict to be useful.

It's too bad that your fix to include euid seems not to work. That would
have been my preference.

Do we want to make use of the environment variable `SUDO_UID` that is set
by `sudo`?

Ciao,
Dscho

Re: a problem with git describe

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sat, 23 Apr 2022, Junio C Hamano wrote:
>
>> Philip Oakley <philipoakley@iee.email> writes:
>>
>> >> guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty
>> > ...
>> > There has also been added an escape hatch of allowing "*" for the
>> > permitted safe directories. but do check the updated manuals, and the
>> > git mailing list archive (update the search in the above link).
>>
>> In this particular case, I do not think '*' is needed, but you need
>> to be careful here.  Whose configuration are you suggesting to add
>> such an entry?  Yourself?  ~root/.gitconfig?
>
> One relatively simple work-around might be to call `sudo chown root .`
> before running `sudo make install`, but of course this would require the
> `rm -rf` to be run via `sudo`, too.

chown root may make it owned by nobody4, though ;-)

> Do we want to make use of the environment variable `SUDO_UID` that is set
> by `sudo`?

"run this command under 'sudo'" would be a social engineering attack
we do not want to defend against, so I am OK with that, but then
allowing

    sudo "GIT_SAFE_DIRECTORIES=. make install"

does not look too bad, either.

Re: a problem with git describe

[Johannes Schindelin]

Hi Taylor,

On Mon, 25 Apr 2022, Taylor Blau wrote:

> On Sat, Apr 23, 2022 at 09:09:59AM -0700, Junio C Hamano wrote:
> > Philip Oakley <philipoakley@iee.email> writes:
> >
> > >> guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty
> > > ...
> > > There has also been added an escape hatch of allowing "*" for the
> > > permitted safe directories. but do check the updated manuals, and the
> > > git mailing list archive (update the search in the above link).
> >
> > In this particular case, I do not think '*' is needed, but you need
> > to be careful here.  Whose configuration are you suggesting to add
> > such an entry?  Yourself?  ~root/.gitconfig?
> >
> > I wonder if we should loosen "the same owner" check somewhat to
> > cover this situation better.  I expect people also run the
> > installation in repositories they own with "sudo make install",
> > and complaining "euid does not own that repository" when it is
> > merely because they are running as root (and their real identity
> > is still in ruid) feels a bit too strict to be useful.
>
> I was thinking about this today and realized that my original train of
> thought along the lines of "ignore the new safety check when the current
> user has higher permissions than the user who owns the repository we're
> working in" was misguided.
>
> What about loosening the check in a different way? Instead of causing
> Git to abort early, what if we:
>
>   - skipped reading the repository's configuration and hooks (unless
>     safe.directory includes $CWD)
>   - emit a warning (which goes away when safe.directory contains $CWD)
>   - otherwise continue executing as normal
>
> That would unbreak the case of $(git describe ...) in our Makefile in
> this instance, without opening ourselves up to execution-via-config.
>
> Though I think we'd have to be slightly more careful than that, since
> we definitely _do_ want to read repository format extensions.
>
> I'm hesitant to recommend reading some parts of the configuration
> without others, though this is slightly different than that. Instead of
> saying "read every entry of config except core.editor, core.pager,
> core.alternateRefsCommand, core.fsmonitor" and so on, this says "when
> operating in a repository not owned by the current user (or the
> repository is in our global safe.directory list) only read repository
> format extensions, but ignore everything else in config and hooks".

This idea to disable "just the unsafe parts" has come up before, and I do
not really like it. It would change Git's behavior in inconsistent,
hard-to-explain ways. For example, we would have to disable clean/smudge
filters, which would then break, say, Git LFS.

Sure, for something like `core.fsmonitor` which is a pure performance
knob, where Git works correctly whether you heed that setting or not (just
with different performance characteristics), it might be fine. But for
things like the clean/smudge filters, it changes behavior. And the worst
part? If we introduce something like this logic ("if the worktree is owned
by somebody else, just ignore the parts of the config that refer to
user-defined programs to be executed"), we give wrong results _without
having a way to tell the user that we do that_. Unless you want to see a
warning "you may see incorrect results" every time you run `sudo git
status`?

Don't get me wrong, we _will_ want to have a serious discussion about the
config and how we let features creep in that can be exploited if an
adversary gains write access to one of the various places from where Git
reads config, and how we can mitigate that. IIRC we used to treat the
config as something safe "because it does not execute code", this came up
e.g. when we introduced `.gitmodules`.

I just don't think that we can use "is this thing owned by us?" as a knob
that asks Git to ignore all config settings referring to paths of
executables it is supposed to run at one stage or another.

Ciao,
Dscho

Re: a problem with git describe

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1928 bytes --]

Hi Carlo,

On Mon, 25 Apr 2022, Carlo Marcelo Arenas Belón wrote:

> On Mon, Apr 25, 2022 at 12:02:45AM -0700, Carlo Marcelo Arenas Belón wrote:
> > On Sun, Apr 24, 2022 at 11:39:27PM -0700, Junio C Hamano wrote:
> > > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> > >
> > > > At that point, though you might as well excempt root from this check
> > >
> > > But "root" or any higher-valued account is what needs this kind of
> > > protection the most, no?
> >
> > correct, and I didn't meant to excempt root from the protection, but
> > from the check that requires that the config file ownership matches.
> >
> > if the config file is owned by root, we already lost, regardless of what
> > uid git is running as.
>
> apologies for my confusing english, hopefully this C is clearer
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 58fd813bd01..6a385be7d1d 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -440,9 +440,19 @@ static inline int git_offset_1st_component(const char *path)
>  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 && st.st_uid && isatty(0)) {
> +		struct stat ttyst;
> +		if (!stat(ttyname(0), &ttyst))
> +			euid = ttyst.st_uid;
> +	}
> +
> +	return st.st_uid == euid;
>  }
>
>  #define is_path_owned_by_current_user is_path_owned_by_current_uid
>
> it uses stdin instead not to fall in the issue that was raised by
> Gábor, but I am affraid that it might need to check all stdnandles for
> a valid tty to be safe, and it looking even more complex.

Maybe a better idea for the `sudo` scenario would be to make use of
`SUDO_UID` (assuming that no adversary can gain control over the user's
environment variables)?

Ciao,
Dscho

Re: a problem with git describe

[Johannes Schindelin]

Hi Junio,

On Sat, 23 Apr 2022, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> > Philip Oakley <philipoakley@iee.email> writes:
> >
> >>> guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty
> >> ...
> >> There has also been added an escape hatch of allowing "*" for the
> >> permitted safe directories. but do check the updated manuals, and the
> >> git mailing list archive (update the search in the above link).
> >
> > In this particular case, I do not think '*' is needed, but you need
> > to be careful here.  Whose configuration are you suggesting to add
> > such an entry?  Yourself?  ~root/.gitconfig?
> >
> > I wonder if we should loosen "the same owner" check somewhat to
> > cover this situation better.  I expect people also run the
> > installation in repositories they own with "sudo make install",
> > and complaining "euid does not own that repository" when it is
> > merely because they are running as root (and their real identity
> > is still in ruid) feels a bit too strict to be useful.
>
> Actually, not quite.  when "git" runs in "sudo git", the real
> identity has long lost, so the below would not help.  Sigh.

Could you help me understand what is going on exactly? How/when is `git`
running `sudo git`? I thought the problem was that `sudo make install`
transitively runs `git describe` with euid 0, but `getuid()` should still
return the non-admin user's ID, no?

Thanks,
Dscho

>
>  git-compat-util.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git i/git-compat-util.h w/git-compat-util.h
> index 63ba89dd31..90dc1b17cd 100644
> --- i/git-compat-util.h
> +++ w/git-compat-util.h
> @@ -398,7 +398,7 @@ static inline int is_path_owned_by_current_uid(const char *path)
>  	struct stat st;
>  	if (lstat(path, &st))
>  		return 0;
> -	return st.st_uid == geteuid();
> +	return st.st_uid == geteuid() || st.st_uid == getuid();
>  }
>
>  #define is_path_owned_by_current_user is_path_owned_by_current_uid
>

Re: a problem with git describe

[Taylor Blau]

On Tue, Apr 26, 2022 at 05:41:36PM +0200, Johannes Schindelin wrote:
> This idea to disable "just the unsafe parts" has come up before, and I do
> not really like it. It would change Git's behavior in inconsistent,
> hard-to-explain ways. For example, we would have to disable clean/smudge
> filters, which would then break, say, Git LFS.

I share your sentiment on the "don't read the unsafe parts" bit.

I think replacing our behavior from 2.35.2 from "bail on every execution
of Git in an unowned directory not listed in safe.directory" to "emit a
warning and don't read config/hooks in an unowned [...]" is a little
friendlier in the sense that it would avoid breaking too many workflows
without opening us up to the attack described and mitigate by that
release.

A warning is definitely appropriate to tell users when we are and aren't
reading config. I strongly dislike "read some parts of config and not
others" though, since it seems (a) even harder to explain and
inconsistent than this, and (b) extremely error prone and fragile in the
sense that we are unlikely to completely enumerate every possible config
-> execution path in git-config(1).

Thanks,
Taylor

RE: a problem with git describe

[rsbecker]

On April 26, 2022 11:44 AM, Johannes Schindelin wrote:
>On Mon, 25 Apr 2022, Carlo Marcelo Arenas Belón wrote:
>
>> On Mon, Apr 25, 2022 at 12:02:45AM -0700, Carlo Marcelo Arenas Belón wrote:
>> > On Sun, Apr 24, 2022 at 11:39:27PM -0700, Junio C Hamano wrote:
>> > > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
>> > >
>> > > > At that point, though you might as well excempt root from this
>> > > > check
>> > >
>> > > But "root" or any higher-valued account is what needs this kind of
>> > > protection the most, no?
>> >
>> > correct, and I didn't meant to excempt root from the protection, but
>> > from the check that requires that the config file ownership matches.
>> >
>> > if the config file is owned by root, we already lost, regardless of
>> > what uid git is running as.
>>
>> apologies for my confusing english, hopefully this C is clearer
>>
>> diff --git a/git-compat-util.h b/git-compat-util.h index
>> 58fd813bd01..6a385be7d1d 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -440,9 +440,19 @@ static inline int git_offset_1st_component(const
>> char *path)  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 && st.st_uid && isatty(0)) {
>> +		struct stat ttyst;
>> +		if (!stat(ttyname(0), &ttyst))
>> +			euid = ttyst.st_uid;
>> +	}
>> +
>> +	return st.st_uid == euid;
>>  }
>>
>>  #define is_path_owned_by_current_user is_path_owned_by_current_uid
>>
>> it uses stdin instead not to fall in the issue that was raised by
>> Gábor, but I am affraid that it might need to check all stdnandles for
>> a valid tty to be safe, and it looking even more complex.
>
>Maybe a better idea for the `sudo` scenario would be to make use of `SUDO_UID`
>(assuming that no adversary can gain control over the user's environment
>variables)?

Please do not assume that the user id 0 represents root. It does not on all systems. This change will cause breakages. We would need a specific comparison in compat.c with platform-specific values.
For example:

#if defined __TANDEM
#define ROOT_UID 65535
#define ROOT_GID 255
#endif

--Randall

Re: a problem with git describe

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Actually, not quite.  when "git" runs in "sudo git", the real
>> identity has long lost, so the below would not help.  Sigh.
>
> Could you help me understand what is going on exactly? How/when is `git`
> running `sudo git`? I thought the problem was that `sudo make install`
> transitively runs `git describe` with euid 0, but `getuid()` should still
> return the non-admin user's ID, no?

The first "git" is literal English meaning, refers to a person ;-)

Anybody building and installing git would do

	$ sudo make
	$ sudo make install

and no, sudo does not leave a clue in getuid() from whom it came from.

Re: a problem with git describe

[Carlo Arenas]

On Tue, Apr 26, 2022 at 8:43 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Maybe a better idea for the `sudo` scenario would be to make use of
> `SUDO_UID` (assuming that no adversary can gain control over the user's
> environment variables)?

I like this idea, and will prepare a formal patch soon also including DOAS_UID
for anyone that might have used that instead of sudo to elevate their
privileges.

Still think that (since we are already touching this) removing the
restriction to
root owned directories might make sense though, ex the following (unrealistic
example) would work:

  $ mkdir -p r/t
  $ sudo chown root r
  $ cd r && sudo git init
  $ cd t && git status

IMHO any directories above that are owned by root and might have a git
configuration must be safe, and therefore shouldn't need to be
explicitly exempted.

I also think that Taylor's suggestion to ignore and warn instead of
abort might be
a better solution to the "configuration file shouldn't be trusted"
issue which is at
the root of this regression, and obviously doing that would make this
change less
critical (users will just get a warning instead of being prevented to
do what worked
for them before and is a safe use case)

Carlo

PS. yes hardcoding uid 0 as root would be avoided, not sure where yet though

Re: a problem with git describe

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> Still think that (since we are already touching this) removing the
> restriction to
> root owned directories might make sense though, ex the following (unrealistic
> example) would work:

I think it is essential to protect unsuspecting "root" user from
wandering into an unfamiliar directory that happens to be a trap,
i.e. I may do something like this as an admin:

    $ sudo sh
    # cd / && find usr bin ... >/var/tmp/mylist.txt
    # cd /var/tmp

with the expectation that I'd then do some text processing on the
mylist.txt file, and going there first would allow me to refer to
the files more easily, instead of having to say:

    # sed -e '... processing ...' </var/tmp/mylist.txt >/var/tmp/out.txt

Alas, you as an attacker have done

    $ cd /var/tmp
    $ git init
    $ edit .git/config

to wait for me.  /var/tmp would be owned by 'root' but allows
anybody to write to it, only forbidding people from removing other
people's stuff.



>   $ mkdir -p r/t
>   $ sudo chown root r
>   $ cd r && sudo git init
>   $ cd t && git status
>
> IMHO any directories above that are owned by root and might have a git
> configuration must be safe, and therefore shouldn't need to be
> explicitly exempted.
>
> I also think that Taylor's suggestion to ignore and warn instead of
> abort might be
> a better solution to the "configuration file shouldn't be trusted"
> issue which is at
> the root of this regression, and obviously doing that would make this
> change less
> critical (users will just get a warning instead of being prevented to
> do what worked
> for them before and is a safe use case)
>
> Carlo
>
> PS. yes hardcoding uid 0 as root would be avoided, not sure where yet though

Re: a problem with git describe

[Carlo Arenas]

On Tue, Apr 26, 2022 at 9:46 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Arenas <carenas@gmail.com> writes:
>
> > Still think that (since we are already touching this) removing the
> > restriction to
> > root owned directories might make sense though, ex the following (unrealistic
> > example) would work:
>
> I think it is essential to protect unsuspecting "root" user from
> wandering into an unfamiliar directory that happens to be a trap,
> i.e. I may do something like this as an admin:
>
>     $ sudo sh
>     # cd / && find usr bin ... >/var/tmp/mylist.txt
>     # cd /var/tmp
>
> with the expectation that I'd then do some text processing on the
> mylist.txt file, and going there first would allow me to refer to
> the files more easily, instead of having to say:
>
>     # sed -e '... processing ...' </var/tmp/mylist.txt >/var/tmp/out.txt
>
> Alas, you as an attacker have done

Which is a career that has ended prematurely, it seems.

>     $ cd /var/tmp
>     $ git init
>     $ edit .git/config
>
> to wait for me.  /var/tmp would be owned by 'root' but allows
> anybody to write to it, only forbidding people from removing other
> people's stuff.

Since we are doing stat in that directory anyway, we should be able
to notice the sticky bit and not fall from that trap, but I get why
"exempting root" would be a bad idea.

Carlo