threads / patch / 38138

patchfsck: properly bound "invalid tag name" error message

Subject: [PATCH] fsck: properly bound "invalid tag name" error message

## tl;dr

10 messages between Dec 8, 2014 and Dec 8, 2014. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Jeff King· Dec 8, 2014, 05:48 UTC · lore

When we detect an invalid tag-name header in a tag object, like, "tag foo bar\n", we feed the pointer starting at "foo bar" to a printf "%s" formatter. This shows the name, as we want, but then it keeps printing the rest of the tag buffer, rather than stopping at the end of the line.

Our tests did not notice because they look only for the matching line, but the bug is that we print much more than we wanted to. So we also adjust the test to be more exact.

Note that when fscking tags with "index-pack --strict", this is even worse. index-pack does not add a trailing NUL-terminator after the object, so we may actually read past the buffer and print uninitialized memory. Running t5302 with valgrind does notice the bug for that reason.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm generally nervous about adding too-specific stderr wording or
formatting to our tests, as I do not want them to be brittle. But I do
not actually think this is substantially different than what other fsck
tests do (i.e., they are already grepping for _half_ the wording
already, so if it changes, they are likely to break, too).

If we care, the test can check test_line_count or similar to make sure there isn't extra data. But I think the way I have written it below is a lot easier for a reader coming later to understand what is going on.

 fsck.c          | 3 ++-
 t/t1450-fsck.sh | 8 ++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)
Show changes to 2 files +8 −3

fsck.c, t/t1450-fsck.sh

diff --git a/fsck.c b/fsck.c
index 2fffa43..88c92e8 100644
--- a/fsck.c
+++ b/fsck.c
@@ -423,7 +423,8 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,
 	}
 	strbuf_addf(&sb, "refs/tags/%.*s", (int)(eol - buffer), buffer);
 	if (check_refname_format(sb.buf, 0))
-		error_func(&tag->object, FSCK_WARN, "invalid 'tag' name: %s", buffer);
+		error_func(&tag->object, FSCK_WARN, "invalid 'tag' name: %.*s",
+			   (int)(eol - buffer), buffer);
 	buffer = eol + 1;
 
 	if (!skip_prefix(buffer, "tagger ", &buffer))
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 019fddd..57ccce5 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -229,8 +229,12 @@ test_expect_success 'tag with incorrect tag name & missing tagger' '
 	echo $tag >.git/refs/tags/wrong &&
 	test_when_finished "git update-ref -d refs/tags/wrong" &&
 	git fsck --tags 2>out &&
-	grep "invalid .tag. name" out &&
-	grep "expected .tagger. line" out
+
+	cat >expect <<-EOF &&
+	warning in tag $tag: invalid '\''tag'\'' name: wrong name format
+	warning in tag $tag: invalid format - expected '\''tagger'\'' line
+	EOF
+	test_cmp expect out
 '
 
 test_expect_success 'tag with bad tagger' '
-- 
2.2.0.390.gf60752d
Jeff King· Dec 8, 2014, 05:57 UTC · re: Jeff King · lore

Re: [PATCH] fsck: properly bound "invalid tag name" error message

On Mon, Dec 08, 2014 at 12:48:12AM -0500, Jeff King wrote:
Show 5 quoted lines
> Note that when fscking tags with "index-pack --strict", this
> is even worse. index-pack does not add a trailing
> NUL-terminator after the object, so we may actually read
> past the buffer and print uninitialized memory. Running
> t5302 with valgrind does notice the bug for that reason.
This merits an additional note (but fortunately not a patch :) ).

After writing the above, I thought for a moment that we might actually read past the end of the buffer in some cases, but I convinced myself otherwise. And I think Dscho and I might have even had this conversation off-list a while ago, but I think it is worth pointing out so that nobody else has to dig into it.

