[PATCHv3] Updated patch series for providing mechanism to list available repositories

[PATCHv3] Updated patch series for providing mechanism to list available repositories

[Greg Brockman]

In this version, I fixed up the problems that Junio noted in my first
patch.  Per Junio's comments on my second patch (git-shell-commands:
Add a command to list bare repos), I added a README file in
contrib/git-shell-commands.  I also squashed the commits creating
files in that directory.

I'll note that I realized (and documented in the README) that since
commands are actually run out of $(cwd)/git-shell-commands, this might
not do what the user expects if run outside of his or her home
directory.  I'm not sure if this is bad behavior; do people have
thoughts?

Once again, thanks Junio for you comments.

[PATCH 1/3] Allow creation of arbitrary git-shell commands

[Greg Brockman]

This provides a mechanism for the server to expose custom
functionality to clients.  My particular use case is that I would like
a way of discovering all repositories available for cloning.  A
client that clones via

  git clone user@example.com

can invoke a command by

  ssh user@example.com $command

Signed-off-by: Greg Brockman <gdb@mit.edu>
---
 shell.c |   34 ++++++++++++++++++++++++++++++++--
 1 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/shell.c b/shell.c
index e4864e0..34159c4 100644
--- a/shell.c
+++ b/shell.c
@@ -3,6 +3,8 @@
 #include "exec_cmd.h"
 #include "strbuf.h"
 
+#define COMMAND_DIR "git-shell-commands"
+
 static int do_generic_cmd(const char *me, char *arg)
 {
 	const char *my_argv[4];
@@ -33,6 +35,20 @@ static int do_cvs_cmd(const char *me, char *arg)
 	return execv_git_cmd(cvsserver_argv);
 }
 
+static int is_valid_cmd_name(const char *cmd)
+{
+	/* Test command contains no . or / characters */
+	return cmd[strcspn(cmd, "./")] == '\0';
+}
+
+static char *make_cmd(const char *prog)
+{
+	char *prefix = xmalloc((strlen(prog) + strlen(COMMAND_DIR) + 2));
+	strcpy(prefix, COMMAND_DIR);
+	strcat(prefix, "/");
+	strcat(prefix, prog);
+	return prefix;
+}
 
 static struct commands {
 	const char *name;
@@ -48,6 +64,7 @@ static struct commands {
 int main(int argc, char **argv)
 {
 	char *prog;
+	const char **user_argv;
 	struct commands *cmd;
 	int devnull_fd;
 
@@ -76,7 +93,7 @@ int main(int argc, char **argv)
 	else if (argc != 3 || strcmp(argv[1], "-c"))
 		die("What do you think I am? A shell?");
 
-	prog = argv[2];
+	prog = xstrdup(argv[2]);
 	if (!strncmp(prog, "git", 3) && isspace(prog[3]))
 		/* Accept "git foo" as if the caller said "git-foo". */
 		prog[3] = '-';
@@ -99,5 +116,18 @@ int main(int argc, char **argv)
 		}
 		exit(cmd->exec(cmd->name, arg));
 	}
-	die("unrecognized command '%s'", prog);
+
+	if (split_cmdline(prog, &user_argv) != -1) {
+		if (is_valid_cmd_name(user_argv[0])) {
+			prog = make_cmd(user_argv[0]);
+			user_argv[0] = prog;
+			execv(user_argv[0], (char *const *) user_argv);
+		}
+		free(prog);
+		free(user_argv);
+		die("unrecognized command '%s'", argv[2]);
+	} else {
+		free(prog);
+		die("invalid command format '%s'", argv[2]);
+	}
 }
-- 
1.7.0.4

[PATCH 2/3] Add interactive mode to git-shell for user-friendliness

[Greg Brockman]

Signed-off-by: Greg Brockman <gdb@mit.edu>
---
 shell.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/shell.c b/shell.c
index 34159c4..f839b53 100644
--- a/shell.c
+++ b/shell.c
@@ -2,8 +2,10 @@
 #include "quote.h"
 #include "exec_cmd.h"
 #include "strbuf.h"
+#include "run-command.h"
 
 #define COMMAND_DIR "git-shell-commands"
