diff mbox series

mailinfo -b: fix an out of bounds access

Message ID pull.1372.git.1664789011089.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 3ef1494685dea925d4e98ed06d9ea3fb5b3ecb89
Headers show
Series mailinfo -b: fix an out of bounds access | expand

Commit Message

Phillip Wood Oct. 3, 2022, 9:23 a.m. UTC 
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(-)


base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14

Comments

Junio C Hamano Oct. 3, 2022, 4:02 p.m. UTC | #1 
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

> 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.

> 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
diff mbox series

Patch

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"