{"thread":{"id":"2139","subject":"[PATCH] git-daemon extra paranoia","startedAt":"2005-10-18T20:54:15Z","lastAt":"2005-10-19T01:18:42Z","messageCount":12,"participants":["H. Peter Anvin","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"10225","messageId":"435560F7.4080006@zytor.com","threadId":"2139","inReplyTo":null,"subject":"[PATCH] git-daemon extra paranoia","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-18T20:54:15Z","receivedAt":"2005-10-18T20:54:15Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"This patch adds some extra paranoia to the git-daemon filename test. In \nparticular, it now rejects pathnames containing // or ending with /; it \nalso adds a redundant test for pathname absoluteness (belts and suspenders.)\n\nSigned-off-by: H. Peter Anvin \n\n\nExtra paranoia about non-canonical pathnames\n\n---\ncommit a22f643931e48a319a70af7e91f809648160ecbf\ntree 9d6934089c2628253d0690efde3fa7f36a1a8861\nparent 4aaa702794447d9b281dd22fe532fd61e02434e1\nauthor Peter Anvin Tue, 18 Oct 2005 13:51:45 -0700\ncommitter Peter Anvin Tue, 18 Oct 2005 13:51:45 -0700\n\n daemon.c | 16 ++++++++++++----\n 1 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\n--- a/daemon.c\n+++ b/daemon.c\n@@ -80,17 +80,25 @@ static int path_ok(const char *dir)\n {\n \tconst char *p = dir;\n \tchar **pp;\n-\tint sl = 1, ndot = 0;\n+\tint sl, ndot;\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 == '/' || *p == '\\0' ) {\n-\t\t\tif ( sl && ndot > 0 && ndot < 3 )\n-\t\t\t\treturn 0; /* . or .. in path */\n+\t\t\tif ( sl && ndot < 3 )\t/* Refuse \"\", \".\" or \"..\" */\n+\t\t\t\treturn 0;\n \t\t\tsl = 1;\n+\n+\t\t\t/* If this was end of string, we passed all tests */\n \t\t\tif ( *p == '\\0' )\n-\t\t\t\tbreak; /* End of string and all is good */\n+\t\t\t\tbreak;\n \t\t} else {\n \t\t\tsl = ndot = 0;\n \t\t}\n"},{"id":"10226","messageId":"7vll0qploy.fsf@assigned-by-dhcp.cox.net","threadId":"2139","inReplyTo":"435560F7.4080006@zytor.com","subject":"Re: [PATCH] git-daemon extra paranoia","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-10-18T21:19:41Z","receivedAt":"2005-10-18T21:19:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H. Peter Anvin\" writes:\n\n> This patch adds some extra paranoia to the git-daemon filename test. In \n> particular, it now rejects pathnames containing // or ending with /; it \n> also adds a redundant test for pathname absoluteness (belts and suspenders.)\n>\n> Signed-off-by: H. Peter Anvin \n> Extra paranoia about non-canonical pathnames\n\nI would understand rejecting /../, and perhaps /./, but why\nreject // in between or / at the end?\n\nEspecially, I think this part in daemon.c::upload():\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\n\tchdir(\".git\");\n\nrelies on the fact that you can say \"/home/junio/git/\" for me to\npublish \"/home/junio/git/.git/\" repository, so I would suspect\nthat it is necessary to allow \"ending with /\" at least.\n"},{"id":"10227","messageId":"4355691D.2010200@zytor.com","threadId":"2139","inReplyTo":"7vll0qploy.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-daemon extra paranoia","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-18T21:29:01Z","receivedAt":"2005-10-18T21:29:01Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Junio C Hamano wrote:\n> \n> I would understand rejecting /../, and perhaps /./, but why\n> reject // in between or / at the end?\n> \n\nFor security, avoiding aliases is highly desirable, and if they're \nuseless the easiest way to do that is to reject. If aliases are \nrequired, which it sounds like it might be, then canonicalization needs \nto be applied.\n\nThis may sound redundant, but a lot of avoiding security holes involves \napplying good practices up front, instead of reactively.\n\nAllowing a terminal slash should be reasonably easy, though.\n\n\t-hpa\n"},{"id":"10228","messageId":"43557254.3010807@zytor.com","threadId":"2139","inReplyTo":"4355691D.2010200@zytor.com","subject":"Re: [PATCH] git-daemon extra paranoia","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-18T22:08:20Z","receivedAt":"2005-10-18T22:08:20Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"H. Peter Anvin wrote:\n> \n> For security, avoiding aliases is highly desirable, and if they're \n> useless the easiest way to do that is to reject. If aliases are \n> required, which it sounds like it might be, then canonicalization needs \n> to be applied.\n> \n> This may sound redundant, but a lot of avoiding security holes involves \n> applying good practices up front, instead of reactively.\n> \n\nI thought I might want to add a bit of an explanation, just for the \npurpose of illustration.\n\nRight now, we use a whitelist for access control. Aliases are not a \nproblem, because they fail shut.\n\nA year from now, someone decides that they want a \"all but\" feature, and \nthus adds a blacklist on top of the whitelist. If aliases are \npermitted, unless the blacklist logic is written very carefully, one \nwould then be able to get around the blacklist by using one of the \naliased paths.\n\nImproper handling of aliases is probably second only to buffer overflows \nand large-string DoS attacks when it comes to security vulnerabilities.\n\n\t-hpa\n"},{"id":"10229","messageId":"43557388.2060508@zytor.com","threadId":"2139","inReplyTo":"43557254.3010807@zytor.com","subject":"[PATCH] Revised - git-daemon extra paranoia","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-18T22:13:28Z","receivedAt":"2005-10-18T22:13:28Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"This patch adds some extra paranoia to the git-daemon filename test. In \nparticular, it now rejects pathnames containing //; it also adds a \nredundant test for pathname absoluteness (belts and suspenders.)\n\nA single / at the end of the path is still permitted, however.\n\nSigned-off-by: H. Peter Anvin \n\n\ndiff --git a/daemon.c b/daemon.c\n--- a/daemon.c\n+++ b/daemon.c\n@@ -80,17 +80,29 @@ static int path_ok(const char *dir)\n {\n \tconst char *p = dir;\n \tchar **pp;\n-\tint sl = 1, ndot = 0;\n+\tint sl, ndot;\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 == '/' || *p == '\\0' ) {\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; /* . or .. in path */\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\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"},{"id":"10230","messageId":"Pine.LNX.4.64.0510181517280.3369@g5.osdl.org","threadId":"2139","inReplyTo":"435560F7.4080006@zytor.com","subject":"Re: [PATCH] git-daemon extra paranoia","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-10-18T22:25:17Z","receivedAt":"2005-10-18T22:25:17Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 18 Oct 2005, H. Peter Anvin wrote:\n>\n> This patch adds some extra paranoia to the git-daemon filename test. In\n> particular, it now rejects pathnames containing // or ending with /; it also\n> adds a redundant test for pathname absoluteness (belts and suspenders.)\n\nHmm. The \"not ending in /\" is a bad test. \n\nEspecially in light of the fact that the git-pack protocol quite by design \ntends to add a \".git\" to the end as a fallback, so that a user that wants \nto specify a particular directory _without_ that fallback needs to have \nthe slash at the end.\n\nNow, git-daemon hasn't implemented that, but I think that was just a \nmistake that grew out of it not getting a lot of testing, since it wasn't \nused much. I personally use the \"without the final .git\" version quite \noften, because it just looks so much nicer for the user.\n\nIn fact, here's a patch that makes git-daemon allow it, and thus match the \nbehaviour of the ssh transport.\n\nThe logic is simple: if the original \"chdir()\" fails, try another one with \n\".git\" appended. This is in _addition_ to doing the 'chdir(\".git\")' later, \nso that if you have a checked-out git repository in /home/linux-2.6.git, \nthen doing a\n\n\tgit pull git://host/home/linux-2.6\n\nwill on the remote end do:\n\n\tchmod(\"/home/linux-2.6\")\t// fails with ENOENT\n\tchmod(\"/home/linux-2.6.git\")\t// works\n\tchmod(\".git\")\t\t\t// works\n\nresulting in it ending up in /home/linux-2.6.git/.git, which is exactly \ncorrect, and where it wants to be.\n\nI personally find it a nice bit of usability enhancement. You can name \nyour git repositories with a \".git\" suffix (which can help all kinds of \nautomated tasks - like autopacking), but you don't force your users to \ncare.\n\n\t\tLinus\n\n---\ndiff --git a/daemon.c b/daemon.c\nindex 11fa3ed..a488512 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -128,8 +128,13 @@ static int upload(char *dir, int dirlen)\n \t}\n \n \tif (chdir(dir) < 0) {\n-\t\tlogerror(\"Cannot chdir('%s'): %s\", dir, strerror(errno));\n-\t\treturn -1;\n+\t\tint err = errno;\n+\t\tstrcpy(dir + dirlen, \".git\");\n+\t\tif (err != ENOENT || chdir(dir) < 0) {\n+\t\t\tdir[dirlen] = 0;\n+\t\t\tlogerror(\"Cannot chdir('%s'): %s\", dir, strerror(err));\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \n \tchdir(\".git\");\n@@ -164,7 +169,12 @@ static int execute(void)\n \tstatic char line[1000];\n \tint len;\n \n-\tlen = packet_read_line(0, line, sizeof(line));\n+\t/*\n+\t * Make sure that we leave room for an extra \".git\" at\n+\t * the end of the line. Note that the packet interfaces\n+\t * already guarantee that there is an ending '\\0'.\n+\t */\n+\tlen = packet_read_line(0, line, sizeof(line)-4);\n \n \tif (len && line[len-1] == '\\n')\n \t\tline[--len] = 0;\n"},{"id":"10231","messageId":"7v64ruo31i.fsf@assigned-by-dhcp.cox.net","threadId":"2139","inReplyTo":"Pine.LNX.4.64.0510181517280.3369@g5.osdl.org","subject":"Re: [PATCH] git-daemon extra paranoia","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-10-18T22:47:53Z","receivedAt":"2005-10-18T22:47:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> \tgit pull git://host/home/linux-2.6\n>\n> will on the remote end do:\n>\n> \tchdir(\"/home/linux-2.6\")\t// fails with ENOENT\n> \tchdir(\"/home/linux-2.6.git\")\t// works\n> \tchdir(\".git\")\t\t\t// works\n>\n> resulting in it ending up in /home/linux-2.6.git/.git, which is exactly \n> correct, and where it wants to be.\n>\n> I personally find it a nice bit of usability enhancement. You can name \n> your git repositories with a \".git\" suffix (which can help all kinds of \n> automated tasks - like autopacking), but you don't force your users to \n> care.\n\nWouldn't having /home/linux-2.6/.git/ repository with\n/home/linux-2.6/ working tree be good enough for that? Instead\nof doing \"find / -type d -name '*.git'\" you could do \"find /\n-type d -name .git\" for automated tasks.\n"},{"id":"10232","messageId":"Pine.LNX.4.64.0510181616500.3369@g5.osdl.org","threadId":"2139","inReplyTo":"7v64ruo31i.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-daemon extra paranoia","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-10-18T23:21:41Z","receivedAt":"2005-10-18T23:21:41Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 18 Oct 2005, Junio C Hamano wrote:\n> \n> Wouldn't having /home/linux-2.6/.git/ repository with\n> /home/linux-2.6/ working tree be good enough for that? Instead\n> of doing \"find / -type d -name '*.git'\" you could do \"find /\n> -type d -name .git\" for automated tasks.\n\nIn this case, yes.\n\nBut In a mixed environment where you might have \"bare\" repositories, you \nwant to have \"reponame.git\" as the repository name.\n\nSo with the rule that (a) try to first append \".git\" and (b) then, after a \nsuccessful chdir, try to go in one more level, you can handle both types, \nwithout ever having to care whether it's checked-out or not.\n\nAnd for secondary projects (where git isn't necessarily the primary source \ncontrol method), I actually use the \"project.git\" naming just to make it \nobvious that this is the \"gitified\" version of the project.\n\nFor example, I keep both my private uemacs and pine source trees as git \nrepositories these days, and I have them under \"~/src/uemacs.git/\" and \n\"~/src/pine.git/\" even though they are checked out and thus actually have \nanother \".git\" inside of them.\n\n\t\tLinus\n"},{"id":"10234","messageId":"435591A3.7030708@zytor.com","threadId":"2139","inReplyTo":"Pine.LNX.4.64.0510181517280.3369@g5.osdl.org","subject":"Re: [PATCH] git-daemon extra paranoia","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-19T00:21:55Z","receivedAt":"2005-10-19T00:21:55Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Linus Torvalds wrote:\n> \n> Hmm. The \"not ending in /\" is a bad test. \n> \n> Especially in light of the fact that the git-pack protocol quite by design \n> tends to add a \".git\" to the end as a fallback, so that a user that wants \n> to specify a particular directory _without_ that fallback needs to have \n> the slash at the end.\n> \n> Now, git-daemon hasn't implemented that, but I think that was just a \n> mistake that grew out of it not getting a lot of testing, since it wasn't \n> used much. I personally use the \"without the final .git\" version quite \n> often, because it just looks so much nicer for the user.\n> \n> In fact, here's a patch that makes git-daemon allow it, and thus match the \n> behaviour of the ssh transport.\n> \n> The logic is simple: if the original \"chdir()\" fails, try another one with \n> \".git\" appended. This is in _addition_ to doing the 'chdir(\".git\")' later, \n> so that if you have a checked-out git repository in /home/linux-2.6.git, \n> then doing a\n> \n\nThis is also exactly the kind of DWIM that tends to result in the kind \nof security holes I described earlier.\n\nThe DWIM aspect is fine, of course, but it has to be done up front: \ninstead of doing just chdir(), each path should be validated through \npath_ok() before even being considered for chdir(). Perhaps the right \nthing to do is to combine the two functions.\n\n\t-hpa\n"},{"id":"10239","messageId":"Pine.LNX.4.64.0510181728490.3369@g5.osdl.org","threadId":"2139","inReplyTo":"435591A3.7030708@zytor.com","subject":"Re: [PATCH] git-daemon extra paranoia","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-10-19T00:41:12Z","receivedAt":"2005-10-19T00:41:12Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 18 Oct 2005, H. Peter Anvin wrote:\n> \n> This is also exactly the kind of DWIM that tends to result in the kind of\n> security holes I described earlier.\n\nI don't agree. \n\nDWIM isn't automatically a security hole. DWIM _can_ be a security hole, \nbut so can anything else that is badly designed or specified.\n\nAnd just appending \".git\" is _not_ badly designed/specified. I did think \nabout the boundary cases, and it's entirely safe:\n\n - it can't result in \"surprises\": if the original pathname doesn't exist, \n then even if there is a race and it got created in between the two \n chdir's as a directory and the name had a slash at the end, adding \n \".git\" is actually safe even if it succeeds: it won't take us anywhere \n surprising. At worst it will take us to the \".git\" directory of a newly \n added git archive, but that's what we wanted anyway, so..\n\n - you can't create \"..\" with it - even if the passed-in filename ended \n with \"xyz/.\", you'll end up with a perfectly safe \"xyz/..git\", so any \n safety checks that were done on the original pathname are still valid \n when appending \".git\" to it.\n\n - and exactly because we don't append slashes or anything like that, the \n end result won't even have anything ambiguous like \"//\" in it.\n\nSo it really doesn't have any downsides that I can see.\n\n> The DWIM aspect is fine, of course, but it has to be done up front: instead of\n> doing just chdir(), each path should be validated through path_ok() before\n> even being considered for chdir(). Perhaps the right thing to do is to\n> combine the two functions.\n\nSure, you could do that, and just replace path_ok + chdir with a \n\"safe_chdir()\". I don't really see the point, unless you want to walk the \npath one component at a time, though (which is really quite expensive).\n\nIf you want to verify that it's still on the same filesystem and didn't \ntraverse any dubious symlinks (the only reason to do the component walking \nafaik), it's actually much cheaper to just do the chdir() and then do a \n\"getcwd()\" to verify that the result matches. At least under Linux.\n\n(That, btw, is likely the right way to do \"valid directory checking\" \nanyway: if you have a white-list of acceptable directories, just do a \nchdir() blindly without any checking, then do \"getcwd()\" and check the \nresult of that against the whitelist - then you can even allow \"..\" etc, \nand never even care)\n\n\t\t\tLinus\n"},{"id":"10240","messageId":"435596CB.6070401@zytor.com","threadId":"2139","inReplyTo":"Pine.LNX.4.64.0510181728490.3369@g5.osdl.org","subject":"Re: [PATCH] git-daemon extra paranoia","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-19T00:43:55Z","receivedAt":"2005-10-19T00:43:55Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Linus Torvalds wrote:\n> And just appending \".git\" is _not_ badly designed/specified. I did think \n> about the boundary cases, and it's entirely safe:\n> \n> - it can't result in \"surprises\": if the original pathname doesn't exist, \n> then even if there is a race and it got created in between the two \n> chdir's as a directory and the name had a slash at the end, adding \n> \".git\" is actually safe even if it succeeds: it won't take us anywhere \n> surprising. At worst it will take us to the \".git\" directory of a newly \n> added git archive, but that's what we wanted anyway, so..\n> \n> - you can't create \"..\" with it - even if the passed-in filename ended \n> with \"xyz/.\", you'll end up with a perfectly safe \"xyz/..git\", so any \n> safety checks that were done on the original pathname are still valid \n> when appending \".git\" to it.\n> \n> - and exactly because we don't append slashes or anything like that, the \n> end result won't even have anything ambiguous like \"//\" in it.\n> \n> So it really doesn't have any downsides that I can see.\n> \n\nConsider the whitelist/blacklist scenario I described in the previous \nemail. You have:\n\nwhitelist:\t/pub/scm\nblacklist:\t/pub/scm/foo/bar.git\n\nIf you can bypass the blacklist by using the pathname /pub/scm/foo/bar, \nthat's bad.\n\n> \n>>The DWIM aspect is fine, of course, but it has to be done up front: instead of\n>>doing just chdir(), each path should be validated through path_ok() before\n>>even being considered for chdir(). Perhaps the right thing to do is to\n>>combine the two functions.\n> \n> Sure, you could do that, and just replace path_ok + chdir with a \n> \"safe_chdir()\". I don't really see the point, unless you want to walk the \n> path one component at a time, though (which is really quite expensive).\n> \n\nThe only reason to do that is to make it less likely that a future \nprogrammer would screw it up.\n\n\t-hpa\n"},{"id":"10247","messageId":"7v3bmyl2x9.fsf@assigned-by-dhcp.cox.net","threadId":"2139","inReplyTo":"435596CB.6070401@zytor.com","subject":"Re: [PATCH] git-daemon extra paranoia","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-10-19T01:18:42Z","receivedAt":"2005-10-19T01:18:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H. Peter Anvin\" writes:\n\n> Consider the whitelist/blacklist scenario I described in the previous \n> email. You have:\n>\n> whitelist:\t/pub/scm\n> blacklist:\t/pub/scm/foo/bar.git\n>\n> If you can bypass the blacklist by using the pathname /pub/scm/foo/bar, \n> that's bad.\n\nI like the simplicity of the check Linus suggested. Given\n/pub/scm/fora/../foo/bar/, you would end up chdir() to\n/pub/scm/foo/bar.git and getcwd() would hit the blacklist\nentry. Which almost means that you do not even need path_ok()\n;-).\n"}]}