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

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 <redacted>
---
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(-)

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

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

On 30.10.21 19:07, René Scharfe wrote:

Thanks. This is obviously correct.

On 30.10.21 19:04, René Scharfe wrote:

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

The fix looks good otherwise.
Thanks!

Am 01.11.21 um 09:40 schrieb Fabian Stelzer:

On 30.10.21 19:04, René Scharfe wrote:

quoted

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.

quoted

 		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é