+#define HELP_COMMAND COMMAND_DIR "/help"
 
 static int do_generic_cmd(const char *me, char *arg)
 {
@@ -50,6 +52,56 @@ static char *make_cmd(const char *prog)
 	return prefix;
 }
 
+static void run_shell(void)
+{
+	int done = 0;
+	static const char *help_argv[] = { HELP_COMMAND, NULL };
+	/* Print help if enabled */
+	run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+
+	do {
+		struct strbuf line = STRBUF_INIT;
+		const char *prog;
+		char *full_cmd;
+		char *rawargs;
+		const char **argv;
+		int code;
+
+		fprintf(stderr, "git> ");
+		if (strbuf_getline(&line, stdin, '\n') == EOF) {
+			fprintf(stderr, "\n");
+			strbuf_release(&line);
+			break;
+		}
+		strbuf_trim(&line);
+		rawargs = strbuf_detach(&line, NULL);
+		if (split_cmdline(rawargs, &argv) == -1) {
+			free(rawargs);
+			continue;
+		}
+
+		prog = argv[0];
+		if (!strcmp(prog, "")) {
+		} else if (!strcmp(prog, "quit") || !strcmp(prog, "logout") ||
+			   !strcmp(prog, "exit") || !strcmp(prog, "bye")) {
+			done = 1;
+		} else if (is_valid_cmd_name(prog)) {
+			full_cmd = make_cmd(prog);
+			argv[0] = full_cmd;
+			code = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);
+			if (code == -1 && errno == ENOENT) {
+				fprintf(stderr, "unrecognized command '%s'\n", prog);
+			}
+			free(full_cmd);
+		} else {
+			fprintf(stderr, "invalid command format '%s'\n", prog);
+		}
+
+		free(argv);
+		free(rawargs);
+	} while (!done);
+}
+
 static struct commands {
 	const char *name;
 	int (*exec)(const char *me, char *arg);
@@ -83,15 +135,26 @@ int main(int argc, char **argv)
 	/*
 	 * Special hack to pretend to be a CVS server
 	 */
-	if (argc == 2 && !strcmp(argv[1], "cvs server"))
+	if (argc == 2 && !strcmp(argv[1], "cvs server")) {
 		argv--;
-
+	}
 	/*
-	 * We do not accept anything but "-c" followed by "cmd arg",
-	 * where "cmd" is a very limited subset of git commands.
+	 * Allow the user to run an interactive shell
 	 */
-	else if (argc != 3 || strcmp(argv[1], "-c"))
-		die("What do you think I am? A shell?");
+	else if (argc == 1) {
+		if (access(COMMAND_DIR, R_OK | X_OK) == -1)
+			die("Sorry, the interactive git-shell is not enabled");
+		run_shell();
+		exit(0);
+	}
+	/*
+	 * We do not accept any other modes except "-c" followed by
+	 * "cmd arg", where "cmd" is a very limited subset of git
+	 * commands or a command in the COMMAND_DIR
+	 */
+	else if (argc != 3 || strcmp(argv[1], "-c")) {
+		die("Run with no arguments or with -c cmd");
+	}
 
 	prog = xstrdup(argv[2]);
 	if (!strncmp(prog, "git", 3) && isspace(prog[3]))
-- 
1.7.0.4

[PATCH 3/3] Add sample commands for git-shell

[Greg Brockman]

Provide a 'list' command to view available bare repositories ending in
.git and a 'help command to display usage.  Also add documentation in
a README

Signed-off-by: Greg Brockman <gdb@mit.edu>
---
 contrib/git-shell-commands/README |   23 +++++++++++++++++++++++
 contrib/git-shell-commands/help   |   16 ++++++++++++++++
 contrib/git-shell-commands/list   |   10 ++++++++++
 3 files changed, 49 insertions(+), 0 deletions(-)
 create mode 100644 contrib/git-shell-commands/README
 create mode 100755 contrib/git-shell-commands/help
 create mode 100755 contrib/git-shell-commands/list

diff --git a/contrib/git-shell-commands/README b/contrib/git-shell-commands/README
new file mode 100644
index 0000000..57d7a79
--- /dev/null
+++ b/contrib/git-shell-commands/README
@@ -0,0 +1,23 @@
+Sample programs callable through git-shell.  Place a directory named
+'git-shell-commands' in the home directory of a user whose shell is
+git-shell.  Then anyone logging in as that user will be able to run
+executables in the 'git-shell-commands' directory.
+
+Note that git-shell assumes its CWD is the user's home directory, so
+trying to 'su' to a user whose shell is git-shell would result in
+running commands out of "$(cwd)/git-shell-commands", which may not be
+desired behavior.
+
+Provided commands:
+
+help: Prints out the names of available commands.  When run
+interactively, git-shell will automatically run 'help' on startup,
+provided it exists.
+
+list: Displays any bare repository whose name ends with ".git" under
+user's home directory.  No other git repositories are visible,
+although they might be clonable through git-shell.  'list' is designed
+to minimize the number of calls to git that must be made in finding
+available repositories; if your setup has additional repositories that
+should be user-discoverable, you may wish to modify 'list'
+accordingly.
diff --git a/contrib/git-shell-commands/help b/contrib/git-shell-commands/help
new file mode 100755
index 0000000..a43fcd6
--- /dev/null
+++ b/contrib/git-shell-commands/help
@@ -0,0 +1,16 @@
+#!/bin/sh
+
+if tty -s; then
+    echo "Run 'help' for help, or 'exit' to leave.  Available commands:"
+else
+    echo "Run 'help' for help.  Available commands:"
+fi
+
+cd "$(dirname "$0")"
+
+for cmd in *; do
+    case "$cmd" in
+	help) ;;
+	*) [ -f "$cmd" ] && [ -x "$cmd" ] && echo "$cmd" ;;
+    esac
+done
diff --git a/contrib/git-shell-commands/list b/contrib/git-shell-commands/list
new file mode 100755
index 0000000..4654535
--- /dev/null
+++ b/contrib/git-shell-commands/list
@@ -0,0 +1,10 @@
+#!/bin/sh
+
+print_if_bare_repo='
+       if "$(git --git-dir="$1" rev-parse --is-bare-repository)" = true
+       then
+               printf "%s\n" "${1#./}"
+       fi
+'
+
+find -type d -name "*.git" -exec sh -c "$print_if_bare_repo" -- \{} \; -prune
-- 
1.7.0.4

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Greg Brockman]

