| Message ID | 28f6267709db78ba526d7ed9fc4a734674697c70.1714763555.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c793f9cb0853b7b173228efa53b32c60e3818598 |
| Headers | show |
| Series | attr.c: move ATTR_MAX_FILE_SIZE check into read_attr_from_buf() | expand |
Taylor Blau <me@ttaylorr.com> writes: > Ensure that we refuse to process a .gitattributes blob exceeding > ATTR_MAX_FILE_SIZE when reading from either an arbitrary tree object or > a sparse directory. This is done by pushing the ATTR_MAX_FILE_SIZE check > down into the low-level `read_attr_from_buf()`. > > In doing so, plug a leak in `read_attr_from_index()` where we would > accidentally leak the large buffer upon detecting it is too large to > process. > > (Since `read_attr_from_buf()` handles a NULL buffer input, we can remove > a NULL check before calling it in `read_attr_from_index()` as well). > > Co-authored-by: Jeff King <peff@peff.net> > Signed-off-by: Jeff King <peff@peff.net> > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- Makes sense. Will queue. Thanks.
On Fri, May 03, 2024 at 03:12:36PM -0400, Taylor Blau wrote: > Commit 3c50032ff52 (attr: ignore overly large gitattributes files, > 2022-12-01) added a defense-in-depth check to ensure that .gitattributes > blobs read from the index do not exceed ATTR_MAX_FILE_SIZE (100 MB). > > But there were two cases added shortly after 3c50032ff52 was written > which do not apply similar protections: > > - 47cfc9bd7d0 (attr: add flag `--source` to work with tree-ish, > 2023-01-14) > > - 4723ae1007f (attr.c: read attributes in a sparse directory, > 2023-08-11) added a similar > > Ensure that we refuse to process a .gitattributes blob exceeding > ATTR_MAX_FILE_SIZE when reading from either an arbitrary tree object or > a sparse directory. This is done by pushing the ATTR_MAX_FILE_SIZE check > down into the low-level `read_attr_from_buf()`. > > In doing so, plug a leak in `read_attr_from_index()` where we would > accidentally leak the large buffer upon detecting it is too large to > process. > > (Since `read_attr_from_buf()` handles a NULL buffer input, we can remove > a NULL check before calling it in `read_attr_from_index()` as well). > > Co-authored-by: Jeff King <peff@peff.net> > Signed-off-by: Jeff King <peff@peff.net> > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- This patch looks good to me, thanks! Patrick
diff --git a/attr.c b/attr.c index 679e42258c..7c380c1731 100644 --- a/attr.c +++ b/attr.c @@ -765,8 +765,8 @@ static struct attr_stack *read_attr_from_file(const char *path, unsigned flags) return res; } -static struct attr_stack *read_attr_from_buf(char *buf, const char *path, - unsigned flags) +static struct attr_stack *read_attr_from_buf(char *buf, size_t length, + const char *path, unsigned flags) { struct attr_stack *res; char *sp; @@ -774,6 +774,11 @@ static struct attr_stack *read_attr_from_buf(char *buf, const char *path, if (!buf) return NULL; + if (length >= ATTR_MAX_FILE_SIZE) { + warning(_("ignoring overly large gitattributes blob '%s'"), path); + free(buf); + return NULL; + } CALLOC_ARRAY(res, 1); for (sp = buf; *sp;) { @@ -813,7 +818,7 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate, return NULL; } - return read_attr_from_buf(buf, path, flags); + return read_attr_from_buf(buf, sz, path, flags); } static struct attr_stack *read_attr_from_index(struct index_state *istate, @@ -860,13 +865,7 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate, stack = read_attr_from_blob(istate, &istate->cache[sparse_dir_pos]->oid, relative_path, flags); } else { buf = read_blob_data_from_index(istate, path, &size); - if (!buf) - return NULL; - if (size >= ATTR_MAX_FILE_SIZE) { - warning(_("ignoring overly large gitattributes blob '%s'"), path); - return NULL; - } - stack = read_attr_from_buf(buf, path, flags); + stack = read_attr_from_buf(buf, size, path, flags); } return stack; } diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index 774b52c298..b007f76fd6 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -572,6 +572,16 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' ' test_cmp expect err ' +test_expect_success EXPENSIVE 'large attributes blob ignored' ' + test_when_finished "git update-index --remove .gitattributes" && + blob=$(dd if=/dev/zero bs=1048576 count=101 2>/dev/null | git hash-object -w --stdin) && + git update-index --add --cacheinfo 100644,$blob,.gitattributes && + tree="$(git write-tree)" && + git check-attr --cached --all --source="$tree" path >/dev/null 2>err && + echo "warning: ignoring overly large gitattributes blob ${SQ}.gitattributes${SQ}" >expect && + test_cmp expect err +' + test_expect_success 'builtin object mode attributes work (dir and regular paths)' ' >normal && attr_check_object_mode normal 100644 &&
Commit 3c50032ff52 (attr: ignore overly large gitattributes files, 2022-12-01) added a defense-in-depth check to ensure that .gitattributes blobs read from the index do not exceed ATTR_MAX_FILE_SIZE (100 MB). But there were two cases added shortly after 3c50032ff52 was written which do not apply similar protections: - 47cfc9bd7d0 (attr: add flag `--source` to work with tree-ish, 2023-01-14) - 4723ae1007f (attr.c: read attributes in a sparse directory, 2023-08-11) added a similar Ensure that we refuse to process a .gitattributes blob exceeding ATTR_MAX_FILE_SIZE when reading from either an arbitrary tree object or a sparse directory. This is done by pushing the ATTR_MAX_FILE_SIZE check down into the low-level `read_attr_from_buf()`. In doing so, plug a leak in `read_attr_from_index()` where we would accidentally leak the large buffer upon detecting it is too large to process. (Since `read_attr_from_buf()` handles a NULL buffer input, we can remove a NULL check before calling it in `read_attr_from_index()` as well). Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- attr.c | 19 +++++++++---------- t/t0003-attributes.sh | 10 ++++++++++ 2 files changed, 19 insertions(+), 10 deletions(-)