git-pull-script hates me

git-pull-script hates me

[Greg KH]

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

Re: git-pull-script hates me

[Linus Torvalds]

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

Re: git-pull-script hates me

[Greg KH]

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 <gregkh@suse.de>

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
 	};
 

Re: git-pull-script hates me

[Greg KH]

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

Re: git-pull-script hates me

[Linus Torvalds]

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

Re: git-pull-script hates me

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> 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 ;-).

Re: git-pull-script hates me

[Linus Torvalds]

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

[PATCH] Make sq_expand() available as sq_quote().

[Junio C Hamano]

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 <junkio@cox.net>
---
>>>>> "JCH" == Junio C Hamano <junkio@cox.net> 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 <sys/wait.h>
 #include <signal.h>
 #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] = "";

[PATCH] Make sq_expand() available as sq_quote().

[Junio C Hamano]

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 <junkio@cox.net>
---
>>>>> "JCH" == Junio C Hamano <junkio@cox.net> 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 <sys/wait.h>
 #include <signal.h>
 #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

[PATCH] Use sq_quote() to properly quote the parameter to call shell.

[Junio C Hamano]

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 <junkio@cox.net>
---
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<h'
    fatal: protocol error: expected sha/ref, got ' abc;d\ef g<h'
    $ git-send-pack --exec=/bin/echo localhost:'0011 abc;d\ef g<h'
    fatal: protocol error: expected sha/ref, got ' abc;d\ef g<h'
    $ git-fetch-pack '0011 abc;d\ef g<h' a
    fatal: git-upload-pack unable to chdir to 0011 abc;d\ef g<h
    fatal: unexpected EOF
    $ git-fetch-pack localhost:'0011 abc;d\ef g<h' a
    fatal: git-upload-pack unable to chdir to 0011 abc;d\ef g<h
    fatal: unexpected EOF

Also by storing the following executable script in
/var/tmp/j0.sh and using git-send-pack --exec=/var/tmp/j0.sh
with the same parameters:

    #!/bin/sh
    o=/var/tmp/j0.out 
    echo $$ >>$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 <sys/wait.h>
 
 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]);