{"thread":{"id":"64","subject":"[PATCH] show-diff shell safety","startedAt":"2005-04-17T00:36:22Z","lastAt":"2005-04-17T01:00:55Z","messageCount":2,"participants":["Junio C Hamano","Paul Jackson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"403","messageId":"7vbr8e44u1.fsf@assigned-by-dhcp.cox.net","threadId":"64","inReplyTo":null,"subject":"[PATCH] show-diff shell safety","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-04-17T00:36:22Z","receivedAt":"2005-04-17T00:36:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The command line for running \"diff\" command is built without\ntaking shell metacharacters into account.  A malicious dircache\nentry \"foo 2>bar\" (yes, a filename with space) would result in\ncreating a file called \"bar\" with the error message \"diff: foo:\nNo such file or directory\" in it.\n\nThis is not just a user screwing over himself.  Such a dircache\ncan be created as a result of a merge with tree from others.\n\nHere is a fix.\n\nTo be applied on top of my previous patches:\n\n    [PATCH] Optionally tell show-diff to show only named files.\n    [PATCH] show-diff -z option for machine readable output.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n show-diff.c |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----\n 1 files changed, 60 insertions(+), 4 deletions(-)\n\nshow-diff.c: cb1b7eaa9758e94a179e73a80b4b52ee7c34ca5d\n--- show-diff.c\n+++ show-diff.c\t2005-04-16 17:25:22.000000000 -0700\n@@ -4,14 +4,70 @@\n  * Copyright (C) Linus Torvalds, 2005\n  */\n #include \"cache.h\"\n+#include <ctype.h>\n \n-static void show_differences(char *name,\n-\tvoid *old_contents, unsigned long long old_size)\n+static char *diff_cmd = \"diff -L '%s' -u -N  - '%s'\";\n+\n+/* Help to copy the thing properly quoted for the shell safety.\n+ * any single quote is replaced with '\\'', and the caller is\n+ * expected to enclose the result within a single quote pair.\n+ *\n+ * E.g.\n+ *  original     sq_expand     result\n+ *  name     ==> name      ==> 'name'\n+ *  a b      ==> a b       ==> 'a b'\n+ *  a'b      ==> a'\\''b    ==> 'a'\\''b'\n+ *\n+ * NOTE! The returned memory belongs to this function so\n+ * do not free it.\n+ */\n+static char *sq_expand(char *src)\n+{\n+\tstatic char *buf = NULL;\n+\tstatic int buf_size = -1;\n+\tint cnt, c;\n+\tchar *cp;\n+\n+\t/* count single quote characters */ \n+\tfor (cnt = 0, cp = src; *cp; cnt++, cp++)\n+\t\tif (*cp == '\\'')\n+\t\t\tcnt += 3;\n+\n+\tif (buf_size < cnt) {\n+\t\tfree(buf);\n+\t\tbuf_size = cnt;\n+\t\tbuf = malloc(cnt);\n+\t}\n+\n+\tcp = buf;\n+\twhile ((c = *src++)) {\n+\t\tif (c != '\\'')\n+\t\t\t*cp++ = c;\n+\t\telse {\n+\t\t\tcp = strcpy(cp, \"'\\\\''\");\n+\t\t\tcp += 4;\n+\t\t}\n+\t}\n+\t*cp = 0;\n+\treturn buf;\n+}\n+\n+static void show_differences(char *name, void *old_contents,\n+\t\t\t     unsigned long long old_size)\n {\n-\tstatic char cmd[1000];\n \tFILE *f;\n+\tstatic char *cmd = NULL;\n+\tstatic int cmd_size = -1;\n+\n+\tchar *name_sq = sq_expand(name);\n+\tint cmd_required_length = strlen(name_sq) * 2 + strlen(diff_cmd);\n \n-\tsnprintf(cmd, sizeof(cmd), \"diff -L %s -u -N  - %s\", name, name);\n+\tif (cmd_size < cmd_required_length) {\n+\t\tfree(cmd);\n+\t\tcmd_size = cmd_required_length;\n+\t\tcmd = malloc(cmd_required_length);\n+\t}\n+\tsnprintf(cmd, cmd_size, diff_cmd, name_sq, name_sq);\n \tf = popen(cmd, \"w\");\n \tif (old_size)\n \t\tfwrite(old_contents, old_size, 1, f);\n\n"},{"id":"409","messageId":"20050416180055.76cb8a25.pj@sgi.com","threadId":"64","inReplyTo":"7vbr8e44u1.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] show-diff shell safety","fromName":"Paul Jackson","fromEmail":"pj@sgi.com","sentAt":"2005-04-17T01:00:55Z","receivedAt":"2005-04-17T01:00:55Z","isPatch":true,"sender":{"key":"pj@sgi.com","avatar":null},"body":"Junio wrote:\n> The command line for running \"diff\" command is built without\n> taking shell metacharacters into account. \n\nAck - you're right.  One should avoid popen and system\nin all but personal hacking code.  There are many ways,\nbeyond just embedded shell redirection, to cause problems\nwith these calls.\n\nOne should directly code execve(), execv(), or execl().\n\nSearch for \"popen system IFS PATH\"\n\n-- \n                  I won't rest till it's the best ...\n                  Programmer, Linux Scalability\n                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401\n"}]}