For the most part, we are fine because we parse the object left-to-right, and barf as soon as we see something unusual (and for this reason, fsck_commit_buffer is also fine). The two suspicious places are:

  1. We call strchr(buffer, '\n'), which looks like it could read
     unbounded when "buffer" is not NUL-terminated. However, early in
     the function we confirm that it contains "\n\n", and we will not
     have parsed past that here. Therefore we know that we will always
     hit a newline.
  2. After finding and parsing a line whose trailing newline is marked
     by "eol", we then set "buffer = eol + 1". This would be wrong if
     eol is at the very end of the buffer (the next step would then
     start reading uninitialized memory).
     But again we are saved by the "\n\n" check. The strchr will always
     find the first, so we know that we have at least one character
     after it (and that character is a newline, which cannot be the
     start of a new header, which will cause us to stop parsing).

I do admit that I am tempted to teach index-pack to always NUL-terminate objects in memory that we feed to fsck, just to be on the safe side. It doesn't cost much, and could prevent a silly mistake (either in the future, or one that I missed in my analysis). The fsck code otherwise generally expects to get the output of read_sha1_file, which has the safety-NUL appended.

-Peff
Johannes Schindelin· Dec 8, 2014, 11:17 UTC · re: Jeff King · lore

Re: [PATCH] fsck: properly bound "invalid tag name" error message

Hi,
On Mon, 8 Dec 2014, Jeff King wrote:
Show 9 quoted lines
> On Mon, Dec 08, 2014 at 12:48:12AM -0500, Jeff King wrote:
> 
> > Note that when fscking tags with "index-pack --strict", this is even
> > worse. index-pack does not add a trailing NUL-terminator after the
> > object, so we may actually read past the buffer and print
> > uninitialized memory. Running t5302 with valgrind does notice the bug
> > for that reason.
> 
> This merits an additional note (but fortunately not a patch :) ).

Wehell... your note about index-pack is definitely worth an additional note, and - as you suggest later - probably also a patch. When I started working on the fsck code handling tags, I did note that the tag buffers were not NUL terminated, mainly due to crashes that were really difficult to debug (I had to resort to the sleep loop trick to attach to a process spawned by git-push) after I added code that assumed the buffers to be NUL terminated.

Show 5 quoted lines
> After writing the above, I thought for a moment that we might actually
> read past the end of the buffer in some cases, but I convinced myself
> otherwise. And I think Dscho and I might have even had this conversation
> off-list a while ago, but I think it is worth pointing out so that
> nobody else has to dig into it.

Yep, we discussed this quite a bit. I argued that the safest thing is *not* to assume that the buffers are NUL terminated because it was not obvious to me how to guarantee NUL-terminated object buffers (because the commit objects are reused by the fsck machinery, not re-read).

Show 10 quoted lines
> For the most part, we are fine because we parse the object
> left-to-right, and barf as soon as we see something unusual (and for
> this reason, fsck_commit_buffer is also fine). The two suspicious places
> are:
> 
>   1. We call strchr(buffer, '\n'), which looks like it could read
>      unbounded when "buffer" is not NUL-terminated. However, early in
>      the function we confirm that it contains "\n\n", and we will not
>      have parsed past that here. Therefore we know that we will always
>      hit a newline.
For reference, this is the code:
	https://github.com/git/git/blob/c18b86734113ee2aeb0e140c922c8fbd4accc860/fsck.c#L241-L259
being called by:
	https://github.com/git/git/blob/c18b86734113ee2aeb0e140c922c8fbd4accc860/fsck.c#L308
and
	https://github.com/git/git/blob/c18b86734113ee2aeb0e140c922c8fbd4accc860/fsck.c#L387
Show 9 quoted lines
>   2. After finding and parsing a line whose trailing newline is marked
>      by "eol", we then set "buffer = eol + 1". This would be wrong if
>      eol is at the very end of the buffer (the next step would then
>      start reading uninitialized memory).
> 
>      But again we are saved by the "\n\n" check. The strchr will always
>      find the first, so we know that we have at least one character
>      after it (and that character is a newline, which cannot be the
>      start of a new header, which will cause us to stop parsing).

Exactly. It is unfortunately a little too brittle for my taste, because it would be relatively easy to break the assumption without noticing. For example, in my upcoming patch series allowing to turn specific fsck errors into mere warnings, it would have been potentially very dangerous to allow demoting that error (no end of header found, NUL inside header) to a warning - because that would have allowed the code to go beyond the buffer.

