git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH] show-diff shell safety

From
Junio C Hamano <junkio@cox.net>
Date
Apr 17, 2005, 00:36 UTC
Message-ID
<7vbr8e44u1.fsf@assigned-by-dhcp.cox.net>

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);
Next: Paul Jackson
Message 1 of 2 in “show-diff shell safety”
  1. show-diff shell safetyJunio C Hamano, Apr 17, 2005
  2. Paul JacksonApr 17, 2005

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.