On Fri, Nov 17, 2017 at 10:42:52AM -0500, Jeff Hostetler wrote: > > 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
