{"thread":{"id":"41129","subject":"Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","startedAt":"2016-01-05T13:44:49Z","lastAt":"2016-02-25T14:12:46Z","messageCount":13,"participants":["Jacek Wielemborek","Jeff King","Jonathan Nieder","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"275376","messageId":"568BC8D1.3080201@gmail.com","threadId":"41129","inReplyTo":null,"subject":"Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Jacek Wielemborek","fromEmail":"d33tah@gmail.com","sentAt":"2016-01-05T13:44:49Z","receivedAt":"2016-01-05T13:44:49Z","isPatch":false,"sender":{"key":"d33tah@gmail.com","avatar":"https://gravatar.com/avatar/d375a08a32bc5df290e96fb3b111d724996c2cdfeda45d45ccea0d61b0b1aabf?d=mp&s=160"},"body":"Hello,\n\nSteps to reproduce:\n\n1. base64 -d and unpack the .tar.gz file from here:\nhttps://gist.github.com/d33tah/4e976f2e043718594a85\n\n2. cd into it, run \"git log\"\n\nI'll be happy to guide you through the fuzzing process - I stopped it at\nthe first crash.\n\nCheers,\nd33tah\n\n"},{"id":"275379","messageId":"20160105152436.GA1205@sigill.intra.peff.net","threadId":"41129","inReplyTo":"568BC8D1.3080201@gmail.com","subject":"Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-05T15:24:36Z","receivedAt":"2016-01-05T15:24:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 05, 2016 at 02:44:49PM +0100, Jacek Wielemborek wrote:\n\n> Steps to reproduce:\n> \n> 1. base64 -d and unpack the .tar.gz file from here:\n> https://gist.github.com/d33tah/4e976f2e043718594a85\n> \n> 2. cd into it, run \"git log\"\n> \n> I'll be happy to guide you through the fuzzing process - I stopped it at\n> the first crash.\n\nI'm not terribly surprised, and we should look into fixing the\nsegfault[1].  But I want to also note that fuzzing packfiles for \"git\nlog\" is not a terribly realistic attack vector.\n\nGit packfiles come from two places:\n\n  1. Local maintenance repacks loose and already-packed objects into a\n     new packfile. We trust the local repack process to generate a valid\n     packfile (though the contents of individual objects may be\n     untrusted, of course).\n\n  2. A fetch or push may introduce a new packfile to the repository, but\n     it does not enter directly. It is passed through \"index-pack\n     --stdin\", which checks it byte by byte, creating its own index, and\n     making sure that both the pack structure is good, and optionally\n     the format of the individual object data.\n\nSo \"index-pack\" is the enforcement point, and the rest of the git\ncommands generally assume that we can trust what is on disk (as it is\nhas either been generated by us, or checked by index-pack).  The rest of\nthe commands do not spend time checking that the on-disk contents are\nsane (though you can run git-fsck if you want to do that).\n\nIf you can find a fuzzed packfile that crashes \"index-pack\", then _that_\nwould be a big deal. I tried AFL against it a few months ago, but didn't\nturn up any hits. I didn't run it for all that long, but I'd be\nsurprised if it is all that fruitful. There are quite a few embedded\nchecksums in a packfile (zlib checksums, as well as a sha1 over the\nwhole file), and index-pack punts when one doesn't match. I think you'd\nneed a fuzzer which is aware of the zlib and packfile formats.\n\n-Peff\n\n[1] I briefly ran your case under valgrind and got:\n\n    ==5409== Invalid read of size 4\n    ==5409==    at 0x55F92A: nth_packed_object_offset (sha1_file.c:2464)\n    ==5409==    by 0x55FBD4: find_pack_entry_one (sha1_file.c:2523)\n    ==5409==    by 0x55FCF7: fill_pack_entry (sha1_file.c:2566)\n    ==5409==    by 0x55FDE2: find_pack_entry (sha1_file.c:2604)\n    ==5409==    by 0x5615BA: has_sha1_file_with_flags (sha1_file.c:3212)\n    ==5409==    by 0x50FC38: has_sha1_file (cache.h:1049)\n    ==5409==    by 0x51043B: parse_object (object.c:259)\n    ==5409==    by 0x546BF9: get_reference (revision.c:254)\n    ==5409==    by 0x54CA15: setup_revisions (revision.c:2342)\n    ==5409==    by 0x4531DA: cmd_log_init_finish (log.c:156)\n    ==5409==    by 0x453465: cmd_log_init (log.c:211)\n    ==5409==    by 0x4547EB: cmd_log (log.c:672)\n    ==5409==  Address 0x840244bc is not stack'd, malloc'd or (recently) free'd\n\n    So I'd guess it's not the pack itself, but rather the .idx which is\n    full of nonsense values. And that's always generated from scratch\n    locally.\n\n    Which isn't to say there aren't _some_ attacks you could do with\n    this. But git's security model has never been that you can untar\n    a random .git directory and use it securely.\n"},{"id":"275407","messageId":"20160106002333.GA16090@google.com","threadId":"41129","inReplyTo":"20160105152436.GA1205@sigill.intra.peff.net","subject":"Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-01-06T00:23:33Z","receivedAt":"2016-01-06T00:23:33Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Git packfiles come from two places:\n>\n>   1. Local maintenance repacks loose and already-packed objects into a\n>      new packfile. We trust the local repack process to generate a valid\n>      packfile (though the contents of individual objects may be\n>      untrusted, of course).\n\nI think we should reconsider such trust.  If one user creates a\nmalicious pack, if another user uses read-only git commands to access\nthe repository (after inspecting .git/config to make sure it doesn't\ncontain anything scary) the result should not be arbitrary code\nexecution.\n\nProducing bogus output or aborting is okay; arbitrary code execution\nless so.\n\nThanks,\nJonathan\n"},{"id":"275414","messageId":"20160106072321.GA18477@sigill.intra.peff.net","threadId":"41129","inReplyTo":"20160106002333.GA16090@google.com","subject":"Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-06T07:23:22Z","receivedAt":"2016-01-06T07:23:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 05, 2016 at 04:23:33PM -0800, Jonathan Nieder wrote:\n\n> > Git packfiles come from two places:\n> >\n> >   1. Local maintenance repacks loose and already-packed objects into a\n> >      new packfile. We trust the local repack process to generate a valid\n> >      packfile (though the contents of individual objects may be\n> >      untrusted, of course).\n> \n> I think we should reconsider such trust.  If one user creates a\n> malicious pack, if another user uses read-only git commands to access\n> the repository (after inspecting .git/config to make sure it doesn't\n> contain anything scary) the result should not be arbitrary code\n> execution.\n> \n> Producing bogus output or aborting is okay; arbitrary code execution\n> less so.\n\nI agree it is better if we can meet this standard, and I didn't mean to\ndiscourage fixes in the area. But I do think it is worth classifying\nthem differently than bugs that can be triggered via the network. The\nattack surface for on-disk attacks is much larger, and the number of\npeople affected is much smaller.\n\nRegarding your example, I'm not sure it's the best motivating example,\nas I imagine hardly anyone examines .git/config. :) A simplified one\nmight be that:\n\n  git fetch me@shared-server:/home/you/foo.git\n\nis running git-upload-pack as me on packfiles created by you (on the\nserver). We normally expect that to be a \"safe\" operation (and it is if\ndone over git:// or similar).\n\n-Peff\n"},{"id":"275422","messageId":"568CDE05.5040602@gmail.com","threadId":"41129","inReplyTo":"20160106002333.GA16090@google.com","subject":"Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Jacek Wielemborek","fromEmail":"d33tah@gmail.com","sentAt":"2016-01-06T09:27:33Z","receivedAt":"2016-01-06T09:27:33Z","isPatch":false,"sender":{"key":"d33tah@gmail.com","avatar":"https://gravatar.com/avatar/d375a08a32bc5df290e96fb3b111d724996c2cdfeda45d45ccea0d61b0b1aabf?d=mp&s=160"},"body":"W dniu 06.01.2016 o 01:23, Jonathan Nieder pisze:\n> Jeff King wrote:\n> \n>> Git packfiles come from two places:\n>>\n>>   1. Local maintenance repacks loose and already-packed objects into a\n>>      new packfile. We trust the local repack process to generate a valid\n>>      packfile (though the contents of individual objects may be\n>>      untrusted, of course).\n> \n> I think we should reconsider such trust.  If one user creates a\n> malicious pack, if another user uses read-only git commands to access\n> the repository (after inspecting .git/config to make sure it doesn't\n> contain anything scary) the result should not be arbitrary code\n> execution.\n> \n> Producing bogus output or aborting is okay; arbitrary code execution\n> less so.\n> \n> Thanks,\n> Jonathan\n\nI'd be happy to help you go through the fuzzing process - I don't have\nenough horsepower and codebase knowledge to do it on my own though. If\nyou have an afl-fuzz question though, let me know.\n\n"},{"id":"275425","messageId":"CACsJy8A3xX02pV7r+vG0BvtFmU=Axx7=2nHXp07zOreXeAj_wg@mail.gmail.com","threadId":"41129","inReplyTo":"20160105152436.GA1205@sigill.intra.peff.net","subject":"Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-01-06T09:46:04Z","receivedAt":"2016-01-06T09:46:04Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 5, 2016 at 10:24 PM, Jeff King <peff@peff.net> wrote:\n> If you can find a fuzzed packfile that crashes \"index-pack\", then _that_\n> would be a big deal.\n\nI'm sure you know this, but if Jacek moves to break index-pack, then\nhe/she should also try to break unpack-objects because sometimes we\nuse that command instead of index-pack.\n-- \nDuy\n"},{"id":"275434","messageId":"568D0DD1.5060300@gmail.com","threadId":"41129","inReplyTo":"CACsJy8A3xX02pV7r+vG0BvtFmU=Axx7=2nHXp07zOreXeAj_wg@mail.gmail.com","subject":"Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Jacek Wielemborek","fromEmail":"d33tah@gmail.com","sentAt":"2016-01-06T12:51:29Z","receivedAt":"2016-01-06T12:51:29Z","isPatch":false,"sender":{"key":"d33tah@gmail.com","avatar":"https://gravatar.com/avatar/d375a08a32bc5df290e96fb3b111d724996c2cdfeda45d45ccea0d61b0b1aabf?d=mp&s=160"},"body":"W dniu 06.01.2016 o 10:46, Duy Nguyen pisze:\n> On Tue, Jan 5, 2016 at 10:24 PM, Jeff King <peff@peff.net> wrote:\n>> If you can find a fuzzed packfile that crashes \"index-pack\", then _that_\n>> would be a big deal.\n> \n> I'm sure you know this, but if Jacek moves to break index-pack, then\n> he/she should also try to break unpack-objects because sometimes we\n> use that command instead of index-pack.\n> \n\nIt sounds that you could use a little explanation on how I found this\ncrashing case and what would it take to fuzz index-pack, according to\nthe conversation I had on #git-devel on irc.freenode.net. Should I\nassume that you know the basic afl-fuzz in my next post?\n\nBTW @Duy, thanks for CC to me, I'm not subscribed to the ML.\n\n"},{"id":"275495","messageId":"xmqqr3ht41w8.fsf@gitster.mtv.corp.google.com","threadId":"41129","inReplyTo":"20160105152436.GA1205@sigill.intra.peff.net","subject":"Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-07T19:37:11Z","receivedAt":"2016-01-07T19:37:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> [1] I briefly ran your case under valgrind and got:\n>\n>     ==5409== Invalid read of size 4\n>     ==5409==    at 0x55F92A: nth_packed_object_offset (sha1_file.c:2464)\n>     ==5409==    by 0x55FBD4: find_pack_entry_one (sha1_file.c:2523)\n>     ==5409==    by 0x55FCF7: fill_pack_entry (sha1_file.c:2566)\n>     ==5409==    by 0x55FDE2: find_pack_entry (sha1_file.c:2604)\n>     ==5409==    by 0x5615BA: has_sha1_file_with_flags (sha1_file.c:3212)\n>     ==5409==    by 0x50FC38: has_sha1_file (cache.h:1049)\n>     ==5409==    by 0x51043B: parse_object (object.c:259)\n>     ==5409==    by 0x546BF9: get_reference (revision.c:254)\n>     ==5409==    by 0x54CA15: setup_revisions (revision.c:2342)\n>     ==5409==    by 0x4531DA: cmd_log_init_finish (log.c:156)\n>     ==5409==    by 0x453465: cmd_log_init (log.c:211)\n>     ==5409==    by 0x4547EB: cmd_log (log.c:672)\n>     ==5409==  Address 0x840244bc is not stack'd, malloc'd or (recently) free'd\n>\n>     So I'd guess it's not the pack itself, but rather the .idx which is\n>     full of nonsense values. And that's always generated from scratch\n>     locally.\n\nAfter I seeing your \"it's not the pack itself but rather the .idx\"\nwithout looking at (rather, conciously avoiding to look at) the\nvalgrind trace, I checked the codepath that starts from\nread_packed_sha1().\n\nWhen we map in the .idx file, we do minimum sanity checks to make\nsure that .idx file itself has sorted fan-out.  We do not check if\nthe object names are sorted, so a bogus .idx could tell us that an\nobject does not exist when it exists in the matching .pack, but that\nis harmless.  Also an unsorted object names will not make our binary\nsearch run in circles while looking things up.  We do not check if\nthe offset of individual objects are within the corresponding .pack\nfile, either.\n\nAnd I arrived at nth_packed_object_offset(), so I think I am hitting\nthe right nail.  There are a few values we read from the .idx file\nhere that we would want to validate for sanity.\n\n\n    off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)\n    {\n            const unsigned char *index = p->index_data;\n            index += 4 * 256;\n            if (p->index_version == 1) {\n                    return ntohl(*((uint32_t *)(index + 24 * n)));\n\nThis can return an offset that goes beyond the end of the packfile,\npossibly leading to a read access of unmapped region.\n\n            } else {\n                    uint32_t off;\n                    index += 8 + p->num_objects * (20 + 4);\n                    off = ntohl(*((uint32_t *)(index + 4 * n)));\n                    if (!(off & 0x80000000))\n                            return off;\n\nThis also can return an offset that goes beyond the end of the\npackfile.\n\nOtherwise 'off' at this point gives an offset into the .idx file\nitself, that is the location of the 8-byte offset into the packfile.\n\n                    index += p->num_objects * 4 + (off & 0x7fffffff) * 8;\n\nAnd this computation can take us beyond the end of the .idx\n\n                    return (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) |\n                                       ntohl(*((uint32_t *)(index + 4)));\n\nOr the value we read with this can take us beyond the end of the\npackfile.\n\n            }\n    }\n\nAs long as the returned value is within the valid region of\npackfile, packed_object_info() and unpack_entry() should be\nresponsible for ensuring that we do not read past the end of the\ndata.  They should both begin by calling use_pack(), which already\nhas the \"offset beyond end of packfile\" check, so I think that they\nwould successfully catch a malicious .idx file that has handcrafted\nbad offsets if we fixed nth_packed_object_offset().\n"},{"id":"275508","messageId":"xmqqtwmp2e6d.fsf@gitster.mtv.corp.google.com","threadId":"41129","inReplyTo":"xmqqr3ht41w8.fsf@gitster.mtv.corp.google.com","subject":"Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-07T22:54:50Z","receivedAt":"2016-01-07T22:54:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When we map in the .idx file, we do only minimum sanity checks to\nmake sure that the .idx file itself has sorted fan-out.  We do not\ncheck if the object names are sorted, so a bogus .idx could tell us\nthat an object does not exist when it exists in the matching .pack,\nbut that is harmless.  Also an unsorted object names will not make\nour binary search run in circles while looking things up.\n\nWe do not check if the offset of individual objects are within the\ncorresponding .pack file, either, and nth_packed_object_offset()\ndoes return the data read from .idx file that is not checked for\nsanity.  use_pack(), which is the helper used by the callers of\nnth_packed_object_offset() that finds the offset in the packfile for\neach object, avoids allowing a read access to mapped pack data\nbeyond the end of it, so it is OK to return bogus value that was\nread from the .idx file from this function, but there is one\ncomputation the function itself does using a possibly bogus value\nread from the disk: to find out where in the secondary offset table\nin the .idx file the offset in the packfile is stored.\n\n---\n\n This is not even compile tested; I just wanted to prevent people\n from adding two unnecessary checks to this function following my\n analysis in the previous message.  I think returning bogus value\n stored in a crafted .idx file from this function is OK, as the\n offset will be first used by use_pack() and the sanity of the\n offset, relative to the packfile size, is checked there, and an\n offset that points to a random point in the packfile will be caught\n by the pack reading code, either by unpack_compressed_entry() or by\n patch_delta(), so that is also safe.\n\n We do need to check the unprotected access here.  Nobody else in\n the current codepath protects us from this access attempting to\n read an unmapped memory and segfault.\n\n sha1_file.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 73ccd49..8aca1f6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2458,6 +2458,13 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)\n \t\toff = ntohl(*((uint32_t *)(index + 4 * n)));\n \t\tif (!(off & 0x80000000))\n \t\t\treturn off;\n+\n+\t\t/* 8-byte offset table */\n+\t\tif ((p->index_size - (8 + 256 * 4 + 28 * p->num_objects + 40))\n+\t\t    <\n+\t\t    (off & 0x7fffffff) * 8)\n+\t\t\tdie(\"offset beyond end of .idx file\");\n+\n \t\tindex += p->num_objects * 4 + (off & 0x7fffffff) * 8;\n \t\treturn (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) |\n \t\t\t\t   ntohl(*((uint32_t *)(index + 4)));\n"},{"id":"275715","messageId":"20160111213351.GA21131@sigill.intra.peff.net","threadId":"41129","inReplyTo":"xmqqtwmp2e6d.fsf@gitster.mtv.corp.google.com","subject":"Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-11T21:33:51Z","receivedAt":"2016-01-11T21:33:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 07, 2016 at 02:54:50PM -0800, Junio C Hamano wrote:\n\n>  This is not even compile tested; I just wanted to prevent people\n>  from adding two unnecessary checks to this function following my\n>  analysis in the previous message.  I think returning bogus value\n>  stored in a crafted .idx file from this function is OK, as the\n>  offset will be first used by use_pack() and the sanity of the\n>  offset, relative to the packfile size, is checked there, and an\n>  offset that points to a random point in the packfile will be caught\n>  by the pack reading code, either by unpack_compressed_entry() or by\n>  patch_delta(), so that is also safe.\n> \n>  We do need to check the unprotected access here.  Nobody else in\n>  the current codepath protects us from this access attempting to\n>  read an unmapped memory and segfault.\n\nI think this is the right track, and it does indeed catch the bogus .idx\nthat started this thread. I agree that we should be OK to hand back a\nbogus offset to the pack code, which already handles bounds-checking due\nto the sliding window code.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 73ccd49..8aca1f6 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2458,6 +2458,13 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)\n>  \t\toff = ntohl(*((uint32_t *)(index + 4 * n)));\n>  \t\tif (!(off & 0x80000000))\n>  \t\t\treturn off;\n> +\n> +\t\t/* 8-byte offset table */\n> +\t\tif ((p->index_size - (8 + 256 * 4 + 28 * p->num_objects + 40))\n> +\t\t    <\n> +\t\t    (off & 0x7fffffff) * 8)\n> +\t\t\tdie(\"offset beyond end of .idx file\");\n> +\n>  \t\tindex += p->num_objects * 4 + (off & 0x7fffffff) * 8;\n>  \t\treturn (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) |\n>  \t\t\t\t   ntohl(*((uint32_t *)(index + 4)));\n\nIt's hard to verify that this is right due to all the magic numbers. :)\n\nThis function advances the \"index\" pointer to access the data, and it\nhas already handled the initial header, v2 object names, etc. I think it\nmight be simpler to compute:\n\n  const unsigned char *end = p->index_data + p->index_size;\n\nand compare the computed \"index\" against that. I suspect the earlier\naccesses of \"index\" can also be fooled using integer overflows (e.g.,\nclaim that we have a huge number of objects, and (20 * p->num_objects)\nmay overflow to arbitrary memory, especially on 32-bit systems where\nit's easy to wrap).\n\n-Peff\n"},{"id":"279148","messageId":"20160224110548.GA21620@sigill.intra.peff.net","threadId":"41129","inReplyTo":"xmqqtwmp2e6d.fsf@gitster.mtv.corp.google.com","subject":"Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-24T11:05:48Z","receivedAt":"2016-02-24T11:05:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 07, 2016 at 02:54:50PM -0800, Junio C Hamano wrote:\n\n> We do not check if the offset of individual objects are within the\n> corresponding .pack file, either, and nth_packed_object_offset()\n> does return the data read from .idx file that is not checked for\n> sanity.  use_pack(), which is the helper used by the callers of\n> nth_packed_object_offset() that finds the offset in the packfile for\n> each object, avoids allowing a read access to mapped pack data\n> beyond the end of it, so it is OK to return bogus value that was\n> read from the .idx file from this function, but there is one\n> computation the function itself does using a possibly bogus value\n> read from the disk: to find out where in the secondary offset table\n> in the .idx file the offset in the packfile is stored.\n\nLooks like this topic got dropped. I was reminded of it when somebody\npointed me to a similar case[1] today which segfaults in a similar way (but\nthis time was caused by actual filesystem corruption).\n\nDid you ever push the patch below further along?\n\nI confirmed that your patch detects and complains on this newer case.\nThough I think there is another similar problem in\nread_v2_anomalous_offsets (I haven't dug, but it triggers for me in\n\"index-pack --verify\").\n\n-Peff\n\n[1] https://github.com/libgit2/libgit2/issues/3556\n"},{"id":"279165","messageId":"xmqqegc2t05g.fsf@gitster.mtv.corp.google.com","threadId":"41129","inReplyTo":"20160224110548.GA21620@sigill.intra.peff.net","subject":"Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-24T18:48:27Z","receivedAt":"2016-02-24T18:48:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Jan 07, 2016 at 02:54:50PM -0800, Junio C Hamano wrote:\n>\n>> We do not check if the offset of individual objects are within the\n>> corresponding .pack file, either, and nth_packed_object_offset()\n>> does return the data read from .idx file that is not checked for\n>> sanity.  use_pack(), which is the helper used by the callers of\n>> nth_packed_object_offset() that finds the offset in the packfile for\n>> each object, avoids allowing a read access to mapped pack data\n>> beyond the end of it, so it is OK to return bogus value that was\n>> read from the .idx file from this function, but there is one\n>> computation the function itself does using a possibly bogus value\n>> read from the disk: to find out where in the secondary offset table\n>> in the .idx file the offset in the packfile is stored.\n>\n> Looks like this topic got dropped. I was reminded of it when somebody\n> pointed me to a similar case[1] today which segfaults in a similar way (but\n> this time was caused by actual filesystem corruption).\n>\n> Did you ever push the patch below further along?\n\nI do not think so, as I didn't \"dig\"; I recall trying to be explicit\nthat it was an illustration to prevent two extra and unnecessary\nchanges I alluded to in the earlier parts of the thread, not a real\npatch.\n\n> I confirmed that your patch detects and complains on this newer case.\n> Though I think there is another similar problem in\n> read_v2_anomalous_offsets (I haven't dug, but it triggers for me in\n> \"index-pack --verify\").\n>\n> -Peff\n>\n> [1] https://github.com/libgit2/libgit2/issues/3556\n"},{"id":"279337","messageId":"20160225141245.GA17651@sigill.intra.peff.net","threadId":"41129","inReplyTo":"xmqqegc2t05g.fsf@gitster.mtv.corp.google.com","subject":"Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-25T14:12:46Z","receivedAt":"2016-02-25T14:12:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 24, 2016 at 10:48:27AM -0800, Junio C Hamano wrote:\n\n> >> We do not check if the offset of individual objects are within the\n> >> corresponding .pack file, either, and nth_packed_object_offset()\n> >> does return the data read from .idx file that is not checked for\n> >> sanity.  use_pack(), which is the helper used by the callers of\n> >> nth_packed_object_offset() that finds the offset in the packfile for\n> >> each object, avoids allowing a read access to mapped pack data\n> >> beyond the end of it, so it is OK to return bogus value that was\n> >> read from the .idx file from this function, but there is one\n> >> computation the function itself does using a possibly bogus value\n> >> read from the disk: to find out where in the secondary offset table\n> >> in the .idx file the offset in the packfile is stored.\n> >\n> > Looks like this topic got dropped. I was reminded of it when somebody\n> > pointed me to a similar case[1] today which segfaults in a similar way (but\n> > this time was caused by actual filesystem corruption).\n> >\n> > Did you ever push the patch below further along?\n> \n> I do not think so, as I didn't \"dig\"; I recall trying to be explicit\n> that it was an illustration to prevent two extra and unnecessary\n> changes I alluded to in the earlier parts of the thread, not a real\n> patch.\n\nThanks. I was planning to dig further, but I didn't want to duplicate\nany work. I've got a series which I'll post momentarily.\n\n-Peff\n"}]}