Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

From: Jeff King <peff@peff.net>
Date: Feb 11, 2013, 04:45 UTC
Message-ID: <20130211044511.GA12809@sigill.intra.peff.net>
In-Reply-To: <20130211012016.GA13243@elie.Belkin>

On Sun, Feb 10, 2013 at 05:20:16PM -0800, Jonathan Nieder wrote:

Show 20 quoted lines

> diff --git a/shell.c b/shell.c
> index 84b237fe..3abc2b84 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -63,10 +63,16 @@ static void cd_to_homedir(void)
>  
>  static void run_shell(void)
>  {
> -	int done = 0;
> +	int done = 0, status;
>  	static const char *help_argv[] = { HELP_COMMAND, NULL };
>  	/* Print help if enabled */
> -	run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
> +	status = run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
> +	if (!status)
> +		; /* success */
> +	else if (status == -1 && errno == ENOENT)
> +		; /* help disabled */
> +	else
> +		exit(status);

One final comment on this. I believe we convert an exit code of 127 from the child into ENOENT. So something like:

  #!/bin/sh
  echo >&2 "Sorry, no interactive shells allowed."
  exti 1

would actually go into the "help disabled" code path and accidentally run an interactive shell. I wondered if this is something that might happen accidentally (since the old semantics of "help" were that exit code did not matter), and if there might be security implications to entering an interactive shell. But I think we are OK for two reasons:

  1. An old script would not be trying to exit with failure and
     expecting to abort the interactive session; that is a new feature
     you are adding. So even if we accidentally exit 127 (because the
     old script relied on a missing command), it is not changing the
     semantics.

  2. Even if we accidentally do enter the interactive prompt, it should
     not be a security issue. It is not like you can then run arbitrary
     commands; unless you have put something else into
     ~/git-shell-commands, the user can only run "help" over and over.

Maybe obvious, but I wanted to note it as part of the review. I think we need to be extra careful with thinking through git-shell security implications, since it is a major potential attack surface for many git setups.

-Peff