{"thread":{"id":"61571","subject":"[PATCH] dir.c: skip .gitignore, etc larger than INT_MAX","startedAt":"2024-05-31T12:00:36Z","lastAt":"2024-06-05T16:23:24Z","messageCount":7,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"496003","messageId":"20240531120034.GA442032@coredump.intra.peff.net","threadId":"61571","inReplyTo":null,"subject":"[PATCH] dir.c: skip .gitignore, etc larger than INT_MAX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T12:00:34Z","receivedAt":"2024-05-31T12:00:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use add_patterns() to read .gitignore, .git/info/exclude, etc, as\nwell as other pattern-like files like sparse-checkout. The parser for\nthese uses an \"int\" as an index, meaning that files over 2GB will\ngenerally cause signed integer overflow and out-of-bounds access.\n\nThis is unlikely to happen in any real files, but we do read .gitignore\nfiles from the tree. A malicious tree could cause an out-of-bounds read\nand segfault (we also write NULs over newlines, so in theory it could be\nan out-of-bounds write, too, but as we go char-by-char, the first thing\nthat happens is trying to read a negative 2GB offset).\n\nWe could fix the most obvious issue by replacing one \"int\" with a\n\"size_t\". But there are tons of \"int\" sprinkled throughout this code for\nthings like pattern lengths, number of patterns, and so on. Since nobody\nwould actually want a 2GB .gitignore file, an easy defensive measure is\nto just refuse to parse them.\n\nThe \"int\" in question is in add_patterns_from_buffer(), so we could\ncatch it there. But by putting the checks in its two callers, we can\nproduce more useful error messages.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nJust something I noticed while working on leaks nearby.\n\n dir.c | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/dir.c b/dir.c\nindex f6066cc01d..914060edfd 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -30,6 +30,7 @@\n #include \"symlinks.h\"\n #include \"trace2.h\"\n #include \"tree.h\"\n+#include \"hex.h\"\n \n /*\n  * Tells read_directory_recursive how a file or directory should be treated.\n@@ -1148,6 +1149,12 @@ static int add_patterns(const char *fname, const char *base, int baselen,\n \t\t}\n \t}\n \n+\tif (size > INT_MAX) {\n+\t\twarning(\"ignoring excessively large pattern file: %s\", fname);\n+\t\tfree(buf);\n+\t\treturn -1;\n+\t}\n+\n \tadd_patterns_from_buffer(buf, size, base, baselen, pl);\n \treturn 0;\n }\n@@ -1204,6 +1211,13 @@ int add_patterns_from_blob_to_list(\n \tif (r != 1)\n \t\treturn r;\n \n+\tif (size > INT_MAX) {\n+\t\twarning(\"ignoring excessively large pattern blob: %s\",\n+\t\t\toid_to_hex(oid));\n+\t\tfree(buf);\n+\t\treturn -1;\n+\t}\n+\n \tadd_patterns_from_buffer(buf, size, base, baselen, pl);\n \treturn 0;\n }\n-- \n2.45.1.727.ge984192922\n"},{"id":"496014","messageId":"xmqqh6eehvj4.fsf@gitster.g","threadId":"61571","inReplyTo":"20240531120034.GA442032@coredump.intra.peff.net","subject":"Re: [PATCH] dir.c: skip .gitignore, etc larger than INT_MAX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-31T15:05:03Z","receivedAt":"2024-05-31T15:05:07Z","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> The \"int\" in question is in add_patterns_from_buffer(), so we could\n> catch it there. But by putting the checks in its two callers, we can\n> produce more useful error messages.\n\nNice trick.  I wondered if we want INT_MAX/2 or something even\nlower, but because once these things are read, we only parse the\ncontents so allowing up to INT_MAX is fine.  It is not like we read\nthis and that and concatenate them into a larger buffer.\n"},{"id":"496015","messageId":"xmqqbk4mhvah.fsf@gitster.g","threadId":"61571","inReplyTo":"20240531120034.GA442032@coredump.intra.peff.net","subject":"Re: [PATCH] dir.c: skip .gitignore, etc larger than INT_MAX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-31T15:10:14Z","receivedAt":"2024-05-31T15:10:20Z","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> We use add_patterns() to read .gitignore, .git/info/exclude, etc, as\n> well as other pattern-like files like sparse-checkout. The parser for\n> these uses an \"int\" as an index, meaning that files over 2GB will\n> generally cause signed integer overflow and out-of-bounds access.\n\nI also wondered if we need similar protection on the attribute side,\nbut it turns out that we process the files one line at a time\nwithout holding everything in core.  Both exclude and attribute\nsubsystem are not protected against parseed _result_ consuming too\nmuch memory, but at least with your patch, we are covered on the\ninput side.\n\nI wonder if it is worth rewriting the exclude side to stream like\nattribute parsing, though.  It probalby is not.\n"},{"id":"496239","messageId":"20240604103947.GC1781455@coredump.intra.peff.net","threadId":"61571","inReplyTo":"xmqqh6eehvj4.fsf@gitster.g","subject":"Re: [PATCH] dir.c: skip .gitignore, etc larger than INT_MAX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:39:47Z","receivedAt":"2024-06-04T10:39:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 31, 2024 at 08:05:03AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The \"int\" in question is in add_patterns_from_buffer(), so we could\n> > catch it there. But by putting the checks in its two callers, we can\n> > produce more useful error messages.\n> \n> Nice trick.  I wondered if we want INT_MAX/2 or something even\n> lower, but because once these things are read, we only parse the\n> contents so allowing up to INT_MAX is fine.  It is not like we read\n> this and that and concatenate them into a larger buffer.\n\nYeah, I think all of the individual string lengths would be fine. It\n_might_ be possible to overflow an array of pointers or similar, though.\nE.g., if you have \"a\\n\" repeated INT_MAX times, then you have INT_MAX/2\nentries. An array of pointers-to-structs, assuming 8-byte pointers,\nwould then need INT_MAX*4 bytes. And indeed, I think we stuff these into\na pattern_list which uses ints.\n\nIn practice I think the call to ALLOC_GROW() would catch that as the\noverflowed value turns into a large size_t. But it probably would be\nsafer to use a much smaller limit.\n\nI was hoping to avoid making up an arbitrary number. But your question\nabout gitattributes reminded me that we already did something similar in\n3c50032ff5 (attr: ignore overly large gitattributes files, 2022-12-01).\nThere it's a hard-coded 100MB limit (without even a config option).\n\nMaybe we should just do the same here?\n\n-Peff\n"},{"id":"496296","messageId":"xmqqikyowqjj.fsf@gitster.g","threadId":"61571","inReplyTo":"20240604103947.GC1781455@coredump.intra.peff.net","subject":"Re: [PATCH] dir.c: skip .gitignore, etc larger than INT_MAX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-04T17:45:04Z","receivedAt":"2024-06-04T17:45:13Z","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> I was hoping to avoid making up an arbitrary number. But your question\n> about gitattributes reminded me that we already did something similar in\n> 3c50032ff5 (attr: ignore overly large gitattributes files, 2022-12-01).\n> There it's a hard-coded 100MB limit (without even a config option).\n>\n> Maybe we should just do the same here?\n\nHmph, I thought the 100MB was only for blobs, as we stream the input\nwe read from a regular file, but we do have the same limit there\npresumably to match what we do to blobs?  I do not mind that, but I\ndo not mind leaving it a future \"consolidate various size limits on\ncontrol files used by Git\" patch, that unifies the limit for attrs,\nexcludes, gitmodules, .git/config, etc.\n\n"},{"id":"496341","messageId":"20240605080308.GA2345232@coredump.intra.peff.net","threadId":"61571","inReplyTo":"xmqqikyowqjj.fsf@gitster.g","subject":"Re: [PATCH] dir.c: skip .gitignore, etc larger than INT_MAX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-05T08:03:08Z","receivedAt":"2024-06-05T08:03:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 04, 2024 at 10:45:04AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I was hoping to avoid making up an arbitrary number. But your question\n> > about gitattributes reminded me that we already did something similar in\n> > 3c50032ff5 (attr: ignore overly large gitattributes files, 2022-12-01).\n> > There it's a hard-coded 100MB limit (without even a config option).\n> >\n> > Maybe we should just do the same here?\n> \n> Hmph, I thought the 100MB was only for blobs, as we stream the input\n> we read from a regular file, but we do have the same limit there\n> presumably to match what we do to blobs?  I do not mind that, but I\n> do not mind leaving it a future \"consolidate various size limits on\n> control files used by Git\" patch, that unifies the limit for attrs,\n> excludes, gitmodules, .git/config, etc.\n\nI think we enforce the limit for files, too. Which makes sense, as we'd\nread .gitattributes checked out from an untrusted tree. Even if we\nstream it line by line, we still end up scaling some structures with the\ntotal number of lines (and of course the file length also bounds the\nline length, though I think we have an even shorter line length limit).\n\nI do not mind if consolidation waits for later, but I guess the\nimmediate question is whether we'd prefer to reduce the limit set by my\npatch to a more security-conservative value. It would be easy to swap\nout INT_MAX for a 100MB #define on top.\n\nMaybe this?\n\n-- >8 --\nSubject: [PATCH] dir.c: reduce max pattern file size to 100MB\n\nIn a2bc523e1e (dir.c: skip .gitignore, etc larger than INT_MAX,\n2024-05-31) we put capped the size of some files whose parsing code and\ndata structures used ints. Setting the limit to INT_MAX was a natural\nspot, since we know the parsing code would misbehave above that.\n\nBut it also leaves the possibility of overflow errors when we multiply\nthat limit to allocate memory. For instance, a file consisting only of\n\"a\\na\\n...\" could have INT_MAX/2 entries. Allocating an array of\npointers for each would need INT_MAX*4 bytes on a 64-bit system, enough\nto overflow a 32-bit int.\n\nSo let's give ourselves a bit more safety margin by giving a much\nsmaller limit. The size 100MB is somewhat arbitrary, but is based on the\nsimilar value for attribute files added by 3c50032ff5 (attr: ignore\noverly large gitattributes files, 2022-12-01).\n\nThere's no particular reason these have to be the same, but the idea is\nthat they are in the ballpark of \"so huge that nobody would care, but\nsmall enough to avoid malicious overflow\". So lacking a better guess, it\nmakes sense to use the same value. The implementation here doesn't share\nthe same constant, but we could change that later (or even give it a\nruntime config knob, though nobody has complained yet about the\nattribute limit).\n\nAnd likewise, let's add a few tests that exercise the limits, based on\nthe attr ones. In this case, though, we never read .gitignore from the\nindex; the blob code is exercised only for sparse filters. So we'll\ntrigger it that way.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n dir.c                               | 10 ++++++++--\n t/t0008-ignores.sh                  |  8 ++++++++\n t/t6112-rev-list-filters-objects.sh | 12 ++++++++++++\n 3 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 914060edfd..ad2b7ebe2d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -32,6 +32,12 @@\n #include \"tree.h\"\n #include \"hex.h\"\n \n+ /*\n+  * The maximum size of a pattern/exclude file. If the file exceeds this size\n+  * we will ignore it.\n+  */\n+#define PATTERN_MAX_FILE_SIZE (100 * 1024 * 1024)\n+\n /*\n  * Tells read_directory_recursive how a file or directory should be treated.\n  * Values are ordered by significance, e.g. if a directory contains both\n@@ -1149,7 +1155,7 @@ static int add_patterns(const char *fname, const char *base, int baselen,\n \t\t}\n \t}\n \n-\tif (size > INT_MAX) {\n+\tif (size > PATTERN_MAX_FILE_SIZE) {\n \t\twarning(\"ignoring excessively large pattern file: %s\", fname);\n \t\tfree(buf);\n \t\treturn -1;\n@@ -1211,7 +1217,7 @@ int add_patterns_from_blob_to_list(\n \tif (r != 1)\n \t\treturn r;\n \n-\tif (size > INT_MAX) {\n+\tif (size > PATTERN_MAX_FILE_SIZE) {\n \t\twarning(\"ignoring excessively large pattern blob: %s\",\n \t\t\toid_to_hex(oid));\n \t\tfree(buf);\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex 361446b2f4..02a18d4fdb 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -945,4 +945,12 @@ test_expect_success SYMLINKS 'symlinks not respected in-tree' '\n \ttest_grep \"unable to access.*gitignore\" err\n '\n \n+test_expect_success EXPENSIVE 'large exclude file ignored in tree' '\n+\ttest_when_finished \"rm .gitignore\" &&\n+\tdd if=/dev/zero of=.gitignore bs=101M count=1 &&\n+\tgit ls-files -o --exclude-standard 2>err &&\n+\techo \"warning: ignoring excessively large pattern file: .gitignore\" >expect &&\n+\ttest_cmp expect err\n+'\n+\n test_done\ndiff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh\nindex 43e1afd44c..0387f35a32 100755\n--- a/t/t6112-rev-list-filters-objects.sh\n+++ b/t/t6112-rev-list-filters-objects.sh\n@@ -701,4 +701,16 @@ test_expect_success 'expand blob limit in protocol' '\n \tgrep \"blob:limit=1024\" trace\n '\n \n+test_expect_success EXPENSIVE 'large sparse filter file ignored' '\n+\tblob=$(dd if=/dev/zero bs=101M count=1 |\n+\t       git hash-object -w --stdin) &&\n+\ttest_must_fail \\\n+\t\tgit rev-list --all --objects --filter=sparse:oid=$blob 2>err &&\n+\tcat >expect <<-EOF &&\n+\twarning: ignoring excessively large pattern blob: $blob\n+\tfatal: unable to parse sparse filter data in $blob\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_done\n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496376","messageId":"xmqqplsvuznq.fsf@gitster.g","threadId":"61571","inReplyTo":"20240605080308.GA2345232@coredump.intra.peff.net","subject":"Re: [PATCH] dir.c: skip .gitignore, etc larger than INT_MAX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-05T16:23:21Z","receivedAt":"2024-06-05T16:23: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> I do not mind if consolidation waits for later, but I guess the\n> immediate question is whether we'd prefer to reduce the limit set by my\n> patch to a more security-conservative value. It would be easy to swap\n> out INT_MAX for a 100MB #define on top.\n>\n> Maybe this?\n\nSounds sensible.  Thanks.\n\n> -- >8 --\n> Subject: [PATCH] dir.c: reduce max pattern file size to 100MB\n>\n> In a2bc523e1e (dir.c: skip .gitignore, etc larger than INT_MAX,\n> 2024-05-31) we put capped the size of some files whose parsing code and\n> data structures used ints. Setting the limit to INT_MAX was a natural\n> spot, since we know the parsing code would misbehave above that.\n>\n> But it also leaves the possibility of overflow errors when we multiply\n> that limit to allocate memory. For instance, a file consisting only of\n> \"a\\na\\n...\" could have INT_MAX/2 entries. Allocating an array of\n> pointers for each would need INT_MAX*4 bytes on a 64-bit system, enough\n> to overflow a 32-bit int.\n>\n> So let's give ourselves a bit more safety margin by giving a much\n> smaller limit. The size 100MB is somewhat arbitrary, but is based on the\n> similar value for attribute files added by 3c50032ff5 (attr: ignore\n> overly large gitattributes files, 2022-12-01).\n>\n> There's no particular reason these have to be the same, but the idea is\n> that they are in the ballpark of \"so huge that nobody would care, but\n> small enough to avoid malicious overflow\". So lacking a better guess, it\n> makes sense to use the same value. The implementation here doesn't share\n> the same constant, but we could change that later (or even give it a\n> runtime config knob, though nobody has complained yet about the\n> attribute limit).\n>\n> And likewise, let's add a few tests that exercise the limits, based on\n> the attr ones. In this case, though, we never read .gitignore from the\n> index; the blob code is exercised only for sparse filters. So we'll\n> trigger it that way.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  dir.c                               | 10 ++++++++--\n>  t/t0008-ignores.sh                  |  8 ++++++++\n>  t/t6112-rev-list-filters-objects.sh | 12 ++++++++++++\n>  3 files changed, 28 insertions(+), 2 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index 914060edfd..ad2b7ebe2d 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -32,6 +32,12 @@\n>  #include \"tree.h\"\n>  #include \"hex.h\"\n>  \n> + /*\n> +  * The maximum size of a pattern/exclude file. If the file exceeds this size\n> +  * we will ignore it.\n> +  */\n> +#define PATTERN_MAX_FILE_SIZE (100 * 1024 * 1024)\n> +\n>  /*\n>   * Tells read_directory_recursive how a file or directory should be treated.\n>   * Values are ordered by significance, e.g. if a directory contains both\n> @@ -1149,7 +1155,7 @@ static int add_patterns(const char *fname, const char *base, int baselen,\n>  \t\t}\n>  \t}\n>  \n> -\tif (size > INT_MAX) {\n> +\tif (size > PATTERN_MAX_FILE_SIZE) {\n>  \t\twarning(\"ignoring excessively large pattern file: %s\", fname);\n>  \t\tfree(buf);\n>  \t\treturn -1;\n> @@ -1211,7 +1217,7 @@ int add_patterns_from_blob_to_list(\n>  \tif (r != 1)\n>  \t\treturn r;\n>  \n> -\tif (size > INT_MAX) {\n> +\tif (size > PATTERN_MAX_FILE_SIZE) {\n>  \t\twarning(\"ignoring excessively large pattern blob: %s\",\n>  \t\t\toid_to_hex(oid));\n>  \t\tfree(buf);\n> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\n> index 361446b2f4..02a18d4fdb 100755\n> --- a/t/t0008-ignores.sh\n> +++ b/t/t0008-ignores.sh\n> @@ -945,4 +945,12 @@ test_expect_success SYMLINKS 'symlinks not respected in-tree' '\n>  \ttest_grep \"unable to access.*gitignore\" err\n>  '\n>  \n> +test_expect_success EXPENSIVE 'large exclude file ignored in tree' '\n> +\ttest_when_finished \"rm .gitignore\" &&\n> +\tdd if=/dev/zero of=.gitignore bs=101M count=1 &&\n> +\tgit ls-files -o --exclude-standard 2>err &&\n> +\techo \"warning: ignoring excessively large pattern file: .gitignore\" >expect &&\n> +\ttest_cmp expect err\n> +'\n> +\n>  test_done\n> diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh\n> index 43e1afd44c..0387f35a32 100755\n> --- a/t/t6112-rev-list-filters-objects.sh\n> +++ b/t/t6112-rev-list-filters-objects.sh\n> @@ -701,4 +701,16 @@ test_expect_success 'expand blob limit in protocol' '\n>  \tgrep \"blob:limit=1024\" trace\n>  '\n>  \n> +test_expect_success EXPENSIVE 'large sparse filter file ignored' '\n> +\tblob=$(dd if=/dev/zero bs=101M count=1 |\n> +\t       git hash-object -w --stdin) &&\n> +\ttest_must_fail \\\n> +\t\tgit rev-list --all --objects --filter=sparse:oid=$blob 2>err &&\n> +\tcat >expect <<-EOF &&\n> +\twarning: ignoring excessively large pattern blob: $blob\n> +\tfatal: unable to parse sparse filter data in $blob\n> +\tEOF\n> +\ttest_cmp expect err\n> +'\n> +\n>  test_done\n"}]}