threads / patch / 29666

patchtag: refuse tag messages that contain NULs

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

## tl;dr

4 messages between Feb 19, 2012 and Feb 21, 2012. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Nguyễn Thái Ngọc Duy· Feb 19, 2012, 11:43 UTC · lore

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(-)
Show changes to 4 files +11 −0

builtin/mktag.c, builtin/tag.c, commit.c, t/t7004-tag.sh

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
Junio C Hamano· Feb 19, 2012, 21:40 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 11 quoted lines
> 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?

Show 13 quoted lines
>  	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?

Nguyễn Thái Ngọc Duy· Feb 20, 2012, 12:38 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 6 files +17 −1

builtin/mktag.c, builtin/tag.c, commit.c, sha1_file.c, t/t7004-tag.sh, t/t7510-signed-commit.sh

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
Junio C Hamano· Feb 21, 2012, 20:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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.

Show 16 quoted lines
>  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?
Show 12 quoted lines
> 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.

← back to recent threads