git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: Limited operations in unsafe repositories

From
Patrick Steinhardt <ps@pks.im>
Date
Jan 11, 2024, 07:17 UTC
Message-ID
<ZZ-V-vwnm2hOkrMC@tanuki>
In-Reply-To
<20240111070114.GB48154@coredump.intra.peff.net>
On Thu, Jan 11, 2024 at 02:01:14AM -0500, Jeff King wrote:
Show 29 quoted lines
> On Wed, Jan 10, 2024 at 11:34:04PM +0000, brian m. carlson wrote:
> 
> > 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).

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
Previous: Jeff KingNext: Jeff King
Message 6 of 7 in “Limited operations in unsafe repositories”
  1. brian m. carlsonJan 7, 2024
  2. Jeff KingJan 10, 2024
  3. brian m. carlsonJan 10, 2024
  4. Junio C HamanoJan 11, 2024
  5. Jeff KingJan 11, 2024
  6. Patrick SteinhardtJan 11, 2024
  7. Jeff KingJan 11, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.