Re: [PATCH] tag: refuse tag messages that contain NULs

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:06

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted hunk

diff --git a/builtin/mktag.c b/builtin/mktag.c
index 640ab64..f51ce20 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c

@@ -165,6 +165,9 @@ int cmd_mktag(int argc, const char **argv, const char *prefix)
 	if (verify_tag(buf.buf, buf.len) < 0)
 		die("invalid tag signature file");
 
+	if (memchr(buf.buf, '\0', buf.len))
+		die("a NUL byte in tag message not allowed.");
+

Is there a good reason why you check _after_ calling verify_tag(), instead
of before?

quoted hunk

 	if (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)
 		die("unable to write tag file");

diff --git a/builtin/tag.c b/builtin/tag.c
index 31f02e8..e66811e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -259,6 +259,8 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
 {
 	if (sign && do_sign(buf) < 0)
 		return error(_("unable to sign the tag"));
+	if (memchr(buf->buf, '\0', buf->len))
+		return error("a NUL byte in tag message not allowed.");

Is there a good reason why you check _after_ calling do_sign(), instead of
before?

[PATCH v2] tag: refuse tag messages that contain NULs

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:53:06

This follows the paranoid spirit in 37576c1 (commit_tree(): refuse
commit messages that contain NULs - 2011-12-15) and stops users from
creating tags that contain NULs. If these tags are merged into a commit
as a mergetag line, it may break the commit header processing badly.

While at it, check for NULs in mergetag and gpgsig commit headers, and
check again at write_sha1_file() as the last resort, mostly to catch
programming errors.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 2012/2/20 Junio C Hamano [off-list ref]:
 >>       if (verify_tag(buf.buf, buf.len) < 0)
 >>               die("invalid tag signature file");
 >>
 >> +     if (memchr(buf.buf, '\0', buf.len))
 >> +             die("a NUL byte in tag message not allowed.");
 >> +
 >
 > Is there a good reason why you check _after_ calling verify_tag(), instead
 > of before?
 [...]
 >>       if (sign && do_sign(buf) < 0)
 >>               return error(_("unable to sign the tag"));
 >> +     if (memchr(buf->buf, '\0', buf->len))
 >> +             return error("a NUL byte in tag message not allowed.");
 >
 > Is there a good reason why you check _after_ calling do_sign(), instead of
 > before?
 
 It's not about after those. It's about right before write_sha1_file().
 I wanted to catch all NULs no matter how they come. But yes the check
 should happen early to avoid wasting user's time (e.g. doing signing)

 So how about this?

 builtin/mktag.c          |    2 ++
 builtin/tag.c            |    2 ++
 commit.c                 |    2 ++
 sha1_file.c              |    6 ++++++
 t/t7004-tag.sh           |    4 ++++
 t/t7510-signed-commit.sh |    2 +-
 6 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/builtin/mktag.c b/builtin/mktag.c
index 640ab64..e579471 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c

@@ -160,6 +160,8 @@ int cmd_mktag(int argc, const char **argv, const char *prefix)
 		die_errno("could not read from stdin");
 	}
 
+	if (memchr(buf.buf, '\0', buf.len))
+		return error("a NUL byte in tag message not allowed.");
 	/* Verify it for some basic sanity: it needs to start with
 	   "object <sha1>\ntype\ntagger " */
 	if (verify_tag(buf.buf, buf.len) < 0)

diff --git a/builtin/tag.c b/builtin/tag.c
index 31f02e8..3e284f7 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -257,6 +257,8 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 
 static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
 {
+	if (memchr(buf->buf, '\0', buf->len))
+		return error("a NUL byte in tag message not allowed.");
 	if (sign && do_sign(buf) < 0)
 		return error(_("unable to sign the tag"));
 	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)

diff --git a/commit.c b/commit.c
index 4b39c19..545325f 100644
--- a/commit.c
+++ b/commit.c

@@ -1144,6 +1144,8 @@ int commit_tree_extended(const struct strbuf *msg, unsigned char *tree,
 		strbuf_addf(&buffer, "encoding %s\n", git_commit_encoding);
 
 	while (extra) {
+		if (memchr(extra->value, '\0', extra->len))
+			return error("a NUL byte in commit header %s not allowed.", extra->key);
 		add_extra_header(&buffer, extra);
 		extra = extra->next;
 	}

diff --git a/sha1_file.c b/sha1_file.c
index 88f2151..2fc8623 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -2519,6 +2519,12 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign
 	char hdr[32];
 	int hdrlen;
 
+	/* GIT_HASH_NUL is for the test suite to hash abitrary content */
+	if (!getenv("GIT_HASH_NUL") &&
+	    (!strcmp(type, commit_type) || !strcmp(type, tag_type)) &&
+	    memchr(buf, '\0', len))
+		return error("BUG: %s message contains NUL.", type);
+
 	/* Normally if we have it in the pack then we do not bother writing
 	 * it out into .git/objects/??/?{38} file.
 	 */

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e93ac73..8cb13e5 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh

@@ -1269,4 +1269,8 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
 	test_must_fail git tag -v -s
 '
 
+test_expect_success 'tag content contains NUL' '
+	test_must_fail git tag -F "$TEST_DIRECTORY"/t3900/UTF-16.txt utf16
+'
+
 test_done

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 1d3c56f..d75a349 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh

@@ -63,7 +63,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
 	git cat-file commit master >raw &&
 	cat raw >forged2 &&
 	echo Qwik | tr "Q" "\000" >>forged2 &&
-	git hash-object -w -t commit forged2 >forged2.commit &&
+	GIT_HASH_NUL=1 git hash-object -w -t commit forged2 >forged2.commit &&
 	git show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&
 	grep "BAD signature from" actual2 &&
 	! grep "Good signature from" actual2

-- 
1.7.8.36.g69ee2

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help