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

[Jeff King <peff@peff.net>]

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

> 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