However...
Show 6 quoted lines
> I do admit that I am tempted to teach index-pack to always NUL-terminate
> objects in memory that we feed to fsck, just to be on the safe side. It
> doesn't cost much, and could prevent a silly mistake (either in the
> future, or one that I missed in my analysis). The fsck code otherwise
> generally expects to get the output of read_sha1_file, which has the
> safety-NUL appended.

If we do that, we have to NUL-terminate all of the objects, correct? I mean, even the blobs and the trees and stuff, because we cannot know beforehand what type of object we're gonna read, right?

Ciao, Dscho

Jeff King· Dec 8, 2014, 11:22 UTC · re: Johannes Schindelin · lore

Re: [PATCH] fsck: properly bound "invalid tag name" error message

On Mon, Dec 08, 2014 at 12:17:10PM +0100, Johannes Schindelin wrote:
Show 10 quoted lines
> > I do admit that I am tempted to teach index-pack to always NUL-terminate
> > objects in memory that we feed to fsck, just to be on the safe side. It
> > doesn't cost much, and could prevent a silly mistake (either in the
> > future, or one that I missed in my analysis). The fsck code otherwise
> > generally expects to get the output of read_sha1_file, which has the
> > safety-NUL appended.
> 
> If we do that, we have to NUL-terminate all of the objects, correct? I
> mean, even the blobs and the trees and stuff, because we cannot know
> beforehand what type of object we're gonna read, right?

I think that is right. It should be a pretty simple change. It looks like we already use xmallocz when creating deltas, and we just need to handle regular objects. I think it could be as simple as this one-liner, but I didn't test anything:

Show changes to builtin/index-pack.c +1 −2
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a369f55..390845d 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -447,7 +447,7 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size,
 	if (type == OBJ_BLOB && size > big_file_threshold)
 		buf = fixed_buf;
 	else
-		buf = xmalloc(size);
+		buf = xmallocz(size);
 
 	memset(&stream, 0, sizeof(stream));
 	git_inflate_init(&stream);

-Peff
Duy Nguyen· Dec 8, 2014, 11:28 UTC · re: Jeff King · lore

Re: [PATCH] fsck: properly bound "invalid tag name" error message

On Mon, Dec 08, 2014 at 12:57:06AM -0500, Jeff King wrote:
> I do admit that I am tempted to teach index-pack to always NUL-terminate
> objects in memory that we feed to fsck, just to be on the safe side. It
> doesn't cost much, and could prevent a silly mistake (either in the
> future, or one that I missed in my analysis).

I think I'm missing a "but.." here. Maybe "but I didn't have time". The change looks simple enough. The remaining *alloc in index-pack is either for arrays, or already NUL-terminated (patch_delta), or does explicit boundary check (compare_objects).

It may be interesting to go over `git grep alloc\(` and see if we should use the allocz version instead. I think in some place we do xmalloc(len + 1) which could be replaced with xmallocz(len)

-- 8< --
Subject: [PATCH] index-pack: terminate object buffers with NUL
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/index-pack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/index-pack.c +2 −2
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a369f55..4632117 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -447,7 +447,7 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size,
 	if (type == OBJ_BLOB && size > big_file_threshold)
 		buf = fixed_buf;
 	else
-		buf = xmalloc(size);
+		buf = xmallocz(size);
 
 	memset(&stream, 0, sizeof(stream));
 	git_inflate_init(&stream);
@@ -552,7 +552,7 @@ static void *unpack_data(struct object_entry *obj,
 	git_zstream stream;
 	int status;
 
-	data = xmalloc(consume ? 64*1024 : obj->size);
+	data = xmallocz(consume ? 64*1024 : obj->size);
 	inbuf = xmalloc((len < 64*1024) ? len : 64*1024);
 
 	memset(&stream, 0, sizeof(stream));
-- 
2.2.0.60.gb7b3c64

-- 8< --
Johannes Schindelin· Dec 8, 2014, 11:35 UTC · re: Duy Nguyen · lore

Re: [PATCH] fsck: properly bound "invalid tag name" error message

Hi Duy,
On Mon, 8 Dec 2014, Duy Nguyen wrote:
Show 7 quoted lines
> On Mon, Dec 08, 2014 at 12:57:06AM -0500, Jeff King wrote:
> > I do admit that I am tempted to teach index-pack to always NUL-terminate
> > objects in memory that we feed to fsck, just to be on the safe side. It
> > doesn't cost much, and could prevent a silly mistake (either in the
> > future, or one that I missed in my analysis).
> 
> I think I'm missing a "but.." here.
The "but..."s I have are:
1) we potentially waste space, and
2) I would like to make really certain, preferably with static analysis,
   that fsck_object() only receives buffers that are NUL terminated, and
   that no call path is missed.

