shell: allow overriding built-in commands

From: Ayman Bagabas <[email protected]>

This patch allows overriding built-in commands by placing a script with the same name under git-shell-commands directory.

This is useful for users who want to extend the built-in commands without replacing the original command binary. For instance, a user wanting to allow only a subset of users to run the git-receive-pack can override the command with a script that checks the user and calls the original command if the user is allowed.

---
[RFC] shell: allow overriding built-in commands

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1930%2Faymanbagabas%2Fshell-override-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1930/aymanbagabas/shell-override-v1
Pull-Request: https://github.com/git/git/pull/1930

 shell.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

index 76333c80686..3a6d2e8a044 100644
@@ -194,9 +194,11 @@ int cmd_main(int argc, const char **argv)
 		/* Accept "git foo" as if the caller said "git-foo". */
 		prog[3] = '-';
 
+	cd_to_homedir();
 	for (cmd = cmd_list ; cmd->name ; cmd++) {
 		int len = strlen(cmd->name);
 		char *arg;
+		char *full_cmd;
 		if (strncmp(cmd->name, prog, len))
 			continue;
 		arg = NULL;

@@ -210,10 +212,16 @@ int cmd_main(int argc, const char **argv)
 		default:
 			continue;
 		}
-		return cmd->exec(cmd->name, arg);
+		/* Allow overriding built-in commands */
+		full_cmd = make_cmd(cmd->name);
+		if (!access(full_cmd, F_OK)) {
+			const char *argv[3] = { cmd->name, arg, NULL };
+			return execv(full_cmd, (char *const *) argv);
+		} else {
+			return cmd->exec(cmd->name, arg);
+		}
 	}
 
-	cd_to_homedir();
 	count = split_cmdline(prog, &user_argv);
 	if (count >= 0) {
 		if (is_valid_cmd_name(user_argv[0])) {

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e

gitgitgadget

Sounds like it'd open a window to generating numerous security vulnerabilities, break git's own commands that exec another git subprocess (e.g. git-stash), make debugging git bug reports harder, and likely break programs that use plumbing commands.

I'd rather we didn't.

How so? The security implications are the same as any script defined under git-shell-commands. Git does not handle authentication nor authorization and it shouldn't do so, and this can allow repository based authorization to happen using git-shell.

Forgive my limited knowledge about git internals but how would this break git's own commands that exec another git subprocess and plumbing commands?

Sorry, I wasn't reading closely enough and missed the part where this was specific to git shell as opposed to a general ability to override built-ins.

From: Ayman Bagabas <[email protected]>

This patch allows overriding the shell built-in commands by placing a script with the same name under git-shell-commands directory.

This is useful for users who want to extend the shell built-in commands without replacing the original command binary. For instance, a user wanting to allow only a subset of users to run the git-receive-pack can override the command with a script that checks the user and calls the original command if the user is allowed.

---
shell: allow overriding built-in commands

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1930%2Faymanbagabas%2Fshell-override-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1930/aymanbagabas/shell-override-v2
Pull-Request: https://github.com/git/git/pull/1930

Range-diff vs v1:

1: 4fe18878afa ! 1: 60c6339e790 [RFC] shell: allow overriding built-in commands
@@ Metadata
Author: Ayman Bagabas <[email protected]>

## Commit message ##
- [RFC] shell: allow overriding built-in commands
+ shell: allow overriding built-in commands

- This patch allows overriding built-in commands by placing a script
- with the same name under git-shell-commands directory.
+ This patch allows overriding the shell built-in commands by placing a
+ script with the same name under git-shell-commands directory.

- This is useful for users who want to extend the built-in commands
+ This is useful for users who want to extend the shell built-in commands
without replacing the original command binary. For instance, a user
- wanting to allow only a subset of users to run the git-receive-pack
- can override the command with a script that checks the user and
- calls the original command if the user is allowed.
+ wanting to allow only a subset of users to run the git-receive-pack can
+ override the command with a script that checks the user and calls the
+ original command if the user is allowed.

- CC: Junio C Hamano <[email protected]>, Jeff King <[email protected]>
- CC: Taylor Blau <[email protected]>

## shell.c ##
@@ shell.c: int cmd_main(int argc, const char **argv)
default:
continue;
}
-- return cmd->exec(cmd->name, arg);
+ /* Allow overriding built-in commands */
+ full_cmd = make_cmd(cmd->name);
+ if (!access(full_cmd, F_OK)) {
+ const char *argv[3] = { cmd->name, arg, NULL };
+ return execv(full_cmd, (char *const *) argv);
-+ } else {
-+ return cmd->exec(cmd->name, arg);
+ }
+ return cmd->exec(cmd->name, arg);
}

- cd_to_homedir();

 shell.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index 76333c80686..76c0157e321 100644
@@ -194,9 +194,11 @@ int cmd_main(int argc, const char **argv)
 		/* Accept "git foo" as if the caller said "git-foo". */
 		prog[3] = '-';
 
+	cd_to_homedir();
 	for (cmd = cmd_list ; cmd->name ; cmd++) {
 		int len = strlen(cmd->name);
 		char *arg;
+		char *full_cmd;
 		if (strncmp(cmd->name, prog, len))
 			continue;
 		arg = NULL;

@@ -210,10 +212,15 @@ int cmd_main(int argc, const char **argv)
 		default:
 			continue;
 		}
