Re: Limited operations in unsafe repositories

From: Jeff King <peff@peff.net>
Date: Jan 11, 2024, 07:01 UTC
Message-ID: <20240111070114.GB48154@coredump.intra.peff.net>
In-Reply-To: <ZZ8pbAMNaBDFgf3G@tapette.crustytoothpaste.net>

On Wed, Jan 10, 2024 at 11:34:04PM +0000, brian m. carlson wrote:

Show 10 quoted lines

> On 2024-01-10 at 12:05:31, Jeff King wrote:
> > My thinking is to flip that around: run all code, but put protection in
> > the spots that do unsafe things, like loading config or examining
> > hooks. I.e., a patch like this:
> 
> I think that's much what I had intended to do with not invoking binaries
> at all, except that it was limited to rev-parse.  I wonder if perhaps we
> could do something similar if we had the `--assume-unsafe` argument you
> proposed, except that we would only allow the `git` binary and always
> pass that argument to it in such a case.

I'm not sure what you mean by "invoking binaries". I had assumed that meant just running other Git sub-processes. But if "--assume-unsafe" is just setting an environment variable, they'd automatically be protected.

> I don't think reading config is intrinsically unsafe; it's more of what
> we do with it, which is spawning external processes, that's the problem.
> I suppose an argument could be made for injecting terminal sequences or
> such, though.  Hooks, obviously, are definitely unsafe.

Right, it's not config itself that's unsafe; it's that many options are. We could try to annotate them to say "it is OK to parse core.eol but not core.pager", presumably with an allow-known-good approach (since so many ard bad!). But that feels like an ongoing maintenance headache, and an easy way to make a mistake (your mention of terminal sequences makes me assume you're thinking of "color.diff.*", etc). A rule like "we do not read repo-level config at all" seems easier to explain (to me, anyway).

-Peff