threads / patch / 56814

patch, 2 partsgpg-interface: handle missing " with " gracefully in parse_ssh_output()

Subject: [PATCH 1/2] gpg-interface: handle missing " with " gracefully in parse_ssh_output()

## tl;dr

8 messages between Oct 30, 2021 and Nov 1, 2021. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

René Scharfe· Oct 30, 2021, 17:04 UTC · lore

If the output of ssh-keygen starts with "Good \"git\" signature for ", but is not followed by " with " for some reason, then parse_ssh_output() uses -1 as the len parameter of xmemdupz(), which in turn will end the program. Reject the signature and carry on instead in that case.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
This code was added after v2.33.0.
Patch formatted with --inter-hunk-context=2 for easier review.

Silly bonus question: What's the purpose of the "+ 1" and "- 1", which seem to cancel each other out?

 gpg-interface.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
Show changes to gpg-interface.c +6 −5
diff --git a/gpg-interface.c b/gpg-interface.c
index 800d8caa67..62d340e78a 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -387,17 +387,19 @@ static void parse_ssh_output(struct signature_check *sigc)
 	line = to_free = xmemdupz(sigc->output, strcspn(sigc->output, "\n"));

 	if (skip_prefix(line, "Good \"git\" signature for ", &line)) {
-		/* Valid signature and known principal */
-		sigc->result = 'G';
-		sigc->trust_level = TRUST_FULLY;
-
 		/* Search for the last "with" to get the full principal */
 		principal = line;
 		do {
 			search = strstr(line, " with ");
 			if (search)
 				line = search + 1;
 		} while (search != NULL);
+		if (line == principal)
+			goto cleanup;
+
+		/* Valid signature and known principal */
+		sigc->result = 'G';
+		sigc->trust_level = TRUST_FULLY;
 		sigc->signer = xmemdupz(principal, line - principal - 1);
 	} else if (skip_prefix(line, "Good \"git\" signature with ", &line)) {
 		/* Valid signature, but key unknown */
--
2.33.1
René Scharfe· Oct 30, 2021, 17:07 UTC · re: René Scharfe · lore

[PATCH 2/2] gpg-interface: avoid buffer overrun in parse_ssh_output()

If the string "key" we found in the output of ssh-keygen happens to be located at the very end of the line, then going four characters further leaves us beyond the end of the string. Explicitly search for the space after "key" to handle a missing one gracefully.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
This code was added after v2.33.0.
 gpg-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to gpg-interface.c +2 −3
diff --git a/gpg-interface.c b/gpg-interface.c
index 62d340e78a..3838536f0a 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -409,9 +409,9 @@ static void parse_ssh_output(struct signature_check *sigc)
 		goto cleanup;
 	}