Hi all,

Just sending a reminder about this patch series--I haven't seen any
comments on it yet, so I assume it's gotten lost in the flurry of
other list activity.

Thanks!

Greg



On Wed, Jul 21, 2010 at 8:15 AM, Greg Brockman <gdb@mit.edu> wrote:
> In this version, I fixed up the problems that Junio noted in my first
> patch.  Per Junio's comments on my second patch (git-shell-commands:
> Add a command to list bare repos), I added a README file in
> contrib/git-shell-commands.  I also squashed the commits creating
> files in that directory.
>
> I'll note that I realized (and documented in the README) that since
> commands are actually run out of $(cwd)/git-shell-commands, this might
> not do what the user expects if run outside of his or her home
> directory.  I'm not sure if this is bad behavior; do people have
> thoughts?
>
> Once again, thanks Junio for you comments.
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 26, 2010 at 22:32, Greg Brockman <gdb@mit.edu> wrote:
> Just sending a reminder about this patch series--I haven't seen any
> comments on it yet, so I assume it's gotten lost in the flurry of
> other list activity.

It would probably help if you re-send the entire thing again. It also
seems to have an unaddressed comment
(<7vlj96m4mc.fsf@alter.siamese.dyndns.org> from Junio).

It's also less confusing if the version of your patch series (see
--subject-prefix in git-format-patch) matches your series. I went
looking for v2-v3 on the list, but found that you were just counting
the two RFC's as v1-v2.

Things quickly fall from the end of the list archive around
here. Don't be afraid to resend. It's also easier to review if some
mails in the old series have subsequent fixup mails.

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Greg Brockman]

> It would probably help if you re-send the entire thing again.
Ok, will do so shortly.

> It also seems to have an unaddressed comment
> (<7vlj96m4mc.fsf@alter.siamese.dyndns.org> from Junio).
Yep, my patch series actually does incorporate that comment.  I didn't
respond to that explicitly because I didn't want to spam the list, but
in the future I'll be sure to respond to comments... I can imagine it
makes things much easier for
people-who-are-not-the-one-writing-the-patch to follow.

