{"thread":{"id":"2902","subject":"[PATCH] sanity check in add_packed_git()","startedAt":"2005-12-21T23:47:09Z","lastAt":"2005-12-22T09:39:24Z","messageCount":4,"participants":["Pavel Roskin","Junio C Hamano","Daniel Barkalow"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"13908","messageId":"1135208829.15567.18.camel@dv","threadId":"2902","inReplyTo":null,"subject":"[PATCH] sanity check in add_packed_git()","fromName":"Pavel Roskin","fromEmail":"proski@gnu.org","sentAt":"2005-12-21T23:47:09Z","receivedAt":"2005-12-21T23:47:09Z","isPatch":true,"sender":{"key":"proski@gnu.org","avatar":null},"body":"add_packed_git() tries to get the pack SHA1 by parsing its name.  It may\naccess uninitialized memory for packs with short names.\n\nSigned-off-by: Pavel Roskin <proski@gnu.org>\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex fa22e9c..d83d824 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -464,7 +464,7 @@ struct packed_git *add_packed_git(char *\n \tp->pack_last_used = 0;\n \tp->pack_use_cnt = 0;\n \tp->pack_local = local;\n-\tif (!get_sha1_hex(path + path_len - 40 - 4, sha1))\n+\tif ((path_len > 44) && !get_sha1_hex(path + path_len - 44, sha1))\n \t\tmemcpy(p->sha1, sha1, 20);\n \treturn p;\n }\n\n\n-- \nRegards,\nPavel Roskin\n"},{"id":"13917","messageId":"7v7j9xvrf3.fsf@assigned-by-dhcp.cox.net","threadId":"2902","inReplyTo":"1135208829.15567.18.camel@dv","subject":"Re: [PATCH] sanity check in add_packed_git()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-22T01:42:08Z","receivedAt":"2005-12-22T01:42:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pavel Roskin <proski@gnu.org> writes:\n\n> add_packed_git() tries to get the pack SHA1 by parsing its name.  It may\n> access uninitialized memory for packs with short names.\n\nThanks.\n\nThis fixes when there is \".git/objects/packs/junk-X.pack\", so in\nthat sense it is a real fix and I'll take it.\n\nHowever, I feel a bit uneasy about this whole thing.  The core\ndoes not care how you name your packs, as long as .pack and .idx\nfiles have the same prefix, but we started relying on the prefix\nbeing \"pack-\" followed by 40 hexadecimal digits since we started\npacked repository support in http-fetch, and we also allowed\nsha1_pack_name() function that shares the assumption to sneak\ninto the real core part of the system around the same time (end\nof July 2005).  git-repack and git-verify-pack stay ignorant\nabout this prefix convention and I think that is a good\nproperty.  However, we might be better off if we declare that\nthe pack files *must* be named following that convention\n(currently nobody enforces it, but as long as the user uses \"git\nrepack\" to create packs, the packs automatically follow that\nconvention), and make check_packed_git_idx() reject a packfile\nwhose name does not begin with \"pack-\" or the 40 hexdigits that\nfollow does not match the hash of the object names the index\nrecords.  If we go that route, then this patch becomes unneeded;\nmore extensive check needs to be done in check_packed_git_idx().\n\nThoughts?\n"},{"id":"13918","messageId":"Pine.LNX.4.64.0512212046340.25300@iabervon.org","threadId":"2902","inReplyTo":"7v7j9xvrf3.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] sanity check in add_packed_git()","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2005-12-22T01:58:44Z","receivedAt":"2005-12-22T01:58:44Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 21 Dec 2005, Junio C Hamano wrote:\n\n> Pavel Roskin <proski@gnu.org> writes:\n> \n> > add_packed_git() tries to get the pack SHA1 by parsing its name.  It may\n> > access uninitialized memory for packs with short names.\n> \n> Thanks.\n> \n> This fixes when there is \".git/objects/packs/junk-X.pack\", so in\n> that sense it is a real fix and I'll take it.\n> \n> However, I feel a bit uneasy about this whole thing.  The core\n> does not care how you name your packs, as long as .pack and .idx\n> files have the same prefix, but we started relying on the prefix\n> being \"pack-\" followed by 40 hexadecimal digits since we started\n> packed repository support in http-fetch, and we also allowed\n> sha1_pack_name() function that shares the assumption to sneak\n> into the real core part of the system around the same time (end\n> of July 2005).  git-repack and git-verify-pack stay ignorant\n> about this prefix convention and I think that is a good\n> property.  However, we might be better off if we declare that\n> the pack files *must* be named following that convention\n> (currently nobody enforces it, but as long as the user uses \"git\n> repack\" to create packs, the packs automatically follow that\n> convention), and make check_packed_git_idx() reject a packfile\n> whose name does not begin with \"pack-\" or the 40 hexdigits that\n> follow does not match the hash of the object names the index\n> records.  If we go that route, then this patch becomes unneeded;\n> more extensive check needs to be done in check_packed_git_idx().\n> \n> Thoughts?\n\nI'd like to require it to be a hash, just because that makes it \nprohibitively difficult to make something people will accept as\npack-05f611b3b8198b262acdf678584d365f8e879aec.pack other than the one from \nthe git repository. Sure, it would only be a minor DoS and a bit of \nconfusion, because it still couldn't do any worse than contain some \ndifferent objects, but it would block people from getting the pack with \nthe objects they want. Since nobody seems interested in naming their \npacks, so far as I know, it seems best to force the names to be \nuniversally unique, modulo an incredible coincidence.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"13924","messageId":"7v4q51qxmb.fsf@assigned-by-dhcp.cox.net","threadId":"2902","inReplyTo":"Pine.LNX.4.64.0512212046340.25300@iabervon.org","subject":"Re: [PATCH] sanity check in add_packed_git()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-22T09:39:24Z","receivedAt":"2005-12-22T09:39:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> I'd like to require it to be a hash, just because that makes it \n> prohibitively difficult to make something people will accept as\n> pack-05f611b3b8198b262acdf678584d365f8e879aec.pack other than the one from \n> the git repository.\n\nHow about doing something like this, then?\n\nI haven't tried this with http-fetch but with this I think we\ncan unconditionally assign p->sha1 in add_packed_git().\n\n-- >8 --\nSubject: [PATCH] Require packfiles to follow the naming convention.\n\nWith this, pack files must be named \"pack-[0-9a-f]{40}.pack\",\nwith corresponding .idx file, and the hexadecimal part must\nmatch the SHA1 checksum git-pack-objects computed when the pack\nwas created originally (i.e. sum over sorted object names of all\nthe contained objects).  The core would ignore a valid packfile\nin $GIT_OBJECT_DIRECTORY/packs that \"git-unpack-objects\" would\nexpand properly if it does not follow the naming convention.\n\n[jc: this breaks existing test t5300.]\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n\n---\n\n sha1_file.c |   38 +++++++++++++++++++++++++++++++++++++-\n 1 files changed, 37 insertions(+), 1 deletions(-)\n\n4ff64a29401b28cfbd925cf86eed8b8e734ed24b\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6011473..01ac925 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -319,18 +319,33 @@ struct packed_git *packed_git;\n static int check_packed_git_idx(const char *path, unsigned long *idx_size_,\n \t\t\t\tvoid **idx_map_)\n {\n+\tSHA_CTX ctx;\n+\tunsigned char sha1[20];\n+\tunsigned char packname[20];\n \tvoid *idx_map;\n \tunsigned int *index;\n \tunsigned long idx_size;\n \tint nr, i;\n-\tint fd = open(path, O_RDONLY);\n+\tint fd;\n \tstruct stat st;\n+\n+\tfd = open(path, O_RDONLY);\n \tif (fd < 0)\n \t\treturn -1;\n \tif (fstat(fd, &st)) {\n \t\tclose(fd);\n \t\treturn -1;\n \t}\n+\n+\t/* We require \"...../pack-XXXX{40}XXXX.idx\" now.\n+\t * 5-byte \"pack-\", 40-byte hex and 4-byte .idx.\n+\t */\n+\ti = strlen(path);\n+\tif (i < 49 || strcmp(path + i - 4, \".idx\") ||\n+\t    strncmp(path + i - 49, \"pack-\", 5) ||\n+\t    get_sha1_hex(path + i - 44, packname))\n+\t\treturn error(\"non index file %s ignored\", path);\n+\n \tidx_size = st.st_size;\n \tidx_map = mmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n@@ -362,6 +377,27 @@ static int check_packed_git_idx(const ch\n \tif (idx_size != 4*256 + nr * 24 + 20 + 20)\n \t\treturn error(\"wrong index file size\");\n \n+\t/*\n+\t * File checksum and index name.\n+\t */\n+\tSHA1_Init(&ctx);\n+\tSHA1_Update(&ctx, idx_map, idx_size-20);\n+\tSHA1_Final(sha1, &ctx);\n+\n+\tif (memcmp(sha1, idx_map + idx_size - 20, 20))\n+\t\treturn error(\"index checksum mismatch\");\n+\n+\tSHA1_Init(&ctx);\n+\tfor (i = 0; i < nr; i++) {\n+\t\tunsigned char *ent = (idx_map + 4*256) + i * 24 + 4;\n+\t\tSHA1_Update(&ctx, ent, 20);\n+\t}\n+\tSHA1_Final(sha1, &ctx);\n+\t\n+\tif (memcmp(sha1, packname, 20))\n+\t\treturn error(\"pack index name %s does not match contents %s\",\n+\t\t\t     path, sha1_to_hex(sha1));\n+\n \treturn 0;\n }\n \n-- \n1.0.GIT\n"}]}