-	key = strstr(line, "key");
+	key = strstr(line, "key ");
 	if (key) {
-		sigc->fingerprint = xstrdup(strstr(line, "key") + 4);
+		sigc->fingerprint = xstrdup(strstr(line, "key ") + 4);
 		sigc->key = xstrdup(sigc->fingerprint);
 	} else {
 		/*
--
2.33.1
Junio C Hamano· Nov 1, 2021, 06:07 UTC · re: René Scharfe · lore

Re: [PATCH 2/2] gpg-interface: avoid buffer overrun in parse_ssh_output()

René Scharfe <l.s.r@web.de> writes:
Show 8 quoted lines
> If the string "key" we found in the output of ssh-keygen happens to be
> located at the very end of the line, then going four characters further
> leaves us beyond the end of the string.  Explicitly search for the
> space after "key" to handle a missing one gracefully.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> This code was added after v2.33.0.
Thanks.

Fabian, we are in -rc phase where we concentrate on fixing bugs in the new code in 'master'. A quick ack, "here is a better way to fix it", or "no, that won't be needed because..." is very much appreciated.

Show 21 quoted lines
>  gpg-interface.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 62d340e78a..3838536f0a 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -409,9 +409,9 @@ static void parse_ssh_output(struct signature_check *sigc)
>  		goto cleanup;
>  	}
>
> -	key = strstr(line, "key");
> +	key = strstr(line, "key ");
>  	if (key) {
> -		sigc->fingerprint = xstrdup(strstr(line, "key") + 4);
> +		sigc->fingerprint = xstrdup(strstr(line, "key ") + 4);
>  		sigc->key = xstrdup(sigc->fingerprint);
>  	} else {
>  		/*
> --
> 2.33.1
Fabian Stelzer· Nov 1, 2021, 08:33 UTC · re: René Scharfe · lore

Re: [PATCH 2/2] gpg-interface: avoid buffer overrun in parse_ssh_output()

On 30.10.21 19:07, René Scharfe wrote:
Show 31 quoted lines
> If the string "key" we found in the output of ssh-keygen happens to be
> located at the very end of the line, then going four characters further
> leaves us beyond the end of the string.  Explicitly search for the
> space after "key" to handle a missing one gracefully.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> This code was added after v2.33.0.
> 
>  gpg-interface.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 62d340e78a..3838536f0a 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -409,9 +409,9 @@ static void parse_ssh_output(struct signature_check *sigc)
>  		goto cleanup;
>  	}
> 
> -	key = strstr(line, "key");
> +	key = strstr(line, "key ");
>  	if (key) {
> -		sigc->fingerprint = xstrdup(strstr(line, "key") + 4);
> +		sigc->fingerprint = xstrdup(strstr(line, "key ") + 4);
>  		sigc->key = xstrdup(sigc->fingerprint);
>  	} else {
>  		/*
> --
> 2.33.1
> 
Thanks. This is obviously correct.
Junio C Hamano· Nov 1, 2021, 17:44 UTC · re: Fabian Stelzer · lore

Re: [PATCH 2/2] gpg-interface: avoid buffer overrun in parse_ssh_output()

Fabian Stelzer <fs@gigacodes.de> writes:
Show 13 quoted lines
> On 30.10.21 19:07, René Scharfe wrote:
>> If the string "key" we found in the output of ssh-keygen happens to be
>> located at the very end of the line, then going four characters further
>> leaves us beyond the end of the string.  Explicitly search for the
>> space after "key" to handle a missing one gracefully.
>> 
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> This code was added after v2.33.0.
>> 
>> 
>
> Thanks. This is obviously correct.
Thanks, both.  Will apply.
Junio C Hamano· Nov 1, 2021, 06:08 UTC · re: René Scharfe · lore

Re: [PATCH 1/2] gpg-interface: handle missing " with " gracefully in parse_ssh_output()

René Scharfe <l.s.r@web.de> writes:
Show 9 quoted lines
> If the output of ssh-keygen starts with "Good \"git\" signature for ",
> but is not followed by " with " for some reason, then parse_ssh_output()
> uses -1 as the len parameter of xmemdupz(), which in turn will end the
> program.  Reject the signature and carry on instead in that case.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> This code was added after v2.33.0.
> Patch formatted with --inter-hunk-context=2 for easier review.
Nice spotting.
> Silly bonus question: What's the purpose of the "+ 1" and "- 1", which
> seem to cancel each other out?

Fabian, we are in -rc phase where we concentrate on fixing bugs in the new code in 'master'. A quick ack, "here is a better way to fix it", or "no, that won't be needed because..." is very much appreciated.

Thanks.
Show 34 quoted lines
>
>  gpg-interface.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 800d8caa67..62d340e78a 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -387,17 +387,19 @@ static void parse_ssh_output(struct signature_check *sigc)
>  	line = to_free = xmemdupz(sigc->output, strcspn(sigc->output, "\n"));
>
>  	if (skip_prefix(line, "Good \"git\" signature for ", &line)) {
> -		/* Valid signature and known principal */
> -		sigc->result = 'G';
> -		sigc->trust_level = TRUST_FULLY;
> -
>  		/* Search for the last "with" to get the full principal */
>  		principal = line;
>  		do {
>  			search = strstr(line, " with ");
>  			if (search)
>  				line = search + 1;
>  		} while (search != NULL);
> +		if (line == principal)
> +			goto cleanup;
> +
> +		/* Valid signature and known principal */
> +		sigc->result = 'G';
> +		sigc->trust_level = TRUST_FULLY;
>  		sigc->signer = xmemdupz(principal, line - principal - 1);
>  	} else if (skip_prefix(line, "Good \"git\" signature with ", &line)) {
>  		/* Valid signature, but key unknown */
> --
> 2.33.1
Fabian Stelzer· Nov 1, 2021, 08:40 UTC · re: René Scharfe · lore

Re: [PATCH 1/2] gpg-interface: handle missing " with " gracefully in parse_ssh_output()

On 30.10.21 19:04, René Scharfe wrote:
Show 12 quoted lines
> If the output of ssh-keygen starts with "Good \"git\" signature for ",
> but is not followed by " with " for some reason, then parse_ssh_output()
> uses -1 as the len parameter of xmemdupz(), which in turn will end the
> program.  Reject the signature and carry on instead in that case.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> This code was added after v2.33.0.
> Patch formatted with --inter-hunk-context=2 for easier review.
> 
> Silly bonus question: What's the purpose of the "+ 1" and "- 1", which
> seem to cancel each other out?

They do. But only for the xmemdupz. It is important for the strstr() to skip over already found strings (" with " could be in the principal name as well - multiple times even). And doing strstr(line +1, ...) can be problematic for the first iteration.

Show 35 quoted lines
> 
>  gpg-interface.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 800d8caa67..62d340e78a 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -387,17 +387,19 @@ static void parse_ssh_output(struct signature_check *sigc)
>  	line = to_free = xmemdupz(sigc->output, strcspn(sigc->output, "\n"));
> 
>  	if (skip_prefix(line, "Good \"git\" signature for ", &line)) {
> -		/* Valid signature and known principal */
> -		sigc->result = 'G';
> -		sigc->trust_level = TRUST_FULLY;
> -
>  		/* Search for the last "with" to get the full principal */
>  		principal = line;
>  		do {
>  			search = strstr(line, " with ");
>  			if (search)
>  				line = search + 1;
>  		} while (search != NULL);
> +		if (line == principal)
> +			goto cleanup;
> +
> +		/* Valid signature and known principal */
> +		sigc->result = 'G';
> +		sigc->trust_level = TRUST_FULLY;
>  		sigc->signer = xmemdupz(principal, line - principal - 1);
>  	} else if (skip_prefix(line, "Good \"git\" signature with ", &line)) {
>  		/* Valid signature, but key unknown */
> --
> 2.33.1
>

The fix looks good otherwise. Thanks!

René Scharfe· Nov 1, 2021, 18:36 UTC · re: Fabian Stelzer · lore

Re: [PATCH 1/2] gpg-interface: handle missing " with " gracefully in parse_ssh_output()

Am 01.11.21 um 09:40 schrieb Fabian Stelzer:
Show 8 quoted lines
> On 30.10.21 19:04, René Scharfe wrote:
>> Silly bonus question: What's the purpose of the "+ 1" and "- 1", which
>> seem to cancel each other out?
>
> They do. But only for the xmemdupz. It is important for the strstr() to
> skip over already found strings (" with " could be in the principal name
> as well - multiple times even). And doing strstr(line +1, ...) can be
> problematic for the first iteration.
Ah, right.
Show 5 quoted lines
>>  		do {
>>  			search = strstr(line, " with ");
>>  			if (search)
>>  				line = search + 1;
>>  		} while (search != NULL);
This can be shortened to:
		while ((search = strstr(line, " with ")))
			line = search + 1;

I still would have tripped over the +1 / -1, though. Calling an rfind() or strrstr() or find_last() function that deals with it internally would have helped me avoid that, I think, but we don't have use for such a thing anywhere else.

René

← back to recent threads