{"thread":{"id":"29666","subject":"[PATCH] tag: refuse tag messages that contain NULs","startedAt":"2012-02-19T11:43:57Z","lastAt":"2012-02-21T20:47:06Z","messageCount":4,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"184947","messageId":"1329651837-1102-1-git-send-email-pclouds@gmail.com","threadId":"29666","inReplyTo":null,"subject":"[PATCH] tag: refuse tag messages that contain NULs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-19T11:43:57Z","receivedAt":"2012-02-19T11:43:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This follows the paranoid spirit in 37576c1 (commit_tree(): refuse\ncommit messages that contain NULs - 2011-12-15) and stops users from\ncreating tags that contain NULs. If these tags are merged into a commit\nas mergetag lines, they may break the commit header processing badly.\n\nWhile at it, check for NULs in mergetag and gpgsig lines before\ncommitting too.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/mktag.c |    3 +++\n builtin/tag.c   |    2 ++\n commit.c        |    2 ++\n t/t7004-tag.sh  |    4 ++++\n 4 files changed, 11 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/mktag.c b/builtin/mktag.c\nindex 640ab64..f51ce20 100644\n--- a/builtin/mktag.c\n+++ b/builtin/mktag.c\n@@ -165,6 +165,9 @@ int cmd_mktag(int argc, const char **argv, const char *prefix)\n \tif (verify_tag(buf.buf, buf.len) < 0)\n \t\tdie(\"invalid tag signature file\");\n \n+\tif (memchr(buf.buf, '\\0', buf.len))\n+\t\tdie(\"a NUL byte in tag message not allowed.\");\n+\n \tif (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)\n \t\tdie(\"unable to write tag file\");\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 31f02e8..e66811e 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -259,6 +259,8 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)\n {\n \tif (sign && do_sign(buf) < 0)\n \t\treturn error(_(\"unable to sign the tag\"));\n+\tif (memchr(buf->buf, '\\0', buf->len))\n+\t\treturn error(\"a NUL byte in tag message not allowed.\");\n \tif (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)\n \t\treturn error(_(\"unable to write tag file\"));\n \treturn 0;\ndiff --git a/commit.c b/commit.c\nindex 4b39c19..545325f 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1144,6 +1144,8 @@ int commit_tree_extended(const struct strbuf *msg, unsigned char *tree,\n \t\tstrbuf_addf(&buffer, \"encoding %s\\n\", git_commit_encoding);\n \n \twhile (extra) {\n+\t\tif (memchr(extra->value, '\\0', extra->len))\n+\t\t\treturn error(\"a NUL byte in commit header %s not allowed.\", extra->key);\n \t\tadd_extra_header(&buffer, extra);\n \t\textra = extra->next;\n \t}\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex e93ac73..8cb13e5 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1269,4 +1269,8 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '\n \ttest_must_fail git tag -v -s\n '\n \n+test_expect_success 'tag content contains NUL' '\n+\ttest_must_fail git tag -F \"$TEST_DIRECTORY\"/t3900/UTF-16.txt utf16\n+'\n+\n test_done\n-- \n1.7.8.36.g69ee2\n"},{"id":"184953","messageId":"7v39a6pkmb.fsf@alter.siamese.dyndns.org","threadId":"29666","inReplyTo":"1329651837-1102-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] tag: refuse tag messages that contain NULs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-19T21:40:44Z","receivedAt":"2012-02-19T21:40:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> diff --git a/builtin/mktag.c b/builtin/mktag.c\n> index 640ab64..f51ce20 100644\n> --- a/builtin/mktag.c\n> +++ b/builtin/mktag.c\n> @@ -165,6 +165,9 @@ int cmd_mktag(int argc, const char **argv, const char *prefix)\n>  \tif (verify_tag(buf.buf, buf.len) < 0)\n>  \t\tdie(\"invalid tag signature file\");\n>  \n> +\tif (memchr(buf.buf, '\\0', buf.len))\n> +\t\tdie(\"a NUL byte in tag message not allowed.\");\n> +\n\nIs there a good reason why you check _after_ calling verify_tag(), instead\nof before?\n\n>  \tif (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)\n>  \t\tdie(\"unable to write tag file\");\n>  \n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 31f02e8..e66811e 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -259,6 +259,8 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)\n>  {\n>  \tif (sign && do_sign(buf) < 0)\n>  \t\treturn error(_(\"unable to sign the tag\"));\n> +\tif (memchr(buf->buf, '\\0', buf->len))\n> +\t\treturn error(\"a NUL byte in tag message not allowed.\");\n\nIs there a good reason why you check _after_ calling do_sign(), instead of\nbefore?\n"},{"id":"184985","messageId":"1329741483-22567-1-git-send-email-pclouds@gmail.com","threadId":"29666","inReplyTo":"7v39a6pkmb.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] tag: refuse tag messages that contain NULs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-20T12:38:03Z","receivedAt":"2012-02-20T12:38:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This follows the paranoid spirit in 37576c1 (commit_tree(): refuse\ncommit messages that contain NULs - 2011-12-15) and stops users from\ncreating tags that contain NULs. If these tags are merged into a commit\nas a mergetag line, it may break the commit header processing badly.\n\nWhile at it, check for NULs in mergetag and gpgsig commit headers, and\ncheck again at write_sha1_file() as the last resort, mostly to catch\nprogramming errors.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n 2012/2/20 Junio C Hamano <gitster@pobox.com>:\n >>       if (verify_tag(buf.buf, buf.len) < 0)\n >>               die(\"invalid tag signature file\");\n >>\n >> +     if (memchr(buf.buf, '\\0', buf.len))\n >> +             die(\"a NUL byte in tag message not allowed.\");\n >> +\n >\n > Is there a good reason why you check _after_ calling verify_tag(), instead\n > of before?\n [...]\n >>       if (sign && do_sign(buf) < 0)\n >>               return error(_(\"unable to sign the tag\"));\n >> +     if (memchr(buf->buf, '\\0', buf->len))\n >> +             return error(\"a NUL byte in tag message not allowed.\");\n >\n > Is there a good reason why you check _after_ calling do_sign(), instead of\n > before?\n \n It's not about after those. It's about right before write_sha1_file().\n I wanted to catch all NULs no matter how they come. But yes the check\n should happen early to avoid wasting user's time (e.g. doing signing)\n\n So how about this?\n\n builtin/mktag.c          |    2 ++\n builtin/tag.c            |    2 ++\n commit.c                 |    2 ++\n sha1_file.c              |    6 ++++++\n t/t7004-tag.sh           |    4 ++++\n t/t7510-signed-commit.sh |    2 +-\n 6 files changed, 17 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/mktag.c b/builtin/mktag.c\nindex 640ab64..e579471 100644\n--- a/builtin/mktag.c\n+++ b/builtin/mktag.c\n@@ -160,6 +160,8 @@ int cmd_mktag(int argc, const char **argv, const char *prefix)\n \t\tdie_errno(\"could not read from stdin\");\n \t}\n \n+\tif (memchr(buf.buf, '\\0', buf.len))\n+\t\treturn error(\"a NUL byte in tag message not allowed.\");\n \t/* Verify it for some basic sanity: it needs to start with\n \t   \"object <sha1>\\ntype\\ntagger \" */\n \tif (verify_tag(buf.buf, buf.len) < 0)\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 31f02e8..3e284f7 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -257,6 +257,8 @@ static void write_tag_body(int fd, const unsigned char *sha1)\n \n static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)\n {\n+\tif (memchr(buf->buf, '\\0', buf->len))\n+\t\treturn error(\"a NUL byte in tag message not allowed.\");\n \tif (sign && do_sign(buf) < 0)\n \t\treturn error(_(\"unable to sign the tag\"));\n \tif (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)\ndiff --git a/commit.c b/commit.c\nindex 4b39c19..545325f 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1144,6 +1144,8 @@ int commit_tree_extended(const struct strbuf *msg, unsigned char *tree,\n \t\tstrbuf_addf(&buffer, \"encoding %s\\n\", git_commit_encoding);\n \n \twhile (extra) {\n+\t\tif (memchr(extra->value, '\\0', extra->len))\n+\t\t\treturn error(\"a NUL byte in commit header %s not allowed.\", extra->key);\n \t\tadd_extra_header(&buffer, extra);\n \t\textra = extra->next;\n \t}\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 88f2151..2fc8623 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2519,6 +2519,12 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n \tchar hdr[32];\n \tint hdrlen;\n \n+\t/* GIT_HASH_NUL is for the test suite to hash abitrary content */\n+\tif (!getenv(\"GIT_HASH_NUL\") &&\n+\t    (!strcmp(type, commit_type) || !strcmp(type, tag_type)) &&\n+\t    memchr(buf, '\\0', len))\n+\t\treturn error(\"BUG: %s message contains NUL.\", type);\n+\n \t/* Normally if we have it in the pack then we do not bother writing\n \t * it out into .git/objects/??/?{38} file.\n \t */\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex e93ac73..8cb13e5 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1269,4 +1269,8 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '\n \ttest_must_fail git tag -v -s\n '\n \n+test_expect_success 'tag content contains NUL' '\n+\ttest_must_fail git tag -F \"$TEST_DIRECTORY\"/t3900/UTF-16.txt utf16\n+'\n+\n test_done\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 1d3c56f..d75a349 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -63,7 +63,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '\n \tgit cat-file commit master >raw &&\n \tcat raw >forged2 &&\n \techo Qwik | tr \"Q\" \"\\000\" >>forged2 &&\n-\tgit hash-object -w -t commit forged2 >forged2.commit &&\n+\tGIT_HASH_NUL=1 git hash-object -w -t commit forged2 >forged2.commit &&\n \tgit show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&\n \tgrep \"BAD signature from\" actual2 &&\n \t! grep \"Good signature from\" actual2\n-- \n1.7.8.36.g69ee2\n"},{"id":"185100","messageId":"7vpqd7gbhx.fsf@alter.siamese.dyndns.org","threadId":"29666","inReplyTo":"1329741483-22567-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] tag: refuse tag messages that contain NULs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-21T20:47:06Z","receivedAt":"2012-02-21T20:47:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n>  It's not about after those. It's about right before write_sha1_file().\n>  I wanted to catch all NULs no matter how they come. But yes the check\n>  should happen early to avoid wasting user's time (e.g. doing signing)\n\nNo, it is not about that.  It is about checking _the input_.  If we had\nbugs in do_sign() that adds what we do not want, that is not a user's\nfault and \"a NUL byte in tag message not allowed\" is an inappropriate\nthing to give to the user.\n\nAnd giving \"We screwed up and added NUL that you cannot work around to\nremove, sorry, you hit a bug.\" is not very useful.\n\n>  So how about this?\n> ...\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 88f2151..2fc8623 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2519,6 +2519,12 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n>  \tchar hdr[32];\n>  \tint hdrlen;\n>  \n> +\t/* GIT_HASH_NUL is for the test suite to hash abitrary content */\n> +\tif (!getenv(\"GIT_HASH_NUL\") &&\n> +\t    (!strcmp(type, commit_type) || !strcmp(type, tag_type)) &&\n> +\t    memchr(buf, '\\0', len))\n> +\t\treturn error(\"BUG: %s message contains NUL.\", type);\n> +\n\nThis is yucky.  Is this really worth it?\n\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index e93ac73..8cb13e5 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -1269,4 +1269,8 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '\n>  \ttest_must_fail git tag -v -s\n>  '\n>  \n> +test_expect_success 'tag content contains NUL' '\n> +\ttest_must_fail git tag -F \"$TEST_DIRECTORY\"/t3900/UTF-16.txt utf16\n> +'\n> +\n\nThis is caught without the change to write_sha1_file(), isn't it?  If so,\nI would say we should drop that GIT_HASH_NUL hunk.\n"}]}