{"thread":{"id":"24455","subject":"[PATCHv3] Updated patch series for providing mechanism to list available repositories","startedAt":"2010-07-21T15:15:52Z","lastAt":"2010-07-29T00:33:42Z","messageCount":23,"participants":["Greg Brockman","Ævar Arnfjörð Bjarmason","Jonathan Nieder","Johannes Sixt","Jakub Narebski","Anders Kaseorg"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"145936","messageId":"1279725355-23016-1-git-send-email-gdb@mit.edu","threadId":"24455","inReplyTo":null,"subject":"[PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Greg Brockman","fromEmail":"gdb@mit.edu","sentAt":"2010-07-21T15:15:52Z","receivedAt":"2010-07-21T15:15:52Z","isPatch":false,"sender":{"key":"gdb@mit.edu","avatar":"https://gravatar.com/avatar/3b907cf60d0cadf6d0659d5429d971b16965fb6b1e3f5acd26a6ba2974b45da4?d=mp&s=160"},"body":"In this version, I fixed up the problems that Junio noted in my first\npatch.  Per Junio's comments on my second patch (git-shell-commands:\nAdd a command to list bare repos), I added a README file in\ncontrib/git-shell-commands.  I also squashed the commits creating\nfiles in that directory.\n\nI'll note that I realized (and documented in the README) that since\ncommands are actually run out of $(cwd)/git-shell-commands, this might\nnot do what the user expects if run outside of his or her home\ndirectory.  I'm not sure if this is bad behavior; do people have\nthoughts?\n\nOnce again, thanks Junio for you comments.\n"},{"id":"145937","messageId":"1279725355-23016-2-git-send-email-gdb@mit.edu","threadId":"24455","inReplyTo":"1279725355-23016-1-git-send-email-gdb@mit.edu","subject":"[PATCH 1/3] Allow creation of arbitrary git-shell commands","fromName":"Greg Brockman","fromEmail":"gdb@mit.edu","sentAt":"2010-07-21T15:15:53Z","receivedAt":"2010-07-21T15:15:53Z","isPatch":true,"sender":{"key":"gdb@mit.edu","avatar":"https://gravatar.com/avatar/3b907cf60d0cadf6d0659d5429d971b16965fb6b1e3f5acd26a6ba2974b45da4?d=mp&s=160"},"body":"This provides a mechanism for the server to expose custom\nfunctionality to clients.  My particular use case is that I would like\na way of discovering all repositories available for cloning.  A\nclient that clones via\n\n  git clone user@example.com\n\ncan invoke a command by\n\n  ssh user@example.com $command\n\nSigned-off-by: Greg Brockman <gdb@mit.edu>\n---\n shell.c |   34 ++++++++++++++++++++++++++++++++--\n 1 files changed, 32 insertions(+), 2 deletions(-)\n\ndiff --git a/shell.c b/shell.c\nindex e4864e0..34159c4 100644\n--- a/shell.c\n+++ b/shell.c\n@@ -3,6 +3,8 @@\n #include \"exec_cmd.h\"\n #include \"strbuf.h\"\n \n+#define COMMAND_DIR \"git-shell-commands\"\n+\n static int do_generic_cmd(const char *me, char *arg)\n {\n \tconst char *my_argv[4];\n@@ -33,6 +35,20 @@ static int do_cvs_cmd(const char *me, char *arg)\n \treturn execv_git_cmd(cvsserver_argv);\n }\n \n+static int is_valid_cmd_name(const char *cmd)\n+{\n+\t/* Test command contains no . or / characters */\n+\treturn cmd[strcspn(cmd, \"./\")] == '\\0';\n+}\n+\n+static char *make_cmd(const char *prog)\n+{\n+\tchar *prefix = xmalloc((strlen(prog) + strlen(COMMAND_DIR) + 2));\n+\tstrcpy(prefix, COMMAND_DIR);\n+\tstrcat(prefix, \"/\");\n+\tstrcat(prefix, prog);\n+\treturn prefix;\n+}\n \n static struct commands {\n \tconst char *name;\n@@ -48,6 +64,7 @@ static struct commands {\n int main(int argc, char **argv)\n {\n \tchar *prog;\n+\tconst char **user_argv;\n \tstruct commands *cmd;\n \tint devnull_fd;\n \n@@ -76,7 +93,7 @@ int main(int argc, char **argv)\n \telse if (argc != 3 || strcmp(argv[1], \"-c\"))\n \t\tdie(\"What do you think I am? A shell?\");\n \n-\tprog = argv[2];\n+\tprog = xstrdup(argv[2]);\n \tif (!strncmp(prog, \"git\", 3) && isspace(prog[3]))\n \t\t/* Accept \"git foo\" as if the caller said \"git-foo\". */\n \t\tprog[3] = '-';\n@@ -99,5 +116,18 @@ int main(int argc, char **argv)\n \t\t}\n \t\texit(cmd->exec(cmd->name, arg));\n \t}\n-\tdie(\"unrecognized command '%s'\", prog);\n+\n+\tif (split_cmdline(prog, &user_argv) != -1) {\n+\t\tif (is_valid_cmd_name(user_argv[0])) {\n+\t\t\tprog = make_cmd(user_argv[0]);\n+\t\t\tuser_argv[0] = prog;\n+\t\t\texecv(user_argv[0], (char *const *) user_argv);\n+\t\t}\n+\t\tfree(prog);\n+\t\tfree(user_argv);\n+\t\tdie(\"unrecognized command '%s'\", argv[2]);\n+\t} else {\n+\t\tfree(prog);\n+\t\tdie(\"invalid command format '%s'\", argv[2]);\n+\t}\n }\n-- \n1.7.0.4\n"},{"id":"145939","messageId":"1279725355-23016-3-git-send-email-gdb@mit.edu","threadId":"24455","inReplyTo":"1279725355-23016-1-git-send-email-gdb@mit.edu","subject":"[PATCH 2/3] Add interactive mode to git-shell for user-friendliness","fromName":"Greg Brockman","fromEmail":"gdb@mit.edu","sentAt":"2010-07-21T15:15:54Z","receivedAt":"2010-07-21T15:15:54Z","isPatch":true,"sender":{"key":"gdb@mit.edu","avatar":"https://gravatar.com/avatar/3b907cf60d0cadf6d0659d5429d971b16965fb6b1e3f5acd26a6ba2974b45da4?d=mp&s=160"},"body":"Signed-off-by: Greg Brockman <gdb@mit.edu>\n---\n shell.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n 1 files changed, 69 insertions(+), 6 deletions(-)\n\ndiff --git a/shell.c b/shell.c\nindex 34159c4..f839b53 100644\n--- a/shell.c\n+++ b/shell.c\n@@ -2,8 +2,10 @@\n #include \"quote.h\"\n #include \"exec_cmd.h\"\n #include \"strbuf.h\"\n+#include \"run-command.h\"\n \n #define COMMAND_DIR \"git-shell-commands\"\n+#define HELP_COMMAND COMMAND_DIR \"/help\"\n \n static int do_generic_cmd(const char *me, char *arg)\n {\n@@ -50,6 +52,56 @@ static char *make_cmd(const char *prog)\n \treturn prefix;\n }\n \n+static void run_shell(void)\n+{\n+\tint done = 0;\n+\tstatic const char *help_argv[] = { HELP_COMMAND, NULL };\n+\t/* Print help if enabled */\n+\trun_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);\n+\n+\tdo {\n+\t\tstruct strbuf line = STRBUF_INIT;\n+\t\tconst char *prog;\n+\t\tchar *full_cmd;\n+\t\tchar *rawargs;\n+\t\tconst char **argv;\n+\t\tint code;\n+\n+\t\tfprintf(stderr, \"git> \");\n+\t\tif (strbuf_getline(&line, stdin, '\\n') == EOF) {\n+\t\t\tfprintf(stderr, \"\\n\");\n+\t\t\tstrbuf_release(&line);\n+\t\t\tbreak;\n+\t\t}\n+\t\tstrbuf_trim(&line);\n+\t\trawargs = strbuf_detach(&line, NULL);\n+\t\tif (split_cmdline(rawargs, &argv) == -1) {\n+\t\t\tfree(rawargs);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tprog = argv[0];\n+\t\tif (!strcmp(prog, \"\")) {\n+\t\t} else if (!strcmp(prog, \"quit\") || !strcmp(prog, \"logout\") ||\n+\t\t\t   !strcmp(prog, \"exit\") || !strcmp(prog, \"bye\")) {\n+\t\t\tdone = 1;\n+\t\t} else if (is_valid_cmd_name(prog)) {\n+\t\t\tfull_cmd = make_cmd(prog);\n+\t\t\targv[0] = full_cmd;\n+\t\t\tcode = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);\n+\t\t\tif (code == -1 && errno == ENOENT) {\n+\t\t\t\tfprintf(stderr, \"unrecognized command '%s'\\n\", prog);\n+\t\t\t}\n+\t\t\tfree(full_cmd);\n+\t\t} else {\n+\t\t\tfprintf(stderr, \"invalid command format '%s'\\n\", prog);\n+\t\t}\n+\n+\t\tfree(argv);\n+\t\tfree(rawargs);\n+\t} while (!done);\n+}\n+\n static struct commands {\n \tconst char *name;\n \tint (*exec)(const char *me, char *arg);\n@@ -83,15 +135,26 @@ int main(int argc, char **argv)\n \t/*\n \t * Special hack to pretend to be a CVS server\n \t */\n-\tif (argc == 2 && !strcmp(argv[1], \"cvs server\"))\n+\tif (argc == 2 && !strcmp(argv[1], \"cvs server\")) {\n \t\targv--;\n-\n+\t}\n \t/*\n-\t * We do not accept anything but \"-c\" followed by \"cmd arg\",\n-\t * where \"cmd\" is a very limited subset of git commands.\n+\t * Allow the user to run an interactive shell\n \t */\n-\telse if (argc != 3 || strcmp(argv[1], \"-c\"))\n-\t\tdie(\"What do you think I am? A shell?\");\n+\telse if (argc == 1) {\n+\t\tif (access(COMMAND_DIR, R_OK | X_OK) == -1)\n+\t\t\tdie(\"Sorry, the interactive git-shell is not enabled\");\n+\t\trun_shell();\n+\t\texit(0);\n+\t}\n+\t/*\n+\t * We do not accept any other modes except \"-c\" followed by\n+\t * \"cmd arg\", where \"cmd\" is a very limited subset of git\n+\t * commands or a command in the COMMAND_DIR\n+\t */\n+\telse if (argc != 3 || strcmp(argv[1], \"-c\")) {\n+\t\tdie(\"Run with no arguments or with -c cmd\");\n+\t}\n \n \tprog = xstrdup(argv[2]);\n \tif (!strncmp(prog, \"git\", 3) && isspace(prog[3]))\n-- \n1.7.0.4\n"},{"id":"145938","messageId":"1279725355-23016-4-git-send-email-gdb@mit.edu","threadId":"24455","inReplyTo":"1279725355-23016-1-git-send-email-gdb@mit.edu","subject":"[PATCH 3/3] Add sample commands for git-shell","fromName":"Greg Brockman","fromEmail":"gdb@mit.edu","sentAt":"2010-07-21T15:15:55Z","receivedAt":"2010-07-21T15:15:55Z","isPatch":true,"sender":{"key":"gdb@mit.edu","avatar":"https://gravatar.com/avatar/3b907cf60d0cadf6d0659d5429d971b16965fb6b1e3f5acd26a6ba2974b45da4?d=mp&s=160"},"body":"Provide a 'list' command to view available bare repositories ending in\n.git and a 'help command to display usage.  Also add documentation in\na README\n\nSigned-off-by: Greg Brockman <gdb@mit.edu>\n---\n contrib/git-shell-commands/README |   23 +++++++++++++++++++++++\n contrib/git-shell-commands/help   |   16 ++++++++++++++++\n contrib/git-shell-commands/list   |   10 ++++++++++\n 3 files changed, 49 insertions(+), 0 deletions(-)\n create mode 100644 contrib/git-shell-commands/README\n create mode 100755 contrib/git-shell-commands/help\n create mode 100755 contrib/git-shell-commands/list\n\ndiff --git a/contrib/git-shell-commands/README b/contrib/git-shell-commands/README\nnew file mode 100644\nindex 0000000..57d7a79\n--- /dev/null\n+++ b/contrib/git-shell-commands/README\n@@ -0,0 +1,23 @@\n+Sample programs callable through git-shell.  Place a directory named\n+'git-shell-commands' in the home directory of a user whose shell is\n+git-shell.  Then anyone logging in as that user will be able to run\n+executables in the 'git-shell-commands' directory.\n+\n+Note that git-shell assumes its CWD is the user's home directory, so\n+trying to 'su' to a user whose shell is git-shell would result in\n+running commands out of \"$(cwd)/git-shell-commands\", which may not be\n+desired behavior.\n+\n+Provided commands:\n+\n+help: Prints out the names of available commands.  When run\n+interactively, git-shell will automatically run 'help' on startup,\n+provided it exists.\n+\n+list: Displays any bare repository whose name ends with \".git\" under\n+user's home directory.  No other git repositories are visible,\n+although they might be clonable through git-shell.  'list' is designed\n+to minimize the number of calls to git that must be made in finding\n+available repositories; if your setup has additional repositories that\n+should be user-discoverable, you may wish to modify 'list'\n+accordingly.\ndiff --git a/contrib/git-shell-commands/help b/contrib/git-shell-commands/help\nnew file mode 100755\nindex 0000000..a43fcd6\n--- /dev/null\n+++ b/contrib/git-shell-commands/help\n@@ -0,0 +1,16 @@\n+#!/bin/sh\n+\n+if tty -s; then\n+    echo \"Run 'help' for help, or 'exit' to leave.  Available commands:\"\n+else\n+    echo \"Run 'help' for help.  Available commands:\"\n+fi\n+\n+cd \"$(dirname \"$0\")\"\n+\n+for cmd in *; do\n+    case \"$cmd\" in\n+\thelp) ;;\n+\t*) [ -f \"$cmd\" ] && [ -x \"$cmd\" ] && echo \"$cmd\" ;;\n+    esac\n+done\ndiff --git a/contrib/git-shell-commands/list b/contrib/git-shell-commands/list\nnew file mode 100755\nindex 0000000..4654535\n--- /dev/null\n+++ b/contrib/git-shell-commands/list\n@@ -0,0 +1,10 @@\n+#!/bin/sh\n+\n+print_if_bare_repo='\n+       if \"$(git --git-dir=\"$1\" rev-parse --is-bare-repository)\" = true\n+       then\n+               printf \"%s\\n\" \"${1#./}\"\n+       fi\n+'\n+\n+find -type d -name \"*.git\" -exec sh -c \"$print_if_bare_repo\" -- \\{} \\; -prune\n-- \n1.7.0.4\n"},{"id":"146437","messageId":"AANLkTin+EMYHrr11Dba9Mob+b_Dar_cedWmTsDF=AHFt@mail.gmail.com","threadId":"24455","inReplyTo":"1279725355-23016-1-git-send-email-gdb@mit.edu","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Greg Brockman","fromEmail":"gdb@mit.edu","sentAt":"2010-07-26T22:32:39Z","receivedAt":"2010-07-26T22:32:39Z","isPatch":false,"sender":{"key":"gdb@mit.edu","avatar":"https://gravatar.com/avatar/3b907cf60d0cadf6d0659d5429d971b16965fb6b1e3f5acd26a6ba2974b45da4?d=mp&s=160"},"body":"Hi all,\n\nJust sending a reminder about this patch series--I haven't seen any\ncomments on it yet, so I assume it's gotten lost in the flurry of\nother list activity.\n\nThanks!\n\nGreg\n\n\n\nOn Wed, Jul 21, 2010 at 8:15 AM, Greg Brockman <gdb@mit.edu> wrote:\n> In this version, I fixed up the problems that Junio noted in my first\n> patch.  Per Junio's comments on my second patch (git-shell-commands:\n> Add a command to list bare repos), I added a README file in\n> contrib/git-shell-commands.  I also squashed the commits creating\n> files in that directory.\n>\n> I'll note that I realized (and documented in the README) that since\n> commands are actually run out of $(cwd)/git-shell-commands, this might\n> not do what the user expects if run outside of his or her home\n> directory.  I'm not sure if this is bad behavior; do people have\n> thoughts?\n>\n> Once again, thanks Junio for you comments.\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n"},{"id":"146438","messageId":"AANLkTilSqePFPkteFd7DBgmdhqJHfUDuW_qhkbWVVb3Y@mail.gmail.com","threadId":"24455","inReplyTo":"AANLkTin+EMYHrr11Dba9Mob+b_Dar_cedWmTsDF=AHFt@mail.gmail.com","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-26T22:54:28Z","receivedAt":"2010-07-26T22:54:28Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Jul 26, 2010 at 22:32, Greg Brockman <gdb@mit.edu> wrote:\n> Just sending a reminder about this patch series--I haven't seen any\n> comments on it yet, so I assume it's gotten lost in the flurry of\n> other list activity.\n\nIt would probably help if you re-send the entire thing again. It also\nseems to have an unaddressed comment\n(<7vlj96m4mc.fsf@alter.siamese.dyndns.org> from Junio).\n\nIt's also less confusing if the version of your patch series (see\n--subject-prefix in git-format-patch) matches your series. I went\nlooking for v2-v3 on the list, but found that you were just counting\nthe two RFC's as v1-v2.\n\nThings quickly fall from the end of the list archive around\nhere. Don't be afraid to resend. It's also easier to review if some\nmails in the old series have subsequent fixup mails.\n"},{"id":"146439","messageId":"AANLkTikG0e5dtGgMe03s=PpG793B-MkrGjdGa0LuZ5zH@mail.gmail.com","threadId":"24455","inReplyTo":"AANLkTilSqePFPkteFd7DBgmdhqJHfUDuW_qhkbWVVb3Y@mail.gmail.com","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Greg Brockman","fromEmail":"gdb@mit.edu","sentAt":"2010-07-26T23:18:38Z","receivedAt":"2010-07-26T23:18:38Z","isPatch":false,"sender":{"key":"gdb@mit.edu","avatar":"https://gravatar.com/avatar/3b907cf60d0cadf6d0659d5429d971b16965fb6b1e3f5acd26a6ba2974b45da4?d=mp&s=160"},"body":"> It would probably help if you re-send the entire thing again.\nOk, will do so shortly.\n\n> It also seems to have an unaddressed comment\n> (<7vlj96m4mc.fsf@alter.siamese.dyndns.org> from Junio).\nYep, my patch series actually does incorporate that comment.  I didn't\nrespond to that explicitly because I didn't want to spam the list, but\nin the future I'll be sure to respond to comments... I can imagine it\nmakes things much easier for\npeople-who-are-not-the-one-writing-the-patch to follow.\n\n> It's also less confusing if the version of your patch series (see\n> --subject-prefix in git-format-patch) matches your series. I went\n> looking for v2-v3 on the list, but found that you were just counting\n> the two RFC's as v1-v2.\nAh.  Will do in the future.\n\n> Things quickly fall from the end of the list archive around\n> here. Don't be afraid to resend. It's also easier to review if some\n> mails in the old series have subsequent fixup mails.\nOk, sure.  it's good to know that that is acceptable.\n\nThanks,\n\nGreg\n"},{"id":"146441","messageId":"20100726232855.GA3157@burratino","threadId":"24455","inReplyTo":"AANLkTilSqePFPkteFd7DBgmdhqJHfUDuW_qhkbWVVb3Y@mail.gmail.com","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-26T23:28:55Z","receivedAt":"2010-07-26T23:28:55Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Mon, Jul 26, 2010 at 22:32, Greg Brockman <gdb@mit.edu> wrote:\n\n>> Just sending a reminder about this patch series--I haven't seen any\n>> comments on it yet, so I assume it's gotten lost in the flurry of\n>> other list activity.\n>\n> It would probably help if you re-send the entire thing again.\n\nWait wait, it’s only been about five days!\n\nI mean, you are free to re-send, but it is probably better to\nsend a link to the gmane archive, like this:\n\n http://thread.gmane.org/gmane.comp.version-control.git/151398\n\nso people can catch up with the earlier discussion.\n\nIn this case, I am nervous about the impact for existing installations\nwith git-shell deployed.  If a person can smuggle in an unpleasant\ngit-shell-commands directory somehow, the effect would not be good.\nMaybe there should be a way to disable this feature systemwide for the\nparanoid (or maybe not; I’m only vaguely worried).\n\nPatch 1 still uses execv(), which is not available on Windows.\n\nHave you tried out these patches \"in the wild\"?  If so, that would be\ninteresting to hear about.\n\nJonathan\n"},{"id":"146443","messageId":"AANLkTikqA3kNif+7Bi+=xkJ2FgCFAsfCj0N5dft5pnFR@mail.gmail.com","threadId":"24455","inReplyTo":"20100726232855.GA3157@burratino","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Greg Brockman","fromEmail":"gdb@mit.edu","sentAt":"2010-07-27T00:20:28Z","receivedAt":"2010-07-27T00:20:28Z","isPatch":false,"sender":{"key":"gdb@mit.edu","avatar":"https://gravatar.com/avatar/3b907cf60d0cadf6d0659d5429d971b16965fb6b1e3f5acd26a6ba2974b45da4?d=mp&s=160"},"body":">>> Just sending a reminder about this patch series--I haven't seen any\n>>> comments on it yet, so I assume it's gotten lost in the flurry of\n>>> other list activity.\n>>\n>> It would probably help if you re-send the entire thing again.\n>\n> Wait wait, it’s only been about five days!\n>\n> I mean, you are free to re-send, but it is probably better to\n> send a link to the gmane archive, like this:\n>\n>  http://thread.gmane.org/gmane.comp.version-control.git/151398\n>\n> so people can catch up with the earlier discussion.\nHaha, ok.  Any rules of thumb for how long to wait until resending\neverything is appropriate?\n\n> In this case, I am nervous about the impact for existing installations\n> with git-shell deployed.  If a person can smuggle in an unpleasant\n> git-shell-commands directory somehow, the effect would not be good.\n> Maybe there should be a way to disable this feature systemwide for the\n> paranoid (or maybe not; I’m only vaguely worried).\nYou may have a point.  Although, if someone can drop in the\ngit-shell-commands directory, he or she can probably also edit one of\nthe git repo's hooks directories.  I'd be curious to hear others'\nopinions on the matter.\n\n> Patch 1 still uses execv(), which is not available on Windows.\nIt seems to me that the existing git-shell calls execv_git_cmd, which\nuses execvp internally.  I know ~nothing about exec on Windows, but\npresumably it doesn't have just one of execv or execvp.  If it does,\nit would be easy enough to switch the execv to execvp, as the commands\nthat are being run are already guaranteed to have a slash.  Or am I\nmissing something silly again?\n\n> Have you tried out these patches \"in the wild\"?  If so, that would be\n> interesting to hear about.\nNot yet.  My $project has deployed an earlier prototype of the patches\nin our dev environment, but we haven't moved it to prod yet.  We'll\nprobably do that next week.\n\nGreg\n"},{"id":"146444","messageId":"20100727005055.GA3882@burratino","threadId":"24455","inReplyTo":"AANLkTikqA3kNif+7Bi+=xkJ2FgCFAsfCj0N5dft5pnFR@mail.gmail.com","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-27T00:50:55Z","receivedAt":"2010-07-27T00:50:55Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Greg Brockman wrote:\n\n> Haha, ok.  Any rules of thumb for how long to wait until resending\n> everything is appropriate?\n\nNot that I know of.  Just imagine yourself on the receiving end:\nwill the resent patches be a welcome relief from the work of\ndigging up the old ones, or will it be adding to a daunting torrent of\nincoming mail?\n\nHowever:\n\n - When patches have changed greatly since the previous round,\n   there is no easy alternative for continuing discussion beyond\n   sending the re-rolled series.\n\n - When the patches are finally in good enough shape to be pulled,\n   there is no way to have an on-list copy of the version merged\n   available except to resend.\n\nSo in those circumstances, one tends to resend without hesitation.\n\n>> Patch 1 still uses execv(), which is not available on Windows.\n>\n> It seems to me that the existing git-shell calls execv_git_cmd, which\n> uses execvp internally.  I know ~nothing about exec on Windows, but\n> presumably it doesn't have just one of execv or execvp.\n\nSee compat/mingw.h.\n\n> If it does,\n> it would be easy enough to switch the execv to execvp, as the commands\n> that are being run are already guaranteed to have a slash.\n\nYes.\n\n> Not yet.  My $project has deployed an earlier prototype of the patches\n> in our dev environment, but we haven't moved it to prod yet.  We'll\n> probably do that next week.\n\nThanks for the update.\n\nHope that helps,\nJonathan\n"},{"id":"146454","messageId":"201007270916.59210.j.sixt@viscovery.net","threadId":"24455","inReplyTo":"AANLkTikqA3kNif+7Bi+=xkJ2FgCFAsfCj0N5dft5pnFR@mail.gmail.com","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-07-27T07:16:58Z","receivedAt":"2010-07-27T07:16:58Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Dienstag, 27. Juli 2010, Greg Brockman wrote:\n> > Patch 1 still uses execv(), which is not available on Windows.\n>\n> It seems to me that the existing git-shell calls execv_git_cmd, which\n> uses execvp internally.  I know ~nothing about exec on Windows, but\n> presumably it doesn't have just one of execv or execvp.\n\nWindows does have execv. The patch is OK in this regard.\n\n-- Hannes\n"},{"id":"146457","messageId":"m3sk35l2n6.fsf@localhost.localdomain","threadId":"24455","inReplyTo":"AANLkTikG0e5dtGgMe03s=PpG793B-MkrGjdGa0LuZ5zH@mail.gmail.com","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-07-27T09:02:38Z","receivedAt":"2010-07-27T09:02:38Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Greg Brockman <gdb@MIT.EDU> writes:\n\n> > It would probably help if you re-send the entire thing again.\n>\n> Ok, will do so shortly.\n> \n> > It also seems to have an unaddressed comment\n> > (<7vlj96m4mc.fsf@alter.siamese.dyndns.org> from Junio).\n>\n> Yep, my patch series actually does incorporate that comment.  I didn't\n> respond to that explicitly because I didn't want to spam the list, but\n> in the future I'll be sure to respond to comments... I can imagine it\n> makes things much easier for\n> people-who-are-not-the-one-writing-the-patch to follow.\n\nThere are two possible solutions to providing comments about patch (I\nthink they are covered in Documentation/SubmittingPatches).\n\nFirst is reply to email like you would usually do, and below some\ndelimiter, e.g. the \"scissors\" line i.e. '-- >8 --' put the patch\nitself.  You might need to start it with 'Subject:' line if the title\nof the patch is different from the subject of email.\n\nSecond, which I think would be more appropriate in your situation, is\nto put comments about patch, for example how you did address the\ncomments, and/or how the patch changed from previous version in the\narea between '---' delimiter line, and diffstat.  See for example\nhttp://permalink.gmane.org/gmane.comp.version-control.git/151703\n\nHTH\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"146501","messageId":"20100727174105.GA5578@burratino","threadId":"24455","inReplyTo":"201007270916.59210.j.sixt@viscovery.net","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-27T17:41:05Z","receivedAt":"2010-07-27T17:41:05Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Sixt wrote:\n\n> Windows does have execv. The patch is OK in this regard.\n\nThanks, that’s a comfort.  Sorry to spread misinformation.\n---\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 9a8e336..9212a12 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -854,6 +854,11 @@ static void mingw_execve(const char *cmd, char *const *argv, char *const *env)\n \t}\n }\n \n+void mingw_execv(const char *cmd, char *const *argv)\n+{\n+\tmingw_execve(cmd, argv, environ);\n+}\n+\n void mingw_execvp(const char *cmd, char *const *argv)\n {\n \tchar **path = get_path_split();\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 3b2477b..d81b2f3 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -237,6 +237,9 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,\n void mingw_execvp(const char *cmd, char *const *argv);\n #define execvp mingw_execvp\n \n+void mingw_execv(const char *cmd, char *const *argv);\n+#define execv mingw_execv\n+\n static inline unsigned int git_ntohl(unsigned int x)\n { return (unsigned int)ntohl(x); }\n #define ntohl git_ntohl\n-- \n"},{"id":"146549","messageId":"AANLkTikr5jjZJa2irLb2rNew8ngJcv3rhcFV+pNRpRrw@mail.gmail.com","threadId":"24455","inReplyTo":"20100727174105.GA5578@burratino","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Greg Brockman","fromEmail":"gdb@mit.edu","sentAt":"2010-07-27T22:43:09Z","receivedAt":"2010-07-27T22:43:09Z","isPatch":false,"sender":{"key":"gdb@mit.edu","avatar":"https://gravatar.com/avatar/3b907cf60d0cadf6d0659d5429d971b16965fb6b1e3f5acd26a6ba2974b45da4?d=mp&s=160"},"body":"Hmm, ok.  So if I'm not mistaken, the only outstanding issue is\nwhether to provide a way to globally disable git-shell-commands.  Do\nyou have a particular threat model in mind?\n\nGreg\n\n\n\nOn Tue, Jul 27, 2010 at 10:41 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Johannes Sixt wrote:\n>\n>> Windows does have execv. The patch is OK in this regard.\n>\n> Thanks, that’s a comfort.  Sorry to spread misinformation.\n> ---\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 9a8e336..9212a12 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -854,6 +854,11 @@ static void mingw_execve(const char *cmd, char *const *argv, char *const *env)\n>        }\n>  }\n>\n> +void mingw_execv(const char *cmd, char *const *argv)\n> +{\n> +       mingw_execve(cmd, argv, environ);\n> +}\n> +\n>  void mingw_execvp(const char *cmd, char *const *argv)\n>  {\n>        char **path = get_path_split();\n> diff --git a/compat/mingw.h b/compat/mingw.h\n> index 3b2477b..d81b2f3 100644\n> --- a/compat/mingw.h\n> +++ b/compat/mingw.h\n> @@ -237,6 +237,9 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,\n>  void mingw_execvp(const char *cmd, char *const *argv);\n>  #define execvp mingw_execvp\n>\n> +void mingw_execv(const char *cmd, char *const *argv);\n> +#define execv mingw_execv\n> +\n>  static inline unsigned int git_ntohl(unsigned int x)\n>  { return (unsigned int)ntohl(x); }\n>  #define ntohl git_ntohl\n> --\n>\n"},{"id":"146558","messageId":"20100728003336.GA2248@dert.cs.uchicago.edu","threadId":"24455","inReplyTo":"AANLkTikr5jjZJa2irLb2rNew8ngJcv3rhcFV+pNRpRrw@mail.gmail.com","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-28T00:33:36Z","receivedAt":"2010-07-28T00:33:36Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Greg Brockman wrote:\n\n> Hmm, ok.  So if I'm not mistaken, the only outstanding issue is\n> whether to provide a way to globally disable git-shell-commands.  Do\n> you have a particular threat model in mind?\n\nNo, it was only a vague thing.  I do not even use git-shell\nmyself, so it was a vague worry for a scenario I am not even\ninvolved in.  So if you have thought it over and decided it is\nnot an issue, that is good enough for me.\n\nWhat would be most comforting is an explanation like this:\n\n \"Uses not using this feature will not be impacted by patch 1,\n  since all it adds is:\n  \n   - some memory allocation\n   - a call to split_cmdline, which I have audited and\n     seems to be safe\n   - an execv that does not permit . or / characters and so\n     can only run commands from the directory the user is\n     in (which would be safe because...\"\n\nActually if I understand correctly I am not comforted at all,\nbecause a former user at a multi-user installation that only has\ngit-shell access now can suddenly run arbitrary commands from\nthe home directory once git is upgraded.\n\nJonathan\n"},{"id":"146560","messageId":"20100728011045.GB2248@dert.cs.uchicago.edu","threadId":"24455","inReplyTo":"AANLkTikr5jjZJa2irLb2rNew8ngJcv3rhcFV+pNRpRrw@mail.gmail.com","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-28T01:10:45Z","receivedAt":"2010-07-28T01:10:45Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Greg Brockman wrote:\n\n> So if I'm not mistaken, the only outstanding issue is\n\nI forgot to say: thanks for writing this series.  Although\nso far I have been lucky enough not to need to interact with\ngit-shell much[1], when in the future I need to set up or\ninteract with a local hosting site the interface this series\ncreates would be pleasant indeed.\n\n[1] except on the client side through Girocco\nhttp://repo.or.cz/w/girocco.git\n"},{"id":"146577","messageId":"AANLkTik1D45_cHPapbmMMys-V544ssCyoxrs5Fxck7oP@mail.gmail.com","threadId":"24455","inReplyTo":"20100728003336.GA2248@dert.cs.uchicago.edu","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Greg Brockman","fromEmail":"gdb@mit.edu","sentAt":"2010-07-28T06:15:49Z","receivedAt":"2010-07-28T06:15:49Z","isPatch":false,"sender":{"key":"gdb@mit.edu","avatar":"https://gravatar.com/avatar/3b907cf60d0cadf6d0659d5429d971b16965fb6b1e3f5acd26a6ba2974b45da4?d=mp&s=160"},"body":"> No, it was only a vague thing.  I do not even use git-shell\n> myself, so it was a vague worry for a scenario I am not even\n> involved in.  So if you have thought it over and decided it is\n> not an issue, that is good enough for me.\n>\n> What would be most comforting is an explanation like this:\n>\n>  \"Uses not using this feature will not be impacted by patch 1,\n>  since all it adds is:\n>\n>   - some memory allocation\n>   - a call to split_cmdline, which I have audited and\n>     seems to be safe\n>   - an execv that does not permit . or / characters and so\n>     can only run commands from the directory the user is\n>     in (which would be safe because...\"\n>\n> Actually if I understand correctly I am not comforted at all,\n> because a former user at a multi-user installation that only has\n> git-shell access now can suddenly run arbitrary commands from\n> the home directory once git is upgraded.\nSo, I think the full story here is that \"if one can create a\ngit-shell-commands directory in the home directory of a user with\nlogin shell git-shell, then the latter user can then run arbitrary\ncommands.\"  So there's a prerequisite of being able to write to the\ngit-shell user's $HOME, but if I can do that, I can presumably clobber\nthe hooks in the git-shell user's git repositories, which can also\nallow arbitrary commands to be run.  So in some sense, providing this\nfunctionality should be no worse than providing hooks.\n\nThat being said, perhaps one place where I could imagine this being\ndifferent is if:\n- a nonbare repository is created in the git-shell user's $HOME directory\n- an attacker creates a 'git-shell-commands' directory in a commit to\nthe repository\n- someone checks out a commit with the 'git-shell-commands' directory.\n\nOne could avoid this by requiring that git-shell verify that the\nuser's home directory is not a non-bare repository.  However, I don't\nview this as a regression because in this case, the attacker could\ncraft the git-shell user's dotfiles.  This would lead to arbitrary\ncommand execution by e.g. setting the pager to /tmp/myevilscript in\n.manpath and running\n\n  ssh git-shell-user@example.com \"git-upload-pack '--help'\"\n\nThat aside, here's an analysis of my patch series:\nPatch 1 just adds\n* Some memory allocation.\n* A call to split_cmdline.  This splits a string on spaces, respecting\nquotes and escaping via \\.  I have audited it and it seems safe.\n* An execv.  The command name is of the form\n\"git-shell-commands/$CLEAN\" where $CLEAN does not contain . or /.\nThus it can only be run from the current working directory.  This will\nbe the git-shell user's $HOME if git-shell was spawned as a login\nshell.  This will be an arbitrary directory if a user can 'su' to the\ngit-shell user.  (I am however starting to lean towards always\nchdir'ing into the git-shell user's $HOME, do people feel strongly\nabout this in either direction?)\n* An error message.\n\nPatch 2 adds\n* A call to run_shell, but only if the 'git-shell-commands' directory\nis accessible.\n* run_shell runs git-shell-commands/help and then runs in a loop\n* a call to split_cmdline on user supplied input\n* the user can type 'quit', 'exit', etc.. which will terminate the\nshell, returning 0.\n* an execv on a command of the form \"git-shell-commands/$CLEAN\", where\nagain $CLEAN does not contain . or /.\n* an invalid command will restart the command loop\n\nPatch 3 adds a list command and a help command to\ncontrib/git-shell-commands, which will only be used if\ngit-shell-commands is enabled.  (Note: I'd like to make a small change\nto list, namely add a 2>/dev/null to the find command.)\n\nSee anything I'm missing?\n\nThanks,\n\nGreg\n"},{"id":"146578","messageId":"20100728064251.GB743@dert.cs.uchicago.edu","threadId":"24455","inReplyTo":"AANLkTik1D45_cHPapbmMMys-V544ssCyoxrs5Fxck7oP@mail.gmail.com","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-28T06:42:51Z","receivedAt":"2010-07-28T06:42:51Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Greg Brockman wrote:\n\n> That aside, here's an analysis of my patch series:\n> Patch 1 just adds\n[...]\n\nAgh, it’s getting late.  In my last message I completely\nforgot about the make_cmd() step.  Sorry to waste your time\non that.\n\nAnd sorry to waste your time in general --- from your description\nit sounds like this could be summarized by:\n\n patch 1 adds\n  memory allocation, split_cmdline call (innocuous things)\n  execv which will fail if git-shell-commands is not a directory\n\n> This will be an arbitrary directory if a user can 'su' to the\n> git-shell user.\n\nThat would be an odd setup, but I guess with shared repositories\nthere's a reason to do it.\n\n> (I am however starting to lean towards always\n> chdir'ing into the git-shell user's $HOME, do people feel strongly\n> about this in either direction?)\n\nI don't feel strongly either way.  It would be a good way to\nput the worry about that attack vector to rest (if you use\ngetpwent instead of getenv to fetch $HOME).\n\nPatch 2 adds the new run_shell() feature, but it is guarded\nwith access(COMMAND_DIR), so existing installations should not be\naffected.\n\nPatch 3 does not even touch git.\n\n> See anything I'm missing?\n\nNo, it looks good to me.\n\nThanks for the patient explanations.\n\nJonathan\n"},{"id":"146579","messageId":"AANLkTikE16GnoRmJ2mxzYJ7hB+5tUbBjvFmZkj12k9+C@mail.gmail.com","threadId":"24455","inReplyTo":"20100728064251.GB743@dert.cs.uchicago.edu","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Greg Brockman","fromEmail":"gdb@mit.edu","sentAt":"2010-07-28T07:06:58Z","receivedAt":"2010-07-28T07:06:58Z","isPatch":false,"sender":{"key":"gdb@mit.edu","avatar":"https://gravatar.com/avatar/3b907cf60d0cadf6d0659d5429d971b16965fb6b1e3f5acd26a6ba2974b45da4?d=mp&s=160"},"body":"> Agh, it’s getting late.  In my last message I completely\n> forgot about the make_cmd() step.  Sorry to waste your time\n> on that.\nNo problem.  It was good to have some pushback so I had to justify my\nassumptions.\n\n>> This will be an arbitrary directory if a user can 'su' to the\n>> git-shell user.\n>\n> That would be an odd setup, but I guess with shared repositories\n> there's a reason to do it.\n>\n>> (I am however starting to lean towards always\n>> chdir'ing into the git-shell user's $HOME, do people feel strongly\n>> about this in either direction?)\n>\n> I don't feel strongly either way.  It would be a good way to\n> put the worry about that attack vector to rest (if you use\n> getpwent instead of getenv to fetch $HOME).\nSure, I'll add some logic to do this.\n\n> Thanks for the patient explanations.\nNo problem.  Thanks for taking the time to read them :).\n\nAnyway, I'll create an updated version of this patch series that deals\nwith the chdir'ing to the user's home directory, and that includes the\n2>/dev/null line in 'list'.\n"},{"id":"146663","messageId":"1280358894.31999.9.camel@balanced-tree","threadId":"24455","inReplyTo":"20100728064251.GB743@dert.cs.uchicago.edu","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2010-07-28T23:14:54Z","receivedAt":"2010-07-28T23:14:54Z","isPatch":false,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On Wed, 2010-07-28 at 01:42 -0500, Jonathan Nieder wrote:\n> (if you use getpwent instead of getenv to fetch $HOME).\n\nThat seems like it could lead to problems with multiple users with the\nsame UID, and possibly also on Windows.  If it’s important to be\nparanoid here, what about all the other places Git already uses\ngetenv(\"HOME\"), including where it reads ~/.gitconfig?\n\nAnders\n"},{"id":"146666","messageId":"20100728235249.GA29156@dert.cs.uchicago.edu","threadId":"24455","inReplyTo":"1280358894.31999.9.camel@balanced-tree","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-28T23:52:49Z","receivedAt":"2010-07-28T23:52:49Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Anders Kaseorg wrote:\n> On Wed, 2010-07-28 at 01:42 -0500, Jonathan Nieder wrote:\n\n>> (if you use getpwent instead of getenv to fetch $HOME).\n>\n> That seems like it could lead to problems with multiple users with the\n> same UID, and possibly also on Windows.  If it’s important to be\n> paranoid here, what about all the other places Git already uses\n> getenv(\"HOME\"), including where it reads ~/.gitconfig?\n\nThanks for a sanity check.  I do not see the multiple-user problem\n(git-shell is meant to be the login shell, no?) but I think you are\nright about using getwpwent instead of $HOME being a pointless\nprecaution.  My confusion came from a misreading of how 'su' works.\n\nHere was my worry: that a user could do something like this:\n\n $ mkdir /tmp/git-shell-commands\n $ ln -s /bin/sh /tmp/git-shell-commands/sh\n $ HOME=/tmp su git -m -c sh;\t\t# (1)\n\nand get a shell with the privileges of the user with git-shell\nas login shell, which is exactly what a restricted shell like\nthis should be preventing.\n\nNow if that is possible, what is to stop me from this?\n\n $ PAGER=evilscript su git -m -c git-receive-pack --help; # (2)\n\nwhich became possible (modulo the su bit) as an unintended\nconsequence when receive-pack became builtin.\n\nIf I understand the manual correctly, then at least on some\nsystems, luckily su protects correctly against such problems.\n\n\t-m\n\t\tPreserve the current environment.\n\n\t\tIf the target user has a restricted shell,\n\t\tthis option has no effect (unless su is\n\t\tcalled by root).\n\nIs that behavior portable?  It certainly seems like the\nonly sane way to behave.  It’s a moot question for the\ninclusion of this patch series: if we need to worry about\n(1), then it is still not a regression because (2) was possible\nalready.\n\nThe same discussion would seem to apply to ssh with\nPermitUserEnvironment enabled.\n"},{"id":"146668","messageId":"AANLkTikaBoMOEGvLU8FL4Cvw4zBecXytvAnAYTS9GBa3@mail.gmail.com","threadId":"24455","inReplyTo":"20100728235249.GA29156@dert.cs.uchicago.edu","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Greg Brockman","fromEmail":"gdb@mit.edu","sentAt":"2010-07-29T00:21:29Z","receivedAt":"2010-07-29T00:21:29Z","isPatch":false,"sender":{"key":"gdb@mit.edu","avatar":"https://gravatar.com/avatar/3b907cf60d0cadf6d0659d5429d971b16965fb6b1e3f5acd26a6ba2974b45da4?d=mp&s=160"},"body":"Anders brings up a good point.\n\nAnd note that as I alluded to before, there is another attack\n\n$ echo 'DEFINE pager evilscript' > /tmp/.manpath\n$ HOME=/tmp su git -m -c \"git-receive-pack '--help'\" (3)\n\nwhich only requires being able to control HOME.\n\n(Incidentally, I just noticed a segfault with\n\n$ unset HOME\n$ su git -m -c \"git-receive-pack '~'\"\n\nthat's probably worth fixing... if people don't think this is too\npedantic of a case to fix, I'll submit a patch for it in a later\nseries [I think the segfault comes from path.c:expand_user_path].)\n\nAnyway, i'll revise my first patch to use HOME rather than getpw*.\n\nGreg\n\n\n\nOn Wed, Jul 28, 2010 at 4:52 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Anders Kaseorg wrote:\n>> On Wed, 2010-07-28 at 01:42 -0500, Jonathan Nieder wrote:\n>\n>>> (if you use getpwent instead of getenv to fetch $HOME).\n>>\n>> That seems like it could lead to problems with multiple users with the\n>> same UID, and possibly also on Windows.  If it’s important to be\n>> paranoid here, what about all the other places Git already uses\n>> getenv(\"HOME\"), including where it reads ~/.gitconfig?\n>\n> Thanks for a sanity check.  I do not see the multiple-user problem\n> (git-shell is meant to be the login shell, no?) but I think you are\n> right about using getwpwent instead of $HOME being a pointless\n> precaution.  My confusion came from a misreading of how 'su' works.\n>\n> Here was my worry: that a user could do something like this:\n>\n>  $ mkdir /tmp/git-shell-commands\n>  $ ln -s /bin/sh /tmp/git-shell-commands/sh\n>  $ HOME=/tmp su git -m -c sh;           # (1)\n>\n> and get a shell with the privileges of the user with git-shell\n> as login shell, which is exactly what a restricted shell like\n> this should be preventing.\n>\n> Now if that is possible, what is to stop me from this?\n>\n>  $ PAGER=evilscript su git -m -c git-receive-pack --help; # (2)\n>\n> which became possible (modulo the su bit) as an unintended\n> consequence when receive-pack became builtin.\n>\n> If I understand the manual correctly, then at least on some\n> systems, luckily su protects correctly against such problems.\n>\n>        -m\n>                Preserve the current environment.\n>\n>                If the target user has a restricted shell,\n>                this option has no effect (unless su is\n>                called by root).\n>\n> Is that behavior portable?  It certainly seems like the\n> only sane way to behave.  It’s a moot question for the\n> inclusion of this patch series: if we need to worry about\n> (1), then it is still not a regression because (2) was possible\n> already.\n>\n> The same discussion would seem to apply to ssh with\n> PermitUserEnvironment enabled.\n>\n"},{"id":"146670","messageId":"20100729003342.GC29156@dert.cs.uchicago.edu","threadId":"24455","inReplyTo":"AANLkTikaBoMOEGvLU8FL4Cvw4zBecXytvAnAYTS9GBa3@mail.gmail.com","subject":"Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-29T00:33:42Z","receivedAt":"2010-07-29T00:33:42Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Greg Brockman wrote:\n\n> (Incidentally, I just noticed a segfault with\n> \n> $ unset HOME\n> $ su git -m -c \"git-receive-pack '~'\"\n> \n> that's probably worth fixing... if people don't think this is too\n> pedantic of a case to fix, I'll submit a patch for it in a later\n> series [I think the segfault comes from path.c:expand_user_path].)\n\nHere’s a patch to save you time. :)\n\nhttp://git.kernel.org/?p=git/git.git;a=commitdiff;h=79bf149\n\n> Anyway, i'll revise my first patch to use HOME rather than getpw*.\n\nFrom the getpwnam(3) man page it looks like that is best practice,\nanyway.\n\nCheers,\nJonathan\n"}]}