> It's also less confusing if the version of your patch series (see
> --subject-prefix in git-format-patch) matches your series. I went
> looking for v2-v3 on the list, but found that you were just counting
> the two RFC's as v1-v2.
Ah.  Will do in the future.

> Things quickly fall from the end of the list archive around
> here. Don't be afraid to resend. It's also easier to review if some
> mails in the old series have subsequent fixup mails.
Ok, sure.  it's good to know that that is acceptable.

Thanks,

Greg

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:
> On Mon, Jul 26, 2010 at 22:32, Greg Brockman <gdb@mit.edu> wrote:

>> Just sending a reminder about this patch series--I haven't seen any
>> comments on it yet, so I assume it's gotten lost in the flurry of
>> other list activity.
>
> It would probably help if you re-send the entire thing again.

Wait wait, it’s only been about five days!

I mean, you are free to re-send, but it is probably better to
send a link to the gmane archive, like this:

 http://thread.gmane.org/gmane.comp.version-control.git/151398

so people can catch up with the earlier discussion.

In this case, I am nervous about the impact for existing installations
with git-shell deployed.  If a person can smuggle in an unpleasant
git-shell-commands directory somehow, the effect would not be good.
Maybe there should be a way to disable this feature systemwide for the
paranoid (or maybe not; I’m only vaguely worried).

Patch 1 still uses execv(), which is not available on Windows.

Have you tried out these patches "in the wild"?  If so, that would be
interesting to hear about.

Jonathan

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Greg Brockman]

>>> Just sending a reminder about this patch series--I haven't seen any
>>> comments on it yet, so I assume it's gotten lost in the flurry of
>>> other list activity.
>>
>> It would probably help if you re-send the entire thing again.
>
> Wait wait, it’s only been about five days!
>
> I mean, you are free to re-send, but it is probably better to
> send a link to the gmane archive, like this:
>
>  http://thread.gmane.org/gmane.comp.version-control.git/151398
>
> so people can catch up with the earlier discussion.
Haha, ok.  Any rules of thumb for how long to wait until resending
everything is appropriate?

> In this case, I am nervous about the impact for existing installations
> with git-shell deployed.  If a person can smuggle in an unpleasant
> git-shell-commands directory somehow, the effect would not be good.
> Maybe there should be a way to disable this feature systemwide for the
> paranoid (or maybe not; I’m only vaguely worried).
You may have a point.  Although, if someone can drop in the
git-shell-commands directory, he or she can probably also edit one of
the git repo's hooks directories.  I'd be curious to hear others'
opinions on the matter.

> Patch 1 still uses execv(), which is not available on Windows.
It seems to me that the existing git-shell calls execv_git_cmd, which
uses execvp internally.  I know ~nothing about exec on Windows, but
presumably it doesn't have just one of execv or execvp.  If it does,
it would be easy enough to switch the execv to execvp, as the commands
that are being run are already guaranteed to have a slash.  Or am I
missing something silly again?

> Have you tried out these patches "in the wild"?  If so, that would be
> interesting to hear about.
Not yet.  My $project has deployed an earlier prototype of the patches
in our dev environment, but we haven't moved it to prod yet.  We'll
probably do that next week.

Greg

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Jonathan Nieder]

Greg Brockman wrote:

> Haha, ok.  Any rules of thumb for how long to wait until resending
> everything is appropriate?

Not that I know of.  Just imagine yourself on the receiving end:
will the resent patches be a welcome relief from the work of
digging up the old ones, or will it be adding to a daunting torrent of
incoming mail?

However:

 - When patches have changed greatly since the previous round,
   there is no easy alternative for continuing discussion beyond
   sending the re-rolled series.

 - When the patches are finally in good enough shape to be pulled,
   there is no way to have an on-list copy of the version merged
   available except to resend.

So in those circumstances, one tends to resend without hesitation.

>> Patch 1 still uses execv(), which is not available on Windows.
>
> It seems to me that the existing git-shell calls execv_git_cmd, which
> uses execvp internally.  I know ~nothing about exec on Windows, but
> presumably it doesn't have just one of execv or execvp.

