From: Omar Polo <op@omarpolo.com>
Subject: Re: dial fix for git-shell
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Wed, 08 Mar 2023 16:01:20 +0100

On 2023/03/08 15:21:42 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Sun, Feb 19, 2023 at 10:18:40AM +0100, Stefan Sperling wrote:
> > On Sat, Feb 18, 2023 at 05:15:38PM +0100, Omar Polo wrote:
> > > since we're apparently in the quoting business I guess we may need
> > > something more robust that handles paths with ' in them.
> > > 
> > > Maybe this is the reason git-shell is so picky.
> > > 
> > > could be even done as a separate step, fwiw ok op@ if you want to just
> > > commit this.
> > 
> > I will think some more about this. I really don't like having to deal
> > with quoting issues at this layer.
> 
> I suspect we really cannot avoid doing some escaping.
> This patch uses the same escaping rules as used by OpenBSD's locale(1).
> 
> ok?
> 
> diff cee3836880940ba0d1203e0682a43a680132bc27 351ffb4d09aa704fe94d74437802d0b5c0812f8f
> commit - cee3836880940ba0d1203e0682a43a680132bc27
> commit + 351ffb4d09aa704fe94d74437802d0b5c0812f8f
> blob - 3325c8994f55721d8588155cdc72b95d11fd2248
> blob + e0687bfb44ce75c1677a5a42f7457e8dd31caaed
> --- lib/dial.c
> +++ lib/dial.c
> @@ -25,6 +25,7 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <limits.h>
>  #include <unistd.h>
>  
>  #include "got_error.h"
> @@ -200,6 +201,55 @@ const struct got_error *
>  	return err;
>  }
>  
> +/*
> + * Escape a given path for the shell which will be started by sshd.
> + * In particular, git-shell is known to require single-quote characters
> + * around its repository path argument and will refuse to run otherwise.
> + */
> +static const struct got_error *
> +escape_path(char *buf, size_t bufsize, const char *path)
> +{
> +	size_t i = 0;
> +	const char *p;
> +
> +	/* We will need at least this much space: */
> +	if (bufsize < strlen(path) + 3)
> +		goto toolong;
> +
> +	memset(buf, 0, bufsize);
> +	buf[i++] = '\''; /* git-shell compat */
> +
> +	p = path;
> +	while (*p != '\0') {
> +		switch (*p) {
> +		case ' ': case '\t': case '\n': case '\'':
> +		case '(': case ')': case '<': case '>':
> +		case '&': case ';': case '|': case '~':
> +		case '"': case '\\': case '$': case '`': 

do we really need to quote all these characters?  Since we're wrapping
everything in _single_ quotes I thought we only needed to quote \ and
the single quote itself.

Then I took a look at git: in quote.c:sq_quote_buf quotes ' and !
which I find admittedly confusing.  it is the function used in
connect.c:git_connect to prepare the ssh connection.

> +			if (i >= bufsize - 1)
> +				goto toolong;
> +			buf[i++] = '\\';
> +			/* FALLTHROUGH */
> +		default:
> +			if (i >= bufsize - 1)
> +				goto toolong;
> +			buf[i++] = *p;
> +			break;
> +		}
> +		p++;
> +	}
> +
> +	if (i >= bufsize - 1)
> +		goto toolong;
> +	buf[i++] = '\'';
> +	if (i >= bufsize)
> +		goto toolong;
> +	buf[i] = '\0';
> +	return NULL;
> +toolong:
> +	return got_error_fmt(GOT_ERR_NO_SPACE, "overlong path: %s", path);
> +}
> +
>  const struct got_error *
>  got_dial_ssh(pid_t *newpid, int *newfd, const char *host,
>      const char *port, const char *path, const char *direction, int verbosity)
> @@ -209,10 +259,15 @@ got_dial_ssh(pid_t *newpid, int *newfd, const char *ho
>  	char cmd[64];
>  	const char *argv[11];
>  	int i = 0, j;
> +	char escaped_path[PATH_MAX];
>  
>  	*newpid = -1;
>  	*newfd = -1;
>  
> +	error = escape_path(escaped_path, sizeof(escaped_path), path);
> +	if (error)
> +		return error;
> +
>  	argv[i++] = GOT_DIAL_PATH_SSH;
>  	if (port != NULL) {
>  		argv[i++] = "-p";
> @@ -228,7 +283,7 @@ got_dial_ssh(pid_t *newpid, int *newfd, const char *ho
>  	argv[i++] = "--";
>  	argv[i++] = (char *)host;
>  	argv[i++] = (char *)cmd;
> -	argv[i++] = (char *)path;
> +	argv[i++] = (char *)escaped_path;
>  	argv[i++] = NULL;
>  	assert(i <= nitems(argv));
>