{"thread":{"id":"61415","subject":"[PATCH] attr.c: move ATTR_MAX_FILE_SIZE check into read_attr_from_buf()","startedAt":"2024-05-03T19:12:39Z","lastAt":"2024-05-06T05:57:25Z","messageCount":3,"participants":["Taylor Blau","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"494047","messageId":"28f6267709db78ba526d7ed9fc4a734674697c70.1714763555.git.me@ttaylorr.com","threadId":"61415","inReplyTo":null,"subject":"[PATCH] attr.c: move ATTR_MAX_FILE_SIZE check into read_attr_from_buf()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-03T19:12:36Z","receivedAt":"2024-05-03T19:12:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Commit 3c50032ff52 (attr: ignore overly large gitattributes files,\n2022-12-01) added a defense-in-depth check to ensure that .gitattributes\nblobs read from the index do not exceed ATTR_MAX_FILE_SIZE (100 MB).\n\nBut there were two cases added shortly after 3c50032ff52 was written\nwhich do not apply similar protections:\n\n - 47cfc9bd7d0 (attr: add flag `--source` to work with tree-ish,\n 2023-01-14)\n\n - 4723ae1007f (attr.c: read attributes in a sparse directory,\n 2023-08-11) added a similar\n\nEnsure that we refuse to process a .gitattributes blob exceeding\nATTR_MAX_FILE_SIZE when reading from either an arbitrary tree object or\na sparse directory. This is done by pushing the ATTR_MAX_FILE_SIZE check\ndown into the low-level `read_attr_from_buf()`.\n\nIn doing so, plug a leak in `read_attr_from_index()` where we would\naccidentally leak the large buffer upon detecting it is too large to\nprocess.\n\n(Since `read_attr_from_buf()` handles a NULL buffer input, we can remove\na NULL check before calling it in `read_attr_from_index()` as well).\n\nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n attr.c | 19 +++++++++----------\n t/t0003-attributes.sh | 10 ++++++++++\n 2 files changed, 19 insertions(+), 10 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 679e42258c..7c380c1731 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -765,8 +765,8 @@ static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)\n \treturn res;\n }\n \n-static struct attr_stack *read_attr_from_buf(char *buf, const char *path,\n-\t\t\t\t\t unsigned flags)\n+static struct attr_stack *read_attr_from_buf(char *buf, size_t length,\n+\t\t\t\t\t const char *path, unsigned flags)\n {\n \tstruct attr_stack *res;\n \tchar *sp;\n@@ -774,6 +774,11 @@ static struct attr_stack *read_attr_from_buf(char *buf, const char *path,\n \n \tif (!buf)\n \t\treturn NULL;\n+\tif (length >= ATTR_MAX_FILE_SIZE) {\n+\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n+\t\tfree(buf);\n+\t\treturn NULL;\n+\t}\n \n \tCALLOC_ARRAY(res, 1);\n \tfor (sp = buf; *sp;) {\n@@ -813,7 +818,7 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n \t\treturn NULL;\n \t}\n \n-\treturn read_attr_from_buf(buf, path, flags);\n+\treturn read_attr_from_buf(buf, sz, path, flags);\n }\n \n static struct attr_stack *read_attr_from_index(struct index_state *istate,\n@@ -860,13 +865,7 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate,\n \t\tstack = read_attr_from_blob(istate, &istate->cache[sparse_dir_pos]->oid, relative_path, flags);\n \t} else {\n \t\tbuf = read_blob_data_from_index(istate, path, &size);\n-\t\tif (!buf)\n-\t\t\treturn NULL;\n-\t\tif (size >= ATTR_MAX_FILE_SIZE) {\n-\t\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n-\t\t\treturn NULL;\n-\t\t}\n-\t\tstack = read_attr_from_buf(buf, path, flags);\n+\t\tstack = read_attr_from_buf(buf, size, path, flags);\n \t}\n \treturn stack;\n }\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 774b52c298..b007f76fd6 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -572,6 +572,16 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '\n \ttest_cmp expect err\n '\n \n+test_expect_success EXPENSIVE 'large attributes blob ignored' '\n+\ttest_when_finished \"git update-index --remove .gitattributes\" &&\n+\tblob=$(dd if=/dev/zero bs=1048576 count=101 2>/dev/null | git hash-object -w --stdin) &&\n+\tgit update-index --add --cacheinfo 100644,$blob,.gitattributes &&\n+\ttree=\"$(git write-tree)\" &&\n+\tgit check-attr --cached --all --source=\"$tree\" path >/dev/null 2>err &&\n+\techo \"warning: ignoring overly large gitattributes blob ${SQ}.gitattributes${SQ}\" >expect &&\n+\ttest_cmp expect err\n+'\n+\n test_expect_success 'builtin object mode attributes work (dir and regular paths)' '\n \t>normal &&\n \tattr_check_object_mode normal 100644 &&\n-- \n2.45.0.36.g67f89f42283.dirty\n"},{"id":"494056","messageId":"xmqqfruy65nh.fsf@gitster.g","threadId":"61415","inReplyTo":"28f6267709db78ba526d7ed9fc4a734674697c70.1714763555.git.me@ttaylorr.com","subject":"Re: [PATCH] attr.c: move ATTR_MAX_FILE_SIZE check into read_attr_from_buf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-03T19:45:38Z","receivedAt":"2024-05-03T19:45:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Ensure that we refuse to process a .gitattributes blob exceeding\n> ATTR_MAX_FILE_SIZE when reading from either an arbitrary tree object or\n> a sparse directory. This is done by pushing the ATTR_MAX_FILE_SIZE check\n> down into the low-level `read_attr_from_buf()`.\n>\n> In doing so, plug a leak in `read_attr_from_index()` where we would\n> accidentally leak the large buffer upon detecting it is too large to\n> process.\n>\n> (Since `read_attr_from_buf()` handles a NULL buffer input, we can remove\n> a NULL check before calling it in `read_attr_from_index()` as well).\n>\n> Co-authored-by: Jeff King \n> Signed-off-by: Jeff King \n> Signed-off-by: Taylor Blau \n> ---\n\nMakes sense. Will queue. Thanks.\n"},{"id":"494112","messageId":"ZjhxQOgRgvv7_gGY@tanuki","threadId":"61415","inReplyTo":"28f6267709db78ba526d7ed9fc4a734674697c70.1714763555.git.me@ttaylorr.com","subject":"Re: [PATCH] attr.c: move ATTR_MAX_FILE_SIZE check into read_attr_from_buf()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-06T05:57:20Z","receivedAt":"2024-05-06T05:57:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 03, 2024 at 03:12:36PM -0400, Taylor Blau wrote:\n> Commit 3c50032ff52 (attr: ignore overly large gitattributes files,\n> 2022-12-01) added a defense-in-depth check to ensure that .gitattributes\n> blobs read from the index do not exceed ATTR_MAX_FILE_SIZE (100 MB).\n> \n> But there were two cases added shortly after 3c50032ff52 was written\n> which do not apply similar protections:\n> \n> - 47cfc9bd7d0 (attr: add flag `--source` to work with tree-ish,\n> 2023-01-14)\n> \n> - 4723ae1007f (attr.c: read attributes in a sparse directory,\n> 2023-08-11) added a similar\n> \n> Ensure that we refuse to process a .gitattributes blob exceeding\n> ATTR_MAX_FILE_SIZE when reading from either an arbitrary tree object or\n> a sparse directory. This is done by pushing the ATTR_MAX_FILE_SIZE check\n> down into the low-level `read_attr_from_buf()`.\n> \n> In doing so, plug a leak in `read_attr_from_index()` where we would\n> accidentally leak the large buffer upon detecting it is too large to\n> process.\n> \n> (Since `read_attr_from_buf()` handles a NULL buffer input, we can remove\n> a NULL check before calling it in `read_attr_from_index()` as well).\n> \n> Co-authored-by: Jeff King \n> Signed-off-by: Jeff King \n> Signed-off-by: Taylor Blau \n> ---\n\nThis patch looks good to me, thanks!\n\nPatrick\n"}]}