Re: Limited operations in unsafe repositories

On Thu, Jan 11, 2024 at 02:01:14AM -0500, Jeff King wrote:

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

quoted

On 2024-01-10 at 12:05:31, Jeff King wrote:

quoted

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.

quoted

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).

With the exemption of the repository format, I assume? We have to parse
things like `core.repositoryFormatVersion` and extensions in order to
figure out how a repository has to be accessed. So I agree that we
should not partition config based on safeness, which is going to be a
headache as you rightly point out. But we can partition based on whether
or not config is required in order to access the repository, where the
set of relevant config keys is a whole lot smaller.

Patrick