{"thread":{"id":"2731","subject":"git pull aborts in 50% of cases","startedAt":"2005-12-02T18:58:43Z","lastAt":"2005-12-03T21:28:07Z","messageCount":17,"participants":["Alexey Dobriyan","H. Peter Anvin","Junio C Hamano","Johannes Schindelin","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"13110","messageId":"43909963.60901@zytor.com","threadId":"2731","inReplyTo":"20051202190412.GA10757@mipter.zuzino.mipt.ru","subject":"Re: git pull aborts in 50% of cases","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-12-02T18:58:43Z","receivedAt":"2005-12-02T18:58:43Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Alexey Dobriyan wrote:\n> $ git pull\n> Already up-to-date.\n> $ git pull\n> Already up-to-date.\n> $ git pull\n> Already up-to-date.\n> $ git pull\n> fatal: unexpected EOF\n> Fetch failure: git://git.kernel.org/pub/scm/git/git.git\n> $ git pull\n> fatal: unexpected EOF\n> Fetch failure: git://git.kernel.org/pub/scm/git/git.git\n> $ git pull\n> Already up-to-date.\n> $ git pull\n> fatal: unexpected EOF\n> Fetch failure: git://git.kernel.org/pub/scm/git/git.git\n> $ git pull\n> fatal: unexpected EOF\n> Fetch failure: git://git.kernel.org/pub/scm/git/git.git\n> \n> Ditto for \"git fetch --tags\".\n> \n> Can somebody explain what's going on?\n> \n\nDo you know which IP address is aborting? There are two servers behind \ngit.kernel.org.\n\n\t-hpa\n"},{"id":"13109","messageId":"20051202190412.GA10757@mipter.zuzino.mipt.ru","threadId":"2731","inReplyTo":null,"subject":"git pull aborts in 50% of cases","fromName":"Alexey Dobriyan","fromEmail":"adobriyan@gmail.com","sentAt":"2005-12-02T19:04:12Z","receivedAt":"2005-12-02T19:04:12Z","isPatch":false,"sender":{"key":"adobriyan@gmail.com","avatar":null},"body":"$ git pull\nAlready up-to-date.\n$ git pull\nAlready up-to-date.\n$ git pull\nAlready up-to-date.\n$ git pull\nfatal: unexpected EOF\nFetch failure: git://git.kernel.org/pub/scm/git/git.git\n$ git pull\nfatal: unexpected EOF\nFetch failure: git://git.kernel.org/pub/scm/git/git.git\n$ git pull\nAlready up-to-date.\n$ git pull\nfatal: unexpected EOF\nFetch failure: git://git.kernel.org/pub/scm/git/git.git\n$ git pull\nfatal: unexpected EOF\nFetch failure: git://git.kernel.org/pub/scm/git/git.git\n\nDitto for \"git fetch --tags\".\n\nCan somebody explain what's going on?\n"},{"id":"13115","messageId":"4390B64E.20601@zytor.com","threadId":"2731","inReplyTo":"20051202211250.GA11384@mipter.zuzino.mipt.ru","subject":"Re: git pull aborts in 50% of cases","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-12-02T21:02:06Z","receivedAt":"2005-12-02T21:02:06Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Alexey Dobriyan wrote:\n> \n> Heisenbug :-\\. I'll send IP next time.\n> \n\nActually, it turns out the two servers were running different versions; \none 0.99.9j and one 0.99.9k. They're both running 0.99.9j now.\n\n0.99.9k is clearly bad.\n\n\t-hpa\n"},{"id":"13114","messageId":"20051202211250.GA11384@mipter.zuzino.mipt.ru","threadId":"2731","inReplyTo":"43909963.60901@zytor.com","subject":"Re: git pull aborts in 50% of cases","fromName":"Alexey Dobriyan","fromEmail":"adobriyan@gmail.com","sentAt":"2005-12-02T21:12:50Z","receivedAt":"2005-12-02T21:12:50Z","isPatch":false,"sender":{"key":"adobriyan@gmail.com","avatar":null},"body":"On Fri, Dec 02, 2005 at 10:58:43AM -0800, H. Peter Anvin wrote:\n> Alexey Dobriyan wrote:\n> >$ git pull\n> >Already up-to-date.\n> >$ git pull\n> >Already up-to-date.\n> >$ git pull\n> >Already up-to-date.\n> >$ git pull\n> >fatal: unexpected EOF\n> >Fetch failure: git://git.kernel.org/pub/scm/git/git.git\n> >$ git pull\n> >fatal: unexpected EOF\n> >Fetch failure: git://git.kernel.org/pub/scm/git/git.git\n> >$ git pull\n> >Already up-to-date.\n> >$ git pull\n> >fatal: unexpected EOF\n> >Fetch failure: git://git.kernel.org/pub/scm/git/git.git\n> >$ git pull\n> >fatal: unexpected EOF\n> >Fetch failure: git://git.kernel.org/pub/scm/git/git.git\n> >\n> >Ditto for \"git fetch --tags\".\n> >\n> >Can somebody explain what's going on?\n> >\n>\n> Do you know which IP address is aborting? There are two servers behind\n> git.kernel.org.\n\nHeisenbug :-\\. I'll send IP next time.\n\n~/linux/linux-linus $ while true; do git pull; done\nAlready up-to-date.\nAlready up-to-date.\nAlready up-to-date.\nAlready up-to-date.\nAlready up-to-date.\nAlready up-to-date.\nAlready up-to-date.\nAlready up-to-date.\nAlready up-to-date.\nAlready up-to-date.\nAlready up-to-date.\n"},{"id":"13117","messageId":"loom.20051202T223152-226@post.gmane.org","threadId":"2731","inReplyTo":"4390B64E.20601@zytor.com","subject":"Re: git pull aborts in 50% of cases","fromName":"Junio C Hamano","fromEmail":"junkio@twinsun.com","sentAt":"2005-12-02T21:41:20Z","receivedAt":"2005-12-02T21:41:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"H. Peter Anvin zytor.com> writes:\n\n> Actually, it turns out the two servers were running different versions; \n> one 0.99.9j and one 0.99.9k. They're both running 0.99.9j now.\n> \n> 0.99.9k is clearly bad.\n\nThis is troublesome, since I do not think it is hitting the one that was\ncausing trouble for the snapshotting procedure, and I cannot seem to reproduce\nit with random set of flags and parameters myself.\n\nI know you run it with --inetd, but what other parameters do you run it with,\nand in what kind of environment? Specifically:\n\n - do you use whitelist, like \"git-daemon --inetd --export-all /pub/scm\"?\n - if so, are any symlinks involved in /pub area?\n - if so, does adding real path like /mnt/real/pub/scm for /pub/scm help?\n - do you run with --strict-paths?\n"},{"id":"13128","messageId":"Pine.LNX.4.63.0512030316520.19086@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"2731","inReplyTo":"4390B64E.20601@zytor.com","subject":"Re: git pull aborts in 50% of cases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2005-12-03T02:18:01Z","receivedAt":"2005-12-03T02:18:01Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 2 Dec 2005, H. Peter Anvin wrote:\n\n> Alexey Dobriyan wrote:\n> > \n> > Heisenbug :-\\. I'll send IP next time.\n> > \n> \n> Actually, it turns out the two servers were running different versions; one\n> 0.99.9j and one 0.99.9k. They're both running 0.99.9j now.\n> \n> 0.99.9k is clearly bad.\n\nHuh? It could be slower, and it could therefore hit the maximum client \ncount faster, but it should not be bad.\n\nAll changes to pull were done in a manner so as to be backward compatible. \nIn both ways.\n\nHth,\nDscho\n"},{"id":"13130","messageId":"7vu0dq29wg.fsf@assigned-by-dhcp.cox.net","threadId":"2731","inReplyTo":"Pine.LNX.4.63.0512030316520.19086@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: git pull aborts in 50% of cases","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-03T02:26:39Z","receivedAt":"2005-12-03T02:26:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> 0.99.9k is clearly bad.\n>\n> Huh? It could be slower, and it could therefore hit the maximum client \n> count faster, but it should not be bad.\n>\n> All changes to pull were done in a manner so as to be backward compatible. \n> In both ways.\n\nI do not think the fetch-pack common computation changes is\ninvolved in this problem at all.\n\nWhat is suspect is the repository validity check code,\nspecifically (quoting from diff between 0.99.9j and 0.99.9k\ndaemon.c::path_ok() function):\n\n+ /* The validation is done on the paths after enter_repo\n+ * canonicalization, so whitelist should be written in\n+ * terms of real pathnames (i.e. after ~user is expanded\n+ * and symlinks resolved).\n+ */\n\nI suspect (but have not heard back from HPA to confirm) that\nkernel.org runs git-daemon with /pub/scm as the whitelist, but\nthere is a symbolic link (or bind mount?) involved, and the real\npath checked based on getcwd() return value is somewhere else.\n"},{"id":"13134","messageId":"43911D9E.5030803@zytor.com","threadId":"2731","inReplyTo":"7vu0dq29wg.fsf@assigned-by-dhcp.cox.net","subject":"Re: git pull aborts in 50% of cases","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-12-03T04:22:54Z","receivedAt":"2005-12-03T04:22:54Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n> \n>>>0.99.9k is clearly bad.\n>>\n>>Huh? It could be slower, and it could therefore hit the maximum client \n>>count faster, but it should not be bad.\n>>\n>>All changes to pull were done in a manner so as to be backward compatible. \n>>In both ways.\n> \n> \n> I do not think the fetch-pack common computation changes is\n> involved in this problem at all.\n> \n> What is suspect is the repository validity check code,\n> specifically (quoting from diff between 0.99.9j and 0.99.9k\n> daemon.c::path_ok() function):\n> \n> + /* The validation is done on the paths after enter_repo\n> + * canonicalization, so whitelist should be written in\n> + * terms of real pathnames (i.e. after ~user is expanded\n> + * and symlinks resolved).\n> + */\n> \n> I suspect (but have not heard back from HPA to confirm) that\n> kernel.org runs git-daemon with /pub/scm as the whitelist, but\n> there is a symbolic link (or bind mount?) involved, and the real\n> path checked based on getcwd() return value is somewhere else.\n\n/pub is a symbolic link. We shouldn't rely on getcwd() for this kind of \nstuff; it's bad for a whole bunch of reasons.\n\n\t-hpa\n"},{"id":"13137","messageId":"7vpsoezf6y.fsf@assigned-by-dhcp.cox.net","threadId":"2731","inReplyTo":"43911D9E.5030803@zytor.com","subject":"Re: git pull aborts in 50% of cases","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-03T09:45:57Z","receivedAt":"2005-12-03T09:45:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H. Peter Anvin\" writes:\n\n> /pub is a symbolic link. We shouldn't rely on getcwd() for this kind of \n> stuff; it's bad for a whole bunch of reasons.\n\nWell, if I recall correctly it was done this way because\nDWIMmery needs to be done before the validation.\n\nAnyway, here is a rewrite of tonight (I resurrected your \"belts\nand suspenders paranoia patch\" for this). Would appreciate it\nif people can try this out (in the proposed updates branch).\n\nThe rules (in 0.99.9k and \"master\" so far) have been that if you\nhave symlinked public, whitelist should say what the canonical\nnames of them are (and the way canonical names are obtained were\ngetcwd()). The rule of this patch is different: whitelist says\nwhat the remote requestor can ask for. So if your /pub is a\nsymlink to /mnt/mnt1/pub, you do not have to say /mnt/mnt1/pub\nto export it. Instead you whitelist /pub (or /pub/scm). Also\nif your ~bob is /home1/bob and ~alice is /home2/alice, you do\nnot say \"/home1 /home2\" -- instead, you say \"~alice ~bob\".\n\n-- >8 --\nSubject: [PATCH] daemon.c and path.enter_repo(): revamp path validation.\n\nThe whitelist of git-daemon is checked against return value from\nenter_repo(), and enter_repo() used to return the value obtained\nfrom getcwd() to avoid directory aliasing issues as discussed\nearier (mid October 2005).\n\nUnfortunately, it did not go well as we hoped.\n\nFor example, /pub on a kernel.org public machine is a symlink to\nits real mountpoint, and it is understandable that the\nadministrator does not want to adjust the whitelist every time\n/pub needs to point at a different partition for storage\nallcation or whatever reasons. Being able to keep using\n/pub/scm as the whitelist is a desirable property.\n\nSo this version of enter_repo() reports what it used to chdir()\nand validate, but does not use getcwd() to canonicalize the\ndirectory name. When it sees a user relative path ~user/path,\nit internally resolves it to try chdir() there, but it still\nreports ~user/path (possibly after appending .git if allowed to\ndo so, in which case it would report ~user/path.git).\n\nWhat this means is that if a whitelist wants to allow a user\nrelative path, it needs to say \"~\" (for all users) or list user\nhome directories like \"~alice\" \"~bob\". And no, you cannot say\n/home if the advertised way to access user home directories are\n~alice,~bob, etc. The whole point of this is to avoid\nunnecessary aliasing issues.\n\nAnyway, because of this, daemon needs to do a bit more work to\nguard itself. Namely, it needs to make sure that the accessor\ndoes not try to exploit its leading path match rule by inserting\n/../ in the middle or hanging /.. at the end. I resurrected the\nbelts and suspender paranoia code HPA did for this purpose.\n\nThis check cannot be done in the enter_repo() unconditionally,\nbecause there are valid callers of enter_repo() that want to\nhonor /../; authorized users coming over ssh to run send-pack\nand fetch-pack should be allowed to do so.\n\nSigned-off-by: Junio C Hamano \n\n---\n\n daemon.c | 64 ++++++++++++++++++++++++--\n path.c | 153 ++++++++++++++++++++++++++++++++++++++++----------------------\n 2 files changed, 159 insertions(+), 58 deletions(-)\n\n018c8d740d18b7fe7d9d5a24fc1d008e29d70bc4\ndiff --git a/daemon.c b/daemon.c\nindex 91b9656..539f6e8 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -82,9 +82,63 @@ static void loginfo(const char *err, ...\n \tva_end(params);\n }\n \n+static int avoid_alias(char *p)\n+{\n+\tint sl, ndot;\n+\n+\t/* \n+\t * This resurrects the belts and suspenders paranoia check by HPA\n+\t * done in <435560F7.4080006@zytor.com> thread, now enter_repo()\n+\t * does not do getcwd() based path canonicalizations.\n+\t *\n+\t * sl becomes true immediately after seeing '/' and continues to\n+\t * be true as long as dots continue after that without intervening\n+\t * non-dot character.\n+\t */\n+\tif (!p || (*p != '/' && *p != '~'))\n+\t\treturn -1;\n+\tsl = 1; ndot = 0;\n+\tp++;\n+\n+\twhile (1) {\n+\t\tchar ch = *p++;\n+\t\tif (sl) {\n+\t\t\tif (ch == '.')\n+\t\t\t\tndot++;\n+\t\t\telse if (ch == '/') {\n+\t\t\t\tif (ndot < 3)\n+\t\t\t\t\t/* reject //, /./ and /../ */\n+\t\t\t\t\treturn -1;\n+\t\t\t\tndot = 0;\n+\t\t\t}\n+\t\t\telse if (ch == 0) {\n+\t\t\t\tif (0 < ndot && ndot < 3)\n+\t\t\t\t\t/* reject /.$ and /..$ */\n+\t\t\t\t\treturn -1;\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\telse\n+\t\t\t\tsl = ndot = 0;\n+\t\t}\n+\t\telse if (ch == 0)\n+\t\t\treturn 0;\n+\t\telse if (ch == '/') {\n+\t\t\tsl = 1;\n+\t\t\tndot = 0;\n+\t\t}\n+\t}\n+}\n+\n static char *path_ok(char *dir)\n {\n-\tchar *path = enter_repo(dir, strict_paths);\n+\tchar *path;\n+\n+\tif (avoid_alias(dir)) {\n+\t\tlogerror(\"'%s': aliased\", dir);\n+\t\treturn NULL;\n+\t}\n+\n+\tpath = enter_repo(dir, strict_paths);\n \n \tif (!path) {\n \t\tlogerror(\"'%s': unable to chdir or not a git archive\", dir);\n@@ -96,9 +150,11 @@ static char *path_ok(char *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 * appends optional {.git,.git/.git} and friends, but \n+\t\t * it does not use getcwd(). So if your /pub is\n+\t\t * a symlink to /mnt/pub, you can whitelist /pub and\n+\t\t * do not have to say /mnt/pub.\n+\t\t * Do not say /pub/.\n \t\t */\n \t\tfor ( pp = ok_paths ; *pp ; pp++ ) {\n \t\t\tint len = strlen(*pp);\ndiff --git a/path.c b/path.c\nindex 2c077c0..334b2bd 100644\n--- a/path.c\n+++ b/path.c\n@@ -131,76 +131,121 @@ int validate_symref(const char *path)\n \treturn -1;\n }\n \n-static char *current_dir(void)\n+static char *user_path(char *buf, char *path, int sz)\n {\n-\treturn getcwd(pathname, sizeof(pathname));\n-}\n-\n-static int user_chdir(char *path)\n-{\n-\tchar *dir = path;\n+\tstruct passwd *pw;\n+\tchar *slash;\n+\tint len, baselen;\n \n-\tif(*dir == '~') {\t\t/* user-relative path */\n-\t\tstruct passwd *pw;\n-\t\tchar *slash = strchr(dir, '/');\n-\n-\t\tdir++;\n-\t\t/* '~/' and '~' (no slash) means users own home-dir */\n-\t\tif(!*dir || *dir == '/')\n-\t\t\tpw = getpwuid(getuid());\n-\t\telse {\n-\t\t\tif (slash) {\n-\t\t\t\t*slash = '\\0';\n-\t\t\t\tpw = getpwnam(dir);\n-\t\t\t\t*slash = '/';\n-\t\t\t}\n-\t\t\telse\n-\t\t\t\tpw = getpwnam(dir);\n+\tif (!path || path[0] != '~')\n+\t\treturn NULL;\n+\tpath++;\n+\tslash = strchr(path, '/');\n+\tif (path[0] == '/' || !path[0]) {\n+\t\tpw = getpwuid(getuid());\n+\t}\n+\telse {\n+\t\tif (slash) {\n+\t\t\t*slash = 0;\n+\t\t\tpw = getpwnam(path);\n+\t\t\t*slash = '/';\n \t\t}\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 -1;\n-\n-\t\tif(!slash || !slash[1]) /* no path following username */\n-\t\t\treturn 0;\n-\n-\t\tdir = slash + 1;\n+\t\telse\n+\t\t\tpw = getpwnam(path);\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 -1;\n-\n-\treturn 0;\n+\tif (!pw || !pw->pw_dir || sz <= strlen(pw->pw_dir))\n+\t\treturn NULL;\n+\tbaselen = strlen(pw->pw_dir);\n+\tmemcpy(buf, pw->pw_dir, baselen);\n+\twhile ((1 < baselen) && (buf[baselen-1] == '/')) {\n+\t\tbuf[baselen-1] = 0;\n+\t\tbaselen--;\n+\t}\n+\tif (slash && slash[1]) {\n+\t\tlen = strlen(slash);\n+\t\tif (sz <= baselen + len)\n+\t\t\treturn NULL;\n+\t\tmemcpy(buf + baselen, slash, len + 1);\n+\t}\n+\treturn buf;\n }\n \n+/*\n+ * First, one directory to try is determined by the following algorithm.\n+ *\n+ * (0) If \"strict\" is given, the path is used as given and no DWIM is\n+ * done. Otherwise:\n+ * (1) \"~/path\" to mean path under the running user's home directory;\n+ * (2) \"~user/path\" to mean path under named user's home directory;\n+ * (3) \"relative/path\" to mean cwd relative directory; or\n+ * (4) \"/absolute/path\" to mean absolute directory.\n+ *\n+ * Unless \"strict\" is given, we try access() for existence of \"%s.git/.git\",\n+ * \"%s/.git\", \"%s.git\", \"%s\" in this order. The first one that exists is\n+ * what we try.\n+ *\n+ * Second, we try chdir() to that. Upon failure, we return NULL.\n+ *\n+ * Then, we try if the current directory is a valid git repository.\n+ * Upon failure, we return NULL.\n+ *\n+ * If all goes well, we return the directory we used to chdir() (but\n+ * before ~user is expanded), avoiding getcwd() resolving symbolic\n+ * links. User relative paths are also returned as they are given,\n+ * except DWIM suffixing.\n+ */\n char *enter_repo(char *path, int strict)\n {\n-\tif(!path)\n+\tstatic char used_path[PATH_MAX];\n+\tstatic char validated_path[PATH_MAX];\n+\n+\tif (!path)\n \t\treturn NULL;\n \n-\tif (strict) {\n-\t\tif (chdir(path) < 0)\n+\tif (!strict) {\n+\t\tstatic const char *suffix[] = {\n+\t\t\t\".git/.git\", \"/.git\", \".git\", \"\", NULL,\n+\t\t};\n+\t\tint len = strlen(path);\n+\t\tint i;\n+\t\twhile ((1 < len) && (path[len-1] == '/')) {\n+\t\t\tpath[len-1] = 0;\n+\t\t\tlen--;\n+\t\t}\n+\t\tif (PATH_MAX <= len)\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\tif (path[0] == '~') {\n+\t\t\tif (!user_path(used_path, path, PATH_MAX))\n+\t\t\t\treturn NULL;\n+\t\t\tstrcpy(validated_path, path);\n+\t\t\tpath = used_path;\n+\t\t}\n+\t\telse if (PATH_MAX - 10 < len)\n \t\t\treturn NULL;\n-\t\t(void)chdir(\".git\");\n+\t\telse {\n+\t\t\tpath = strcpy(used_path, path);\n+\t\t\tstrcpy(validated_path, path);\n+\t\t}\n+\t\tlen = strlen(path);\n+\t\tfor (i = 0; suffix[i]; i++) {\n+\t\t\tstrcpy(path + len, suffix[i]);\n+\t\t\tif (!access(path, F_OK)) {\n+\t\t\t\tstrcat(validated_path, suffix[i]);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (!suffix[i] || chdir(path))\n+\t\t\treturn NULL;\n+\t\tpath = validated_path;\n \t}\n+\telse if (chdir(path))\n+\t\treturn NULL;\n \n-\tif(access(\"objects\", X_OK) == 0 && access(\"refs\", X_OK) == 0 &&\n-\t validate_symref(\"HEAD\") == 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\tcheck_repository_format();\n-\t\treturn current_dir();\n+\t\treturn path;\n \t}\n \n \treturn NULL;\n-- \n0.99.9.GIT\n"},{"id":"13150","messageId":"4391F04E.1050002@zytor.com","threadId":"2731","inReplyTo":"7vpsoezf6y.fsf@assigned-by-dhcp.cox.net","subject":"Re: git pull aborts in 50% of cases","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-12-03T19:21:50Z","receivedAt":"2005-12-03T19:21:50Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Junio C Hamano wrote:\n> \"H. Peter Anvin\" writes:\n> \n> \n>>/pub is a symbolic link. We shouldn't rely on getcwd() for this kind of \n>>stuff; it's bad for a whole bunch of reasons.\n> \n> \n> Well, if I recall correctly it was done this way because\n> DWIMmery needs to be done before the validation.\n> \n> Anyway, here is a rewrite of tonight (I resurrected your \"belts\n> and suspenders paranoia patch\" for this). Would appreciate it\n> if people can try this out (in the proposed updates branch).\n> \n> The rules (in 0.99.9k and \"master\" so far) have been that if you\n> have symlinked public, whitelist should say what the canonical\n> names of them are (and the way canonical names are obtained were\n> getcwd()). The rule of this patch is different: whitelist says\n> what the remote requestor can ask for. So if your /pub is a\n> symlink to /mnt/mnt1/pub, you do not have to say /mnt/mnt1/pub\n> to export it. Instead you whitelist /pub (or /pub/scm). Also\n> if your ~bob is /home1/bob and ~alice is /home2/alice, you do\n> not say \"/home1 /home2\" -- instead, you say \"~alice ~bob\".\n> \n\nYup, this is the way to do it. Forcing people to use canonical names is \nquite a nonstarter.\n\n\t-hpa\n"},{"id":"13152","messageId":"7vzmnivuz8.fsf_-_@assigned-by-dhcp.cox.net","threadId":"2731","inReplyTo":"7vpsoezf6y.fsf@assigned-by-dhcp.cox.net","subject":"[RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-03T19:30:51Z","receivedAt":"2005-12-03T19:30:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Having slept over the patch I am responding to, I tend to think\nthis is more trouble than its worth. Validating the\ndirectory enter_repo() chdir()'ed into and validated to be a\ngood git repository should be done on its canonical name as\ngetcwd() returns, not with a userland aliasing avoidance.\n\nAs an administrator, being able to say /pub/scm on the\nwhitelist, knowing /pub to be a symbolic link points at\nsomewhere today but maybe at different place tomorrow, and not\nhaving to adjust the whitelist whenever that happens, is indeed\nnice. We do not allow the remote requestor to say /../ in the\npath, so we trap him within the directories the whitelist\ndescribes.\n\nNot.\n\nFor example, I can by mistake create a symbolic link:\n\n\tln -s /home /pub/scm/git/git.git/oops\n\nnow accesses /pub/scm/git/oops/hpa/secret.git/ is not\nrestricted. We could hand-resolve the each level from the\nrequest to see if no \"funny\" symbolic links are involved, but\nwhat is the definition of \"funny\"? When we see /pub pointing at\nsomewhere in /mnt/disk47/slice31, we should not complain. When\nwe see \"oops\" under git in the above example, we would want to\ncomplain. These things are hard to get right.\n\nI tend to say that the 0.99.9k (and the current master) rule to\nmake validation always work on what getcwd() gives back is\neasier to understand (which generally means safer). Can I talk\nyou into adjusting your whitelist on kernel.org machines?\n"},{"id":"13153","messageId":"4391F4DD.2060002@zytor.com","threadId":"2731","inReplyTo":"7vzmnivuz8.fsf_-_@assigned-by-dhcp.cox.net","subject":"Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-12-03T19:41:17Z","receivedAt":"2005-12-03T19:41:17Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Junio C Hamano wrote:\n> \n> For example, I can by mistake create a symbolic link:\n> \n> \tln -s /home /pub/scm/git/git.git/oops\n> \n> now accesses /pub/scm/git/oops/hpa/secret.git/ is not\n> restricted. We could hand-resolve the each level from the\n> request to see if no \"funny\" symbolic links are involved, but\n> what is the definition of \"funny\"? When we see /pub pointing at\n> somewhere in /mnt/disk47/slice31, we should not complain. When\n> we see \"oops\" under git in the above example, we would want to\n> complain. These things are hard to get right.\n> \n\nActually, it's a policy decision whether or not symlinks should be \nallowed to exit space like that; in Apache, for example, it's a \nconfigurable.\n\n> I tend to say that the 0.99.9k (and the current master) rule to\n> make validation always work on what getcwd() gives back is\n> easier to understand (which generally means safer). Can I talk\n> you into adjusting your whitelist on kernel.org machines?\n\nI'm not happy about it, but it's not a huge deal on kernel.org. \nHowever, I think it's the wrong thing, especially in the light of \nallowing user-relative paths.\n\nAt the very least, if you insist on using getcwd() names, you should \npre-canonicalize the whitelist, too.\n\n\t-hpa\n"},{"id":"13154","messageId":"Pine.LNX.4.64.0512031156070.3099@g5.osdl.org","threadId":"2731","inReplyTo":"4391F4DD.2060002@zytor.com","subject":"Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-12-03T19:56:37Z","receivedAt":"2005-12-03T19:56:37Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 3 Dec 2005, H. Peter Anvin wrote:\n> \n> At the very least, if you insist on using getcwd() names, you should\n> pre-canonicalize the whitelist, too.\n\nThat would probably solve the problem and sounds like the right \nuser-friendly solution.\n\n\t\tLinus\n"},{"id":"13155","messageId":"7vvey6vsop.fsf@assigned-by-dhcp.cox.net","threadId":"2731","inReplyTo":"4391F4DD.2060002@zytor.com","subject":"Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-03T20:20:22Z","receivedAt":"2005-12-03T20:20:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H. Peter Anvin\" writes:\n\n> At the very least, if you insist on using getcwd() names, you should \n> pre-canonicalize the whitelist, too.\n\nWith the current \"prefix\" rule (and not allowing /ho to match\n/home) that sounds possible and sensivle, but that is not nice\nin the long run. We may later want to say \"/pub/git/**/*.git\"\nfor example to mean \"any subdirectory under /pub/git but the\nbase directory name must be something ending with '.git'\".\n\nHmm...\n"},{"id":"13156","messageId":"439203F6.1040505@zytor.com","threadId":"2731","inReplyTo":"7vvey6vsop.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-12-03T20:45:42Z","receivedAt":"2005-12-03T20:45:42Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Junio C Hamano wrote:\n> \"H. Peter Anvin\" writes:\n> \n>>At the very least, if you insist on using getcwd() names, you should \n>>pre-canonicalize the whitelist, too.\n> \n> With the current \"prefix\" rule (and not allowing /ho to match\n> /home) that sounds possible and sensivle, but that is not nice\n> in the long run. We may later want to say \"/pub/git/**/*.git\"\n> for example to mean \"any subdirectory under /pub/git but the\n> base directory name must be something ending with '.git'\".\n> \n> Hmm...\n> \n\nYep, this stuff is hard. For example, on kernel.org I'm not concerned \nabout symbolic links; the likelihood of an accidental symbolic link that \nwould violate security is very small. Other applications might be \ndifferent.\n\nArguably, the correct interface is to modularize it, and have both the \nuser request, the post-DWIM output, and the\n\n\t-hpa\n"},{"id":"13157","messageId":"7vslt9vpxo.fsf@assigned-by-dhcp.cox.net","threadId":"2731","inReplyTo":"Pine.LNX.4.64.0512031156070.3099@g5.osdl.org","subject":"Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-03T21:19:47Z","receivedAt":"2005-12-03T21:19:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Sat, 3 Dec 2005, H. Peter Anvin wrote:\n>> \n>> At the very least, if you insist on using getcwd() names, you should\n>> pre-canonicalize the whitelist, too.\n>\n> That would probably solve the problem and sounds like the right \n> user-friendly solution.\n\nI agree with you that limiting with names exposed to the\nend-user is the right approach and we should avoid having\nadministrators to use getcwd() names. So in that sense, I think\nthe patch I sent earlier is going in the right direction.\n\nTo cope with the \"oops\" symlink problem, we could introduce\n\"--strict-symlink\" flag to daemon that does these things:\n\n - enter_repo() returns three things: \"~alice/foo.git/.git\",\n \"/home/alice/foo.git/.git\", and \"/home/alice\"; the first is\n what remote requested with DWIM, the second is where it\n chdir()ed, and the third is what it expanded the\n user-relative base to. None of them uses getcwd(). When\n user-relative path is not involved, the first two are the\n same, and the last one is undefined (and not used).\n\n - daemon checks the whitelist with the first one, and if the\n path is not allowed, the processing stops there with failure.\n Without --strict-symlink, this is the only test done with\n whitelist. This means the whitelist should have /pub/scm and\n ~alice, not /mnt/disk47/slice31/scm nor /home2/alice.\n\n With --strict-symlink, it uses the latter two with the\n whitelist entry it matched, to determine where to start\n further \"strict symlink check\". The part that matched the\n whitelist is OK and the rest is checked [*1*]:\n\n - If \"~alice\" was whitelisted, it knows ~alice expanded to\n /home/alice, and starts checking from /home/alice/foo.git\n and then checks /home/alice/foo.git/.git.\n\n - If \"~alice/foo.git\" was whitelisted, it knows it expands to\n /home/alice/foo.git, and checks /home/alice/foo.git/.git.\n\n - If a whitelist entry \"/pub/scm\" was matched against a\n request \"/pub/scm/git/git.git\", it checks /pub/scm/git and\n /pub/scm/git.git\n\n The strict symlink check tries to readlink() each of what are\n to be checked by the above logic, and rejects if it was found\n to be a symlink that starts with a \"/\" (i.e. absolute\n pathname) or anything that has undesiable aliasing effect;\n \"belts and suspender paranoia\" in daemon.c::avoid_alias(),\n which is stricter than needed but is safe is a good starting\n point, but we may want to allow things that do not step\n outside the prefix we matched.\n\nHowever, I am not ready to do all of the above, not just yet.\n\nSince I wanted to do another maintenance update this weekend,\nI'd throw in the last-night's patch in the master so that at\nleast 0.99.9l works with /pub whitelist, knowing the \"oops\"\nsymlink issue is still to be solved. That way we can keep the\nusers' configuration the same when later we introduce all of the\nabove.\n\nThoughts?\n\n[Footnote]\n\n*1* If we later introduce \"/pub/scm/**/*.git\", we allow symlinks\nin the first directories without glob patterns, i.e. \"/pub/scm\",\nso this is somewhat future-proof.\n"},{"id":"13158","messageId":"7vmzjhvpjs.fsf@assigned-by-dhcp.cox.net","threadId":"2731","inReplyTo":"7vslt9vpxo.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-03T21:28:07Z","receivedAt":"2005-12-03T21:28:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\nTrivial typo.\n\n> - If a whitelist entry \"/pub/scm\" was matched against a\n> request \"/pub/scm/git/git.git\", it checks /pub/scm/git and\n> /pub/scm/git.git\n\nObviously \"/pub/scm/git\" and \"/pub/scm/git/git.git\" are the ones\nthat are checked here. Whitelist \"/pub/scm\" means \"/pub/scm\nand under, and the administrator knows /pub or /pub/scm may be\nsymlinks pointing at places he wants them to, so do not check\nand complain where they point at\".\n"}]}