# git-pull-script hates me 10 messages from 2005-07-06 to 2005-07-08. Participants: Greg KH, Linus Torvalds, Junio C Hamano. Thread: https://gitlist.dev/t/1146 ## Greg KH, 2005-07-06 20:31 Subject: git-pull-script hates me Message-ID: <20050706203139.GA19198@kroah.com> URL: https://gitlist.dev/e/20050706203139.GA19198%40kroah.com ``` I just updated to the latest git tree, and now get the following when I try to pull from a ssh repo: $ git-pull-script gregkh@someserver.org:/public_html/udev.git/ fatal: I don't like '@'. Sue me. So I drop the @ and then get: $ git-pull-script someserver.org:/public_html/udev.git/ fatal: I don't like '_'. Sue me. This worked just fine before I updated git :( Any hints? Or should I be using a different command to do pulls? thanks, greg k-h ``` ## Linus Torvalds, 2005-07-06 20:37 Subject: Re: git-pull-script hates me Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0507061335100.4159%40g5.osdl.org In-Reply-To: <20050706203139.GA19198@kroah.com> ``` On Wed, 6 Jul 2005, Greg KH wrote: > > I just updated to the latest git tree, and now get the following when I > try to pull from a ssh repo: > > $ git-pull-script gregkh@someserver.org:/public_html/udev.git/ > fatal: I don't like '@'. Sue me. > > So I drop the @ and then get: > $ git-pull-script someserver.org:/public_html/udev.git/ > fatal: I don't like '_'. Sue me. Heh. It really is personal. The new git-pack handling tries to avoid special characters, because it passes some things off to a shell (ie it opens up an ssh connection. But yeah, it's being a bit too anal. Just look at connect.c: shell_safe(), and add both '_' and '@' to the safe list (and any other safe characters), and off you go. And if somebody wants to add code to do proper escaping of the non-safe ones, we can do that too. I was just lazy and added the characters I ever use ;) Linus ``` ## Greg KH, 2005-07-06 20:46 Subject: Re: git-pull-script hates me Message-ID: <20050706204627.GA19481@kroah.com> URL: https://gitlist.dev/e/20050706204627.GA19481%40kroah.com In-Reply-To: ``` On Wed, Jul 06, 2005 at 01:37:55PM -0700, Linus Torvalds wrote: > > > On Wed, 6 Jul 2005, Greg KH wrote: > > > > I just updated to the latest git tree, and now get the following when I > > try to pull from a ssh repo: > > > > $ git-pull-script gregkh@someserver.org:/public_html/udev.git/ > > fatal: I don't like '@'. Sue me. > > > > So I drop the @ and then get: > > $ git-pull-script someserver.org:/public_html/udev.git/ > > fatal: I don't like '_'. Sue me. > > Heh. It really is personal. > > The new git-pack handling tries to avoid special characters, because it > passes some things off to a shell (ie it opens up an ssh connection. > > But yeah, it's being a bit too anal. Just look at connect.c: shell_safe(), > and add both '_' and '@' to the safe list (and any other safe characters), > and off you go. Ok, below is a patch for this. It works, but then errors out with: bash: git-upload-pack: command not found fatal: unexpected EOF So I'm guessing that I have to convince the server owner to update their version of git too? thanks, greg k-h -------------------- Subject: allow _ and @ in addresses Signed-off-by: Greg Kroah-Hartman diff --git a/connect.c b/connect.c --- a/connect.c +++ b/connect.c @@ -57,6 +57,7 @@ static char *shell_safe(char *url) ['A'...'Z'] = 1, ['.'] = 1, ['/'] = 1, ['-'] = 1, ['+'] = 1, + ['@'] = 1, ['_'] = 1, [':'] = 1 }; ``` ## Greg KH, 2005-07-06 20:54 Subject: Re: git-pull-script hates me Message-ID: <20050706205442.GA19653@kroah.com> URL: https://gitlist.dev/e/20050706205442.GA19653%40kroah.com In-Reply-To: <20050706204627.GA19481@kroah.com> ``` On Wed, Jul 06, 2005 at 01:46:27PM -0700, Greg KH wrote: > Ok, below is a patch for this. It works, but then errors out with: > bash: git-upload-pack: command not found > fatal: unexpected EOF > > So I'm guessing that I have to convince the server owner to update their > version of git too? Ok, so they did that, and the pull now works, sorry for the noise. thanks, greg k-h ``` ## Linus Torvalds, 2005-07-06 20:58 Subject: Re: git-pull-script hates me Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0507061357130.4675%40g5.osdl.org In-Reply-To: <20050706204627.GA19481@kroah.com> ``` On Wed, 6 Jul 2005, Greg KH wrote: > > Ok, below is a patch for this. It works, but then errors out with: > bash: git-upload-pack: command not found > fatal: unexpected EOF > > So I'm guessing that I have to convince the server owner to update their > version of git too? The easiest way is to just build git on the other end yourself, and make sure you have PATH=~/bin:$PATH in your .bashrc. That way you're not at the mercy of the sysadmin on the other side. Linus ``` ## Junio C Hamano, 2005-07-06 21:23 Subject: Re: git-pull-script hates me Message-ID: <7v64vnwrwg.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v64vnwrwg.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` >>>>> "LT" == Linus Torvalds writes: LT> But yeah, it's being a bit too anal. Just look at connect.c: shell_safe(), LT> and add both '_' and '@' to the safe list (and any other safe characters), LT> and off you go. LT> And if somebody wants to add code to do proper escaping of the non-safe LT> ones, we can do that too. I was just lazy and added the characters I ever LT> use ;) Anybody who is interested in doing this can just move sq_expand() from diff.c to some public library and expose it in cache.h. I am not going to do it myself immediately so there is no worry to race against me ;-). ``` ## Linus Torvalds, 2005-07-06 21:36 Subject: Re: git-pull-script hates me Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0507061434010.3847%40g5.osdl.org In-Reply-To: <7v64vnwrwg.fsf@assigned-by-dhcp.cox.net> ``` On Wed, 6 Jul 2005, Junio C Hamano wrote: > > Anybody who is interested in doing this can just move > sq_expand() from diff.c to some public library and expose it in > cache.h. No, that doesn't work at all. "sq_expand()" tries to protect things inside single quotes. That's a totally different problem (and btw, it does so badly: it doesn't quote '\' for example). For a shell command line, there are _tons_ of special characters that you mustn't pass through. Things like ';', '<', '>', '&' all have magic meaning and are not valid in the destination name. Not to mention just simple whitespace. Linus ``` ## Junio C Hamano, 2005-07-08 06:55 Subject: [PATCH] Make sq_expand() available as sq_quote(). Message-ID: <7vd5ptdbwl.fsf_-_@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vd5ptdbwl.fsf_-_%40assigned-by-dhcp.cox.net In-Reply-To: <7v64vnwrwg.fsf@assigned-by-dhcp.cox.net> ``` A useful shell safety helper sq_expand() was hidden as a static function in diff.c. Extract it out and make it available as sq_quote(). Signed-off-by: Junio C Hamano --- >>>>> "JCH" == Junio C Hamano writes: JCH> I am not going to do it myself immediately so there is no worry JCH> to race against me ;-). Well, I ended up doing it myself just as a diversion from thinking about pulls ;-). As you said, one _must_ be very careful when feeding a shell, but being careful does not necessarily hard. This is the first part; the next one will depend on it. Makefile | 3 +++ diff.c | 47 ++++++----------------------------------------- 2 files changed, 9 insertions(+), 41 deletions(-) 0ab0d4c45dc7c531dd54f2d29a2199d4f4192018 diff --git a/Makefile b/Makefile --- a/Makefile +++ b/Makefile @@ -65,6 +65,9 @@ LIB_H=cache.h object.h blob.h tree.h com LIB_H += strbuf.h LIB_OBJS += strbuf.o +LIB_H += quote.h +LIB_OBJS += quote.o + LIB_H += diff.h count-delta.h LIB_OBJS += diff.o diffcore-rename.o diffcore-pickaxe.o diffcore-pathspec.o \ count-delta.o diffcore-break.o diffcore-order.o diff --git a/diff.c b/diff.c --- a/diff.c +++ b/diff.c @@ -5,6 +5,7 @@ #include #include #include "cache.h" +#include "quote.h" #include "diff.h" #include "diffcore.h" @@ -40,42 +41,6 @@ static const char *external_diff(void) return external_diff_cmd; } -/* Help to copy the thing properly quoted for the shell safety. - * any single quote is replaced with '\'', and the caller is - * expected to enclose the result within a single quote pair. - * - * E.g. - * original sq_expand result - * name ==> name ==> 'name' - * a b ==> a b ==> 'a b' - * a'b ==> a'\''b ==> 'a'\''b' - */ -static char *sq_expand(const char *src) -{ - static char *buf = NULL; - int cnt, c; - const char *cp; - char *bp; - - /* count bytes needed to store the quoted string. */ - for (cnt = 1, cp = src; *cp; cnt++, cp++) - if (*cp == '\'') - cnt += 3; - - buf = xmalloc(cnt); - bp = buf; - while ((c = *src++)) { - if (c != '\'') - *bp++ = c; - else { - bp = strcpy(bp, "'\\''"); - bp += 4; - } - } - *bp = 0; - return buf; -} - static struct diff_tempfile { const char *name; /* filename external diff should read from */ char hex[41]; @@ -167,16 +132,16 @@ static void builtin_diff(const char *nam int complete_rewrite) { int i, next_at, cmd_size; - const char *diff_cmd = "diff -L'%s%s' -L'%s%s'"; - const char *diff_arg = "'%s' '%s'||:"; /* "||:" is to return 0 */ + const char *diff_cmd = "diff -L%s%s -L%s%s"; + const char *diff_arg = "%s %s||:"; /* "||:" is to return 0 */ const char *input_name_sq[2]; const char *path0[2]; const char *path1[2]; const char *name_sq[2]; char *cmd; - name_sq[0] = sq_expand(name_a); - name_sq[1] = sq_expand(name_b); + name_sq[0] = sq_quote(name_a); + name_sq[1] = sq_quote(name_b); /* diff_cmd and diff_arg have 6 %s in total which makes * the sum of these strings 12 bytes larger than required. @@ -186,7 +151,7 @@ static void builtin_diff(const char *nam cmd_size = (strlen(diff_cmd) + strlen(diff_opts) + strlen(diff_arg) - 9); for (i = 0; i < 2; i++) { - input_name_sq[i] = sq_expand(temp[i].name); + input_name_sq[i] = sq_quote(temp[i].name); if (!strcmp(temp[i].name, "/dev/null")) { path0[i] = "/dev/null"; path1[i] = ""; ``` ## Junio C Hamano, 2005-07-08 06:58 Subject: [PATCH] Make sq_expand() available as sq_quote(). Message-ID: <7v3bqpdbs7.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v3bqpdbs7.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <7v64vnwrwg.fsf@assigned-by-dhcp.cox.net> ``` A useful shell safety helper sq_expand() was hidden as a static function in diff.c. Extract it out and make it available as sq_quote(). Signed-off-by: Junio C Hamano --- >>>>> "JCH" == Junio C Hamano writes: JCH> I am not going to do it myself immediately so there is no worry JCH> to race against me ;-). Well, I ended up doing it myself just as a diversion from thinking about pulls ;-). As you said, one _must_ be very careful when feeding a shell, but being careful does not necessarily hard. This is the first part; the next one will depend on it. Makefile | 3 +++ diff.c | 47 ++++++----------------------------------------- quote.c | 41 +++++++++++++++++++++++++++++++++++++++++ quote.h | 26 ++++++++++++++++++++++++++ 4 files changed, 76 insertions(+), 41 deletions(-) create mode 100644 quote.c create mode 100644 quote.h 500d2196566027419b1b8effc28f78eb9a60f086 diff --git a/Makefile b/Makefile --- a/Makefile +++ b/Makefile @@ -65,6 +65,9 @@ LIB_H=cache.h object.h blob.h tree.h com LIB_H += strbuf.h LIB_OBJS += strbuf.o +LIB_H += quote.h +LIB_OBJS += quote.o + LIB_H += diff.h count-delta.h LIB_OBJS += diff.o diffcore-rename.o diffcore-pickaxe.o diffcore-pathspec.o \ count-delta.o diffcore-break.o diffcore-order.o diff --git a/diff.c b/diff.c --- a/diff.c +++ b/diff.c @@ -5,6 +5,7 @@ #include #include #include "cache.h" +#include "quote.h" #include "diff.h" #include "diffcore.h" @@ -40,42 +41,6 @@ static const char *external_diff(void) return external_diff_cmd; } -/* Help to copy the thing properly quoted for the shell safety. - * any single quote is replaced with '\'', and the caller is - * expected to enclose the result within a single quote pair. - * - * E.g. - * original sq_expand result - * name ==> name ==> 'name' - * a b ==> a b ==> 'a b' - * a'b ==> a'\''b ==> 'a'\''b' - */ -static char *sq_expand(const char *src) -{ - static char *buf = NULL; - int cnt, c; - const char *cp; - char *bp; - - /* count bytes needed to store the quoted string. */ - for (cnt = 1, cp = src; *cp; cnt++, cp++) - if (*cp == '\'') - cnt += 3; - - buf = xmalloc(cnt); - bp = buf; - while ((c = *src++)) { - if (c != '\'') - *bp++ = c; - else { - bp = strcpy(bp, "'\\''"); - bp += 4; - } - } - *bp = 0; - return buf; -} - static struct diff_tempfile { const char *name; /* filename external diff should read from */ char hex[41]; @@ -167,16 +132,16 @@ static void builtin_diff(const char *nam int complete_rewrite) { int i, next_at, cmd_size; - const char *diff_cmd = "diff -L'%s%s' -L'%s%s'"; - const char *diff_arg = "'%s' '%s'||:"; /* "||:" is to return 0 */ + const char *diff_cmd = "diff -L%s%s -L%s%s"; + const char *diff_arg = "%s %s||:"; /* "||:" is to return 0 */ const char *input_name_sq[2]; const char *path0[2]; const char *path1[2]; const char *name_sq[2]; char *cmd; - name_sq[0] = sq_expand(name_a); - name_sq[1] = sq_expand(name_b); + name_sq[0] = sq_quote(name_a); + name_sq[1] = sq_quote(name_b); /* diff_cmd and diff_arg have 6 %s in total which makes * the sum of these strings 12 bytes larger than required. @@ -186,7 +151,7 @@ static void builtin_diff(const char *nam cmd_size = (strlen(diff_cmd) + strlen(diff_opts) + strlen(diff_arg) - 9); for (i = 0; i < 2; i++) { - input_name_sq[i] = sq_expand(temp[i].name); + input_name_sq[i] = sq_quote(temp[i].name); if (!strcmp(temp[i].name, "/dev/null")) { path0[i] = "/dev/null"; path1[i] = ""; diff --git a/quote.c b/quote.c new file mode 100644 --- /dev/null +++ b/quote.c @@ -0,0 +1,41 @@ +#include "cache.h" +#include "quote.h" + +/* Help to copy the thing properly quoted for the shell safety. + * any single quote is replaced with '\'', and the caller is + * expected to enclose the result within a single quote pair. + * + * E.g. + * original sq_quote result + * name ==> name ==> 'name' + * a b ==> a b ==> 'a b' + * a'b ==> a'\''b ==> 'a'\''b' + */ +char *sq_quote(const char *src) +{ + static char *buf = NULL; + int cnt, c; + const char *cp; + char *bp; + + /* count bytes needed to store the quoted string. */ + for (cnt = 3, cp = src; *cp; cnt++, cp++) + if (*cp == '\'') + cnt += 3; + + buf = xmalloc(cnt); + bp = buf; + *bp++ = '\''; + while ((c = *src++)) { + if (c != '\'') + *bp++ = c; + else { + bp = strcpy(bp, "'\\''"); + bp += 4; + } + } + *bp++ = '\''; + *bp = 0; + return buf; +} + diff --git a/quote.h b/quote.h new file mode 100644 --- /dev/null +++ b/quote.h @@ -0,0 +1,26 @@ +#ifndef QUOTE_H +#define QUOTE_H + + +/* Help to copy the thing properly quoted for the shell safety. + * any single quote is replaced with '\'', and the whole thing + * is enclosed in a single quote pair. + * + * For example, if you are passing the result to system() as an + * argument: + * + * sprintf(cmd, "foobar %s %s", sq_quote(arg0), sq_quote(arg1)) + * + * would be appropriate. If the system() is going to call ssh to + * run the command on the other side: + * + * sprintf(cmd, "git-diff-tree %s %s", sq_quote(arg0), sq_quote(arg1)); + * sprintf(rcmd, "ssh %s %s", sq_quote(host), sq_quote(cmd)); + * + * Note that the above examples leak memory! Remember to free result from + * sq_quote() in a real application. + */ + +char *sq_quote(const char *src); + +#endif ``` ## Junio C Hamano, 2005-07-08 07:02 Subject: [PATCH] Use sq_quote() to properly quote the parameter to call shell. Message-ID: <7vslypbx0j.fsf_-_@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vslypbx0j.fsf_-_%40assigned-by-dhcp.cox.net In-Reply-To: <7v3bqpdbs7.fsf@assigned-by-dhcp.cox.net> ``` This tries to be more lenient to the users and stricter to the attackers by quoting the input properly for shell safety, instead of forbidding certain characters from the input. Things to note: - We do not quote "prog" parameter (which comes from --exec). The user should know what he is doing. --exec='echo foo' will supply the first two parameters to the resulting command, while --exec="'echo foo'" will give the first parameter, a single string with a space inside. - We do not care too much about leaking the sq_quote() output just before running exec(). Signed-off-by: Junio C Hamano --- This depends on the previous [PATCH] Make sq_expand() available as sq_quote(). Please discard the one I sent by mistake that lacks additions of quote.[ch] files. To verify the stuff is quoted properly, I used the following command invocations. They show that the metacharacters are passed unmolested by the intervening shell and ssh commands: $ git-send-pack --exec=/bin/echo '0011 abc;d\ef g>$o i=0 echo "0: [$0]" >>$o for x do i=$(expr $i + 1) echo "$i: [$x]" >>$o done it can be verified that the funny string is indeed passed as a single parameter to it. connect.c | 33 +++------------------------------ 1 files changed, 3 insertions(+), 30 deletions(-) e6e65f02919048f37467a9aca55ed19892dd2a7e diff --git a/connect.c b/connect.c --- a/connect.c +++ b/connect.c @@ -1,5 +1,6 @@ #include "cache.h" #include "pkt-line.h" +#include "quote.h" #include int get_ack(int fd, unsigned char *result_sha1) @@ -42,34 +43,6 @@ int path_match(const char *path, int nr, } /* - * First, make it shell-safe. We do this by just disallowing any - * special characters. Somebody who cares can do escaping and let - * through the rest. But since we're doing to feed this to ssh as - * a command line, we're going to be pretty damn anal for now. - */ -static char *shell_safe(char *url) -{ - char *n = url; - unsigned char c; - static const char flags[256] = { - ['0'...'9'] = 1, - ['a'...'z'] = 1, - ['A'...'Z'] = 1, - ['.'] = 1, ['/'] = 1, - ['-'] = 1, ['+'] = 1, - [':'] = 1, ['_'] = 1, - ['@'] = 1, [','] = 1, - ['~'] = 1, ['^'] = 1, - }; - - while ((c = *n++) != 0) { - if (flags[c] != 1) - die("I don't like '%c'. Sue me.", c); - } - return url; -} - -/* * Yeah, yeah, fixme. Need to pass in the heads etc. */ int git_connect(int fd[2], char *url, const char *prog) @@ -80,7 +53,6 @@ int git_connect(int fd[2], char *url, co int pipefd[2][2]; pid_t pid; - url = shell_safe(url); host = NULL; path = url; colon = strchr(url, ':'); @@ -89,11 +61,12 @@ int git_connect(int fd[2], char *url, co host = url; path = colon+1; } - snprintf(command, sizeof(command), "%s %s", prog, path); if (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0) die("unable to create pipe pair for communication"); pid = fork(); if (!pid) { + snprintf(command, sizeof(command), "%s %s", prog, + sq_quote(path)); dup2(pipefd[1][0], 0); dup2(pipefd[0][1], 1); close(pipefd[0][0]); ```