The patch looks good, of course, but I lack the broad overview of Git's source code - it has been years since I was familiar enough with it to know the places touching particular functions from the top of my head.

Ciao, Dscho

Jeff King· Dec 8, 2014, 11:47 UTC · re: Johannes Schindelin · lore

Re: [PATCH] fsck: properly bound "invalid tag name" error message

On Mon, Dec 08, 2014 at 12:35:27PM +0100, Johannes Schindelin wrote:
Show 13 quoted lines
> On Mon, 8 Dec 2014, Duy Nguyen wrote:
> 
> > On Mon, Dec 08, 2014 at 12:57:06AM -0500, Jeff King wrote:
> > > I do admit that I am tempted to teach index-pack to always NUL-terminate
> > > objects in memory that we feed to fsck, just to be on the safe side. It
> > > doesn't cost much, and could prevent a silly mistake (either in the
> > > future, or one that I missed in my analysis).
> > 
> > I think I'm missing a "but.." here.
> 
> The "but..."s I have are:
> 
> 1) we potentially waste space, and

I think this can be ignored. It's 1 byte per object, and only while we keep the object in RAM. Also, we already do it for buffers read from read_sha1_file, so when you run "git log" every commit buffer we keep in RAM is already doing this (and has been since basically day one).

> 2) I would like to make really certain, preferably with static analysis,
>    that fsck_object() only receives buffers that are NUL terminated, and
>    that no call path is missed.

I know this is not as good as a real static analysis, but I was concerned about this exact thing about a year ago (I think in relation to commit parsing for pretty-printing) and traced all of the paths through which you can get an object; they all end up in the same few code paths that all xmallocz: unpack_sha1_file for loose objects, unpack_compressed_entry for pack bases, and patch_delta for deltas.

Index-pack and unpack-objects are the odd men out here because they are processing objects that are not actually in the repository yet. I think the spots Duy pointed out probably cover index-pack. It looks like builtin/unpack-objects.c:get_data needs the same treatment.

I know that Duy mentioned a while ago killing off unpack-objects and rolling its functionality into index-pack. That would be a very nice thing to do if somebody can find the time. There's a fair bit of duplication, and index-pack receives a lot more attention (so it's faster, and probably more robust against weird incoming packs).

-Peff
Johannes Schindelin· Dec 8, 2014, 13:46 UTC · re: Jeff King · lore

Re: [PATCH] fsck: properly bound "invalid tag name" error message

Hi Peff,
On Mon, 8 Dec 2014, Jeff King wrote:
Show 20 quoted lines
> On Mon, Dec 08, 2014 at 12:35:27PM +0100, Johannes Schindelin wrote:
> 
> > On Mon, 8 Dec 2014, Duy Nguyen wrote:
> > 
> > > On Mon, Dec 08, 2014 at 12:57:06AM -0500, Jeff King wrote:
> > > > I do admit that I am tempted to teach index-pack to always NUL-terminate
> > > > objects in memory that we feed to fsck, just to be on the safe side. It
> > > > doesn't cost much, and could prevent a silly mistake (either in the
> > > > future, or one that I missed in my analysis).
> > > 
> > > I think I'm missing a "but.." here.
> > 
> > The "but..."s I have are:
> > 
> > 1) we potentially waste space, and
> 
> I think this can be ignored. It's 1 byte per object, and only while we
> keep the object in RAM. Also, we already do it for buffers read from
> read_sha1_file, so when you run "git log" every commit buffer we keep in
> RAM is already doing this (and has been since basically day one).
Fine with me.
Show 10 quoted lines
> > 2) I would like to make really certain, preferably with static analysis,
> >    that fsck_object() only receives buffers that are NUL terminated, and
> >    that no call path is missed.
> 
> I know this is not as good as a real static analysis, but I was
> concerned about this exact thing about a year ago (I think in relation
> to commit parsing for pretty-printing) and traced all of the paths
> through which you can get an object; they all end up in the same few
> code paths that all xmallocz: unpack_sha1_file for loose objects,
> unpack_compressed_entry for pack bases, and patch_delta for deltas.

