> No, it was only a vague thing. I do not even use git-shell > myself, so it was a vague worry for a scenario I am not even > involved in. So if you have thought it over and decided it is > not an issue, that is good enough for me. > > What would be most comforting is an explanation like this: > > "Uses not using this feature will not be impacted by patch 1, > since all it adds is: > > - some memory allocation > - a call to split_cmdline, which I have audited and > seems to be safe > - an execv that does not permit . or / characters and so > can only run commands from the directory the user is > in (which would be safe because..." > > Actually if I understand correctly I am not comforted at all, > because a former user at a multi-user installation that only has > git-shell access now can suddenly run arbitrary commands from > the home directory once git is upgraded. So, I think the full story here is that "if one can create a git-shell-commands directory in the home directory of a user with login shell git-shell, then the latter user can then run arbitrary commands." So there's a prerequisite of being able to write to the git-shell user's $HOME, but if I can do that, I can presumably clobber the hooks in the git-shell user's git repositories, which can also allow arbitrary commands to be run. So in some sense, providing this functionality should be no worse than providing hooks. That being said, perhaps one place where I could imagine this being different is if: - a nonbare repository is created in the git-shell user's $HOME directory - an attacker creates a 'git-shell-commands' directory in a commit to the repository - someone checks out a commit with the 'git-shell-commands' directory. One could avoid this by requiring that git-shell verify that the user's home directory is not a non-bare repository. However, I don't view this as a regression because in this case, the attacker could craft the git-shell user's dotfiles. This would lead to arbitrary command execution by e.g. setting the pager to /tmp/myevilscript in .manpath and running ssh git-shell-user@xxxxxxxxxxx "git-upload-pack '--help'" That aside, here's an analysis of my patch series: Patch 1 just adds * Some memory allocation. * A call to split_cmdline. This splits a string on spaces, respecting quotes and escaping via \. I have audited it and it seems safe. * An execv. The command name is of the form "git-shell-commands/$CLEAN" where $CLEAN does not contain . or /. Thus it can only be run from the current working directory. This will be the git-shell user's $HOME if git-shell was spawned as a login shell. This will be an arbitrary directory if a user can 'su' to the git-shell user. (I am however starting to lean towards always chdir'ing into the git-shell user's $HOME, do people feel strongly about this in either direction?) * An error message. Patch 2 adds * A call to run_shell, but only if the 'git-shell-commands' directory is accessible. * run_shell runs git-shell-commands/help and then runs in a loop * a call to split_cmdline on user supplied input * the user can type 'quit', 'exit', etc.. which will terminate the shell, returning 0. * an execv on a command of the form "git-shell-commands/$CLEAN", where again $CLEAN does not contain . or /. * an invalid command will restart the command loop Patch 3 adds a list command and a help command to contrib/git-shell-commands, which will only be used if git-shell-commands is enabled. (Note: I'd like to make a small change to list, namely add a 2>/dev/null to the find command.) See anything I'm missing? Thanks, Greg -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html