{"thread":{"id":"37008","subject":"[PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs","startedAt":"2014-06-27T11:01:17Z","lastAt":"2014-06-28T05:34:09Z","messageCount":4,"participants":["Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"245062","messageId":"1403866877-15733-1-git-send-email-mhagger@alum.mit.edu","threadId":"37008","inReplyTo":null,"subject":"[PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-06-27T11:01:17Z","receivedAt":"2014-06-27T11:01:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When reading a symbolic ref via resolve_gitlink_ref_recursive(), check\nthat the reference name that is pointed at is formatted correctly,\nusing the same check as resolve_ref_unsafe() uses for non-gitlink\nreferences. This prevents bogosity like\n\n ref: ../../other/file\n\nfrom causing problems.\n\nSigned-off-by: Michael Haggerty \n---\nGiven that symbolic references cannot be transferred via the Git\nprotocol, I do not expect this bug to be exploitable.\n\n refs.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex dc45774..7da8e7d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1273,6 +1273,9 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs,\n \twhile (isspace(*p))\n \t\tp++;\n \n+\tif (check_refname_format(p, REFNAME_ALLOW_ONELEVEL))\n+\t\treturn -1;\n+\n \treturn resolve_gitlink_ref_recursive(refs, p, sha1, recursion+1);\n }\n \n-- \n2.0.0\n"},{"id":"245083","messageId":"xmqqa98y2q8k.fsf@gitster.dls.corp.google.com","threadId":"37008","inReplyTo":"1403866877-15733-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-27T17:53:47Z","receivedAt":"2014-06-27T17:53:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> When reading a symbolic ref via resolve_gitlink_ref_recursive(), check\n> that the reference name that is pointed at is formatted correctly,\n> using the same check as resolve_ref_unsafe() uses for non-gitlink\n> references. This prevents bogosity like\n>\n> ref: ../../other/file\n>\n> from causing problems.\n\nI do agree that a textual symref \"ref: ../../x/y\" that is stored in\n\".git/HEAD\" or in \".git/refs/L\" will step outside \".git/\" and it is\nproblematic. But if \".git/refs/heads/a/b/LINK\" has \"ref: ../../x\"\nin it, shouldn't we interpret it as referring to the ref at\n\"refs/heads/x\"?\n\n> Given that symbolic references cannot be transferred via the Git\n> protocol, I do not expect this bug to be exploitable.\n>\n> refs.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/refs.c b/refs.c\n> index dc45774..7da8e7d 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1273,6 +1273,9 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs,\n> \twhile (isspace(*p))\n> \t\tp++;\n> \n> +\tif (check_refname_format(p, REFNAME_ALLOW_ONELEVEL))\n> +\t\treturn -1;\n> +\n> \treturn resolve_gitlink_ref_recursive(refs, p, sha1, recursion+1);\n> }\n"},{"id":"245084","messageId":"xmqq61jm2pz7.fsf@gitster.dls.corp.google.com","threadId":"37008","inReplyTo":"xmqqa98y2q8k.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-27T17:59:24Z","receivedAt":"2014-06-27T17:59:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Michael Haggerty writes:\n>\n>> When reading a symbolic ref via resolve_gitlink_ref_recursive(), check\n>> that the reference name that is pointed at is formatted correctly,\n>> using the same check as resolve_ref_unsafe() uses for non-gitlink\n>> references. This prevents bogosity like\n>>\n>> ref: ../../other/file\n>>\n>> from causing problems.\n>\n> I do agree that a textual symref \"ref: ../../x/y\" that is stored in\n> \".git/HEAD\" or in \".git/refs/L\" will step outside \".git/\" and it is\n> problematic. But if \".git/refs/heads/a/b/LINK\" has \"ref: ../../x\"\n> in it, shouldn't we interpret it as referring to the ref at\n> \"refs/heads/x\"?\n\nActually, the textual symrefs have been invented to replace symbolic\nlinks used for .git/HEAD on symlink-incapable filesystems, and we do\neven not let the filesystem follow symlinks. The rule we have there\nare like so:\n\n\t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n\t\tif (S_ISLNK(st.st_mode)) {\n\t\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n\t\t\tif (len < 0) {\n\t\t\t\tif (errno == ENOENT || errno == EINVAL)\n\t\t\t\t\t/* inconsistent with lstat; retry */\n\t\t\t\t\tgoto stat_ref;\n\t\t\t\telse\n\t\t\t\t\treturn NULL;\n\t\t\t}\n\t\t\tbuffer[len] = 0;\n\t\t\tif (starts_with(buffer, \"refs/\") &&\n\t\t\t\t\t!check_refname_format(buffer, 0)) {\n\t\t\t\tstrcpy(refname_buffer, buffer);\n\t\t\t\trefname = refname_buffer;\n\t\t\t\tif (flag)\n\t\t\t\t\t*flag |= REF_ISSYMREF;\n\t\t\t\tcontinue;\n\t\t\t}\n\t\t}\n\nSo we should do exactly the same check, I would think, no?\n\nIn a typical clone, the \".git/refs/remotes/origin/HEAD\" textual\nsymref stores \"ref: refs/remotes/origin/master\" and it is neither\n\"ref: master\" nor \"ref: ./master\", so it should be sensible to\ninsist on \"must start with 'refs/' and its format valid.\"\n"},{"id":"245115","messageId":"53AE53D1.9020905@alum.mit.edu","threadId":"37008","inReplyTo":"xmqq61jm2pz7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-06-28T05:34:09Z","receivedAt":"2014-06-28T05:34:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/27/2014 07:59 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Michael Haggerty writes:\n>>\n>>> When reading a symbolic ref via resolve_gitlink_ref_recursive(), check\n>>> that the reference name that is pointed at is formatted correctly,\n>>> using the same check as resolve_ref_unsafe() uses for non-gitlink\n>>> references. This prevents bogosity like\n>>>\n>>> ref: ../../other/file\n>>>\n>>> from causing problems.\n>>\n>> I do agree that a textual symref \"ref: ../../x/y\" that is stored in\n>> \".git/HEAD\" or in \".git/refs/L\" will step outside \".git/\" and it is\n>> problematic. But if \".git/refs/heads/a/b/LINK\" has \"ref: ../../x\"\n>> in it, shouldn't we interpret it as referring to the ref at\n>> \"refs/heads/x\"?\n\nI've never seen that usage, nor seen it advocated. Symrefs are not\npropagated via the Git protocol, so even if somebody were doing this\nprivately, it could hardly be a project-wide practice. I can't think of\na practical use for this feature. And it would be mildly annoying to\nimplement. So my inclination is to forbid it.\n\n> Actually, the textual symrefs have been invented to replace symbolic\n> links used for .git/HEAD on symlink-incapable filesystems, and we do\n> even not let the filesystem follow symlinks. The rule we have there\n> are like so:\n> \n> \t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n> \t\tif (S_ISLNK(st.st_mode)) {\n> \t\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n> \t\t\tif (len < 0) {\n> \t\t\t\tif (errno == ENOENT || errno == EINVAL)\n> \t\t\t\t\t/* inconsistent with lstat; retry */\n> \t\t\t\t\tgoto stat_ref;\n> \t\t\t\telse\n> \t\t\t\t\treturn NULL;\n> \t\t\t}\n> \t\t\tbuffer[len] = 0;\n> \t\t\tif (starts_with(buffer, \"refs/\") &&\n> \t\t\t\t\t!check_refname_format(buffer, 0)) {\n> \t\t\t\tstrcpy(refname_buffer, buffer);\n> \t\t\t\trefname = refname_buffer;\n> \t\t\t\tif (flag)\n> \t\t\t\t\t*flag |= REF_ISSYMREF;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t}\n> \n> So we should do exactly the same check, I would think, no?\n\nI think you overlooked that if the (starts_with() &&\n!check_refname_format()) check fails, execution falls through, ending up\nhere:\n\n\t\t/*\n\t\t * Anything else, just open it and try to use it as\n\t\t * a ref\n\t\t */\n\t\tfd = open(path, O_RDONLY);\n\t\tif (fd < 0) {\n\t\t\tif (errno == ENOENT)\n\t\t\t\t/* inconsistent with lstat; retry */\n\t\t\t\tgoto stat_ref;\n\t\t\telse\n\t\t\t\treturn NULL;\n\t\t}\n\t\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n\t\tclose(fd);\n\t\t[...etc...]\n\nThis has been the behavior since time immemorial [1].\n\nIn fact, another bug (which I probably introduced) is that in the case\nof a symlink that points at a non-existent file, this code goes into an\ninfinite loop due to the \"if (errno == ENOENT) goto stat_ref\" in the\ncode that I quoted. My mistake was forgetting that lstat() is statting\nthe link whereas open() follows the link, so the success of the former\ndoes not imply that the latter should not ENOENT.\n\nI suggest we fix both problems by making the code behave the way you\n*thought* it behaves: symlinks are never followed via the filesystem,\nbut if the symlink contents have the form of a legitimate refname that\nstarts with \"refs/\", then we follow it the same way as we would follow a\ntextual-style symref.\n\n> In a typical clone, the \".git/refs/remotes/origin/HEAD\" textual\n> symref stores \"ref: refs/remotes/origin/master\" and it is neither\n> \"ref: master\" nor \"ref: ./master\", so it should be sensible to\n> insist on \"must start with 'refs/' and its format valid.\"\n\nYes, we don't even have a notation for \"relative refnames\" because we\nwould have no way to distinguish them from \"absolute refnames\" except\nmaybe via some artifice like a \"./\" prefix.\n\nMichael\n\n[1] Where by \"time immemorial\" I mean \"since before I ever touched refs.c\".\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}