Thank you for sharing the analysis. This is exactly what I was looking for.

> Index-pack and unpack-objects are the odd men out here because they are
> processing objects that are not actually in the repository yet. I think
> the spots Duy pointed out probably cover index-pack. It looks like
> builtin/unpack-objects.c:get_data needs the same treatment.

I just started working on that. To see the progress, please have a look here:

	https://github.com/dscho/git/pull/5

Ciao, Dscho

Johannes Schindelin· Dec 8, 2014, 14:17 UTC · re: Duy Nguyen · lore

[PATCH v2] index-pack: terminate object buffers with NUL

From: Duy Nguyen <pclouds@gmail.com>

We have some tricky checks in fsck that rely on a side effect of require_end_of_header(), and would otherwise easily run outside non-NUL-terminated buffers. This is a bit brittle, so let's make sure that only NUL-terminated buffers are passed around to begin with.

Jeff "Peff" King contributed the detailed analysis which call paths are involved and pointed out that we also have to patch the get_data() function in unpack-objects.c, which is what Johannes "Dscho" Schindelin implemented.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Analyzed-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	On Mon, 8 Dec 2014, Duy Nguyen wrote:
	> Subject: [PATCH] index-pack: terminate object buffers with NUL
	> 
	> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
	Here is a patch that is updated with Peff's suggested
	unpack-object.c:get_data change.
	While it is not as good as Peff's analysis, I can provide an
	additional data point: the test suite passes cleanly even with
		https://github.com/dscho/git/commit/567ad592
	applied (and with 567ad592, but without below changes, at least
	t1050 does not pass).
 builtin/index-pack.c     | 4 ++--
 builtin/unpack-objects.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 2 files +3 −3

builtin/index-pack.c, builtin/unpack-objects.c

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a369f55..4632117 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -447,7 +447,7 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size,
 	if (type == OBJ_BLOB && size > big_file_threshold)
 		buf = fixed_buf;
 	else
-		buf = xmalloc(size);
+		buf = xmallocz(size);
 
 	memset(&stream, 0, sizeof(stream));
 	git_inflate_init(&stream);
@@ -552,7 +552,7 @@ static void *unpack_data(struct object_entry *obj,
 	git_zstream stream;
 	int status;
 
-	data = xmalloc(consume ? 64*1024 : obj->size);
+	data = xmallocz(consume ? 64*1024 : obj->size);
 	inbuf = xmalloc((len < 64*1024) ? len : 64*1024);
 
 	memset(&stream, 0, sizeof(stream));
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 855d94b..ac66672 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -91,7 +91,7 @@ static void use(int bytes)
 static void *get_data(unsigned long size)
 {
 	git_zstream stream;
-	void *buf = xmalloc(size);
+	void *buf = xmallocz(size);
 
 	memset(&stream, 0, sizeof(stream));
 
-- 
1.8.4.msysgit.0.dirty
Johannes Schindelin· Dec 8, 2014, 11:01 UTC · re: Jeff King · lore

Re: [PATCH] fsck: properly bound "invalid tag name" error message

Hi Peff,
On Mon, 8 Dec 2014, Jeff King wrote:
Show 9 quoted lines
> When we detect an invalid tag-name header in a tag object,
> like, "tag foo bar\n", we feed the pointer starting at "foo
> bar" to a printf "%s" formatter. This shows the name, as we
> want, but then it keeps printing the rest of the tag buffer,
> rather than stopping at the end of the line.
> 
> Our tests did not notice because they look only for the
> matching line, but the bug is that we print much more than
> we wanted to. So we also adjust the test to be more exact.
Good catch, thank you!
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Ciao, Dscho

← back to recent threads