+		/* Allow overriding built-in commands */
+		full_cmd = make_cmd(cmd->name);
+		if (!access(full_cmd, F_OK)) {
+			const char *argv[3] = { cmd->name, arg, NULL };
+			return execv(full_cmd, (char *const *) argv);
+		}
 		return cmd->exec(cmd->name, arg);
 	}
 
-	cd_to_homedir();
 	count = split_cmdline(prog, &user_argv);
 	if (count >= 0) {
 		if (is_valid_cmd_name(user_argv[0])) {

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e

gitgitgadget

I'm not at all sure about any security implications, but aside from that, I suspect this:

should use X_OK rather than F_OK.

Chris

Good catch!

Shouldn't we also check for both F_OK?

Any success for R_OK, W_OK, or X_OK implies F_OK.

I'm not a fan of access() calls in general for multiple reasons (they get suggested for security tests in setuid programs, but you race from "access seems OK" to "actually try to access" against anyone who modifies the name-to-underlying-entity binding, so in general the "right" test is "do the operation with the appropriate permissions in place, then see if it actually worked", which is atomic). But in this particular case it's probably not that important (we seem to make a lot of assumptions about security, or perhaps lack of its importance, in git-shell).

Chris

From: Ayman Bagabas <[email protected]>

This patch allows overriding the shell built-in commands by placing a script with the same name under git-shell-commands directory.

This is useful for users who want to extend the shell built-in commands without replacing the original command binary. For instance, a user wanting to allow only a subset of users to run the git-receive-pack can override the command with a script that checks the user and calls the original command if the user is allowed.

---
shell: allow overriding built-in commands

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1930%2Faymanbagabas%2Fshell-override-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1930/aymanbagabas/shell-override-v3
Pull-Request: https://github.com/git/git/pull/1930

Range-diff vs v2:

1: 60c6339e790 ! 1: 7e6996d199e shell: allow overriding built-in commands

     @@ shell.c: int cmd_main(int argc, const char **argv)
       		}
      +		/* Allow overriding built-in commands */
      +		full_cmd = make_cmd(cmd->name);
     -+		if (!access(full_cmd, F_OK)) {
     ++		if (!access(full_cmd, X_OK)) {
      +			const char *argv[3] = { cmd->name, arg, NULL };
      +			return execv(full_cmd, (char *const *) argv);
      +		}

 shell.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index 76333c80686..8c7f4388bd5 100644
@@ -194,9 +194,11 @@ int cmd_main(int argc, const char **argv)
 		/* Accept "git foo" as if the caller said "git-foo". */
 		prog[3] = '-';
 
+	cd_to_homedir();
 	for (cmd = cmd_list ; cmd->name ; cmd++) {
 		int len = strlen(cmd->name);
 		char *arg;
+		char *full_cmd;
 		if (strncmp(cmd->name, prog, len))
 			continue;
 		arg = NULL;

@@ -210,10 +212,15 @@ int cmd_main(int argc, const char **argv)
 		default:
 			continue;
 		}
+		/* Allow overriding built-in commands */
+		full_cmd = make_cmd(cmd->name);
+		if (!access(full_cmd, X_OK)) {
+			const char *argv[3] = { cmd->name, arg, NULL };
+			return execv(full_cmd, (char *const *) argv);
+		}
 		return cmd->exec(cmd->name, arg);
 	}
 
-	cd_to_homedir();
 	count = split_cmdline(prog, &user_argv);
 	if (count >= 0) {
 		if (is_valid_cmd_name(user_argv[0])) {

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e

gitgitgadget

OK. We do not allow users to override normal Git commands with aliases, etc. But in the case of git-shell, those names are really a well-known API that a client is using, and this is the only opportunity an admin has to plug in between the client request and Git just running the command.

So it seems like a reasonable goal. A more restricted approach might be to provide a more formal hook/plugin interface. E.g., to run a hook script with the command name and arguments, and have it return success/failure to allow the to proceed.

That's not quite as flexible (in your approach I could replace what upload-pack is doing entirely, cache its output, and so on). But it might be harder for admins to screw up. I dunno.

Let's look at the patch...

Yeah, we usually try not to be overly flexible for that reason, but given that "git shell" is so limited that rewriting its services wholesale is not all that much of a deal, I think it is OK.

I however wonder if it is worth admins' time and effort to add features to "git-shell" using this new facility, or if they are better off using something more established like gitolite once they want to go fancier beyond what the basic "git-shell" offers.

Yeah, I left my general opinions on git-shell unspoken. ;)

For features, I think you are probably better off with something like gitolite (which I think does have some access control).

For security, I'd be a little scared of git-shell, just because it's not run all that frequently and we've had issues with it before (e.g., integer overflows). If you're taking requests from untrusted clients, you're probably better off configuring http service.

I also imagine there may be restricted shell implementations that are more general and more battle-hardened, that you could configure to only run a few commands. But I haven't looked at that space (because again, I'd suggest just git-over-http).

-Peff

Gitolite is a great software, but it also has its limitations. It couples authentication and authorization in the same system. However, I'm looking for something more flexible that I can plug whatever authentication or authorization system to the mix similar to git-http-backend paired with apache/nginx/h2o/etc.

That's a fair point. Perhaps writing my own restricted shell might be the best solution for what I'm looking for :/

If you know any general restricted shell implementations please do tell. I'm looking for an SSH solution something pluggable like git-http-backend that I can build on top of.

Honestly, git-shell's simplicity is what got my interest at first. The fact that it's not secure and not run frequently can change and be improved.

- Ayman