{"thread":{"id":"46841","subject":"[PATCH 0/3] validate_headref: avoid reading uninitialized bytes","startedAt":"2017-09-27T06:16:51Z","lastAt":"2017-09-27T07:28:16Z","messageCount":8,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"329012","messageId":"20170927061644.gwlhxaiairiy2njp@sigill.intra.peff.net","threadId":"46841","inReplyTo":null,"subject":"[PATCH 0/3] validate_headref: avoid reading uninitialized bytes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:16:45Z","receivedAt":"2017-09-27T06:16:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While looking at read_in_full() callers for a nearby patch series, I\nnoticed this fairly harmless bug. The first patch fixes it, and the\nother two do some cleanups on top.\n\nI'm tempted to say the whole thing ought to just use a strbuf. We\ntypically only call this function once per program, and git HEAD files\nare small enough not to worry about. On the other hand, the point of the\nfunction is to see if this is in fact a git repository. So in the rare\ncase that we see a file named \".git/HEAD\" that's actually a 2GB video\nfile, we'd prefer to find out after reading only 256 bytes, not the\nwhole thing.\n\n [1/3]: validate_headref: NUL-terminate HEAD buffer\n [2/3]: validate_headref: use skip_prefix for symref parsing\n [3/3]: validate_headref: use get_oid_hex for detached HEADs\n\n path.c | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\n-Peff\n"},{"id":"329013","messageId":"20170927061722.7ubswfgykxiawzh3@sigill.intra.peff.net","threadId":"46841","inReplyTo":"20170927061644.gwlhxaiairiy2njp@sigill.intra.peff.net","subject":"[PATCH 1/3] validate_headref: NUL-terminate HEAD buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:17:23Z","receivedAt":"2017-09-27T06:17:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are checking to see if we have a git repo, we peek\ninto the HEAD file and see if it's a plausible symlink,\nsymref, or detached HEAD.\n\nFor the latter two, we read the contents with read_in_full(),\nwhich means they aren't NUL-terminated. The symref check is\ncareful to respect the length we got, but the sha1 check\nwill happily parse up to 40 bytes, even if we read fewer.\n\nE.g.,:\n\n echo 1234 >.git/HEAD\n git rev-parse\n\nwill parse 36 uninitialized bytes from our stack buffer.\n\nThis isn't a big deal in practice. Our buffer is 256 bytes,\nso we know we'll never read outside of it. The worst case is\nthat the uninitialized bytes look like valid hex, and we\nclaim a bogus HEAD file is valid. The chances of this\nhappening randomly are quite slim, but let's be careful.\n\nOne option would be to check that \"len == 41\" before feeding\nthe buffer to get_sha1_hex(). But we'd like to eventually\nprepare for a world with variable-length hashes. Let's\nNUL-terminate as soon as we've read the buffer (we already\neven leave a spare byte to do so!). That fixes this problem\nwithout depending on the size of an object id.\n\nSigned-off-by: Jeff King \n---\n path.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/path.c b/path.c\nindex b533ec938d..3e4d7505ef 100644\n--- a/path.c\n+++ b/path.c\n@@ -662,6 +662,10 @@ int validate_headref(const char *path)\n \tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n \tclose(fd);\n \n+\tif (len < 0)\n+\t\treturn -1;\n+\tbuffer[len] = '\\0';\n+\n \t/*\n \t * Is it a symbolic ref?\n \t */\n-- \n2.14.2.988.g01c8b37dde\n\n"},{"id":"329014","messageId":"20170927061726.gat3ykqx4fdstzq6@sigill.intra.peff.net","threadId":"46841","inReplyTo":"20170927061644.gwlhxaiairiy2njp@sigill.intra.peff.net","subject":"[PATCH 2/3] validate_headref: use skip_prefix for symref parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:17:26Z","receivedAt":"2017-09-27T06:17:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since the previous commit guarantees that our symref buffer\nis NUL-terminated, we can just use skip_prefix() and friends\nto parse it. This is shorter and saves us having to deal\nwith magic numbers and keeping the \"len\" counter up to date.\n\nWhile we're at it, let's name the rather obscure \"buf\" to\n\"refname\", since that is the thing we are parsing with it.\n\nSigned-off-by: Jeff King \n---\n path.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 3e4d7505ef..2ef3a86613 100644\n--- a/path.c\n+++ b/path.c\n@@ -637,7 +637,8 @@ void strbuf_git_common_path(struct strbuf *sb,\n int validate_headref(const char *path)\n {\n \tstruct stat st;\n-\tchar *buf, buffer[256];\n+\tchar buffer[256];\n+\tconst char *refname;\n \tunsigned char sha1[20];\n \tint fd;\n \tssize_t len;\n@@ -669,14 +670,10 @@ int validate_headref(const char *path)\n \t/*\n \t * Is it a symbolic ref?\n \t */\n-\tif (len < 4)\n-\t\treturn -1;\n-\tif (!memcmp(\"ref:\", buffer, 4)) {\n-\t\tbuf = buffer + 4;\n-\t\tlen -= 4;\n-\t\twhile (len && isspace(*buf))\n-\t\t\tbuf++, len--;\n-\t\tif (len >= 5 && !memcmp(\"refs/\", buf, 5))\n+\tif (skip_prefix(buffer, \"ref:\", &refname)) {\n+\t\twhile (isspace(*refname))\n+\t\t\trefname++;\n+\t\tif (starts_with(refname, \"refs/\"))\n \t\t\treturn 0;\n \t}\n \n-- \n2.14.2.988.g01c8b37dde\n\n"},{"id":"329015","messageId":"20170927061736.reeavh5r3tyqovbc@sigill.intra.peff.net","threadId":"46841","inReplyTo":"20170927061644.gwlhxaiairiy2njp@sigill.intra.peff.net","subject":"[PATCH 3/3] validate_headref: use get_oid_hex for detached HEADs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:17:36Z","receivedAt":"2017-09-27T06:17:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a candidate HEAD isn't a symref, we check that it\ncontains a viable sha1. But in a post-sha1 world, we should\nbe checking whether it has any plausible object-id.\n\nWe can do that by switching to get_oid_hex().\n\nNote that both before and after this patch, we only check\nfor a plausible object id at the start of the file, and then\ncall that good enough. We ignore any content _after_ the\nhex, so a string like:\n\n 0123456789012345678901234567890123456789 foo\n\nis accepted. Though we do put extra bytes like this into\nsome pseudorefs (e.g., FETCH_HEAD), we don't typically do so\nwith HEAD. We could tighten this up by using parse_oid_hex(),\nlike:\n\n if (!parse_oid_hex(buffer, &oid, &end) &&\n *end++ == '\\n' && *end == '\\0')\n return 0;\n\nBut we're probably better to remain on the loose side. We're\njust checking here for a plausible-looking repository\ndirectory, so heuristics are acceptable (if we really want\nto be meticulous, we should use the actual ref code to parse\nHEAD).\n\nSigned-off-by: Jeff King \n---\n path.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 2ef3a86613..5aa9244eb2 100644\n--- a/path.c\n+++ b/path.c\n@@ -639,7 +639,7 @@ int validate_headref(const char *path)\n \tstruct stat st;\n \tchar buffer[256];\n \tconst char *refname;\n-\tunsigned char sha1[20];\n+\tstruct object_id oid;\n \tint fd;\n \tssize_t len;\n \n@@ -680,7 +680,7 @@ int validate_headref(const char *path)\n \t/*\n \t * Is this a detached HEAD?\n \t */\n-\tif (!get_sha1_hex(buffer, sha1))\n+\tif (!get_oid_hex(buffer, &oid))\n \t\treturn 0;\n \n \treturn -1;\n-- \n2.14.2.988.g01c8b37dde\n"},{"id":"329026","messageId":"xmqqefqs4o41.fsf@gitster.mtv.corp.google.com","threadId":"46841","inReplyTo":"20170927061722.7ubswfgykxiawzh3@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] validate_headref: NUL-terminate HEAD buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-27T07:06:22Z","receivedAt":"2017-09-27T07:06:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/path.c b/path.c\n> index b533ec938d..3e4d7505ef 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -662,6 +662,10 @@ int validate_headref(const char *path)\n> \tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n> \tclose(fd);\n> \n> +\tif (len < 0)\n> +\t\treturn -1;\n> +\tbuffer[len] = '\\0';\n> +\n> \t/*\n> \t * Is it a symbolic ref?\n> \t */\n\nA few tangents I noticed:\n\n - the result of readlink should be checked with starts_with() in\n the modern codebase (#leftoverbits).\n\n - buffer[256] would mean that we cannot have a branch whose name is\n more than a couple of hundred bytes long; as you said, we may be\n better off using strbuf_read to read the whole thing.\n\nNeither should be touched by this patch, of course.\n\nThanks.\n"},{"id":"329027","messageId":"xmqqa81g4o2j.fsf@gitster.mtv.corp.google.com","threadId":"46841","inReplyTo":"20170927061726.gat3ykqx4fdstzq6@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] validate_headref: use skip_prefix for symref parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-27T07:07:16Z","receivedAt":"2017-09-27T07:07:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Since the previous commit guarantees that our symref buffer\n> is NUL-terminated, we can just use skip_prefix() and friends\n> to parse it. This is shorter and saves us having to deal\n> with magic numbers and keeping the \"len\" counter up to date.\n\nAh, I should have read the whole thing before starting to type a\nresponse to 1/3 ;-)\n\nLooks good.\n"},{"id":"329029","messageId":"20170927071625.3dtljf5oaiaa5pjq@sigill.intra.peff.net","threadId":"46841","inReplyTo":"xmqqefqs4o41.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] validate_headref: NUL-terminate HEAD buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T07:16:25Z","receivedAt":"2017-09-27T07:16:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 27, 2017 at 04:06:22PM +0900, Junio C Hamano wrote:\n\n> A few tangents I noticed:\n> \n> - the result of readlink should be checked with starts_with() in\n> the modern codebase (#leftoverbits).\n\nYes, though it needs to NUL-terminate, too (readlink does not do so\nautomatically). Again, we seem to have left room for the NUL but didn't\nbother to put it there.\n\n> - buffer[256] would mean that we cannot have a branch whose name is\n> more than a couple of hundred bytes long; as you said, we may be\n> better off using strbuf_read to read the whole thing.\n\nHeh, I almost talked about this in the cover letter, but didn't want to\ngo off on a tangent. But since you mention it...\n\nI had the same concern, but actually truncation is not a problem here\n(for a symlink or a symref). We are only seeing if the contents look\nvaguely correct, so really we never parse past \"refs/\" in either case.\n\nThe real symref resolution happens in refs/files-backend.c these days,\nand does use a strbuf.\n\n-Peff\n"},{"id":"329030","messageId":"xmqq60c44n3r.fsf@gitster.mtv.corp.google.com","threadId":"46841","inReplyTo":"20170927071625.3dtljf5oaiaa5pjq@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] validate_headref: NUL-terminate HEAD buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-27T07:28:08Z","receivedAt":"2017-09-27T07:28:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I had the same concern, but actually truncation is not a problem here\n> (for a symlink or a symref). We are only seeing if the contents look\n> vaguely correct, so really we never parse past \"refs/\" in either case.\n\nAhh, OK.\n\nIt seems that at least for this week, my reviews always show you\nthought things more thoroughly than I did, which is good in a weird\nsort of sense ;-)\n"}]}