Re: [PATCH v2 2/3] safe.directory: normalize the configured path

From: Jeff King <peff@peff.net>
Date: Jul 27, 2024, 22:05 UTC
Message-ID: <20240727220524.GA1263829@coredump.intra.peff.net>
In-Reply-To: <xmqqikwsyz6d.fsf@gitster.g>

On Fri, Jul 26, 2024 at 08:02:02AM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > This may be a dumb question, but... will this always work, if the config
> > option is not necessarily an exact path, and might have globbing
> > characters in it?
> >
> > We don't currently treat it like a wildmatch glob, but:
> >
> >   1. We do allow "/*" on the end. Should we strip that off so it doesn't
> >      confuse the realpath lookup?
> 
> This one I wondered, too, but the test seems to show that it is OK ;-)

Hmm. Would it depend on what's in the filesystem, though? That is, if I had a file named "*", would it get confused?

That is unlikely in normal use, of course, but can we do something tricky with it? I think probably no? Accessing "/path/to/*" implies that the user meant to allow all of "/path/to", which would include that. So the most an attacker could do is _disable_ safe.directory for something that should have it on.

Show 7 quoted lines

> >   2. This is shutting the door on using wildmatch or similar in the
> >      future. Is that OK?
> 
> I am inclined to keep this part and any other logic that are meant
> to address "security" things simple and stupid.  So in that sense,
> I am not so worried that it would be hard to retrofit wildmatch to
> this codepath.

That's my general instinct, too. But I wanted to make sure we were explicit about the choice.

Show 8 quoted lines

> > Should we be quiet there, and maybe fall back to using the
> > non-normalized path (though I guess if we could not normalize it, that
> > probably means it could never match anyway)?
> 
> The only reason I did the warning was because it makes me feel a bit
> uneasy to have a configuration item that either gets ignored or
> triggers a fallback behaviour and not to tell users about it.  Other
> than that, I have no strong preference.

Yes, that was my first thought, too, but I noticed the bogus messages while playing with it. I _think_ if it triggers it means that it points to something that doesn't exist (or you don't have permission to read), in which case it would by definition not have matched. So silently ignoring might not be so bad.

-Peff