Hi, Jeff King wrote: > On Thu, Feb 22, 2018 at 11:38:14AM -0800, Jonathan Nieder wrote: >> To be clear, which of the following are you (most) worried about? >> >> 1. being invoked with --help and spawning a pager >> 2. receiving and acting on options between 'git' and 'upload-pack' >> 3. repository discovery >> 4. pager config >> 5. alias discovery >> 6. increased code surface / unknown threats > > My immediate concern is (4). Thanks for clarifying. > But my greater concern is that people who > work on git.c should not have to worry about accidentally violating this > principle when they add a new feature or config option. That sounds like a combination of (6) and insufficient documentation or tests. Ideas for how we can help prevent such accidents? > In other words, it seems like an accident waiting to happen. I'd be more > amenable to it if there was some compelling reason for it to be a > builtin, but I don't see one listed in the commit message. I see only > "let's make it easier to share the code", which AFAICT is equally served > by just lib-ifying the code and calling it from the standalone > upload-pack.c. If we have so little control of the common code used by git commands that could be invoked by a remote user, I think we're in trouble already. I don't think being a builtin vs not makes that significantly different, since there are plenty of builtins that can be triggered by remote users. Further, if we have so little control over the security properties of git.c, what hope do we have of making the rest of libgit.a usable in secure code? In other words, having to pay more attention to the git wrapper from a security pov actually feels to me like a *good* thing. The git wrapper is the entry point to almost all git commands. If it is an accident waiting to happen, then anything that calls git commands is already an accident waiting to happen. So how can we make it not an accident waiting to happen? :) >> Although in most setups the user does not control the config files on >> a server, item (4) looks like a real issue worth solving. I think we >> should introduce a flag to skip looking for pager config. We could >> use it for receive-pack, too. > > There's not much point for receive-pack. It respects hooks, so any > security ship has already sailed there. Yet there are plenty of cases where people who can push are not supposed to have root privilege. I am not worried about hooks specifically (although the changes described at [1] might help and I still plan to work on those) but I am worried about e.g. commandline injection issues. I don't think we can treat receive-pack as out of scope. And to be clear, I don't think you were saying receive-pack *is* out of scope. But you seem to be trying to draw some boundary, where I only see something fuzzier (e.g. if a bug only applies to receive-pack, then that certainly decreases its impact, but it doesn't make the impact go away). Thanks, Jonathan [1] https://public-inbox.org/git/20171002234517.GV19555@xxxxxxxxxxxxxxxxxxxxxxxxx/