See compat/mingw.h.

> If it does,
> it would be easy enough to switch the execv to execvp, as the commands
> that are being run are already guaranteed to have a slash.

Yes.

> Not yet.  My $project has deployed an earlier prototype of the patches
> in our dev environment, but we haven't moved it to prod yet.  We'll
> probably do that next week.

Thanks for the update.

Hope that helps,
Jonathan

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Johannes Sixt]

On Dienstag, 27. Juli 2010, Greg Brockman wrote:
> > Patch 1 still uses execv(), which is not available on Windows.
>
> It seems to me that the existing git-shell calls execv_git_cmd, which
> uses execvp internally.  I know ~nothing about exec on Windows, but
> presumably it doesn't have just one of execv or execvp.

Windows does have execv. The patch is OK in this regard.

-- Hannes

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Jakub Narebski]

Greg Brockman <gdb@MIT.EDU> writes:

> > It would probably help if you re-send the entire thing again.
>
> Ok, will do so shortly.
> 
> > It also seems to have an unaddressed comment
> > (<7vlj96m4mc.fsf@alter.siamese.dyndns.org> from Junio).
>
> Yep, my patch series actually does incorporate that comment.  I didn't
> respond to that explicitly because I didn't want to spam the list, but
> in the future I'll be sure to respond to comments... I can imagine it
> makes things much easier for
> people-who-are-not-the-one-writing-the-patch to follow.

There are two possible solutions to providing comments about patch (I
think they are covered in Documentation/SubmittingPatches).

First is reply to email like you would usually do, and below some
delimiter, e.g. the "scissors" line i.e. '-- >8 --' put the patch
itself.  You might need to start it with 'Subject:' line if the title
of the patch is different from the subject of email.

Second, which I think would be more appropriate in your situation, is
to put comments about patch, for example how you did address the
comments, and/or how the patch changed from previous version in the
area between '---' delimiter line, and diffstat.  See for example
http://permalink.gmane.org/gmane.comp.version-control.git/151703

HTH
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Jonathan Nieder]

Johannes Sixt wrote:

> Windows does have execv. The patch is OK in this regard.

Thanks, that’s a comfort.  Sorry to spread misinformation.
---
diff --git a/compat/mingw.c b/compat/mingw.c
index 9a8e336..9212a12 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -854,6 +854,11 @@ static void mingw_execve(const char *cmd, char *const *argv, char *const *env)
 	}
 }
 
+void mingw_execv(const char *cmd, char *const *argv)
+{
+	mingw_execve(cmd, argv, environ);
+}
+
 void mingw_execvp(const char *cmd, char *const *argv)
 {
 	char **path = get_path_split();
diff --git a/compat/mingw.h b/compat/mingw.h
index 3b2477b..d81b2f3 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -237,6 +237,9 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,
 void mingw_execvp(const char *cmd, char *const *argv);
 #define execvp mingw_execvp
 
+void mingw_execv(const char *cmd, char *const *argv);
+#define execv mingw_execv
+
 static inline unsigned int git_ntohl(unsigned int x)
 { return (unsigned int)ntohl(x); }
 #define ntohl git_ntohl
-- 

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Greg Brockman]

Hmm, ok.  So if I'm not mistaken, the only outstanding issue is
whether to provide a way to globally disable git-shell-commands.  Do
you have a particular threat model in mind?

Greg



On Tue, Jul 27, 2010 at 10:41 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Johannes Sixt wrote:
>
>> Windows does have execv. The patch is OK in this regard.
>
> Thanks, that’s a comfort.  Sorry to spread misinformation.
> ---
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 9a8e336..9212a12 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -854,6 +854,11 @@ static void mingw_execve(const char *cmd, char *const *argv, char *const *env)
>        }
>  }
>
> +void mingw_execv(const char *cmd, char *const *argv)
> +{
> +       mingw_execve(cmd, argv, environ);
> +}
> +
>  void mingw_execvp(const char *cmd, char *const *argv)
>  {
>        char **path = get_path_split();
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 3b2477b..d81b2f3 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -237,6 +237,9 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,
>  void mingw_execvp(const char *cmd, char *const *argv);
>  #define execvp mingw_execvp
>
> +void mingw_execv(const char *cmd, char *const *argv);
> +#define execv mingw_execv
> +
>  static inline unsigned int git_ntohl(unsigned int x)
>  { return (unsigned int)ntohl(x); }
>  #define ntohl git_ntohl
> --
>

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Jonathan Nieder]

