{"thread":{"id":"44153","subject":"Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0","startedAt":"2016-09-25T14:31:20Z","lastAt":"2016-09-27T19:15:00Z","messageCount":22,"participants":["Gustavo Grieco","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"302520","messageId":"1825523389.8224664.1474812766424.JavaMail.zimbra@imag.fr","threadId":"44153","inReplyTo":"1399913289.8224468.1474810664933.JavaMail.zimbra@imag.fr","subject":"Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0","fromName":"Gustavo Grieco","fromEmail":"gustavo.grieco@imag.fr","sentAt":"2016-09-25T14:12:46Z","receivedAt":"2016-09-25T14:31:20Z","isPatch":false,"sender":{"key":"gustavo.grieco@imag.fr","avatar":null},"body":"Hi,\n\nWe found a stack read out-of-bounds parsing object files using git 2.10.0. It was tested on ArchLinux x86_64. To reproduce, first recompile git with ASAN support and then execute:\n\n$ git init ; mkdir -p .git/objects/b2 ; printf 'x' > .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0\n\nFinally you can trigger the bug using several commands from git (other commands that parses all objects will work too), for instance:\n\n$ git fsck\n\nThe ASAN report is here:\n\n==2763==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe16e4a690 at pc 0x0000006fe5dc bp 0x7ffe16e4a530 sp 0x7ffe16e4a520\nREAD of size 1 at 0x7ffe16e4a690 thread T0\n    #0 0x6fe5db in parse_sha1_header_extended /home/g/Work/Code/git-2.10.0/sha1_file.c:1684\n    #1 0x702cd4 in sha1_loose_object_info /home/g/Work/Code/git-2.10.0/sha1_file.c:2660\n    #2 0x70332c in sha1_object_info_extended /home/g/Work/Code/git-2.10.0/sha1_file.c:2696\n    #3 0x7038e0 in sha1_object_info /home/g/Work/Code/git-2.10.0/sha1_file.c:2745\n    #4 0x648498 in parse_object /home/g/Work/Code/git-2.10.0/object.c:260\n    #5 0x48d46d in fsck_sha1 builtin/fsck.c:367\n    #6 0x48da47 in fsck_loose builtin/fsck.c:493\n    #7 0x707514 in for_each_file_in_obj_subdir /home/g/Work/Code/git-2.10.0/sha1_file.c:3477\n    #8 0x70775b in for_each_loose_file_in_objdir_buf /home/g/Work/Code/git-2.10.0/sha1_file.c:3512\n    #9 0x707885 in for_each_loose_file_in_objdir /home/g/Work/Code/git-2.10.0/sha1_file.c:3532\n    #10 0x48dc1d in fsck_object_dir builtin/fsck.c:521\n    #11 0x48e2e6 in cmd_fsck builtin/fsck.c:644\n    #12 0x407a8f in run_builtin /home/g/Work/Code/git-2.10.0/git.c:352\n    #13 0x407e35 in handle_builtin /home/g/Work/Code/git-2.10.0/git.c:539\n    #14 0x408175 in run_argv /home/g/Work/Code/git-2.10.0/git.c:593\n    #15 0x408458 in cmd_main /home/g/Work/Code/git-2.10.0/git.c:665\n    #16 0x53fc70 in main /home/g/Work/Code/git-2.10.0/common-main.c:40\n    #17 0x7f0f46d43290 in __libc_start_main (/usr/lib/libc.so.6+0x20290)\n    #18 0x405209 in _start (/home/g/Work/Code/git-2.10.0/git+0x405209)\n\nAddress 0x7ffe16e4a690 is located in stack of thread T0 at offset 192 in frame\n    #0 0x702834 in sha1_loose_object_info /home/g/Work/Code/git-2.10.0/sha1_file.c:2614\n\n  This frame has 5 object(s):\n    [32, 40) 'mapsize'\n    [96, 120) 'hdrbuf'\n    [160, 192) 'hdr' <== Memory access at offset 192 overflows this variable\n    [224, 368) 'st'\n    [416, 576) 'stream'\nHINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext\n      (longjmp and C++ exceptions *are* supported)\nSUMMARY: AddressSanitizer: stack-buffer-overflow /home/g/Work/Code/git-2.10.0/sha1_file.c:1684 in parse_sha1_header_extended\nShadow bytes around the buggy address:\n  0x100042dc1480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00\n  0x100042dc1490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00\n  0x100042dc14a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00\n  0x100042dc14b0: 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f4\n  0x100042dc14c0: f4 f4 f2 f2 f2 f2 00 00 00 f4 f2 f2 f2 f2 00 00\n=>0x100042dc14d0: 00 00[f2]f2 f2 f2 00 00 00 00 00 00 00 00 00 00\n  0x100042dc14e0: 00 00 00 00 00 00 00 00 f4 f4 f2 f2 f2 f2 00 00\n  0x100042dc14f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00\n  0x100042dc1500: 00 00 f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00\n  0x100042dc1510: 00 00 00 00 f1 f1 f1 f1 04 f4 f4 f4 f2 f2 f2 f2\n  0x100042dc1520: 00 00 00 00 00 f4 f4 f4 f3 f3 f3 f3 00 00 00 00\nShadow byte legend (one shadow byte represents 8 application bytes):\n  Addressable:           00\n  Partially addressable: 01 02 03 04 05 06 07 \n  Heap left redzone:       fa\n  Heap right redzone:      fb\n  Freed heap region:       fd\n  Stack left redzone:      f1\n  Stack mid redzone:       f2\n  Stack right redzone:     f3\n  Stack partial redzone:   f4\n  Stack after return:      f5\n  Stack use after scope:   f8\n  Global redzone:          f9\n  Global init order:       f6\n  Poisoned by user:        f7\n  Container overflow:      fc\n  Array cookie:            ac\n  Intra object redzone:    bb\n  ASan internal:           fe\n  Left alloca redzone:     ca\n  Right alloca redzone:    cb\n\n\nRegards,\nGustavo.\n"},{"id":"302547","messageId":"xmqqbmzbwmfc.fsf@gitster.mtv.corp.google.com","threadId":"44153","inReplyTo":"1825523389.8224664.1474812766424.JavaMail.zimbra@imag.fr","subject":"Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T00:10:31Z","receivedAt":"2016-09-26T00:10:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Gustavo Grieco <gustavo.grieco@imag.fr> writes:\n\n> We found a stack read out-of-bounds parsing object files using git 2.10.0. It was tested on ArchLinux x86_64. To reproduce, first recompile git with ASAN support and then execute:\n>\n> $ git init ; mkdir -p .git/objects/b2 ; printf 'x' > .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0\n\nInteresting.  If you prepare such a broken loose object file in your\nlocal repository, I would expect that either unpack_sha1_header() or\nunpack_sha1_header_to_strbuf() that sha1_loose_object_info() calls\nwould detect and barf by noticing that an error came from libz while\nit attempts to inflate and would not even call parse_sha1_header.\n\nBut it is nevertheless bad to assume that whatever happens to\ninflate without an error must be formatted correctly to allow\nparsing (i.e. has ' ' and then NUL termination within the first 32\nbytes after inflation), which is exactly what the hdr[32] is saying.\n\nPerhaps we need something like the following to tighten the\ncodepath.\n\nNote that this is totally unteseted and not thought through; I\nbriefly thought about what unpack_sha1_header_to_strbuf() does with\nthis change (it first lets unpack_sha1_header() to attempt with a\nsmall buffer but it seems to discard the error code from it before\nseeing if the returned buffer has NUL in it); there may be bad\ninteractions with it.\n\n\n sha1_file.c | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 60ff21f..dfcbd76 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1648,6 +1648,8 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \n int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)\n {\n+\tint status;\n+\n \t/* Get the data stream */\n \tmemset(stream, 0, sizeof(*stream));\n \tstream->next_in = map;\n@@ -1656,7 +1658,15 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma\n \tstream->avail_out = bufsiz;\n \n \tgit_inflate_init(stream);\n-\treturn git_inflate(stream, 0);\n+\tstatus = git_inflate(stream, 0);\n+\tif (status)\n+\t\treturn status;\n+\n+\t/* Make sure we got the terminating NUL for the object header */\n+\tif (!memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n+\t\treturn -1;\n+\n+\treturn 0;\n }\n \n static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,\n@@ -1758,6 +1768,8 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,\n \t\tchar c = *hdr++;\n \t\tif (c == ' ')\n \t\t\tbreak;\n+\t\tif (!c)\n+\t\t\tdie(\"invalid object header\");\n \t\ttype_len++;\n \t}\n \n\n"},{"id":"302551","messageId":"xmqqshsnuvvz.fsf_-_@gitster.mtv.corp.google.com","threadId":"44153","inReplyTo":"xmqqbmzbwmfc.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] unpack_sha1_header(): detect malformed object header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T04:29:04Z","receivedAt":"2016-09-26T04:29:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When opening a loose object file, we often do this sequence:\n\n - prepare a short buffer for the object header (on stack)\n\n - call unpack_sha1_header() and have early part of the object data\n   inflated, enough to fill the buffer\n\n - parse that data in the short buffer, assuming that the first part\n   of the object is <type> SP <length> NUL\n\nNobody in this sequence however actually verifies that the loop that\ntries to find SP that must come after the typename or NUL that must\ncome after the length exist in the inflated data.  Because the\nparsing function parse_sha1_header_extended() is not even given the\nnumber of bytes inflated into the header buffer, it can easily read\npast it, looking for the SP byte that may not even exist.\n\nA variant recently introduced to support \"--allow-unknown-type\"\noption of \"git cat-file -t\" changes the second step to use\nunpack_sha1_header_to_strbuf(), but the story is essentially the\nsame.  It did check to see if it saw enough to include NUL, but\nnobody checked for SP before calling the parsing function.\n\nTo correct this, do these three things:\n\n - rename unpack_sha1_header() to unpack_sha1_short_header() and\n   have unpack_sha1_header_to_strbuf() keep calling that as its\n   helper function.  This will detect and report zlib errors, but is\n   not aware of the format of a loose object (as before).\n\n - introduce unpack_sha1_header() that calls the same helper\n   function, and when zlib reports it inflated OK into the buffer,\n   check if the buffer has both SP and NUL in this order.  This\n   would ensure that parsing function will terminate within the\n   buffer that holds the inflated header.\n\n - update unpack_sha1_header_to_strbuf() to check if the resulting\n   buffer has both SP and NUL in this order for the same effect.\n\nReported-by: Gustavo Grieco <gustavo.grieco@imag.fr>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Unlike the \"something like this\" version, this does the \"we got\n   some data, does it look like an object header, safely parseable\n   by our parser?\" check in the unpack code, without touching the\n   parser, as I think that division of labor between the unpacker\n   and the parser makes more sense.\n\n   The strbuf codepath came in 46f03448 (\"sha1_file: support reading\n   from a loose object of unknown type\", 2015-05-03) by Karthik,\n   whose log says it was written by me, and helped by Peff, so I'm\n   asking these two to lend their eyes.\n\n sha1_file.c | 40 +++++++++++++++++++++++++++++++++++++---\n 1 file changed, 37 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b9c1fa3..445e763 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1646,7 +1646,9 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \treturn used;\n }\n \n-int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)\n+static int unpack_sha1_short_header(git_zstream *stream,\n+\t\t\t\t    unsigned char *map, unsigned long mapsize,\n+\t\t\t\t    void *buffer, unsigned long bufsiz)\n {\n \t/* Get the data stream */\n \tmemset(stream, 0, sizeof(*stream));\n@@ -1659,13 +1661,37 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma\n \treturn git_inflate(stream, 0);\n }\n \n+int unpack_sha1_header(git_zstream *stream,\n+\t\t       unsigned char *map, unsigned long mapsize,\n+\t\t       void *buffer, unsigned long bufsiz)\n+{\n+\tconst char *eoh;\n+\tint status = unpack_sha1_short_header(stream, map, mapsize,\n+\t\t\t\t\t      buffer, bufsiz);\n+\n+\tif (status < Z_OK)\n+\t\treturn status;\n+\n+\t/* Make sure we have the terminating NUL */\n+\teoh = memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer);\n+\tif (!eoh)\n+\t\treturn -1;\n+\t/* Make sure we have ' ' at the end of type */\n+\tif (!memchr(buffer, ' ', eoh - (const char *)buffer))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,\n \t\t\t\t\tunsigned long mapsize, void *buffer,\n \t\t\t\t\tunsigned long bufsiz, struct strbuf *header)\n {\n+\tconst char *eoh;\n \tint status;\n \n-\tstatus = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);\n+\tstatus = unpack_sha1_short_header(stream, map, mapsize, buffer, bufsiz);\n+\tif (status < Z_OK)\n+\t\treturn -1;\n \n \t/*\n \t * Check if entire header is unpacked in the first iteration.\n@@ -1686,11 +1712,19 @@ static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,\n \t\tstatus = git_inflate(stream, 0);\n \t\tstrbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);\n \t\tif (memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n-\t\t\treturn 0;\n+\t\t\tgoto enough;\n \t\tstream->next_out = buffer;\n \t\tstream->avail_out = bufsiz;\n \t} while (status != Z_STREAM_END);\n \treturn -1;\n+\n+enough:\n+\teoh = memchr(header->buf, '\\0', header->len);\n+\tif (!eoh)\n+\t\tdie(\"BUG: the NUL we earlier saw is gone???\");\n+\tif (!memchr(header->buf, ' ', eoh - header->buf))\n+\t\treturn -1;\n+\treturn 0;\n }\n \n static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long size, const unsigned char *sha1)\n"},{"id":"302575","messageId":"20160926135055.zeagw6mj3udzhv7s@sigill.intra.peff.net","threadId":"44153","inReplyTo":"xmqqbmzbwmfc.fsf@gitster.mtv.corp.google.com","subject":"Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-26T13:50:55Z","receivedAt":"2016-09-26T13:51:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 25, 2016 at 05:10:31PM -0700, Junio C Hamano wrote:\n\n> Gustavo Grieco <gustavo.grieco@imag.fr> writes:\n> \n> > We found a stack read out-of-bounds parsing object files using git 2.10.0. It was tested on ArchLinux x86_64. To reproduce, first recompile git with ASAN support and then execute:\n> >\n> > $ git init ; mkdir -p .git/objects/b2 ; printf 'x' > .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0\n> \n> Interesting.  If you prepare such a broken loose object file in your\n> local repository, I would expect that either unpack_sha1_header() or\n> unpack_sha1_header_to_strbuf() that sha1_loose_object_info() calls\n> would detect and barf by noticing that an error came from libz while\n> it attempts to inflate and would not even call parse_sha1_header.\n> \n> But it is nevertheless bad to assume that whatever happens to\n> inflate without an error must be formatted correctly to allow\n> parsing (i.e. has ' ' and then NUL termination within the first 32\n> bytes after inflation), which is exactly what the hdr[32] is saying.\n\nYeah. I also was surprised that we didn't barf on a zlib failure. But\nbased on previous debugging of corrupted zlib data, my recollection\nis that there are a large number of weird corruptions that zlib will\nhappily pass back and only later complain about a checksum error. So\npresumably \"x\" is one of those, and it might not hold for other\ncorruptions (but I didn't try).\n\n> Note that this is totally unteseted and not thought through; I\n> briefly thought about what unpack_sha1_header_to_strbuf() does with\n> this change (it first lets unpack_sha1_header() to attempt with a\n> small buffer but it seems to discard the error code from it before\n> seeing if the returned buffer has NUL in it); there may be bad\n> interactions with it.\n\nYeah, that seems wrong. I don't think it would involve an out of bounds\nread, but we probably could fail to correctly report zlib corruption.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 60ff21f..dfcbd76 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1648,6 +1648,8 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n>  \n>  int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)\n>  {\n> +\tint status;\n> +\n>  \t/* Get the data stream */\n>  \tmemset(stream, 0, sizeof(*stream));\n>  \tstream->next_in = map;\n> @@ -1656,7 +1658,15 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma\n>  \tstream->avail_out = bufsiz;\n>  \n>  \tgit_inflate_init(stream);\n> -\treturn git_inflate(stream, 0);\n> +\tstatus = git_inflate(stream, 0);\n> +\tif (status)\n> +\t\treturn status;\n> +\n> +\t/* Make sure we got the terminating NUL for the object header */\n> +\tif (!memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n> +\t\treturn -1;\n> +\n> +\treturn 0;\n\nThis doesn't look too invasive as an approach, though I would have done\nit differently. We're making the assumption that once there is a NUL,\nthe header-parser won't do anything stupid, which creates a coupling\nbetween those two bits of code. My inclination would have been to just\ntreat the header as a ptr/len pair, and make sure the parser never reads\npast the end.\n\nBut I implemented that, and it _is_ rather invasive. And it's not like\ncoupling unpack_sha1_header() and parse_sha1_header() is all that\nterrible; they are meant to be paired.\n\nI haven't read through your follow-up yet; I'll do that before posting\nmy version.\n\n>  static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,\n> @@ -1758,6 +1768,8 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,\n>  \t\tchar c = *hdr++;\n>  \t\tif (c == ' ')\n>  \t\t\tbreak;\n> +\t\tif (!c)\n> +\t\t\tdie(\"invalid object header\");\n>  \t\ttype_len++;\n>  \t}\n\nWe keep reading from hdr after this, though I think those bits would all\nbail correctly on seeing NUL.\n\n-Peff\n"},{"id":"302576","messageId":"20160926140309.l2h4b65gpqyutepn@sigill.intra.peff.net","threadId":"44153","inReplyTo":"xmqqshsnuvvz.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] unpack_sha1_header(): detect malformed object header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-26T14:03:09Z","receivedAt":"2016-09-26T14:03:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 25, 2016 at 09:29:04PM -0700, Junio C Hamano wrote:\n\n> To correct this, do these three things:\n> \n>  - rename unpack_sha1_header() to unpack_sha1_short_header() and\n>    have unpack_sha1_header_to_strbuf() keep calling that as its\n>    helper function.  This will detect and report zlib errors, but is\n>    not aware of the format of a loose object (as before).\n\nThis step makes sense to me, and is a problem in the original you posted\n(i.e., we may not see all of the header in the strbuf variant). Your\nrefactor looks good.\n\n>  - introduce unpack_sha1_header() that calls the same helper\n>    function, and when zlib reports it inflated OK into the buffer,\n>    check if the buffer has both SP and NUL in this order.  This\n>    would ensure that parsing function will terminate within the\n>    buffer that holds the inflated header.\n> \n>  - update unpack_sha1_header_to_strbuf() to check if the resulting\n>    buffer has both SP and NUL in this order for the same effect.\n\nThis part I don't understand, though. We clearly need to look for the\nNUL. But why do we need to look for the space? The loop in\nparse_sha1_header() can easily detect this as it looks for the end of\nthe type name (and if it hits the end-of-string, can bail as in your\noriginal patch).\n\nI.e., the root of the problem is that we pass parse_sha1_header() a the\n\"ptr\" half of a ptr/len buffer, and it has no idea how much we read.\nBut once we get it that information (either by passing the length, or by\nensuring that the buffer is NUL-terminated, it should be easy for it to\ndo the right thing.\n\nAnyway, here's my ptr/len version (which passes the length back out of\nunpack_sha1_header via an in/out pointer). After thinking on it, though,\nI'm of the opinion that we're better off just ensuring that \"hdr\" is\nNUL-terminated. We end up assuming that anyway later, since we have to\nknow how much of the header buffer was consumed by parsing.\n\nDo note the final call below in the streaming loose-open code, which\nexhibits that, but also seems to call parse_sha1_header() without\nchecking its return value. I think that needs fixed regardless of the\napproach.\n\n---\ndiff --git a/cache.h b/cache.h\nindex d0494c8..e89dcff 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1121,8 +1121,9 @@ extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned c\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n extern int git_open_noatime(const char *name);\n extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);\n-extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);\n-extern int parse_sha1_header(const char *hdr, unsigned long *sizep);\n+extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned\n+\t\t\t      long mapsize, void *buffer, size_t *bufsiz);\n+extern int parse_sha1_header(const char *hdr, size_t len, unsigned long *sizep);\n \n /* global flag to enable extra checks when accessing packed objects */\n extern int do_check_packed_object_crc;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b9c1fa3..326593b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1646,41 +1646,50 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \treturn used;\n }\n \n-int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)\n+int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize,\n+\t\t       void *buffer, size_t *bufsiz)\n {\n+\tint ret;\n+\n \t/* Get the data stream */\n \tmemset(stream, 0, sizeof(*stream));\n \tstream->next_in = map;\n \tstream->avail_in = mapsize;\n \tstream->next_out = buffer;\n-\tstream->avail_out = bufsiz;\n+\tstream->avail_out = *bufsiz;\n \n \tgit_inflate_init(stream);\n-\treturn git_inflate(stream, 0);\n+\tret = git_inflate(stream, 0);\n+\t*bufsiz -= stream->avail_out;\n+\treturn ret;\n }\n \n-static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,\n-\t\t\t\t\tunsigned long mapsize, void *buffer,\n-\t\t\t\t\tunsigned long bufsiz, struct strbuf *header)\n+static int unpack_sha1_header_to_strbuf(git_zstream *stream,\n+\t\t\t\t\tunsigned char *map, unsigned long mapsize,\n+\t\t\t\t\tvoid *buffer, size_t *bufsiz,\n+\t\t\t\t\tstruct strbuf *header)\n {\n+\tsize_t initial_len = *bufsiz;\n \tint status;\n \n-\tstatus = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);\n+\tstatus = unpack_sha1_header(stream, map, mapsize, buffer, &initial_len);\n \n \t/*\n \t * Check if entire header is unpacked in the first iteration.\n \t */\n-\tif (memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n+\tif (memchr(buffer, '\\0', initial_len)) {\n+\t\t*bufsiz = initial_len;\n \t\treturn 0;\n+\t}\n \n \t/*\n \t * buffer[0..bufsiz] was not large enough.  Copy the partial\n \t * result out to header, and then append the result of further\n \t * reading the stream.\n \t */\n-\tstrbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);\n+\tstrbuf_add(header, buffer, initial_len);\n \tstream->next_out = buffer;\n-\tstream->avail_out = bufsiz;\n+\tstream->avail_out = *bufsiz;\n \n \tdo {\n \t\tstatus = git_inflate(stream, 0);\n@@ -1688,7 +1697,7 @@ static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,\n \t\tif (memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n \t\t\treturn 0;\n \t\tstream->next_out = buffer;\n-\t\tstream->avail_out = bufsiz;\n+\t\tstream->avail_out = *bufsiz;\n \t} while (status != Z_STREAM_END);\n \treturn -1;\n }\n@@ -1743,9 +1752,11 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s\n  * too permissive for what we want to check. So do an anal\n  * object header parse by hand.\n  */\n-static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,\n-\t\t\t       unsigned int flags)\n+static int parse_sha1_header_extended(const char *hdr, size_t len,\n+\t\t\t\t      struct object_info *oi,\n+\t\t\t\t      unsigned int flags)\n {\n+\tconst char *end = hdr + len;\n \tconst char *type_buf = hdr;\n \tunsigned long size;\n \tint type, type_len = 0;\n@@ -1754,12 +1765,14 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,\n \t * The type can be of any size but is followed by\n \t * a space.\n \t */\n-\tfor (;;) {\n+\twhile (hdr < end) {\n \t\tchar c = *hdr++;\n \t\tif (c == ' ')\n \t\t\tbreak;\n \t\ttype_len++;\n \t}\n+\tif (hdr >= end)\n+\t\treturn -1;\n \n \ttype = type_from_string_gently(type_buf, type_len, 1);\n \tif (oi->typename)\n@@ -1781,10 +1794,10 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,\n \t * decimal format (ie \"010\" is not valid).\n \t */\n \tsize = *hdr++ - '0';\n-\tif (size > 9)\n+\tif (size > 9 || hdr >= end)\n \t\treturn -1;\n \tif (size) {\n-\t\tfor (;;) {\n+\t\twhile (hdr >= end) {\n \t\t\tunsigned long c = *hdr - '0';\n \t\t\tif (c > 9)\n \t\t\t\tbreak;\n@@ -1799,17 +1812,17 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,\n \t/*\n \t * The length must be followed by a zero byte\n \t */\n-\treturn *hdr ? -1 : type;\n+\treturn hdr >= end || *hdr ? -1 : type;\n }\n \n-int parse_sha1_header(const char *hdr, unsigned long *sizep)\n+int parse_sha1_header(const char *hdr, size_t len, unsigned long *sizep)\n {\n \tstruct object_info oi;\n \n \toi.sizep = sizep;\n \toi.typename = NULL;\n \toi.typep = NULL;\n-\treturn parse_sha1_header_extended(hdr, &oi, LOOKUP_REPLACE_OBJECT);\n+\treturn parse_sha1_header_extended(hdr, len, &oi, LOOKUP_REPLACE_OBJECT);\n }\n \n static void *unpack_sha1_file(void *map, unsigned long mapsize, enum object_type *type, unsigned long *size, const unsigned char *sha1)\n@@ -1817,9 +1830,11 @@ static void *unpack_sha1_file(void *map, unsigned long mapsize, enum object_type\n \tint ret;\n \tgit_zstream stream;\n \tchar hdr[8192];\n+\tsize_t hdr_len = sizeof(hdr);\n \n-\tret = unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr));\n-\tif (ret < Z_OK || (*type = parse_sha1_header(hdr, size)) < 0)\n+\tret = unpack_sha1_header(&stream, map, mapsize, hdr, &hdr_len);\n+\tif (ret < Z_OK ||\n+\t    (*type = parse_sha1_header(hdr, hdr_len, size)) < 0)\n \t\treturn NULL;\n \n \treturn unpack_sha1_rest(&stream, hdr, *size, sha1);\n@@ -2697,6 +2712,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[32];\n+\tsize_t hdr_len = sizeof(hdr);\n \tstruct strbuf hdrbuf = STRBUF_INIT;\n \n \tif (oi->delta_base_sha1)\n@@ -2725,19 +2741,19 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \tif (oi->disk_sizep)\n \t\t*oi->disk_sizep = mapsize;\n \tif ((flags & LOOKUP_UNKNOWN_OBJECT)) {\n-\t\tif (unpack_sha1_header_to_strbuf(&stream, map, mapsize, hdr, sizeof(hdr), &hdrbuf) < 0)\n+\t\tif (unpack_sha1_header_to_strbuf(&stream, map, mapsize, hdr, &hdr_len, &hdrbuf) < 0)\n \t\t\tstatus = error(\"unable to unpack %s header with --allow-unknown-type\",\n \t\t\t\t       sha1_to_hex(sha1));\n-\t} else if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n+\t} else if (unpack_sha1_header(&stream, map, mapsize, hdr, &hdr_len) < 0)\n \t\tstatus = error(\"unable to unpack %s header\",\n \t\t\t       sha1_to_hex(sha1));\n \tif (status < 0)\n \t\t; /* Do nothing */\n \telse if (hdrbuf.len) {\n-\t\tif ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)\n+\t\tif ((status = parse_sha1_header_extended(hdrbuf.buf, hdrbuf.len, oi, flags)) < 0)\n \t\t\tstatus = error(\"unable to parse %s header with --allow-unknown-type\",\n \t\t\t\t       sha1_to_hex(sha1));\n-\t} else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)\n+\t} else if ((status = parse_sha1_header_extended(hdr, hdr_len, oi, flags)) < 0)\n \t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n \tgit_inflate_end(&stream);\n \tmunmap(map, mapsize);\ndiff --git a/streaming.c b/streaming.c\nindex 3c48f04..ee73544 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -334,6 +334,7 @@ static struct stream_vtbl loose_vtbl = {\n \n static open_method_decl(loose)\n {\n+\tsize_t len = sizeof(st->u.loose.hdr);\n \tst->u.loose.mapped = map_sha1_file(sha1, &st->u.loose.mapsize);\n \tif (!st->u.loose.mapped)\n \t\treturn -1;\n@@ -341,13 +342,14 @@ static open_method_decl(loose)\n \t\t\t       st->u.loose.mapped,\n \t\t\t       st->u.loose.mapsize,\n \t\t\t       st->u.loose.hdr,\n-\t\t\t       sizeof(st->u.loose.hdr)) < 0) {\n+\t\t\t       &len) < 0) {\n \t\tgit_inflate_end(&st->z);\n \t\tmunmap(st->u.loose.mapped, st->u.loose.mapsize);\n \t\treturn -1;\n \t}\n \n-\tparse_sha1_header(st->u.loose.hdr, &st->size);\n+\tif (parse_sha1_header(st->u.loose.hdr, len, &st->size) < 0)\n+\t\treturn -1;\n \tst->u.loose.hdr_used = strlen(st->u.loose.hdr) + 1;\n \tst->u.loose.hdr_avail = st->z.total_out;\n \tst->z_state = z_used;\n"},{"id":"302581","messageId":"xmqqfuomvdqe.fsf@gitster.mtv.corp.google.com","threadId":"44153","inReplyTo":"20160926140309.l2h4b65gpqyutepn@sigill.intra.peff.net","subject":"Re: [PATCH] unpack_sha1_header(): detect malformed object header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T16:15:53Z","receivedAt":"2016-09-26T16:16:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This part I don't understand, though. We clearly need to look for the\n> NUL. But why do we need to look for the space? The loop in\n> parse_sha1_header() can easily detect this as it looks for the end of\n> the type name (and if it hits the end-of-string, can bail as in your\n> original patch).\n> I.e., the root of the problem is that we pass parse_sha1_header() a the\n> \"ptr\" half of a ptr/len buffer, and it has no idea how much we read.\n> But once we get it that information (either by passing the length, or by\n> ensuring that the buffer is NUL-terminated, it should be easy for it to\n> do the right thing.\n\nYup.\n\n> Anyway, here's my ptr/len version (which passes the length back out of\n> unpack_sha1_header via an in/out pointer). After thinking on it, though,\n> I'm of the opinion that we're better off just ensuring that \"hdr\" is\n> NUL-terminated. We end up assuming that anyway later, since we have to\n> know how much of the header buffer was consumed by parsing.\n\nI'd agree, not because I didn't first go in this <ptr,len> route\nmyself, but because the attached change does look quite invasive.\nAlso, I think it is OK to ask unpack_*_header() to fail if what it\nturns can no way be a header, e.g. lacks NUL termination.\n\n> Do note the final call below in the streaming loose-open code, which\n> exhibits that, but also seems to call parse_sha1_header() without\n> checking its return value. I think that needs fixed regardless of the\n> approach.\n\nGood that your attempt to signature-changing change caught it.  I'll\ntake a further look.\n\nThanks.\n"},{"id":"302594","messageId":"xmqqshsmtvju.fsf@gitster.mtv.corp.google.com","threadId":"44153","inReplyTo":"xmqqfuomvdqe.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] unpack_sha1_header(): detect malformed object header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T17:33:57Z","receivedAt":"2016-09-26T17:34:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Good that your attempt to signature-changing change caught it.  I'll\n> take a further look.\n\nSo here are two patch series.  The first one makes sure all callers\nof parse_sha1_header() check the returned status.\n\n-- >8 --\nFrom: Junio C Hamano <gitster@pobox.com>\nDate: Mon, 26 Sep 2016 09:23:41 -0700\nSubject: [PATCH 1/2] streaming: make sure to notice corrupt object\n\nThe streaming read interface from a loose object called\nparse_sha1_header() but discarded its return value, without noticing\na potential error.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n streaming.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/streaming.c b/streaming.c\nindex 811fcc2..884a8f1 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -347,7 +347,8 @@ static open_method_decl(loose)\n \t\treturn -1;\n \t}\n \n-\tparse_sha1_header(st->u.loose.hdr, &st->size);\n+\tif (parse_sha1_header(st->u.loose.hdr, &st->size) < 0)\n+\t\treturn -1;\n \tst->u.loose.hdr_used = strlen(st->u.loose.hdr) + 1;\n \tst->u.loose.hdr_avail = st->z.total_out;\n \tst->z_state = z_used;\n-- \n2.10.0-533-ga18d90d\n\n"},{"id":"302596","messageId":"xmqqoa3atviv.fsf@gitster.mtv.corp.google.com","threadId":"44153","inReplyTo":"xmqqfuomvdqe.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] unpack_sha1_header(): detect malformed object header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T17:34:32Z","receivedAt":"2016-09-26T17:34:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"And the second one, that no longer checks SP in unpacker, looks like\nthis.\n\n-- >8 --\nSubject: [PATCH] unpack_sha1_header(): detect malformed object header\n\nWhen opening a loose object file, we often do this sequence:\n\n - prepare a short buffer for the object header (on stack)\n\n - call unpack_sha1_header() and have early part of the object data\n   inflated, enough to fill the buffer\n\n - parse that data in the short buffer, assuming that the first part\n   of the object is <typename> SP <length> NUL\n\nBecause the parsing function parse_sha1_header_extended() is not\ngiven the number of bytes inflated into the header buffer, it you\ncraft a file whose early part inflates a garbage sequence without SP\nor NUL, and replace a loose object with it, it will end up reading\npast the end of the inflated data.\n\nTo correct this, do the following four things:\n\n - rename unpack_sha1_header() to unpack_sha1_short_header() and\n   have unpack_sha1_header_to_strbuf() keep calling that as its\n   helper function.  This will detect and report zlib errors, but is\n   not aware of the format of a loose object (as before).\n\n - introduce unpack_sha1_header() that calls the same helper\n   function, and when zlib reports it inflated OK into the buffer,\n   check if the inflated data has NUL.  This would ensure that\n   parsing function will terminate within the buffer that holds the\n   inflated header.\n\n - update unpack_sha1_header_to_strbuf() to check if the resulting\n   buffer has NUL for the same effect.\n\n - update parse_sha1_header_extended() to make sure that its loop to\n   find the SP that terminates the <typename> stops at NUL.\n\nEssentially, this makes unpack_*() functions that are asked to\nunpack a loose object header to be a bit more strict and detect an\ninput that cannot possibly be a valid object header, even before the\nparsing function kicks in.\n\nReported-by: Gustavo Grieco <gustavo.grieco@imag.fr>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n sha1_file.c | 26 ++++++++++++++++++++++++--\n 1 file changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 17262e1..f7054d3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1566,7 +1566,9 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \treturn used;\n }\n \n-int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)\n+static int unpack_sha1_short_header(git_zstream *stream,\n+\t\t\t\t    unsigned char *map, unsigned long mapsize,\n+\t\t\t\t    void *buffer, unsigned long bufsiz)\n {\n \t/* Get the data stream */\n \tmemset(stream, 0, sizeof(*stream));\n@@ -1579,13 +1581,31 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma\n \treturn git_inflate(stream, 0);\n }\n \n+int unpack_sha1_header(git_zstream *stream,\n+\t\t       unsigned char *map, unsigned long mapsize,\n+\t\t       void *buffer, unsigned long bufsiz)\n+{\n+\tint status = unpack_sha1_short_header(stream, map, mapsize,\n+\t\t\t\t\t      buffer, bufsiz);\n+\n+\tif (status < Z_OK)\n+\t\treturn status;\n+\n+\t/* Make sure we have the terminating NUL */\n+\tif (!memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,\n \t\t\t\t\tunsigned long mapsize, void *buffer,\n \t\t\t\t\tunsigned long bufsiz, struct strbuf *header)\n {\n \tint status;\n \n-\tstatus = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);\n+\tstatus = unpack_sha1_short_header(stream, map, mapsize, buffer, bufsiz);\n+\tif (status < Z_OK)\n+\t\treturn -1;\n \n \t/*\n \t * Check if entire header is unpacked in the first iteration.\n@@ -1676,6 +1696,8 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,\n \t */\n \tfor (;;) {\n \t\tchar c = *hdr++;\n+\t\tif (!c)\n+\t\t\treturn -1;\n \t\tif (c == ' ')\n \t\t\tbreak;\n \t\ttype_len++;\n-- \n2.10.0-533-ga18d90d\n\n"},{"id":"302597","messageId":"20160926173540.j4rxcks3pmdcyvtd@sigill.intra.peff.net","threadId":"44153","inReplyTo":"xmqqshsmtvju.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] unpack_sha1_header(): detect malformed object header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-26T17:35:41Z","receivedAt":"2016-09-26T17:35:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 26, 2016 at 10:33:57AM -0700, Junio C Hamano wrote:\n\n> -- >8 --\n> From: Junio C Hamano <gitster@pobox.com>\n> Date: Mon, 26 Sep 2016 09:23:41 -0700\n> Subject: [PATCH 1/2] streaming: make sure to notice corrupt object\n> \n> The streaming read interface from a loose object called\n> parse_sha1_header() but discarded its return value, without noticing\n> a potential error.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  streaming.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/streaming.c b/streaming.c\n> index 811fcc2..884a8f1 100644\n> --- a/streaming.c\n> +++ b/streaming.c\n> @@ -347,7 +347,8 @@ static open_method_decl(loose)\n>  \t\treturn -1;\n>  \t}\n>  \n> -\tparse_sha1_header(st->u.loose.hdr, &st->size);\n> +\tif (parse_sha1_header(st->u.loose.hdr, &st->size) < 0)\n> +\t\treturn -1;\n\nDo you have to git_inflate_end() and munmap() here, as the error path\nabove does (this was missing from my patch, too)?\n\n-Peff\n"},{"id":"302599","messageId":"20160926173815.rnup2fqynrmja4x5@sigill.intra.peff.net","threadId":"44153","inReplyTo":"xmqqoa3atviv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] unpack_sha1_header(): detect malformed object header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-26T17:38:16Z","receivedAt":"2016-09-26T17:38:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 26, 2016 at 10:34:32AM -0700, Junio C Hamano wrote:\n\n> And the second one, that no longer checks SP in unpacker, looks like\n> this.\n\nThis looks good from a cursory read (but I am about to go to sleep, so\nmight be a bit less careful than usual :) ).\n\n-Peff\n"},{"id":"302601","messageId":"xmqqbmzatv9u.fsf@gitster.mtv.corp.google.com","threadId":"44153","inReplyTo":"20160926173540.j4rxcks3pmdcyvtd@sigill.intra.peff.net","subject":"Re: [PATCH] unpack_sha1_header(): detect malformed object header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T17:39:57Z","receivedAt":"2016-09-26T17:40:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> diff --git a/streaming.c b/streaming.c\n>> index 811fcc2..884a8f1 100644\n>> --- a/streaming.c\n>> +++ b/streaming.c\n>> @@ -347,7 +347,8 @@ static open_method_decl(loose)\n>>  \t\treturn -1;\n>>  \t}\n>>  \n>> -\tparse_sha1_header(st->u.loose.hdr, &st->size);\n>> +\tif (parse_sha1_header(st->u.loose.hdr, &st->size) < 0)\n>> +\t\treturn -1;\n>\n> Do you have to git_inflate_end() and munmap() here, as the error path\n> above does (this was missing from my patch, too)?\n\nAh, definitely.  We'd need to be consistent; otherwise we'd be\neither leaking resources (or existing one double-freeing).\n"},{"id":"302602","messageId":"790613313.8353074.1474912139102.JavaMail.zimbra@imag.fr","threadId":"44153","inReplyTo":"xmqqbmzbwmfc.fsf@gitster.mtv.corp.google.com","subject":"Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0","fromName":"Gustavo Grieco","fromEmail":"gustavo.grieco@imag.fr","sentAt":"2016-09-26T17:48:59Z","receivedAt":"2016-09-26T17:49:14Z","isPatch":false,"sender":{"key":"gustavo.grieco@imag.fr","avatar":null},"body":"Hello,\n\nNow that the cause of this issue is identified, i would like to know if there is an impact in the security, so i can request a CVE if necessary.\n\nThanks!\n"},{"id":"302604","messageId":"xmqq37kmtukf.fsf@gitster.mtv.corp.google.com","threadId":"44153","inReplyTo":"790613313.8353074.1474912139102.JavaMail.zimbra@imag.fr","subject":"Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T17:55:12Z","receivedAt":"2016-09-26T17:55:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Gustavo Grieco <gustavo.grieco@imag.fr> writes:\n\n> Now that the cause of this issue is identified, i would like to\n> know if there is an impact in the security, so i can request a CVE\n> if necessary.\n\nI am inclined to say that it has no security implications.  You have\nto be able to write a bogus loose object in an object store you\nalready have write access to in the first place, in order to cause\nthis read-only access that goes beyond what is allocated, so at the\nworst, what you can do is to hurt yourself, and you can already hurt\nyourself in various other ways.\n"},{"id":"302605","messageId":"48506427.8357405.1474912907126.JavaMail.zimbra@imag.fr","threadId":"44153","inReplyTo":"xmqq37kmtukf.fsf@gitster.mtv.corp.google.com","subject":"Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0","fromName":"Gustavo Grieco","fromEmail":"gustavo.grieco@imag.fr","sentAt":"2016-09-26T18:01:47Z","receivedAt":"2016-09-26T18:01:59Z","isPatch":false,"sender":{"key":"gustavo.grieco@imag.fr","avatar":null},"body":"Fair enough. We are testing our tool to try to find bugs/vulnerabilities in several git implementations. I will report here my results if i can find some other memory issue in this git client.\n\n----- Original Message -----\n> Gustavo Grieco <gustavo.grieco@imag.fr> writes:\n> \n> > Now that the cause of this issue is identified, i would like to\n> > know if there is an impact in the security, so i can request a CVE\n> > if necessary.\n> \n> I am inclined to say that it has no security implications.  You have\n> to be able to write a bogus loose object in an object store you\n> already have write access to in the first place, in order to cause\n> this read-only access that goes beyond what is allocated, so at the\n> worst, what you can do is to hurt yourself, and you can already hurt\n> yourself in various other ways.\n> \n"},{"id":"302606","messageId":"xmqqy42esfh2.fsf@gitster.mtv.corp.google.com","threadId":"44153","inReplyTo":"48506427.8357405.1474912907126.JavaMail.zimbra@imag.fr","subject":"Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T18:06:33Z","receivedAt":"2016-09-26T18:06:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Gustavo Grieco <gustavo.grieco@imag.fr> writes:\n\n> Fair enough. We are testing our tool to try to find\n> bugs/vulnerabilities in several git implementations. I will report\n> here my results if i can find some other memory issue in this git\n> client.\n\nThanks.  With or without security implications, it is basic codebase\nhygiene to identify and correct these issues, and your help is\nhighly appreciated.\n"},{"id":"302607","messageId":"xmqqtwd2sf9t.fsf@gitster.mtv.corp.google.com","threadId":"44153","inReplyTo":"xmqq37kmtukf.fsf@gitster.mtv.corp.google.com","subject":"Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T18:10:54Z","receivedAt":"2016-09-26T18:11:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I am inclined to say that it has no security implications.  You have\n> to be able to write a bogus loose object in an object store you\n> already have write access to in the first place, in order to cause\n> this ...\n\nNote that you could social-engineer others to fetch from you and\nfeed a small enough update that results in loose objects created in\ntheir repositories, without you having a direct write access to the\nrepository.\n\nThe codepath under discussion in this thread however cannot be used\nas an attack vector via that route, because the \"fetch from\nelsewhere\" codepath runs verification of the incoming data stream\nbefore storing the results (either in loose object files, or in a\npackfile) on disk.\n\n"},{"id":"302668","messageId":"140080419.8376708.1474942402912.JavaMail.zimbra@imag.fr","threadId":"44153","inReplyTo":"xmqqtwd2sf9t.fsf@gitster.mtv.corp.google.com","subject":"Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0","fromName":"Gustavo Grieco","fromEmail":"gustavo.grieco@imag.fr","sentAt":"2016-09-27T02:13:22Z","receivedAt":"2016-09-27T02:13:37Z","isPatch":false,"sender":{"key":"gustavo.grieco@imag.fr","avatar":null},"body":"Btw, this other test case will trigger a similar issue, but in another line of code:\n\nTo reproduce: \n\n$ git init ; mkdir -p .git/objects/b2 ; printf 'eJwNwoENgDAIBECkDsII5Z8CHagLGPePXu59zjHGRIOZG3OzI/lnRc4KemXDPdYSml6iQ+4ATIZ+nAEK4g==' | base64 -d > .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0\n\nThen:\n\n$ git fsck\n\nnotice: HEAD points to an unborn branch (master)\n=================================================================\n==24569==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe7645fda0 at pc 0x0000006fe799 bp 0x7ffe7645fc40 sp 0x7ffe7645fc30\nREAD of size 1 at 0x7ffe7645fda0 thread T0\n    #0 0x6fe798 in parse_sha1_header_extended /home/g/Work/Code/git-2.10.0/sha1_file.c:1714\n...\n\nIt will be nice to test the current patch.\n\n----- Original Message -----\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > I am inclined to say that it has no security implications.  You have\n> > to be able to write a bogus loose object in an object store you\n> > already have write access to in the first place, in order to cause\n> > this ...\n> \n> Note that you could social-engineer others to fetch from you and\n> feed a small enough update that results in loose objects created in\n> their repositories, without you having a direct write access to the\n> repository.\n> \n> The codepath under discussion in this thread however cannot be used\n> as an attack vector via that route, because the \"fetch from\n> elsewhere\" codepath runs verification of the incoming data stream\n> before storing the results (either in loose object files, or in a\n> packfile) on disk.\n> \n> \n"},{"id":"302669","messageId":"381383122.8376940.1474943423005.JavaMail.zimbra@imag.fr","threadId":"44153","inReplyTo":"1825523389.8224664.1474812766424.JavaMail.zimbra@imag.fr","subject":"Possible integer overflow parsing malformed objects in git 2.10.0","fromName":"Gustavo Grieco","fromEmail":"gustavo.grieco@imag.fr","sentAt":"2016-09-27T02:30:23Z","receivedAt":"2016-09-27T02:30:32Z","isPatch":false,"sender":{"key":"gustavo.grieco@imag.fr","avatar":null},"body":"Hi,\n\nWe found a malformed object file that triggers an allocation with a negative size when parsed in git 2.10.0. It can be caused by an integer overflow somewhere, so it is better to verify how the code got such value. It was tested on ArchLinux x86_64. To reproduce, first recompile git with ASAN support and then execute:\n\n$ git init ; mkdir -p .git/objects/b2 ; printf 'eJyVT8ERAjEIXKiEBpyBHJdcCroGHAvQjyX49m1ZtmADQjL68uMnZFnYZU/HfRfb3Gtz17Y07etqXhX6ul9uAnCJh6DCAKxUCWABok9J2PN8jYn42iwqYA2OYoKRzVAY67mYgIOfQP8WOthUKubNt6V6/yn5YSPEowsxKGPk0Jdq6ZLKxJYX2LTjYTNi52WTAN4RVyPd' | base64 -d > .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0\n\nFinally you can trigger the bug using several commands from git (other commands that parses all objects will work too), for instance:\n\n$ git fsck\n\nThe ASAN report is here:\n\n==24709==WARNING: AddressSanitizer failed to allocate 0xffffffffffffff65 bytes\n==24709==AddressSanitizer's allocator is terminating the process instead of returning 0\n==24709==If you don't like this behavior set allocator_may_return_null=1\n==24709==AddressSanitizer CHECK failed: /build/gcc-multilib/src/gcc/libsanitizer/sanitizer_common/sanitizer_allocator.cc:145 \"((0)) != (0)\" (0x0, 0x0)\n    #0 0x7f571ae467aa in AsanCheckFailed /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_rtl.cc:65\n    #1 0x7f571ae4d163 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) /build/gcc-multilib/src/gcc/libsanitizer/sanitizer_common/sanitizer_common.cc:157\n    #2 0x7f571ae4b326 in __sanitizer::ReportAllocatorCannotReturnNull() /build/gcc-multilib/src/gcc/libsanitizer/sanitizer_common/sanitizer_allocator.cc:145\n    #3 0x7f571ad9b2f4 in __sanitizer::CombinedAllocator<__sanitizer::SizeClassAllocator64<105553116266496ul, 4398046511104ul, 0ul, __sanitizer::SizeClassMap<17ul, 128ul, 16ul>, __asan::AsanMapUnmapCallback>, __sanitizer::SizeClassAllocatorLocalCache<__sanitizer::SizeClassAllocator64<105553116266496ul, 4398046511104ul, 0ul, __sanitizer::SizeClassMap<17ul, 128ul, 16ul>, __asan::AsanMapUnmapCallback> >, __sanitizer::LargeMmapAllocator<__asan::AsanMapUnmapCallback> >::ReturnNullOrDie() /build/gcc-multilib/src/gcc/libsanitizer/sanitizer_common/sanitizer_allocator.h:1315\n    #4 0x7f571ad9b2f4 in __asan::Allocator::Allocate(unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType, bool) /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_allocator.cc:357\n    #5 0x7f571ad9b2f4 in __asan::asan_malloc(unsigned long, __sanitizer::BufferedStackTrace*) /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_allocator.cc:716\n    #6 0x7f571ae3ce24 in __interceptor_malloc /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:63\n    #7 0x767816 in do_xmalloc /home/g/Work/Code/git-2.10.0/wrapper.c:59\n    #8 0x76794c in do_xmallocz /home/g/Work/Code/git-2.10.0/wrapper.c:99\n    #9 0x7679bd in xmallocz /home/g/Work/Code/git-2.10.0/wrapper.c:107\n    #10 0x6fe36c in unpack_sha1_rest /home/g/Work/Code/git-2.10.0/sha1_file.c:1625\n    #11 0x6feb40 in unpack_sha1_file /home/g/Work/Code/git-2.10.0/sha1_file.c:1751\n    #12 0x703fe0 in read_object /home/g/Work/Code/git-2.10.0/sha1_file.c:2811\n    #13 0x70410a in read_sha1_file_extended /home/g/Work/Code/git-2.10.0/sha1_file.c:2834\n    #14 0x647676 in read_sha1_file /home/g/Work/Code/git-2.10.0/cache.h:1056\n    #15 0x648545 in parse_object /home/g/Work/Code/git-2.10.0/object.c:269\n    #16 0x48d46d in fsck_sha1 builtin/fsck.c:367\n    #17 0x48da47 in fsck_loose builtin/fsck.c:493\n    #18 0x707514 in for_each_file_in_obj_subdir /home/g/Work/Code/git-2.10.0/sha1_file.c:3477\n    #19 0x70775b in for_each_loose_file_in_objdir_buf /home/g/Work/Code/git-2.10.0/sha1_file.c:3512\n    #20 0x707885 in for_each_loose_file_in_objdir /home/g/Work/Code/git-2.10.0/sha1_file.c:3532\n    #21 0x48dc1d in fsck_object_dir builtin/fsck.c:521\n    #22 0x48e2e6 in cmd_fsck builtin/fsck.c:644\n    #23 0x407a8f in run_builtin /home/g/Work/Code/git-2.10.0/git.c:352\n    #24 0x407e35 in handle_builtin /home/g/Work/Code/git-2.10.0/git.c:539\n    #25 0x408175 in run_argv /home/g/Work/Code/git-2.10.0/git.c:593\n    #26 0x408458 in cmd_main /home/g/Work/Code/git-2.10.0/git.c:665\n    #27 0x53fc70 in main /home/g/Work/Code/git-2.10.0/common-main.c:40\n    #28 0x7f5719f46290 in __libc_start_main (/usr/lib/libc.so.6+0x20290)\n    #29 0x405209 in _start (/home/g/Work/Code/git-2.10.0/git+0x405209)\n\n\nThis test case was found using QuickFuzz.\n\n\nRegards,\nGustavo.\n"},{"id":"302683","messageId":"20160927071936.5hneqhv22zw73asy@sigill.intra.peff.net","threadId":"44153","inReplyTo":"xmqqtwd2sf9t.fsf@gitster.mtv.corp.google.com","subject":"Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-27T07:19:36Z","receivedAt":"2016-09-27T07:20:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 26, 2016 at 11:10:54AM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > I am inclined to say that it has no security implications.  You have\n> > to be able to write a bogus loose object in an object store you\n> > already have write access to in the first place, in order to cause\n> > this ...\n> \n> Note that you could social-engineer others to fetch from you and\n> feed a small enough update that results in loose objects created in\n> their repositories, without you having a direct write access to the\n> repository.\n> \n> The codepath under discussion in this thread however cannot be used\n> as an attack vector via that route, because the \"fetch from\n> elsewhere\" codepath runs verification of the incoming data stream\n> before storing the results (either in loose object files, or in a\n> packfile) on disk.\n\nI don't think it could be used at all for anything that speaks the git\nprotocol, because the object header is not present at all in a packfile.\nSo even if you hit unpack-objects, it would be writing the (correct)\nloose object header itself.\n\nBut when we grab loose objects _directly_ from a remote, as in dumb-http\nfetch, I'd suspect that the code doing the verification calls\nunpack_sha1_header() as part of it. So I didn't test, but I'd strongly\nsuspect that's a viable attack vector.\n\nI'm not sure what the actual attack would look like, though, aside from\nlocally accessing memory in a read-only way.\n\n-Peff\n"},{"id":"302686","messageId":"20160927080755.evlq3sfkyoolixop@sigill.intra.peff.net","threadId":"44153","inReplyTo":"381383122.8376940.1474943423005.JavaMail.zimbra@imag.fr","subject":"Re: Possible integer overflow parsing malformed objects in git 2.10.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-27T08:07:55Z","receivedAt":"2016-09-27T08:08:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 27, 2016 at 04:30:23AM +0200, Gustavo Grieco wrote:\n\n> We found a malformed object file that triggers an allocation with a\n> negative size when parsed in git 2.10.0. It can be caused by an\n> integer overflow somewhere, so it is better to verify how the code got\n> such value.\n\nAre you sure this is triggering a negative integer?\n\nThe zlib-inflated contents for the object in your example look like:\n\n  (gdb) print hdr\n  $2 = \"tree 18446744073709551460\\000...\"\n\nIOW, this really _is_ a gigantic number, but still within 2^64. So when\nwe feed it to malloc, that really is correct. And we'd expect malloc to\nreturn NULL, at which point we'll call die, which should look like this\n(which I get when running without ASAN):\n\n  $ git fsck\n  fatal: Out of memory, malloc failed (tried to allocate 18446744073709551461 bytes)\n\nYou'll note that's 1 more than the value in the object; that addition\nhappens via xmallocz() and _is_ checked for integer overflow.\n\n> The ASAN report is here:\n> \n> ==24709==WARNING: AddressSanitizer failed to allocate 0xffffffffffffff65 bytes\n> ==24709==AddressSanitizer's allocator is terminating the process instead of returning 0\n> ==24709==If you don't like this behavior set allocator_may_return_null=1\n\nI don't think this is an overflow at all. This is just ASAN having\nreally conservative debugging defaults. A real malloc would return NULL,\nand git would notice and abort.\n\nIf you follow its suggestion, you get:\n\n  $ ASAN_OPTIONS=allocator_may_return_null=1 git fsck\n  ==19380==WARNING: AddressSanitizer failed to allocate 0xffffffffffffff65 bytes\n  ==19380==WARNING: AddressSanitizer failed to allocate 0xffffffffffffff65 bytes\n  fatal: Out of memory, malloc failed (tried to allocate 18446744073709551461 bytes)\n\nas expected.  So I don't think there is any bug at all in the example\nyou gave, only a silly-sized object that we cannot handle.\n\nThat being said, the parse_sha1_header() function clearly does not\ndetect overflow at all when parsing the size. So on a 32-bit system, you\nend up with:\n\n  $ git fsck\n  fatal: Out of memory, malloc failed (tried to allocate 4294967141 bytes)\n\nwhich is not correct, but I'm not sure it's a security problem.  Integer\noverflows are an issue if they cause us to under-allocate, and then to\nwrite more bytes than we allocated. In this case, I would expect\nunpack_sha1_rest() to never write more bytes than the \"size\" we parsed\nand allocated (and to complain if the number of bytes we get from the\nzlib sequence do not exactly match the claimed size).\n\nSo a more interesting example is more like \"ULONG_MAX + 5\", where we\nwould overflow to 5 bytes. And we'd hope that unpack_sha1_rest does not\never write more than 5 bytes. From my reading and a few tests with gdb,\nit does not. However, it also does not notice that there were more bytes\nthat we didn't use.\n\nSo I think there's room for improved diagnosis of bogus situations\n(including integer overflows), but I don't see any actual security bugs.\n\n-Peff\n"},{"id":"302712","messageId":"xmqqponpqqri.fsf@gitster.mtv.corp.google.com","threadId":"44153","inReplyTo":"20160927080755.evlq3sfkyoolixop@sigill.intra.peff.net","subject":"Re: Possible integer overflow parsing malformed objects in git 2.10.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-27T15:57:53Z","receivedAt":"2016-09-27T15:58:02Z","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> That being said, the parse_sha1_header() function clearly does not\n> detect overflow at all when parsing the size. So on a 32-bit system, you\n> end up with:\n>\n>   $ git fsck\n>   fatal: Out of memory, malloc failed (tried to allocate 4294967141 bytes)\n>\n> which is not correct, but I'm not sure it's a security problem.  Integer\n> overflows are an issue if they cause us to under-allocate, and then to\n> write more bytes than we allocated. In this case, I would expect\n> unpack_sha1_rest() to never write more bytes than the \"size\" we parsed\n> and allocated (and to complain if the number of bytes we get from the\n> zlib sequence do not exactly match the claimed size).\n>\n> So a more interesting example is more like \"ULONG_MAX + 5\", where we\n> would overflow to 5 bytes. And we'd hope that unpack_sha1_rest does not\n> ever write more than 5 bytes. From my reading and a few tests with gdb,\n> it does not. However, it also does not notice that there were more bytes\n> that we didn't use.\n>\n> So I think there's room for improved diagnosis of bogus situations\n> (including integer overflows), but I don't see any actual security bugs.\n\nI agree with the overall conclusion.  This does look like an attempt\nto throw random fuzz at Git and see if and how it breaks, and in this\nparticular one Git is simply doing the right thing (and the fault lies\nin the way how ASAN is used and how its result was interpreted).\n\nThrowing random fuzz to see what breaks is not a bad thing to do\nper-se, but anybody who does so without wearing a black hat needs to\nkeep two things in mind:\n\n * When a random fuzz attempt does uncover a security issue,\n   reporting it here on this list is a grossly irresponsible way to\n   disclose the issue.  We have the git-security list for that.\n\n * A random fuzz may stop Git and that may be perfectly legit thing\n   to happen, e.g. the data may request a large but still valid\n   amount of memory to be allocated that happens not to fit in the\n   hardware the fuzz attempt is being run, and xmalloc() may detect\n   the situation and die, like the above example.  False positives\n   are expected and you want to make sure you cull them before\n   making your reports.  Otherwise, they will unnecessary burden\n   people who are doing real work, i.e. reproduce and correct\n   problems that may be security related that are irresponsibly\n   disclosed here quickly enough to minimize damage.\n\nThanks.\n\n"},{"id":"302744","messageId":"1528663490.8544563.1475003685969.JavaMail.zimbra@imag.fr","threadId":"44153","inReplyTo":"xmqqponpqqri.fsf@gitster.mtv.corp.google.com","subject":"Re: Possible integer overflow parsing malformed objects in git 2.10.0","fromName":"Gustavo Grieco","fromEmail":"gustavo.grieco@imag.fr","sentAt":"2016-09-27T19:14:45Z","receivedAt":"2016-09-27T19:15:00Z","isPatch":false,"sender":{"key":"gustavo.grieco@imag.fr","avatar":null},"body":"----- Original Message -----\n> Jeff King <peff@peff.net> writes:\n> \n> > That being said, the parse_sha1_header() function clearly does not\n> > detect overflow at all when parsing the size. So on a 32-bit system, you\n> > end up with:\n> >\n> >   $ git fsck\n> >   fatal: Out of memory, malloc failed (tried to allocate 4294967141 bytes)\n> >\n> > which is not correct, but I'm not sure it's a security problem.  Integer\n> > overflows are an issue if they cause us to under-allocate, and then to\n> > write more bytes than we allocated. In this case, I would expect\n> > unpack_sha1_rest() to never write more bytes than the \"size\" we parsed\n> > and allocated (and to complain if the number of bytes we get from the\n> > zlib sequence do not exactly match the claimed size).\n> >\n> > So a more interesting example is more like \"ULONG_MAX + 5\", where we\n> > would overflow to 5 bytes. And we'd hope that unpack_sha1_rest does not\n> > ever write more than 5 bytes. From my reading and a few tests with gdb,\n> > it does not. However, it also does not notice that there were more bytes\n> > that we didn't use.\n> >\n> > So I think there's room for improved diagnosis of bogus situations\n> > (including integer overflows), but I don't see any actual security bugs.\n\nGreat, it is exactly the type of analysis i was expecting. \n\n> \n> I agree with the overall conclusion.  This does look like an attempt\n> to throw random fuzz at Git and see if and how it breaks, and in this\n> particular one Git is simply doing the right thing (and the fault lies\n> in the way how ASAN is used and how its result was interpreted).\n> \n> Throwing random fuzz to see what breaks is not a bad thing to do\n> per-se, but anybody who does so without wearing a black hat needs to\n> keep two things in mind:\n> \n>  * When a random fuzz attempt does uncover a security issue,\n>    reporting it here on this list is a grossly irresponsible way to\n>    disclose the issue.  We have the git-security list for that.\n\nThat is reasonable, indeed. As we discussed, this type of issues are very unlikely to be easily exploited (or even possible), so i did not think that it could be irresponsible to post this issue here. I will be happy to post only in git-security if you think my reports can uncover security issues (but so far, it was not the case). It is also interesting to mention that git-security is not linked anywhere in the official website (git-scm.com) or the github repository (github.com/git/git).\n\n> \n>  * A random fuzz may stop Git and that may be perfectly legit thing\n>    to happen, e.g. the data may request a large but still valid\n>    amount of memory to be allocated that happens not to fit in the\n>    hardware the fuzz attempt is being run, and xmalloc() may detect\n>    the situation and die, like the above example.  False positives\n>    are expected and you want to make sure you cull them before\n>    making your reports.  Otherwise, they will unnecessary burden\n>    people who are doing real work, i.e. reproduce and correct\n>    problems that may be security related that are irresponsibly\n>    disclosed here quickly enough to minimize damage.\n\nI try to discard false positives as much as possible. Despite we just started adding the git generation module to our tool, we got hundreds of aborts caused by the allocation limits of AddressSanitizer: I only reported the one which specifically contains a size that could be interpreted as negative number (in fact, valgrind will also report this issue a 'fishy size' for malloc).\nAlso, I think my reports should are clear enough to reproduce any issue and i carefully check every test case for reproducibility, still i am open to suggestion on how to improve my bug reports! \n\n> \n> Thanks.\n> \n> \n"}]}