{"thread":{"id":"58543","subject":"[PATCH] mailinfo -b: fix an out of bounds access","startedAt":"2022-10-03T09:23:44Z","lastAt":"2022-10-03T16:02:17Z","messageCount":2,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"464065","messageId":"pull.1372.git.1664789011089.gitgitgadget@gmail.com","threadId":"58543","inReplyTo":null,"subject":"[PATCH] mailinfo -b: fix an out of bounds access","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-03T09:23:30Z","receivedAt":"2022-10-03T09:23:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nTo remove bracketed strings containing \"PATCH\" from the subject line\ncleanup_subject() scans the subject for the opening bracket using an\noffset from the beginning of the line. It then searches for the\nclosing bracket with strchr(). To calculate the length of the\nbracketed string it unfortunately adds rather than subtracts the\noffset from the result of strchr(). This leads to an out of bounds\naccess in memmem() when looking to see if the brackets contain\n\"PATCH\".\n\nWe have tests that trigger this bug that were added in ae52d57f0b\n(t5100: add some more mailinfo tests, 2017-05-31). The commit message\nmentions that they are marked test_expect_failure as they trigger an\nassertion in strbuf_splice(). While it is reassuring that\nstrbuf_splice() detects the problem and dies in retrospect that should\nperhaps have warranted a little more investigation. The bug was\nintroduced by 17635fc900 (mailinfo: -b option keeps [bracketed]\nstrings that is not a [PATCH] marker, 2009-07-15). I think the reason\nit has survived so long is that '-b' is not a popular option and\nwithout it the offset is always zero.\n\nThis was found by the address sanitizer while I was cleaning up the\ntest_todo idea in [1].\n\n[1] https://lore.kernel.org/git/db558292-2783-3270-4824-43757822a389@gmail.com/\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    mailinfo -b: fix an out of bounds access\n    \n    I wonder if instead of dying when an assertion fails in strbuf_splice()\n    it should call BUG() instead.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1372%2Fphillipwood%2Fmailinfo-b-fix-out-of-bounds-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1372/phillipwood/mailinfo-b-fix-out-of-bounds-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1372\n\n mailinfo.c          | 2 +-\n t/t5100-mailinfo.sh | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 9621ba62a39..833d28612f7 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -317,7 +317,7 @@ static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject)\n \t\t\tpos = strchr(subject->buf + at, ']');\n \t\t\tif (!pos)\n \t\t\t\tbreak;\n-\t\t\tremove = pos - subject->buf + at + 1;\n+\t\t\tremove = pos - (subject->buf + at) + 1;\n \t\t\tif (!mi->keep_non_patch_brackets_in_subject ||\n \t\t\t    (7 <= remove &&\n \t\t\t     memmem(subject->buf + at, remove, \"PATCH\", 5)))\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex cebad1048cf..db11cababd3 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -201,13 +201,13 @@ test_expect_success 'mailinfo -b double [PATCH]' '\n \ttest z\"$subj\" = z\"Subject: message\"\n '\n \n-test_expect_failure 'mailinfo -b trailing [PATCH]' '\n+test_expect_success 'mailinfo -b trailing [PATCH]' '\n \tsubj=\"$(echo \"Subject: [other] [PATCH] message\" |\n \t\tgit mailinfo -b /dev/null /dev/null)\" &&\n \ttest z\"$subj\" = z\"Subject: [other] message\"\n '\n \n-test_expect_failure 'mailinfo -b separated double [PATCH]' '\n+test_expect_success 'mailinfo -b separated double [PATCH]' '\n \tsubj=\"$(echo \"Subject: [PATCH] [other] [PATCH] message\" |\n \t\tgit mailinfo -b /dev/null /dev/null)\" &&\n \ttest z\"$subj\" = z\"Subject: [other] message\"\n\nbase-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14\n-- \ngitgitgadget\n"},{"id":"464080","messageId":"xmqq8rlwyjy6.fsf@gitster.g","threadId":"58543","inReplyTo":"pull.1372.git.1664789011089.gitgitgadget@gmail.com","subject":"Re: [PATCH] mailinfo -b: fix an out of bounds access","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-03T16:02:09Z","receivedAt":"2022-10-03T16:02:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> We have tests that trigger this bug that were added in ae52d57f0b\n> (t5100: add some more mailinfo tests, 2017-05-31). The commit message\n> mentions that they are marked test_expect_failure as they trigger an\n> assertion in strbuf_splice(). While it is reassuring that\n> strbuf_splice() detects the problem and dies in retrospect that should\n> perhaps have warranted a little more investigation.\n\nIndeed.\n\n> diff --git a/mailinfo.c b/mailinfo.c\n> index 9621ba62a39..833d28612f7 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -317,7 +317,7 @@ static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject)\n>  \t\t\tpos = strchr(subject->buf + at, ']');\n>  \t\t\tif (!pos)\n>  \t\t\t\tbreak;\n> -\t\t\tremove = pos - subject->buf + at + 1;\n> +\t\t\tremove = pos - (subject->buf + at) + 1;\n\nHow embarrassing.  \n\nIt really is (&pos[1] - &subject->buf[at]), subtracting from the\naddress of one past the closing ']' and the address of opening '[',\nso the rewrite is correct.\n\nThanks.\n\n> diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\n> index cebad1048cf..db11cababd3 100755\n> --- a/t/t5100-mailinfo.sh\n> +++ b/t/t5100-mailinfo.sh\n> @@ -201,13 +201,13 @@ test_expect_success 'mailinfo -b double [PATCH]' '\n>  \ttest z\"$subj\" = z\"Subject: message\"\n>  '\n>  \n> -test_expect_failure 'mailinfo -b trailing [PATCH]' '\n> +test_expect_success 'mailinfo -b trailing [PATCH]' '\n>  \tsubj=\"$(echo \"Subject: [other] [PATCH] message\" |\n>  \t\tgit mailinfo -b /dev/null /dev/null)\" &&\n>  \ttest z\"$subj\" = z\"Subject: [other] message\"\n>  '\n>  \n> -test_expect_failure 'mailinfo -b separated double [PATCH]' '\n> +test_expect_success 'mailinfo -b separated double [PATCH]' '\n>  \tsubj=\"$(echo \"Subject: [PATCH] [other] [PATCH] message\" |\n>  \t\tgit mailinfo -b /dev/null /dev/null)\" &&\n>  \ttest z\"$subj\" = z\"Subject: [other] message\"\n>\n> base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14\n"}]}