Re: a problem with git describe

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 26, 2022, 15:41 UTC
Message-ID: <nycvar.QRO.7.76.6.2204261730210.355@tvgsbejvaqbjf.bet>
In-Reply-To: <YmdQcrl/uu7HFBlc@nand.local>

Hi Taylor,

On Mon, 25 Apr 2022, Taylor Blau wrote:

Show 46 quoted lines

> 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