* [PATCH] tag: refuse tag messages that contain NULs @ 2012-02-19 11:43 Nguyễn Thái Ngọc Duy 2012-02-19 21:40 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2012-02-19 11:43 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] tag: refuse tag messages that contain NULs 2012-02-19 11:43 [PATCH] tag: refuse tag messages that contain NULs Nguyễn Thái Ngọc Duy @ 2012-02-19 21:40 ` Junio C Hamano 2012-02-20 12:38 ` [PATCH v2] " Nguyễn Thái Ngọc Duy 0 siblings, 1 reply; 4+ messages in thread From: Junio C Hamano @ 2012-02-19 21:40 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git 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? ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] tag: refuse tag messages that contain NULs 2012-02-19 21:40 ` Junio C Hamano @ 2012-02-20 12:38 ` Nguyễn Thái Ngọc Duy 2012-02-21 20:47 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2012-02-20 12:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] tag: refuse tag messages that contain NULs 2012-02-20 12:38 ` [PATCH v2] " Nguyễn Thái Ngọc Duy @ 2012-02-21 20:47 ` Junio C Hamano 0 siblings, 0 replies; 4+ messages in thread From: Junio C Hamano @ 2012-02-21 20:47 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-02-21 20:47 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-19 11:43 [PATCH] tag: refuse tag messages that contain NULs Nguyễn Thái Ngọc Duy 2012-02-19 21:40 ` Junio C Hamano 2012-02-20 12:38 ` [PATCH v2] " Nguyễn Thái Ngọc Duy 2012-02-21 20:47 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).