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

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

[Jeff King]

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

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

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

[Jeff King]

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 :) ).

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

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

[Johannes Schindelin]

Hi Peff,

On Mon, 8 Dec 2014, Jeff King wrote:

> 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

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

[Johannes Schindelin]

Hi,

On Mon, 8 Dec 2014, Jeff King wrote:

> 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.

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

> 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

>   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...

> 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

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

[Jeff King]

On Mon, Dec 08, 2014 at 12:17:10PM +0100, Johannes Schindelin 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). 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:

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

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

[Duy Nguyen]

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

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

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

[Johannes Schindelin]

Hi Duy,

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

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

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

[Jeff King]

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

> 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

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

[Johannes Schindelin]

Hi Peff,

On Mon, 8 Dec 2014, Jeff King wrote:

> 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.

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2550 bytes --]

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

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