Re: [RFC/PATCH 0/6] hash-object: use fsck to check objects

[Jeff King <peff@peff.net>]

On Wed, Jan 18, 2023 at 03:35:06PM -0500, Jeff King wrote:

> The other option is having the fsck code avoid looking past the size it
> was given. I think the intent is that this should work, from commits
> like 4d0d89755e (Make sure fsck_commit_buffer() does not run out of the
> buffer, 2014-09-11). We do use skip_prefix() and parse_oid_hex(), which
> won't respect the size, but I think[1] that's OK because we'll have
> parsed up to the end-of-header beforehand (and those functions would
> never match past there).
> 
> Which would mean that 9a1a3a4d4c (mktag: allow omitting the header/body
> \n separator, 2021-01-05) and acf9de4c94 (mktag: use fsck instead of
> custom verify_tag(), 2021-01-05) were buggy, and we can just fix them.
> 
> [1] But I said "I think" above because it can get pretty subtle. There's
>     some more discussion in this thread:
> 
>       https://lore.kernel.org/git/20150625155128.C3E9738005C@gemini.denx.de/
> 
>     but I haven't yet convinced myself it's safe. This is exactly the
>     kind of analysis I wish I had the power to nerd-snipe René into.

I poked at this a bit more, and it definitely isn't safe. I think the
use of skip_prefix(), etc, is OK, because they'd always stop at an
unexpected newline. But verify_headers() is only confirming that we have
a series of complete lines, and we might end with no "\n\n" (and hence
no commit/tag message). And so the obvious case that fools us is one
where the data simply ends at a newline, but we are missing one or more
headers. So a truncated commit like:

  tree 1234567890123456789012345678901234567890

(with the newline at the end of the "tree" line, but nothing else) will
cause fsck_commit() to look past the "size" we pass it. With all of the
current callers, that means it sees a NUL and bails. So it's not
currently a bug, but it becomes one if we can feed it arbitrary buffers.

Fixing it isn't _too_ bad, and could look something like this:

diff --git a/fsck.c b/fsck.c
index c2c8facd2d..3f0bb3e350 100644
--- a/fsck.c
+++ b/fsck.c
@@ -834,6 +834,7 @@ static int fsck_commit(const struct object_id *oid,
 	unsigned author_count;
 	int err;
 	const char *buffer_begin = buffer;
+	const char *buffer_end = buffer + size;
 	const char *p;
 
 	if (verify_headers(buffer, size, oid, OBJ_COMMIT, options))
@@ -847,7 +848,7 @@ static int fsck_commit(const struct object_id *oid,
 			return err;
 	}
 	buffer = p + 1;
-	while (skip_prefix(buffer, "parent ", &buffer)) {
+	while (buffer < buffer_end && skip_prefix(buffer, "parent ", &buffer)) {
 		if (parse_oid_hex(buffer, &parent_oid, &p) || *p != '\n') {
 			err = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1, "invalid 'parent' line format - bad sha1");
 			if (err)
@@ -856,7 +857,7 @@ static int fsck_commit(const struct object_id *oid,
 		buffer = p + 1;
 	}
 	author_count = 0;
-	while (skip_prefix(buffer, "author ", &buffer)) {
+	while (buffer < buffer_end && skip_prefix(buffer, "author ", &buffer)) {
 		author_count++;
 		err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);
 		if (err)
@@ -868,7 +869,7 @@ static int fsck_commit(const struct object_id *oid,
 		err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, "invalid format - multiple 'author' lines");
 	if (err)
 		return err;
-	if (!skip_prefix(buffer, "committer ", &buffer))
+	if (buffer >= buffer_end || !skip_prefix(buffer, "committer ", &buffer))
 		return report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER, "invalid format - expected 'committer' line");
 	err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);
 	if (err)

And then the tag side would need something similar. I'd probably also
sprinkle some comments in verify_headers() and its callers documenting
our assumptions and what's OK to do (string-like parsing functions work
as long as they stop when they hit a newline). That, plus a few tests
covering the problematic cases to avoid regressions, would probably be
OK.

I think fsck_tree() is mostly fine, as the tree-iterating code detects
truncation. Though I do find the use of strlen() in decode_tree_entry()
a little suspicious (and that would be true of the current code, as
well, since it powers hash-object's existing parsing check).

-Peff