[PATCH] mailinfo -b: fix an out of bounds access

Phillip Wood via GitGitGadget· Oct 3, 2022, 09:23 UTC · lore

From: Phillip Wood <phillip.wood@dunelm.org.uk>

To remove bracketed strings containing "PATCH" from the subject line cleanup_subject() scans the subject for the opening bracket using an offset from the beginning of the line. It then searches for the closing bracket with strchr(). To calculate the length of the bracketed string it unfortunately adds rather than subtracts the offset from the result of strchr(). This leads to an out of bounds access in memmem() when looking to see if the brackets contain "PATCH".

We have tests that trigger this bug that were added in ae52d57f0b (t5100: add some more mailinfo tests, 2017-05-31). The commit message mentions that they are marked test_expect_failure as they trigger an assertion in strbuf_splice(). While it is reassuring that strbuf_splice() detects the problem and dies in retrospect that should perhaps have warranted a little more investigation. The bug was introduced by 17635fc900 (mailinfo: -b option keeps [bracketed] strings that is not a [PATCH] marker, 2009-07-15). I think the reason it has survived so long is that '-b' is not a popular option and without it the offset is always zero.

This was found by the address sanitizer while I was cleaning up the test_todo idea in [1].

[1] https://lore.kernel.org/git/db558292-2783-3270-4824-43757822a389@gmail.com/

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
    mailinfo -b: fix an out of bounds access
    
    I wonder if instead of dying when an assertion fails in strbuf_splice()
    it should call BUG() instead.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1372%2Fphillipwood%2Fmailinfo-b-fix-out-of-bounds-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1372/phillipwood/mailinfo-b-fix-out-of-bounds-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1372

 mailinfo.c          | 2 +-
 t/t5100-mailinfo.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Show changes to 2 files +3 −3

mailinfo.c, t/t5100-mailinfo.sh

diff --git a/mailinfo.c b/mailinfo.c
index 9621ba62a39..833d28612f7 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -317,7 +317,7 @@ static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject)
 			pos = strchr(subject->buf + at, ']');
 			if (!pos)
 				break;
-			remove = pos - subject->buf + at + 1;
+			remove = pos - (subject->buf + at) + 1;
 			if (!mi->keep_non_patch_brackets_in_subject ||
 			    (7 <= remove &&
 			     memmem(subject->buf + at, remove, "PATCH", 5)))
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index cebad1048cf..db11cababd3 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -201,13 +201,13 @@ test_expect_success 'mailinfo -b double [PATCH]' '
 	test z"$subj" = z"Subject: message"
 '
 
-test_expect_failure 'mailinfo -b trailing [PATCH]' '
+test_expect_success 'mailinfo -b trailing [PATCH]' '
 	subj="$(echo "Subject: [other] [PATCH] message" |
 		git mailinfo -b /dev/null /dev/null)" &&
 	test z"$subj" = z"Subject: [other] message"
 '
 
-test_expect_failure 'mailinfo -b separated double [PATCH]' '
+test_expect_success 'mailinfo -b separated double [PATCH]' '
 	subj="$(echo "Subject: [PATCH] [other] [PATCH] message" |
 		git mailinfo -b /dev/null /dev/null)" &&
 	test z"$subj" = z"Subject: [other] message"

base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14

-- 
gitgitgadget

Junio C Hamano· Oct 3, 2022, 16:02 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH] mailinfo -b: fix an out of bounds access

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

> We have tests that trigger this bug that were added in ae52d57f0b
> (t5100: add some more mailinfo tests, 2017-05-31). The commit message
> mentions that they are marked test_expect_failure as they trigger an
> assertion in strbuf_splice(). While it is reassuring that
> strbuf_splice() detects the problem and dies in retrospect that should
> perhaps have warranted a little more investigation.

Indeed.

Show 10 quoted lines

> diff --git a/mailinfo.c b/mailinfo.c
> index 9621ba62a39..833d28612f7 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -317,7 +317,7 @@ static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject)
>  			pos = strchr(subject->buf + at, ']');
>  			if (!pos)
>  				break;
> -			remove = pos - subject->buf + at + 1;
> +			remove = pos - (subject->buf + at) + 1;

How embarrassing.

It really is (&pos[1] - &subject->buf[at]), subtracting from the address of one past the closing ']' and the address of opening '[', so the rewrite is correct.

Thanks.

Show 22 quoted lines

> diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
> index cebad1048cf..db11cababd3 100755
> --- a/t/t5100-mailinfo.sh
> +++ b/t/t5100-mailinfo.sh
> @@ -201,13 +201,13 @@ test_expect_success 'mailinfo -b double [PATCH]' '
>  	test z"$subj" = z"Subject: message"
>  '
>  
> -test_expect_failure 'mailinfo -b trailing [PATCH]' '
> +test_expect_success 'mailinfo -b trailing [PATCH]' '
>  	subj="$(echo "Subject: [other] [PATCH] message" |
>  		git mailinfo -b /dev/null /dev/null)" &&
>  	test z"$subj" = z"Subject: [other] message"
>  '
>  
> -test_expect_failure 'mailinfo -b separated double [PATCH]' '
> +test_expect_success 'mailinfo -b separated double [PATCH]' '
>  	subj="$(echo "Subject: [PATCH] [other] [PATCH] message" |
>  		git mailinfo -b /dev/null /dev/null)" &&
>  	test z"$subj" = z"Subject: [other] message"
>
> base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14

patchmailinfo -b: fix an out of bounds access