[PATCH] tag: refuse tag messages that contain NULs

[PATCH] tag: refuse tag messages that contain NULs

[Nguyễn Thái Ngọc Duy]

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 mergetag lines, they may break the commit header processing badly.

While at it, check for NULs in mergetag and gpgsig lines before
committing too.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/mktag.c |    3 +++
 builtin/tag.c   |    2 ++
 commit.c        |    2 ++
 t/t7004-tag.sh  |    4 ++++
 4 files changed, 11 insertions(+), 0 deletions(-)

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.");
+
 	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.");
 	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
 		return error(_("unable to write tag file"));
 	return 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/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
-- 
1.7.8.36.g69ee2

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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

>  	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

[Nguyễn Thái Ngọc Duy]

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 <pclouds@gmail.com>
---
 2012/2/20 Junio C Hamano <gitster@pobox.com>:
 >>       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

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

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

No, it is not about that.  It is about checking _the input_.  If we had
bugs in do_sign() that adds what we do not want, that is not a user's
fault and "a NUL byte in tag message not allowed" is an inappropriate
thing to give to the user.

And giving "We screwed up and added NUL that you cannot work around to
remove, sorry, you hit a bug." is not very useful.

>  So how about this?
> ...
> 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);
> +

This is yucky.  Is this really worth it?

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

This is caught without the change to write_sha1_file(), isn't it?  If so,
I would say we should drop that GIT_HASH_NUL hunk.