{"thread":{"id":"1957","subject":"git-daemon: path validation, export all option","startedAt":"2005-09-27T02:13:32Z","lastAt":"2005-09-27T16:56:18Z","messageCount":7,"participants":["H. Peter Anvin","Junio C Hamano","Anton Altaparmakov","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"9355","messageId":"4338AACC.1050305@zytor.com","threadId":"1957","inReplyTo":null,"subject":"git-daemon: path validation, export all option","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-09-27T02:13:32Z","receivedAt":"2005-09-27T02:13:32Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"A first attempt to make git-daemon a bit more suitable for kernel.org \nuse: it allows the user to specify a whitelist of directories, rejects \npaths which have . or .. in them (to avoid bypassing the whitelist), and \nallows for an --export-all option.\n\nSigned-off-by: H. Peter Anvin \n\n\nSupport a modicum of path validation, and allow an export all trees option.\n\n---\ncommit 4ae95682694a1cd05ee2029fe241ad90d43c8c0e\ntree 4188c26501c852ba9c1b1a3f39276d3ac7dc3f8a\nparent 152da3dfcf2c16d7c240a0dbdcb8a3ae1d332d81\nauthor H. Peter Anvin Mon, 26 Sep 2005 19:10:55 -0700\ncommitter H. Peter Anvin Mon, 26 Sep 2005 19:10:55 -0700\n\n daemon.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----\n 1 files changed, 67 insertions(+), 5 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\n--- a/daemon.c\n+++ b/daemon.c\n@@ -12,7 +12,13 @@\n static int log_syslog;\n static int verbose;\n \n-static const char daemon_usage[] = \"git-daemon [--verbose] [--syslog] [--inetd | --port=n]\";\n+static const char daemon_usage[] = \"git-daemon [--verbose] [--syslog] [--inetd | --port=n] [--export-all] [directory...]\";\n+\n+/* List of acceptable pathname prefixes */\n+static char **ok_paths = NULL;\n+\n+/* If this is set, git-daemon-export-ok is not required */\n+static int export_all_trees = 0;\n \n \n static void logreport(int priority, const char *err, va_list params)\n@@ -69,15 +75,61 @@ void loginfo(const char *err, ...)\n \tva_end(params);\n }\n \n+static int path_ok(const char *dir)\n+{\n+\tconst char *p = dir;\n+\tchar **pp;\n+\tint sl = 1, ndot = 0;\n+\n+\tfor (;;) {\n+\t\tif ( *p == '.' ) {\n+\t\t\tndot++;\n+\t\t} else if ( *p == '/' || *p == '\\0' ) {\n+\t\t\tif ( sl && ndot > 0 && ndot < 3 )\n+\t\t\t\treturn 0; /* . or .. in path */\n+\t\t\tsl = 1;\n+\t\t\tif ( *p == '\\0' )\n+\t\t\t\tbreak; /* End of string and all is good */\n+\t\t} else {\n+\t\t\tsl = ndot = 0;\n+\t\t}\n+\t\tp++;\n+\t}\n+\n+\tif ( ok_paths && *ok_paths ) {\n+\t\tint ok = 0;\n+\t\tint dirlen = strlen(dir); /* read_packet_line can return embedded \\0 */\n+\n+\t\tfor ( pp = ok_paths ; *pp ; pp++ ) {\n+\t\t\tint len = strlen(*pp);\n+\t\t\tif ( len <= dirlen &&\n+\t\t\t !strncmp(*pp, dir, len) &&\n+\t\t\t (dir[len] == '/' || dir[len] == '\\0') ) {\n+\t\t\t\tok = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif ( !ok )\n+\t\t\treturn 0; /* Path not in whitelist */\n+\t}\n+\n+\treturn 1;\t\t/* Path acceptable */\n+}\n \n static int upload(char *dir, int dirlen)\n {\n \tloginfo(\"Request for '%s'\", dir);\n+\n+\tif (!path_ok(dir)) {\n+\t\tlogerror(\"Forbidden directory: %s\\n\", dir);\n+\t\treturn -1;\n+\t}\n+\n \tif (chdir(dir) < 0) {\n \t\tlogerror(\"Cannot chdir('%s'): %s\", dir, strerror(errno));\n \t\treturn -1;\n \t}\n-\tchdir(\".git\");\n \n \t/*\n \t * Security on the cheap.\n@@ -86,10 +138,10 @@ static int upload(char *dir, int dirlen)\n \t * a \"git-daemon-export-ok\" flag that says that the other side\n \t * is ok with us doing this.\n \t */\n-\tif (access(\"git-daemon-export-ok\", F_OK) ||\n+\tif ((!export_all_trees && access(\"git-daemon-export-ok\", F_OK)) ||\n \t access(\"objects/00\", X_OK) ||\n \t access(\"HEAD\", R_OK)) {\n-\t\tlogerror(\"Not a valid gitd-enabled repository: '%s'\", dir);\n+\t\tlogerror(\"Not a valid git-daemon-enabled repository: '%s'\", dir);\n \t\treturn -1;\n \t}\n \n@@ -441,7 +493,6 @@ int main(int argc, char **argv)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n-\n \t\tif (!strcmp(arg, \"--inetd\")) {\n \t\t\tinetd_mode = 1;\n \t\t\tcontinue;\n@@ -455,6 +506,17 @@ int main(int argc, char **argv)\n \t\t\topenlog(\"git-daemon\", 0, LOG_DAEMON);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--export-all\")) {\n+\t\t\texport_all_trees = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(arg, \"--\")) {\n+\t\t\tok_paths = &argv[i+1];\n+\t\t\tbreak;\n+\t\t} else if (arg[0] != '-') {\n+\t\t\tok_paths = &argv[i];\n+\t\t\tbreak;\n+\t\t}\n \n \t\tusage(daemon_usage);\n \t}\n"},{"id":"9357","messageId":"7vslvr6t1u.fsf@assigned-by-dhcp.cox.net","threadId":"1957","inReplyTo":"4338AACC.1050305@zytor.com","subject":"Re: git-daemon: path validation, export all option","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-27T04:19:57Z","receivedAt":"2005-09-27T04:19:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H. Peter Anvin\" writes:\n\n> A first attempt to make git-daemon a bit more suitable for kernel.org \n> use: it allows the user to specify a whitelist of directories, rejects \n> paths which have . or .. in them (to avoid bypassing the whitelist), and \n> allows for an --export-all option.\n>\n> Signed-off-by: H. Peter Anvin \n\nI understand the motivation behind --export-all and directory\nwhitelist and these changes look good. Thanks.\n\n> +\tif ( ok_paths && *ok_paths ) {\n> +\t\tint ok = 0;\n> +...\n> +\t}\n> +\n> +\treturn 1;\t\t/* Path acceptable */\n> +}\n\nA microNit. You could lose 'int ok' and return 1 directly where\nyou assign 1 to it and break.\n\n> -\tchdir(\".git\");\n\nI am unsure about this removal of \"minor convenience feature\".\nAlthough I do not think git-daemon is widely used on the field,\nthis change breaks existing setup if there is any.\n"},{"id":"9374","messageId":"1127809831.28407.6.camel@imp.csi.cam.ac.uk","threadId":"1957","inReplyTo":"7vslvr6t1u.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-daemon: path validation, export all option","fromName":"Anton Altaparmakov","fromEmail":"aia21@cam.ac.uk","sentAt":"2005-09-27T08:30:31Z","receivedAt":"2005-09-27T08:30:31Z","isPatch":false,"sender":{"key":"aia21@cam.ac.uk","avatar":null},"body":"On Mon, 2005-09-26 at 21:19 -0700, Junio C Hamano wrote:\n> \"H. Peter Anvin\" writes:\n> \n> > A first attempt to make git-daemon a bit more suitable for kernel.org \n> > use: it allows the user to specify a whitelist of directories, rejects \n> > paths which have . or .. in them (to avoid bypassing the whitelist), and \n> > allows for an --export-all option.\n> >\n> > Signed-off-by: H. Peter Anvin \n> \n> I understand the motivation behind --export-all and directory\n> whitelist and these changes look good. Thanks.\n> \n> > +\tif ( ok_paths && *ok_paths ) {\n> > +\t\tint ok = 0;\n> > +...\n> > +\t}\n> > +\n> > +\treturn 1;\t\t/* Path acceptable */\n> > +}\n> \n> A microNit. You could lose 'int ok' and return 1 directly where\n> you assign 1 to it and break.\n> \n> > -\tchdir(\".git\");\n> \n> I am unsure about this removal of \"minor convenience feature\".\n> Although I do not think git-daemon is widely used on the field,\n> this change breaks existing setup if there is any.\n\nPlease drop this one line change. It certainly breaks my personal\nsetup. And all git tools are happy with being given the \"master\"\ndirectory or the \"master/.git\" so there is no reason for git-daemon not\nto accept that, too.\n\nIf hpa really can't live with the chdir, maybe we could add a\n\"--strict-git-paths\" option or something that will not do the chdir? It\nwould be only a few lines of code in git-daemon to parse the option and\nthen the chdir would become\n\nif (!strict_git_paths)\n\tchdir(\".git\");\n\nBest regards,\n\n Anton\n-- \nAnton Altaparmakov (replace at with @)\nUnix Support, Computing Service, University of Cambridge, CB2 3QH, UK\nLinux NTFS maintainer / IRC: #ntfs on irc.freenode.net\nWWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/\n"},{"id":"9390","messageId":"Pine.LNX.4.58.0509270802140.3308@g5.osdl.org","threadId":"1957","inReplyTo":"4338AACC.1050305@zytor.com","subject":"Re: git-daemon: path validation, export all option","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-09-27T15:03:54Z","receivedAt":"2005-09-27T15:03:54Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 26 Sep 2005, H. Peter Anvin wrote:\n>\n> A first attempt to make git-daemon a bit more suitable for kernel.org \n> use: it allows the user to specify a whitelist of directories, rejects \n> paths which have . or .. in them (to avoid bypassing the whitelist), and \n> allows for an --export-all option.\n\nRemoving the \"chdir(\".git\")\" thing is very wrong, though. Why do it?\n\nIt's very much on purpose: you can export even \"regular\" git trees (ie \ntrees you have checked out) without the other side having to say\n\n\tgit clone machine.com:/home/torvalds/v2.6/linux/.git\n\nwhere the final \"/.git\" is just stupid.\n\n\t\tLinus\n"},{"id":"9394","messageId":"433966F2.3090304@zytor.com","threadId":"1957","inReplyTo":"Pine.LNX.4.58.0509270802140.3308@g5.osdl.org","subject":"Re: git-daemon: path validation, export all option","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-09-27T15:36:18Z","receivedAt":"2005-09-27T15:36:18Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Linus Torvalds wrote:\n> \n> Removing the \"chdir(\".git\")\" thing is very wrong, though. Why do it?\n> \n> It's very much on purpose: you can export even \"regular\" git trees (ie \n> trees you have checked out) without the other side having to say\n> \n> \tgit clone machine.com:/home/torvalds/v2.6/linux/.git\n> \n> where the final \"/.git\" is just stupid.\n> \n\nAgreed. I wasn't thinking too hard about it, and it doesn't do any harm \nsince failure is ignored.\n\n\t-hpa\n"},{"id":"9396","messageId":"43396FF9.1000900@zytor.com","threadId":"1957","inReplyTo":"7vslvr6t1u.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-daemon: path validation, export all option","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-09-27T16:14:49Z","receivedAt":"2005-09-27T16:14:49Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Junio C Hamano wrote:\n> \n> A microNit. You could lose 'int ok' and return 1 directly where\n> you assign 1 to it and break.\n> \n\nI guess I personally prefer the coding style where the straigh-line flow \nof control is the normal one. It prevents the \"oops\" of someone wanting \nto add code to it later.\n\n> \n>>-\tchdir(\".git\");\n> \n> I am unsure about this removal of \"minor convenience feature\".\n> Although I do not think git-daemon is widely used on the field,\n> this change breaks existing setup if there is any.\n\nI have restored this and make the requested RPM changes. I have left a \npullable tree at:\n\nmaster.kernel.org:/home/hpa/git/daemon.git\n\n... in order to preserve the commit structure.\n\n\t-hpa\n"},{"id":"9397","messageId":"7vd5mu30wd.fsf@assigned-by-dhcp.cox.net","threadId":"1957","inReplyTo":"43396FF9.1000900@zytor.com","subject":"Re: git-daemon: path validation, export all option","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-27T16:56:18Z","receivedAt":"2005-09-27T16:56:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H. Peter Anvin\" writes:\n\n> I have restored this and make the requested RPM changes. I have left a \n> pullable tree at:\n>\n> master.kernel.org:/home/hpa/git/daemon.git\n>\n> ... in order to preserve the commit structure.\n\nThanks. Will pull tonight.\n"}]}