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

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

[Ayman Bagabas via GitGitGadget]

From: Ayman Bagabas <ayman.bagabas@gmail.com>

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.

CC: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>
CC: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
---
    [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(-)

diff --git a/shell.c b/shell.c
index 76333c80686..3a6d2e8a044 100644
--- a/shell.c
+++ b/shell.c
@@ -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

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

[Elijah Newren]

On Sat, Mar 22, 2025 at 3:02 AM Ayman Bagabas via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Ayman Bagabas <ayman.bagabas@gmail.com>
>
> 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.

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.

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

[Ayman Bagabas]

> On Mar 22, 2025, at 8:39 PM, Elijah Newren <newren@gmail.com> wrote:
> 
> On Sat, Mar 22, 2025 at 3:02 AM Ayman Bagabas via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 
>> From: Ayman Bagabas <ayman.bagabas@gmail.com>
>> 
>> 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.
> 
> 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.

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?

> 
> I'd rather we didn't.

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

[Elijah Newren]

On Sat, Mar 22, 2025 at 11:02 AM Ayman Bagabas <ayman.bagabas@gmail.com> wrote:
>
> > On Mar 22, 2025, at 8:39 PM, Elijah Newren <newren@gmail.com> wrote:
> >
> > On Sat, Mar 22, 2025 at 3:02 AM Ayman Bagabas via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Ayman Bagabas <ayman.bagabas@gmail.com>
> >>
> >> 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.
> >
> > 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.
>
> 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.

[PATCH v2] shell: allow overriding built-in commands

[Ayman Bagabas via GitGitGadget]

From: Ayman Bagabas <ayman.bagabas@gmail.com>

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.

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
---
    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 <ayman.bagabas@gmail.com>
      
       ## 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 <gitster@pobox.com>, Jeff King <peff@peff.net>
     -    CC: Taylor Blau <me@ttaylorr.com>
          Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
      
       ## 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(-)

diff --git a/shell.c b/shell.c
index 76333c80686..76c0157e321 100644
--- a/shell.c
+++ b/shell.c
@@ -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

Re: [PATCH v2] shell: allow overriding built-in commands

[Chris Torek]

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

On Sat, Mar 22, 2025 at 5:13 PM Ayman Bagabas via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> +               if (!access(full_cmd, F_OK)) {

should use X_OK rather than F_OK.

Chris

Re: [PATCH v2] shell: allow overriding built-in commands

[Ayman Bagabas]

> On Mar 23, 2025, at 4:11 AM, Chris Torek <chris.torek@gmail.com> wrote:
> 
> I'm not at all sure about any security implications, but aside from that, I
> suspect this:
> 
> On Sat, Mar 22, 2025 at 5:13 PM Ayman Bagabas via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> +               if (!access(full_cmd, F_OK)) {
> 
> should use X_OK rather than F_OK.

Good catch!

Shouldn't we also check for both F_OK?

> 
> Chris

Re: [PATCH v2] shell: allow overriding built-in commands

[Chris Torek]

On Sun, Mar 23, 2025 at 8:05 AM Ayman Bagabas <ayman.bagabas@gmail.com> wrote:
> Shouldn't we also check [access()] 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

[PATCH v3] shell: allow overriding built-in commands

[Ayman Bagabas via GitGitGadget]

From: Ayman Bagabas <ayman.bagabas@gmail.com>

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.

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
---
    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(-)

diff --git a/shell.c b/shell.c
index 76333c80686..8c7f4388bd5 100644
--- a/shell.c
+++ b/shell.c
@@ -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

Re: [PATCH v3] shell: allow overriding built-in commands

[Jeff King]

On Sun, Mar 23, 2025 at 03:29:30PM +0000, Ayman Bagabas via GitGitGadget wrote:

> From: Ayman Bagabas <ayman.bagabas@gmail.com>
> 
> 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.

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...

> diff --git a/shell.c b/shell.c
> index 76333c80686..8c7f4388bd5 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -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++) {

Hmm, so we have moved the cd_to_homedir() call up, which used to happen
after this loop. This means that when running a builtin command found in
the loop, our working directory will potentially be different now than
it was before your patch.

That seems like an unintended side effect. Though I admit I am not sure
why git-shell would be running in anything but the user's homedir in the
first place.

> +		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);

This leaks full_cmd if the exec call fails, I'd think?

> +			const char *argv[3] = { cmd->name, arg, NULL };
> +			return execv(full_cmd, (char *const *) argv);

So we just stuff "arg" into the argv we pass to the script. But isn't it
supposed to be a shell command, that could have quoted arguments? For
user-defined commands, we call split_cmdline() to get the real array,
and pass it to the sub-program.  For the built-in commands, we seem to
cheat a little and just assume it is a single string, which we pick
apart with sq_dequote().

But either way what your patch is doing seems wrong. Your custom
git-upload-pack (or whatever) script will get passed the quoted value,
and have to unquote itself. I guess if that were documented it _could_
be the right thing, but it seems rather unfriendly and unlike how the
other user-defined commands work (and of course it's not actually
documented).

You also miss out on the option-injection protections from 3ec804490a
(shell: disallow repo names beginning with dash, 2017-04-29). We skip
those for user-defined commands, but I think you'd probably want them
for something meant to be a wrapper around the built-in command.
Likewise the setup_path() magic done by do_generic_cmd().

So it seems like rather than running execv() ourselves here, this should
probably do one of:

  a. Break out of the loop, skipping the built-in command, so that we
     can run it as a regular user-defined command.

  b. Hook into do_generic_cmd() instead, after we've done our de-quoting
     and checked for option injection.

Of the two, I think (b) is probably the least surprising in terms of
what the wrapper script has to do.

If this were just a hook that asked "can we run this command", then none
of this would matter. Running it would be a separate step.

-Peff

Re: [PATCH v3] shell: allow overriding built-in commands

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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.

Re: [PATCH v3] shell: allow overriding built-in commands

[Jeff King]

On Sun, Mar 23, 2025 at 10:27:32PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> 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

Re: [PATCH v3] shell: allow overriding built-in commands

[Ayman Bagabas]

> On Mar 24, 2025, at 11:28 PM, Jeff King <peff@peff.net> wrote:
> 
> On Sun, Mar 23, 2025 at 10:27:32PM -0700, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>> 
>>> 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.
>> 
>> 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).

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.

> 
> 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.

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

> 
> 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).

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