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

Re: [PATCH v4 4/6] list-objects: filter objects in traverse_commit_list

From
Jeff King <peff@peff.net>
Date
Nov 17, 2017, 22:19 UTC
Message-ID
<20171117221936.zsokhvmslnf6jete@sigill.intra.peff.net>
In-Reply-To
<6f433987-f91b-d5b5-242e-3a241b7442c7@jeffhostetler.com>
On Fri, Nov 17, 2017 at 10:42:52AM -0500, Jeff Hostetler wrote:
Show 16 quoted lines
> > Yes, I share the same feeling.  It does not help that the series
> > defines its own notion of arg_needs_armor() and uses it to set a
> > field called requires_armor that is not yet used, the definition of
> > "armor"ing being each byte getting encoded as two hexadecimal digits
> > without any sign (which makes me wonder what a receiver of
> > "deadbeef" would do---did it receive an armored string or a plain
> > one???).  I do not understand why these strings are not passed as
> > opaque sequences of bytes and instead converted at this low a layer.
> 
> I'm probably being too paranoid.  My fear is that a client could pass
> an expression to clone/fetch/fetch-pack that would be sent to the
> server and evaluated by the interface between upload-pack and pack-objects.
> I'm not worried about the pack-protocol transport.  I'm mainly concerned
> in how upload-pack passes that *client-expression* to pack-objects and are
> there ways for that to go south on the server with a carefully crafted
> expression.

I think you have to trust that those interfaces are capable of passing raw bytes, whether directly via execve() or because we got the quoting right. If there's a bug there, it's going to be a bigger problem than just this code path (and the fix needs to be there, not second-guessing it in the callers).

So I'd say that yeah, you are being too paranoid.

As an aside, though, I wonder if these client expressions should be fed over stdin to pack-objects. That removes any argv limits we might run into on the server side. It also removes shell injections as a possibility, though of course we'd need quoting in that layer to avoid argument-injection to pack-objects.

-Peff
Previous: Jeff Hostetler
Message 16 of 16 in “Partial clone part 1: object filtering”
  1. 0/6 Partial clone part 1: object filteringJeff Hostetler, Nov 16, 2017
  2. 1/6 dir: allow exclusions from blob in addition to fileJeff Hostetler, Nov 16, 2017
  3. 3/6 oidset: add iterator methods to oidsetJeff Hostetler, Nov 16, 2017
  4. 2/6 oidmap: add oidmap iterator methodsJeff Hostetler, Nov 16, 2017
  5. 5/6 rev-list: add list-objects filtering supportJeff Hostetler, Nov 16, 2017
  6. Jonathan TanNov 16, 2017
  7. Junio C HamanoNov 17, 2017
  8. Jeff HostetlerNov 17, 2017
  9. 6/6 pack-objects: add list-objects filteringJeff Hostetler, Nov 16, 2017
  10. 4/6 list-objects: filter objects in traverse_commit_listJeff Hostetler, Nov 16, 2017
  11. Jonathan TanNov 16, 2017
  12. Jeff HostetlerNov 16, 2017
  13. Jeff KingNov 16, 2017
  14. Junio C HamanoNov 17, 2017
  15. Jeff HostetlerNov 17, 2017
  16. Jeff KingNov 17, 2017

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.