Greg Brockman wrote:

> Hmm, ok.  So if I'm not mistaken, the only outstanding issue is
> whether to provide a way to globally disable git-shell-commands.  Do
> you have a particular threat model in mind?

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.

Jonathan

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Jonathan Nieder]

Greg Brockman wrote:

> So if I'm not mistaken, the only outstanding issue is

I forgot to say: thanks for writing this series.  Although
so far I have been lucky enough not to need to interact with
git-shell much[1], when in the future I need to set up or
interact with a local hosting site the interface this series
creates would be pleasant indeed.

[1] except on the client side through Girocco
http://repo.or.cz/w/girocco.git

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Greg Brockman]

> 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@example.com "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

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Jonathan Nieder]

Greg Brockman wrote:

> That aside, here's an analysis of my patch series:
> Patch 1 just adds
[...]

Agh, it’s getting late.  In my last message I completely
forgot about the make_cmd() step.  Sorry to waste your time
on that.

And sorry to waste your time in general --- from your description
it sounds like this could be summarized by:

 patch 1 adds
  memory allocation, split_cmdline call (innocuous things)
  execv which will fail if git-shell-commands is not a directory

> This will be an arbitrary directory if a user can 'su' to the
> git-shell user.

That would be an odd setup, but I guess with shared repositories
there's a reason to do it.

> (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?)

I don't feel strongly either way.  It would be a good way to
put the worry about that attack vector to rest (if you use
getpwent instead of getenv to fetch $HOME).

Patch 2 adds the new run_shell() feature, but it is guarded
with access(COMMAND_DIR), so existing installations should not be
affected.

Patch 3 does not even touch git.

> See anything I'm missing?

No, it looks good to me.

Thanks for the patient explanations.

Jonathan

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Greg Brockman]

> Agh, it’s getting late.  In my last message I completely
> forgot about the make_cmd() step.  Sorry to waste your time
> on that.
No problem.  It was good to have some pushback so I had to justify my
assumptions.

>> This will be an arbitrary directory if a user can 'su' to the
>> git-shell user.
>
> That would be an odd setup, but I guess with shared repositories
> there's a reason to do it.
>
>> (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?)
>
> I don't feel strongly either way.  It would be a good way to
> put the worry about that attack vector to rest (if you use
> getpwent instead of getenv to fetch $HOME).
Sure, I'll add some logic to do this.

> Thanks for the patient explanations.
No problem.  Thanks for taking the time to read them :).

Anyway, I'll create an updated version of this patch series that deals
with the chdir'ing to the user's home directory, and that includes the
2>/dev/null line in 'list'.

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Anders Kaseorg]

On Wed, 2010-07-28 at 01:42 -0500, Jonathan Nieder wrote:
> (if you use getpwent instead of getenv to fetch $HOME).

That seems like it could lead to problems with multiple users with the
same UID, and possibly also on Windows.  If it’s important to be
paranoid here, what about all the other places Git already uses
getenv("HOME"), including where it reads ~/.gitconfig?

Anders

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Jonathan Nieder]

Anders Kaseorg wrote:
> On Wed, 2010-07-28 at 01:42 -0500, Jonathan Nieder wrote:

>> (if you use getpwent instead of getenv to fetch $HOME).
>
> That seems like it could lead to problems with multiple users with the
> same UID, and possibly also on Windows.  If it’s important to be
> paranoid here, what about all the other places Git already uses
> getenv("HOME"), including where it reads ~/.gitconfig?

Thanks for a sanity check.  I do not see the multiple-user problem
(git-shell is meant to be the login shell, no?) but I think you are
right about using getwpwent instead of $HOME being a pointless
precaution.  My confusion came from a misreading of how 'su' works.

Here was my worry: that a user could do something like this:

 $ mkdir /tmp/git-shell-commands
 $ ln -s /bin/sh /tmp/git-shell-commands/sh
 $ HOME=/tmp su git -m -c sh;		# (1)

