From: Omar Polo Subject: Re: dial fix for git-shell To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Wed, 08 Mar 2023 17:38:53 +0100 On 2023/03/08 16:54:57 +0100, Stefan Sperling wrote: > On Wed, Mar 08, 2023 at 04:01:20PM +0100, Omar Polo wrote: > > 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. > > I don't know. Are there rules in POSIX we can refer to? > > I took the quoting rules from /usr/bin/locale. > I presume it makes it safe to run `eval` on locale's output (though > I'd have to ask guenther@ for specifics). In any case, given that > the remote server might be spawning a regular unix shell I think it > makes sense to quote this argument by default, such that a bad path > argument accidentally produced by some script will not cause problems. > (Disregarding people trying to play jokes by removing this safe-guard > and sending garbage on purpose, then it becomes a server-side problem). The major difference is that locale use double quotes, git-shell requires single. Single quotes are easier to work with, since you just need to escape an inner ' character. I was wrong in my initial message, \ can't be used to escape the single quoting: $ /bin/echo '\' \ (was tricked by how the ksh' echo builtin process some \c sequences.) Furthermore, your escape_path routine will produce wrong results since it's thought to be used to quote inside double quotes string and not single ones. Consider: $ /bin/echo '\$foo' \$foo $ /bin/echo "\$foo" $foo > In case of gotsh/gotd we don't care, and we also don't really care > whether any of these characters work in a gotd.conf repository path. > Some of them probably won't ever work because of parse.y limitations? unless you're willing to accept a patch to add some way to add fancy characters in gotd.conf I guess this won't bite us. I don't know how many peoples have git repos with path containing control codes... > One problem could be that someone out there is using some characters in > a path (perhaps ~ could appear somehwere) and someone else wants to use > 'got clone' with the resulting URL. In that case they can always use Git as > a workaround, or we can adjust our quoting as such problems get uncovered. > Should we provide an option to turn the quoting off, just in case? I don't think it is worth it. Quoting inside single quotes *should* be simple and I don't think we need to provide an option, env variable or even a compile-time toggle, nor having folks using git for this use case. > > 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. > > No idea. > Even just the single-quote requirement of git-shell is silly to begin with. > git-shell should be doing its own escaping, assuming arbitrary input, not > require the input to be already quoted somehow. it's probably just a safe-guard in case someone switches back from git-shell to a regular shell? Just wondering. How about diff below? the question now becomes why git escapes ! too, any idea? I skimmed thru csh(1) and it seem to imply that single-quotes behaves like in sh(1). maybe some other popular shell has some quirks? diff /home/op/w/got commit - cee3836880940ba0d1203e0682a43a680132bc27 path + /home/op/w/got blob - 3325c8994f55721d8588155cdc72b95d11fd2248 file + lib/dial.c --- lib/dial.c +++ lib/dial.c @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -200,6 +201,47 @@ 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) +{ + const char *p; + char *q; + + p = path; + q = buf; + + if (bufsize > 1) + *q++ = '\''; + + while (*p != '\0' && (q - buf < bufsize)) { + /* git escapes ! too */ + if (*p != '\'' && *p != '!') { + *q++ = *p++; + continue; + } + + if (q - buf + 4 >= bufsize) + break; + *q++ = '\''; + *q++ = '\\'; + *q++ = *p++; + *q++ = '\''; + } + + if (*p == '\0' && (q - buf + 1 < bufsize)) { + *q++ = '\''; + *q = '\0'; + return NULL; + } + + 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) @@ -207,12 +249,17 @@ got_dial_ssh(pid_t *newpid, int *newfd, const char *ho const struct got_error *error = NULL; int pid, pfd[2]; char cmd[64]; + char escaped_path[PATH_MAX]; const char *argv[11]; int i = 0, j; *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";