[PATCH] show-diff shell safety

[PATCH] show-diff shell safety

[Junio C Hamano]

The command line for running "diff" command is built without
taking shell metacharacters into account.  A malicious dircache
entry "foo 2>bar" (yes, a filename with space) would result in
creating a file called "bar" with the error message "diff: foo:
No such file or directory" in it.

This is not just a user screwing over himself.  Such a dircache
can be created as a result of a merge with tree from others.

Here is a fix.

To be applied on top of my previous patches:

    [PATCH] Optionally tell show-diff to show only named files.
    [PATCH] show-diff -z option for machine readable output.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 show-diff.c |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 60 insertions(+), 4 deletions(-)

show-diff.c: cb1b7eaa9758e94a179e73a80b4b52ee7c34ca5d
--- show-diff.c
+++ show-diff.c	2005-04-16 17:25:22.000000000 -0700
@@ -4,14 +4,70 @@
  * Copyright (C) Linus Torvalds, 2005
  */
 #include "cache.h"
+#include <ctype.h>
 
-static void show_differences(char *name,
-	void *old_contents, unsigned long long old_size)
+static char *diff_cmd = "diff -L '%s' -u -N  - '%s'";
+
+/* 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'
+ *
+ * NOTE! The returned memory belongs to this function so
+ * do not free it.
+ */
+static char *sq_expand(char *src)
+{
+	static char *buf = NULL;
+	static int buf_size = -1;
+	int cnt, c;
+	char *cp;
+
+	/* count single quote characters */ 
+	for (cnt = 0, cp = src; *cp; cnt++, cp++)
+		if (*cp == '\'')
+			cnt += 3;
+
+	if (buf_size < cnt) {
+		free(buf);
+		buf_size = cnt;
+		buf = malloc(cnt);
+	}
+
+	cp = buf;
+	while ((c = *src++)) {
+		if (c != '\'')
+			*cp++ = c;
+		else {
+			cp = strcpy(cp, "'\\''");
+			cp += 4;
+		}
+	}
+	*cp = 0;
+	return buf;
+}
+
+static void show_differences(char *name, void *old_contents,
+			     unsigned long long old_size)
 {
-	static char cmd[1000];
 	FILE *f;
+	static char *cmd = NULL;
+	static int cmd_size = -1;
+
+	char *name_sq = sq_expand(name);
+	int cmd_required_length = strlen(name_sq) * 2 + strlen(diff_cmd);
 
-	snprintf(cmd, sizeof(cmd), "diff -L %s -u -N  - %s", name, name);
+	if (cmd_size < cmd_required_length) {
+		free(cmd);
+		cmd_size = cmd_required_length;
+		cmd = malloc(cmd_required_length);
+	}
+	snprintf(cmd, cmd_size, diff_cmd, name_sq, name_sq);
 	f = popen(cmd, "w");
 	if (old_size)
 		fwrite(old_contents, old_size, 1, f);

Re: [PATCH] show-diff shell safety

[Paul Jackson]

Junio wrote:
> The command line for running "diff" command is built without
> taking shell metacharacters into account. 

Ack - you're right.  One should avoid popen and system
in all but personal hacking code.  There are many ways,
beyond just embedded shell redirection, to cause problems
with these calls.

One should directly code execve(), execv(), or execl().

Search for "popen system IFS PATH"

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401