and get a shell with the privileges of the user with git-shell
as login shell, which is exactly what a restricted shell like
this should be preventing.

Now if that is possible, what is to stop me from this?

 $ PAGER=evilscript su git -m -c git-receive-pack --help; # (2)

which became possible (modulo the su bit) as an unintended
consequence when receive-pack became builtin.

If I understand the manual correctly, then at least on some
systems, luckily su protects correctly against such problems.

	-m
		Preserve the current environment.

		If the target user has a restricted shell,
		this option has no effect (unless su is
		called by root).

Is that behavior portable?  It certainly seems like the
only sane way to behave.  It’s a moot question for the
inclusion of this patch series: if we need to worry about
(1), then it is still not a regression because (2) was possible
already.

The same discussion would seem to apply to ssh with
PermitUserEnvironment enabled.

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Greg Brockman]

Anders brings up a good point.

And note that as I alluded to before, there is another attack

$ echo 'DEFINE pager evilscript' > /tmp/.manpath
$ HOME=/tmp su git -m -c "git-receive-pack '--help'" (3)

which only requires being able to control HOME.

(Incidentally, I just noticed a segfault with

$ unset HOME
$ su git -m -c "git-receive-pack '~'"

that's probably worth fixing... if people don't think this is too
pedantic of a case to fix, I'll submit a patch for it in a later
series [I think the segfault comes from path.c:expand_user_path].)

Anyway, i'll revise my first patch to use HOME rather than getpw*.

Greg



On Wed, Jul 28, 2010 at 4:52 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Anders Kaseorg wrote:
>> On Wed, 2010-07-28 at 01:42 -0500, Jonathan Nieder wrote:
>
>>> (if you use getpwent instead of getenv to fetch $HOME).
>>
>> That seems like it could lead to problems with multiple users with the
>> same UID, and possibly also on Windows.  If it’s important to be
>> paranoid here, what about all the other places Git already uses
>> getenv("HOME"), including where it reads ~/.gitconfig?
>
> Thanks for a sanity check.  I do not see the multiple-user problem
> (git-shell is meant to be the login shell, no?) but I think you are
> right about using getwpwent instead of $HOME being a pointless
> precaution.  My confusion came from a misreading of how 'su' works.
>
> Here was my worry: that a user could do something like this:
>
>  $ mkdir /tmp/git-shell-commands
>  $ ln -s /bin/sh /tmp/git-shell-commands/sh
>  $ HOME=/tmp su git -m -c sh;           # (1)
>
> and get a shell with the privileges of the user with git-shell
> as login shell, which is exactly what a restricted shell like
> this should be preventing.
>
> Now if that is possible, what is to stop me from this?
>
>  $ PAGER=evilscript su git -m -c git-receive-pack --help; # (2)
>
> which became possible (modulo the su bit) as an unintended
> consequence when receive-pack became builtin.
>
> If I understand the manual correctly, then at least on some
> systems, luckily su protects correctly against such problems.
>
>        -m
>                Preserve the current environment.
>
>                If the target user has a restricted shell,
>                this option has no effect (unless su is
>                called by root).
>
> Is that behavior portable?  It certainly seems like the
> only sane way to behave.  It’s a moot question for the
> inclusion of this patch series: if we need to worry about
> (1), then it is still not a regression because (2) was possible
> already.
>
> The same discussion would seem to apply to ssh with
> PermitUserEnvironment enabled.
>

Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

[Jonathan Nieder]

Greg Brockman wrote:

> (Incidentally, I just noticed a segfault with
> 
> $ unset HOME
> $ su git -m -c "git-receive-pack '~'"
> 
> that's probably worth fixing... if people don't think this is too
> pedantic of a case to fix, I'll submit a patch for it in a later
> series [I think the segfault comes from path.c:expand_user_path].)

Here’s a patch to save you time. :)

http://git.kernel.org/?p=git/git.git;a=commitdiff;h=79bf149

> Anyway, i'll revise my first patch to use HOME rather than getpw*.

From the getpwnam(3) man page it looks like that is best practice,
anyway.

Cheers,
Jonathan