[PATCH] attr.c: move ATTR_MAX_FILE_SIZE check into read_attr_from_buf()

[PATCH] attr.c: move ATTR_MAX_FILE_SIZE check into read_attr_from_buf()

[Taylor Blau]

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(-)

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 &&
-- 
2.45.0.36.g67f89f42283.dirty

Re: [PATCH] attr.c: move ATTR_MAX_FILE_SIZE check into read_attr_from_buf()

[Junio C Hamano]

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.

Re: [PATCH] attr.c: move ATTR_MAX_FILE_SIZE check into read_attr_from_buf()

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1381 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]