{"thread":{"id":"2576","subject":"[PATCH 5/5] git-daemon support for user-relative paths.","startedAt":"2005-11-17T19:37:14Z","lastAt":"2005-11-21T23:29:28Z","messageCount":12,"participants":["Andreas Ericsson","Junio C Hamano","H. Peter Anvin","Matthias Urlichs"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"12137","messageId":"20051117193714.428785C7FA@nox.op5.se","threadId":"2576","inReplyTo":null,"subject":"[PATCH 5/5] git-daemon support for user-relative paths.","fromName":"Andreas Ericsson","fromEmail":"exon@op5.se","sentAt":"2005-11-17T19:37:14Z","receivedAt":"2005-11-17T19:37:14Z","isPatch":true,"sender":{"key":"exon@op5.se","avatar":"https://gravatar.com/avatar/b948c4f759e868f8e721e545e37afe2cf89cfa8e2ee8b70a432f0d76aee39891?d=mp&s=160"},"body":"\nDropped a fair amount of reundant code in favour of the library code\nin path.c\n\nAdded option --strict-paths with documentation, with backwards\ncompatibility for whitelist entries with symlinks.\n\nEverything that worked earlier still works insofar as I have\nremembered testing it.\n\nSigned-off-by: Andreas Ericsson <ae@op5.se>\n\n---\n\n Documentation/git-daemon.txt       |   16 ++++\n Documentation/pull-fetch-param.txt |    7 +-\n daemon.c                           |  136 ++++++++++++++----------------------\n 3 files changed, 72 insertions(+), 87 deletions(-)\n\napplies-to: d1e53de141c477b0b7dd2c6bd5897ea6239a7b20\n4c4049426e6202bdb1429f5aeb4e5429df41c015\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 3783858..972e0e1 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -29,9 +29,15 @@ This is ideally suited for read-only upd\n \n OPTIONS\n -------\n++--strict-paths::\n+\tMatch paths exactly (i.e. don't allow \"/foo/repo\" when the real path is\n+\t\"/foo/repo.git\" or \"/foo/repo/.git\") and don't do user-relative paths.\n+\tgit-daemon will refuse to start when this option is enabled and no\n+\twhitelist is specified.\n+\n --export-all::\n \tAllow pulling from all directories that look like GIT repositories\n-\t(have the 'objects' subdirectory and a 'HEAD' file), even if they\n+\t(have the 'objects' and 'refs' subdirectories), even if they\n \tdo not have the 'git-daemon-export-ok' file.\n \n --inetd::\n@@ -57,9 +63,15 @@ OPTIONS\n --verbose::\n \tLog details about the incoming connections and requested files.\n \n+<directory>::\n+\tA directory to add to the whitelist of allowed directories. Unless\n+\t--strict-paths is specified this will also include subdirectories\n+\tof each named directory.\n+\n Author\n ------\n-Written by Linus Torvalds <torvalds@osdl.org> and YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>\n+Written by Linus Torvalds <torvalds@osdl.org>, YOSHIFUJI Hideaki\n+<yoshfuji@linux-ipv6.org> and the git-list <git@vger.kernel.org>\n \n Documentation\n --------------\ndiff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt\nindex a7628aa..6413d52 100644\n--- a/Documentation/pull-fetch-param.txt\n+++ b/Documentation/pull-fetch-param.txt\n@@ -9,15 +9,16 @@\n - http://host.xz/path/to/repo.git/\n - https://host.xz/path/to/repo.git/\n - git://host.xz/path/to/repo.git/\n+- git://host.xz/~user/path/to/repo.git/\n - ssh://host.xz/path/to/repo.git/\n - ssh://host.xz/~user/path/to/repo.git/\n - ssh://host.xz/~/path/to/repo.git\n ===============================================================\n +\n \tSSH Is the default transport protocol and also supports an\n-\tscp-like syntax.  Both syntaxes support username expansion.\n-\tThe following three are identical to the last three above,\n-\trespectively:\n+\tscp-like syntax.  Both syntaxes support username expansion,\n+\tas does the native git protocol. The following three are\n+\tidentical to the last three above, respectively:\n +\n ===============================================================\n - host.xz:/path/to/repo.git/\ndiff --git a/daemon.c b/daemon.c\nindex 2b81152..ac4c94b 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -15,10 +15,11 @@ static int verbose;\n \n static const char daemon_usage[] =\n \"git-daemon [--verbose] [--syslog] [--inetd | --port=n] [--export-all]\\n\"\n-\"           [--timeout=n] [--init-timeout=n] [directory...]\";\n+\"           [--timeout=n] [--init-timeout=n] [--strict-paths] [directory...]\";\n \n /* List of acceptable pathname prefixes */\n static char **ok_paths = NULL;\n+static int strict_paths = 0;\n \n /* If this is set, git-daemon-export-ok is not required */\n static int export_all_trees = 0;\n@@ -81,69 +82,56 @@ static void loginfo(const char *err, ...\n \tva_end(params);\n }\n \n-static int path_ok(const char *dir)\n+static char *path_ok(char *dir)\n {\n-\tconst char *p = dir;\n-\tchar **pp;\n-\tint sl, ndot;\n+\tchar *path = enter_repo(dir, strict_paths);\n \n-\t/* The pathname here should be an absolute path. */\n-\tif ( *p++ != '/' )\n-\t\treturn 0;\n-\n-\tsl = 1;  ndot = 0;\n-\n-\tfor (;;) {\n-\t\tif ( *p == '.' ) {\n-\t\t\tndot++;\n-\t\t} else if ( *p == '\\0' ) {\n-\t\t\t/* Reject \".\" and \"..\" at the end of the path */\n-\t\t\tif ( sl && ndot > 0 && ndot < 3 )\n-\t\t\t\treturn 0;\n-\n-\t\t\t/* Otherwise OK */\n-\t\t\tbreak;\n-\t\t} else if ( *p == '/' ) {\n-\t\t\t/* Refuse \"\", \".\" or \"..\" */\n-\t\t\tif ( sl && ndot < 3 )\n-\t\t\t\treturn 0;\n-\t\t\tsl = 1;\n-\t\t\tndot = 0;\n-\t\t} else {\n-\t\t\tsl = ndot = 0;\n-\t\t}\n-\t\tp++;\n+\tif (!path) {\n+\t\tlogerror(\"'%s': unable to chdir or not a git archive\", dir);\n+\t\treturn NULL;\n \t}\n \n \tif ( ok_paths && *ok_paths ) {\n-\t\tint ok = 0;\n+\t\tchar **pp = NULL;\n \t\tint dirlen = strlen(dir);\n+\t\tint pathlen = strlen(path);\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/* because of symlinks we must match both what the\n+\t\t\t * user passed and the canonicalized path, otherwise\n+\t\t\t * the user can send a string matching either a whitelist\n+\t\t\t * entry or an actual directory exactly and still not\n+\t\t\t * get through */\n+\t\t\tif (len <= pathlen && !memcmp(*pp, path, len)) {\n+\t\t\t\tif (path[len] == '\\0' || (!strict_paths && path[len] == '/'))\n+\t\t\t\t\treturn path;\n+\t\t\t}\n+\t\t\tif (len <= dirlen && !memcmp(*pp, dir, len)) {\n+\t\t\t\tif (dir[len] == '\\0' || (!strict_paths && dir[len] == '/'))\n+\t\t\t\t\treturn path;\n \t\t\t}\n \t\t}\n-\n-\t\tif ( !ok )\n-\t\t\treturn 0; /* Path not in whitelist */\n+\t}\n+\telse {\n+\t\t/* be backwards compatible */\n+\t\tif (!strict_paths)\n+\t\t\treturn path;\n \t}\n \n-\treturn 1;\t\t/* Path acceptable */\n+\tlogerror(\"'%s': not in whitelist\", path);\n+\treturn NULL;\t\t/* Fallthrough. Deny by default */\n }\n \n-static int set_dir(const char *dir)\n+static int upload(char *dir)\n {\n-\tif (!path_ok(dir)) {\n-\t\terrno = EACCES;\n-\t\treturn -1;\n-\t}\n+\t/* Timeout as string */\n+\tchar timeout_buf[64];\n+\tconst char *path;\n \n-\tif ( chdir(dir) )\n+\tloginfo(\"Request for '%s'\", dir);\n+\n+\tif (!(path = path_ok(dir)))\n \t\treturn -1;\n \n \t/*\n@@ -152,45 +140,17 @@ static int set_dir(const char *dir)\n \t * We want a readable HEAD, usable \"objects\" directory, and\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+\t * path_ok() uses enter_repo() and does whitelist checking.\n+\t * We only need to make sure the repository is exported.\n \t */\n+\n \tif (!export_all_trees && access(\"git-daemon-export-ok\", F_OK)) {\n+\t\tlogerror(\"'%s': repository not exported.\", path);\n \t\terrno = EACCES;\n \t\treturn -1;\n \t}\n \n-\tif (access(\"objects/\", X_OK) || access(\"HEAD\", R_OK)) {\n-\t\terrno = EINVAL;\n-\t\treturn -1;\n-\t}\n-\n-\t/* If all this passed, we're OK */\n-\treturn 0;\n-}\n-\n-static int upload(char *dir)\n-{\n-\t/* Try paths in this order */\n-\tstatic const char *paths[] = { \"%s\", \"%s/.git\", \"%s.git\", \"%s.git/.git\", NULL };\n-\tconst char **pp;\n-\t/* Enough for the longest path above including final null */\n-\tint buflen = strlen(dir)+10;\n-\tchar *dirbuf = xmalloc(buflen);\n-\t/* Timeout as string */\n-\tchar timeout_buf[64];\n-\n-\tloginfo(\"Request for '%s'\", dir);\n-\n-\tfor ( pp = paths ; *pp ; pp++ ) {\n-\t\tsnprintf(dirbuf, buflen, *pp, dir);\n-\t\tif ( !set_dir(dirbuf) )\n-\t\t\tbreak;\n-\t}\n-\n-\tif ( !*pp ) {\n-\t\tlogerror(\"Cannot set directory '%s': %s\", dir, strerror(errno));\n-\t\treturn -1;\n-\t}\n-\n \t/*\n \t * We'll ignore SIGTERM from now on, we have a\n \t * good client.\n@@ -200,7 +160,7 @@ static int upload(char *dir)\n \tsnprintf(timeout_buf, sizeof timeout_buf, \"--timeout=%u\", timeout);\n \n \t/* git-upload-pack only ever reads stuff, so this is safe */\n-\texeclp(\"git-upload-pack\", \"git-upload-pack\", \"--strict\", timeout_buf, \".\", NULL);\n+\texeclp(\"git-upload-pack\", \"git-upload-pack\", \"--strict\", timeout_buf, path, NULL);\n \treturn -1;\n }\n \n@@ -216,7 +176,7 @@ static int execute(void)\n \tif (len && line[len-1] == '\\n')\n \t\tline[--len] = 0;\n \n-\tif (!strncmp(\"git-upload-pack /\", line, 17))\n+\tif (!strncmp(\"git-upload-pack \", line, 16))\n \t\treturn upload(line+16);\n \n \tlogerror(\"Protocol error: '%s'\", line);\n@@ -617,6 +577,10 @@ int main(int argc, char **argv)\n \t\t\tinit_timeout = atoi(arg+15);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--strict-paths\")) {\n+\t\t\tstrict_paths = 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@@ -631,6 +595,14 @@ int main(int argc, char **argv)\n \tif (log_syslog)\n \t\topenlog(\"git-daemon\", 0, LOG_DAEMON);\n \n+\tif (strict_paths && (!ok_paths || !*ok_paths)) {\n+\t\tif (!inetd_mode)\n+\t\t\tdie(\"git-daemon: option --strict-paths requires a whitelist\");\n+\n+\t\tlogerror(\"option --strict-paths requires a whitelist\");\n+\t\texit (1);\n+\t}\n+\n \tif (inetd_mode) {\n \t\tfclose(stderr); //FIXME: workaround\n \t\treturn execute();\n---\n0.99.9.GIT\n"},{"id":"12171","messageId":"7voe4ird8v.fsf@assigned-by-dhcp.cox.net","threadId":"2576","inReplyTo":"20051117193714.428785C7FA@nox.op5.se","subject":"Re: [PATCH 5/5] git-daemon support for user-relative paths.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-11-18T00:49:52Z","receivedAt":"2005-11-18T00:49:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"exon@op5.se (Andreas Ericsson) writes:\n\n> Everything that worked earlier still works insofar as I have\n> remembered testing it.\n\nHmph...\n\n> @@ -152,45 +140,17 @@ static int set_dir(const char *dir)\n>  \t * We want a readable HEAD, usable \"objects\" directory, and\n>  \t * a \"git-daemon-export-ok\" flag that says that the other side\n>  \t * is ok with us doing this.\n\nWell, not anymore about HEAD as far as I can tell...  Maybe in\nenter_repo ([PATCH 1/5]) we need to do something like what\nsetup.c::is_toplevel_directory() does?\n\n> -static int upload(char *dir)\n> -{\n> -\t/* Try paths in this order */\n> -\tstatic const char *paths[] = { \"%s\", \"%s/.git\", \"%s.git\", \"%s.git/.git\", NULL };\n\nI think this list was added relatively recently as a usability\nmeasure.  Maybe we would want an equivalent in enter_repo()?\nUnder strict-path, I think not doing any DWIM like this is fine,\nbut otherwise I suspect changing this would break existing\nremotes/origin file people may have.  In addition enter_repo()\nas posted does its own DWIM to chdir to \".git\" unconditionally\nas I pointed out...\n\nNeeds a bit more thought, but I think otherwise the basic idea\nis right.\n"},{"id":"12210","messageId":"437DAA66.6070301@op5.se","threadId":"2576","inReplyTo":"7voe4ird8v.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 5/5] git-daemon support for user-relative paths.","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2005-11-18T10:18:14Z","receivedAt":"2005-11-18T10:18:14Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n> exon@op5.se (Andreas Ericsson) writes:\n> \n> \n>>Everything that worked earlier still works insofar as I have\n>>remembered testing it.\n> \n> \n> Hmph...\n> \n\n:)\n\nI haven't used the daemon without our patches. Everything works as per \nspec now though.\n\n> \n>>@@ -152,45 +140,17 @@ static int set_dir(const char *dir)\n>> \t * We want a readable HEAD, usable \"objects\" directory, and\n>> \t * a \"git-daemon-export-ok\" flag that says that the other side\n>> \t * is ok with us doing this.\n> \n> \n> Well, not anymore about HEAD as far as I can tell...  Maybe in\n> enter_repo ([PATCH 1/5]) we need to do something like what\n> setup.c::is_toplevel_directory() does?\n> \n\nUmm... Perhaps. I just noticed that it's possible for the .git/objects \ndirectory to go missing though.\n\n> \n>>-static int upload(char *dir)\n>>-{\n>>-\t/* Try paths in this order */\n>>-\tstatic const char *paths[] = { \"%s\", \"%s/.git\", \"%s.git\", \"%s.git/.git\", NULL };\n> \n> \n> I think this list was added relatively recently as a usability\n> measure.  Maybe we would want an equivalent in enter_repo()?\n\n\nIt's already there but in a different format. Adding \"if (!strict)\" to \nthe previously unconditional 'chdir(\".git\");' won't change that.\n\nLike I said, I made sure everything that worked before works now too.\n\n\n> Under strict-path, I think not doing any DWIM like this is fine,\n> but otherwise I suspect changing this would break existing\n> remotes/origin file people may have.  In addition enter_repo()\n> as posted does its own DWIM to chdir to \".git\" unconditionally\n> as I pointed out...\n> \n\n\nDWIM? That's an acronym I don't know.\n\n\n> Needs a bit more thought, but I think otherwise the basic idea\n> is right.\n> \n\nAnything I should change before \"take four\" ?\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"12390","messageId":"pan.2005.11.18.17.57.52.158522@smurf.noris.de","threadId":"2576","inReplyTo":"437DAA66.6070301@op5.se","subject":"Re: [PATCH 5/5] git-daemon support for user-relative paths.","fromName":"Matthias Urlichs","fromEmail":"smurf@smurf.noris.de","sentAt":"2005-11-18T17:57:53Z","receivedAt":"2005-11-18T17:57:53Z","isPatch":true,"sender":{"key":"matthias@urlichs.de","avatar":"https://gravatar.com/avatar/2708905af227313eba6f2b2ae0f7d0259b5ac5d71baef58fe5a13c699ce0bbf0?d=mp&s=160"},"body":"Hi, Andreas Ericsson wrote:\n\n> DWIM? That's an acronym I don't know.\n\n\"Do What I Mean\".\n\nIt's actually meant to be DMNS (\"Do what i Mean, Not what i Say\"),\nbut some early AI hackers at MIT shortened it maliciously so that\nthey could claim at least *some* success.  :-)\n\n-- \nMatthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de\nDisclaimer: The quote was selected randomly. Really. | http://smurf.noris.de\n - -\ndo you want my self-identities alphabetically, chronologically, or in\nrandom order?  -- Misha\n"},{"id":"12266","messageId":"437E3C7C.6000303@zytor.com","threadId":"2576","inReplyTo":"437DAA66.6070301@op5.se","subject":"Re: [PATCH 5/5] git-daemon support for user-relative paths.","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-11-18T20:41:32Z","receivedAt":"2005-11-18T20:41:32Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Andreas Ericsson wrote:\n> \n> It's already there but in a different format. Adding \"if (!strict)\" to \n> the previously unconditional 'chdir(\".git\");' won't change that.\n> \n> Like I said, I made sure everything that worked before works now too.\n> \n>> Under strict-path, I think not doing any DWIM like this is fine,\n>> but otherwise I suspect changing this would break existing\n>> remotes/origin file people may have.  In addition enter_repo()\n>> as posted does its own DWIM to chdir to \".git\" unconditionally\n>> as I pointed out...\n> \n> DWIM? That's an acronym I don't know.\n> \n\nDWIM = \"Do What I Mean\", i.e. program trying to be clever.  A (usually) \ngood thing for usability, a very bad thing for security.\n\nIn particular, DWIM is bad for security when you have a flow like:\n\n\tuser input -> security check -> DWIM\n\n... which lets the user subvert the security check by knowing how the \nDWIM will mangle the input.  What's worse, programmers like yourself \nfrequently say \"oh, it's okay, though, I know what the DWIM does and it \ncan't break the security checks I do.\"\n\nWell, then someone comes along and changes either the security checks \n(e.g. add a blacklist), or the DWIM, or both.  Security hole opens.\n\nTherefore, the flow must *ALWAYS* be:\n\n\tuser input -> DWIM -> security check\n\n\nYour patch re-introduces the incorrect flow.\n\n\t-hpa\n"},{"id":"12253","messageId":"7viruphd6v.fsf@assigned-by-dhcp.cox.net","threadId":"2576","inReplyTo":"437DAA66.6070301@op5.se","subject":"Re: [PATCH 5/5] git-daemon support for user-relative paths.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-11-18T21:13:28Z","receivedAt":"2005-11-18T21:13:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Ericsson <ae@op5.se> writes:\n\n>>>-static int upload(char *dir)\n>>>-{\n>>>-\t/* Try paths in this order */\n>>>-\tstatic const char *paths[] = { \"%s\", \"%s/.git\", \"%s.git\", \"%s.git/.git\", NULL };\n>\n>...\n>\n>> Under strict-path, I think not doing any DWIM like this is fine,\n>> but otherwise I suspect changing this would break existing\n>> remotes/origin file people may have.  In addition enter_repo()\n>> as posted does its own DWIM to chdir to \".git\" unconditionally\n>> as I pointed out...\n>\n> DWIM? That's an acronym I don't know.\n\n\"Do what I mean\".  It lets users say:\n\n\tgit clone git://sample.xz/pub/uemacs uemacs\n\nwhen the repository on the server side is at any of the\nfollowing places:\n\n\t/pub/uemacs\n\n\t-- a regular naked repository, with subdirectories\n           /pub/uemacs/refs and /pub/uemacs/objects/, obviously.\n\n        /pub/uemacs/.git\n\n        -- /pub/uemacs is an ordinary repository with possibly a\n           working tree; has /pub/uemacs/.git/refs and friends.\n\n\t/pub/uemacs.git\n\n        -- when above two do not exist but this does; a regular\n\t   naked repository, with subdirectories\n\t   /pub/uemacs.git/refs and friends.\n\n\t/pub/uemacs.git/.git\n\n        -- no /pub/uemacs, and /pub/uemacs.git is an ordinary\n           repository with possibly a working tree; has\n           /pub/uemacs.git/.git/refs and friends.\n\nwhich is a nice feature, but under --strict-path we need to be\ncareful that we apply whitelist correctly while allowing DWIM.\n"},{"id":"12287","messageId":"7voe4hfssj.fsf@assigned-by-dhcp.cox.net","threadId":"2576","inReplyTo":"437DAA66.6070301@op5.se","subject":"Re: [PATCH 5/5] git-daemon support for user-relative paths.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-11-18T23:19:24Z","receivedAt":"2005-11-18T23:19:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Ericsson <ae@op5.se> writes:\n\n>> I think this list was added relatively recently as a usability\n>> measure.  Maybe we would want an equivalent in enter_repo()?\n>\n> It's already there but in a different format.\n\nI noticed that after I asked that question.  Thanks.\n\n>> Under strict-path, I think not doing any DWIM like this is fine,\n>> but otherwise I suspect changing this would break existing\n>> remotes/origin file people may have.  In addition enter_repo()\n>> as posted does its own DWIM to chdir to \".git\" unconditionally\n>> as I pointed out...\n>\n> DWIM? That's an acronym I don't know.\n\nI think you got HPA's message about it and its security\nimplications.\n\n>> Needs a bit more thought, but I think otherwise the basic idea\n>> is right.\n>\n> Anything I should change before \"take four\" ?\n\nI think it might make sense to inserting something like the\nattached untested patch in your series, between library and\nupload-pack.  The validation done by path_ok() in git-daemon\nprobabaly needs to lose alternate checks and validate only the\npath returned by enter_repo().  This would make writing\nwhitelist by git-daemon administrator a bit more cumbersome, but\nI suspect at the same time would make auditing easier.  So the\nseries would become:\n\n[1/6] Library code for user-relative paths, take three.\n[2/6] Do not DWIM in userpath library under strict mode.\n[3/6] Server-side support for user-relative paths.\n[4/6] Client side support for user-relative paths.\n[5/6] Documentation update for user-relative paths.\n[6/6] git-daemon support for user-relative paths.\n\n-- >8 -- cut here -- >8 --\nSubject: [PATCH] Do not DWIM in userpath library under strict mode.\n\nThis should force git-daemon administrator's job a bit harder\nbecause the exact paths need to be given in the whitelist, but\nat the same time makes the auditing easier.\n\nThis moves validate_symref() from refs.c to path.c, because we\nneed to link git-daemon with path.c for its \"enter_repo()\", but\nwe do not want to link the daemon with the rest of git libraries\nand its requirements.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n\n---\n\n path.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++--------------\n refs.c |   40 ---------------------------------\n 2 files changed, 60 insertions(+), 57 deletions(-)\n\napplies-to: 7223b28ede4511977d1e71f45fb2867780027235\nbd8770050ee2c8d7aa5b2c3f138bc65d83a974e8\ndiff --git a/path.c b/path.c\nindex 5b61709..d635470 100644\n--- a/path.c\n+++ b/path.c\n@@ -91,20 +91,55 @@ char *safe_strncpy(char *dest, const cha\n \treturn dest;\n }\n \n+int validate_symref(const char *path)\n+{\n+\tstruct stat st;\n+\tchar *buf, buffer[256];\n+\tint len, fd;\n+\n+\tif (lstat(path, &st) < 0)\n+\t\treturn -1;\n+\n+\t/* Make sure it is a \"refs/..\" symlink */\n+\tif (S_ISLNK(st.st_mode)) {\n+\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n+\t\tif (len >= 5 && !memcmp(\"refs/\", buffer, 5))\n+\t\t\treturn 0;\n+\t\treturn -1;\n+\t}\n+\n+\t/*\n+\t * Anything else, just open it and try to see if it is a symbolic ref.\n+\t */\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn -1;\n+\tlen = read(fd, buffer, sizeof(buffer)-1);\n+\tclose(fd);\n+\n+\t/*\n+\t * Is it a symbolic ref?\n+\t */\n+\tif (len < 4 || memcmp(\"ref:\", buffer, 4))\n+\t\treturn -1;\n+\tbuf = buffer + 4;\n+\tlen -= 4;\n+\twhile (len && isspace(*buf))\n+\t\tbuf++, len--;\n+\tif (len >= 5 && !memcmp(\"refs/\", buf, 5))\n+\t\treturn 0;\n+\treturn -1;\n+}\n+\n static char *current_dir()\n {\n \treturn getcwd(pathname, sizeof(pathname));\n }\n \n-/* Take a raw path from is_git_repo() and canonicalize it using Linus'\n- * idea of a blind chdir() and getcwd(). */\n-static const char *canonical_path(char *path, int strict)\n+static int user_chdir(char *path)\n {\n \tchar *dir = path;\n \n-\tif(strict && *dir != '/')\n-\t\treturn NULL;\n-\n \tif(*dir == '~') {\t\t/* user-relative path */\n \t\tstruct passwd *pw;\n \t\tchar *slash = strchr(dir, '/');\n@@ -125,19 +160,19 @@ static const char *canonical_path(char *\n \n \t\t/* make sure we got something back that we can chdir() to */\n \t\tif(!pw || chdir(pw->pw_dir) < 0)\n-\t\t\treturn NULL;\n+\t\t\treturn -1;\n \n \t\tif(!slash || !slash[1]) /* no path following username */\n-\t\t\treturn current_dir();\n+\t\t\treturn 0;\n \n \t\tdir = slash + 1;\n \t}\n \n \t/* ~foo/path/to/repo is now path/to/repo and we're in foo's homedir */\n \tif(chdir(dir) < 0)\n-\t\treturn NULL;\n+\t\treturn -1;\n \n-\treturn current_dir();\n+\treturn 0;\n }\n \n char *enter_repo(char *path, int strict)\n@@ -145,16 +180,24 @@ char *enter_repo(char *path, int strict)\n \tif(!path)\n \t\treturn NULL;\n \n-\tif(!canonical_path(path, strict)) {\n-\t\tif(strict || !canonical_path(mkpath(\"%s.git\", path), strict))\n+\tif (strict) {\n+\t\tif((path[0] != '/') || chdir(path) < 0)\n \t\t\treturn NULL;\n \t}\n+\telse {\n+\t\tif (!*path)\n+\t\t\t; /* happy -- no chdir */\n+\t\telse if (!user_chdir(path))\n+\t\t\t; /* happy -- as given */\n+\t\telse if (!user_chdir(mkpath(\"%s.git\", path)))\n+\t\t\t; /* happy -- uemacs --> uemacs.git */\n+\t\telse\n+\t\t\treturn NULL;\n+\t\t(void)chdir(\".git\");\n+\t}\n \n-\t/* This is perfectly safe, and people tend to think of the directory\n-\t * where they ran git-init-db as their repository, so humour them. */\n-\t(void)chdir(\".git\");\n-\n-\tif(access(\"objects\", X_OK) == 0 && access(\"refs\", X_OK) == 0) {\n+\tif(access(\"objects\", X_OK) == 0 && access(\"refs\", X_OK) == 0 &&\n+\t   validate_symref(\"HEAD\") == 0) {\n \t\tputenv(\"GIT_DIR=.\");\n \t\treturn current_dir();\n \t}\ndiff --git a/refs.c b/refs.c\nindex f324be5..ac26198 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -10,46 +10,6 @@\n #define USE_SYMLINK_HEAD 1\n #endif\n \n-int validate_symref(const char *path)\n-{\n-\tstruct stat st;\n-\tchar *buf, buffer[256];\n-\tint len, fd;\n-\n-\tif (lstat(path, &st) < 0)\n-\t\treturn -1;\n-\n-\t/* Make sure it is a \"refs/..\" symlink */\n-\tif (S_ISLNK(st.st_mode)) {\n-\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n-\t\tif (len >= 5 && !memcmp(\"refs/\", buffer, 5))\n-\t\t\treturn 0;\n-\t\treturn -1;\n-\t}\n-\n-\t/*\n-\t * Anything else, just open it and try to see if it is a symbolic ref.\n-\t */\n-\tfd = open(path, O_RDONLY);\n-\tif (fd < 0)\n-\t\treturn -1;\n-\tlen = read(fd, buffer, sizeof(buffer)-1);\n-\tclose(fd);\n-\n-\t/*\n-\t * Is it a symbolic ref?\n-\t */\n-\tif (len < 4 || memcmp(\"ref:\", buffer, 4))\n-\t\treturn -1;\n-\tbuf = buffer + 4;\n-\tlen -= 4;\n-\twhile (len && isspace(*buf))\n-\t\tbuf++, len--;\n-\tif (len >= 5 && !memcmp(\"refs/\", buf, 5))\n-\t\treturn 0;\n-\treturn -1;\n-}\n-\n const char *resolve_ref(const char *path, unsigned char *sha1, int reading)\n {\n \tint depth = MAXDEPTH, len;\n---\n0.99.9.GIT\n"},{"id":"12276","messageId":"437E67AC.2010400@op5.se","threadId":"2576","inReplyTo":"7voe4hfssj.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 5/5] git-daemon support for user-relative paths.","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2005-11-18T23:45:48Z","receivedAt":"2005-11-18T23:45:48Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n> \n> I think it might make sense to inserting something like the\n> attached untested patch in your series, between library and\n> upload-pack.\n\nI'll run the clone/fetch/push test-suite again tomorrow, with this \napplied. It looks good though.\n\n>  The validation done by path_ok() in git-daemon\n> probabaly needs to lose alternate checks and validate only the\n> path returned by enter_repo().  This would make writing\n> whitelist by git-daemon administrator a bit more cumbersome,\n\n\nNot necessarily. The repositories in the whitelist should be validated \n(and possibly converted) using the path_ok() function. This will also \nmake it possible to catch typos and permission errors that are just \nplain annoying for the admins.\n\nIn non-strict mode this isn't really a problem so long as all \nwhitelist-paths are absolute and doesn't contain any symlinks, although \nwe could use the chdir() + getcwd() thingie since we don't need the \nability to go back to where we started and that's what will be used \nlater when serving the repos.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"12424","messageId":"7vfypquz88.fsf@assigned-by-dhcp.cox.net","threadId":"2576","inReplyTo":"437E67AC.2010400@op5.se","subject":"Re: [PATCH 5/5] git-daemon support for user-relative paths.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-11-21T09:28:07Z","receivedAt":"2005-11-21T09:28:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Ericsson <ae@op5.se> writes:\n\n> I'll run the clone/fetch/push test-suite again tomorrow, with this \n> applied. It looks good though.\n\nSorry, but there was a thinko in my butchered version of\nenter_repo().  While allowing only absolute path was good for\nthe version with your daemon.c change, it was not with the\ncurrent one that runs upload-pack with \".\" as repo.  In either\ncase we _do_ chdir() to it after validating the path, so I am\nwondering if it is a good idea to keep sending \".\" as repo when\nexecuting upload-pack with this patch as well.  This does not\nmake any practical difference, but I think it makes the intent\nclearer -- \"we are already there so do not try going anywhere\nelse\".\n\nSo I am thinking about applying something like this patch\non top of the last part of your patch.\n\n - Do validation only on canonicalized paths;\n - Run upload-pack with \".\" as repo, not full path;\n - allow trailing slash under --strict-paths i.e. \"git://host/my/repo.git/\"\n\nWhat do you think?\n\n---\n\ndiff --git a/daemon.c b/daemon.c\nindex ac4c94b..21d8260 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -93,22 +93,19 @@ static char *path_ok(char *dir)\n \n \tif ( ok_paths && *ok_paths ) {\n \t\tchar **pp = NULL;\n-\t\tint dirlen = strlen(dir);\n \t\tint pathlen = strlen(path);\n \n+\t\t/* The validation is done on the paths after enter_repo\n+\t\t * canonicalization, so whitelist should be written in\n+\t\t * terms of real pathnames (i.e. after ~user is expanded\n+\t\t * and symlinks resolved).\n+\t\t */\n \t\tfor ( pp = ok_paths ; *pp ; pp++ ) {\n \t\t\tint len = strlen(*pp);\n-\t\t\t/* because of symlinks we must match both what the\n-\t\t\t * user passed and the canonicalized path, otherwise\n-\t\t\t * the user can send a string matching either a whitelist\n-\t\t\t * entry or an actual directory exactly and still not\n-\t\t\t * get through */\n \t\t\tif (len <= pathlen && !memcmp(*pp, path, len)) {\n-\t\t\t\tif (path[len] == '\\0' || (!strict_paths && path[len] == '/'))\n-\t\t\t\t\treturn path;\n-\t\t\t}\n-\t\t\tif (len <= dirlen && !memcmp(*pp, dir, len)) {\n-\t\t\t\tif (dir[len] == '\\0' || (!strict_paths && dir[len] == '/'))\n+\t\t\t\tif (path[len] == '\\0' ||\n+\t\t\t\t    (path[len] == '/' &&\n+\t\t\t\t     (!strict_paths || path[len+1] == 0)))\n \t\t\t\t\treturn path;\n \t\t\t}\n \t\t}\n@@ -160,7 +157,7 @@ static int upload(char *dir)\n \tsnprintf(timeout_buf, sizeof timeout_buf, \"--timeout=%u\", timeout);\n \n \t/* git-upload-pack only ever reads stuff, so this is safe */\n-\texeclp(\"git-upload-pack\", \"git-upload-pack\", \"--strict\", timeout_buf, path, NULL);\n+\texeclp(\"git-upload-pack\", \"git-upload-pack\", \"--strict\", timeout_buf, \".\", NULL);\n \treturn -1;\n }\n \n\n\n \n"},{"id":"12425","messageId":"7vbr0euy8n.fsf@assigned-by-dhcp.cox.net","threadId":"2576","inReplyTo":"7vfypquz88.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 5/5] git-daemon support for user-relative paths.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-11-21T09:49:28Z","receivedAt":"2005-11-21T09:49:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> ...So I am thinking about applying something like this patch\n> on top of the last part of your patch.\n>\n>  - Do validation only on canonicalized paths;\n>  - Run upload-pack with \".\" as repo, not full path;\n>  - allow trailing slash under --strict-paths i.e. \"git://host/my/repo.git/\"\n\nSorry, the last piece was totally unneeded; we are operating on\nreturn value from getcwd at this point.\n"},{"id":"12427","messageId":"4381AB38.7090209@op5.se","threadId":"2576","inReplyTo":"7vfypquz88.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 5/5] git-daemon support for user-relative paths.","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2005-11-21T11:10:48Z","receivedAt":"2005-11-21T11:10:48Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n> Andreas Ericsson <ae@op5.se> writes:\n> \n> \n>>I'll run the clone/fetch/push test-suite again tomorrow, with this \n>>applied. It looks good though.\n> \n> \n> Sorry, but there was a thinko in my butchered version of\n> enter_repo().  While allowing only absolute path was good for\n> the version with your daemon.c change, it was not with the\n> current one that runs upload-pack with \".\" as repo.  In either\n> case we _do_ chdir() to it after validating the path, so I am\n> wondering if it is a good idea to keep sending \".\" as repo when\n> executing upload-pack with this patch as well.\n\nIt might be, and it's good since it prevents the otherwise possible race \nthat occurs when git-upload-pack chdir()'s again.\n\n>  This does not\n> make any practical difference, but I think it makes the intent\n> clearer -- \"we are already there so do not try going anywhere\n> else\".\n> \n\nSo enter_repo allows \".\" (exactly and without chdir()) and all paths \nstarting with '/' if strict?\n\n> So I am thinking about applying something like this patch\n> on top of the last part of your patch.\n> \n>  - Do validation only on canonicalized paths;\n>  - Run upload-pack with \".\" as repo, not full path;\n>  - allow trailing slash under --strict-paths i.e. \"git://host/my/repo.git/\"\n> \n> What do you think?\n> \n\nApart from comments and indentation it's more or less exactly what I \nhave in my revised git-daemon patch (although without what you mentioned \nin your own reply to this mail).\n\nDo you want the revised one from me or will you apply the original with \nthis on top?\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"12491","messageId":"7vhda5mvfr.fsf@assigned-by-dhcp.cox.net","threadId":"2576","inReplyTo":"4381AB38.7090209@op5.se","subject":"Re: [PATCH 5/5] git-daemon support for user-relative paths.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-11-21T23:29:28Z","receivedAt":"2005-11-21T23:29:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Ericsson <ae@op5.se> writes:\n\n> Apart from comments and indentation it's more or less exactly what I \n> have in my revised git-daemon patch (although without what you mentioned \n> in your own reply to this mail).\n>\n> Do you want the revised one from me or will you apply the original with \n> this on top?\n\nWell, let's push out what we have so far to \"master\" and then\nfinish up whatever breakage if any in tree.\n"}]}