Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore

Am Tue, 10 Jul 2018 11:49:31 -0400
schrieb Jeff King [off-list ref]:

On Tue, Jul 10, 2018 at 10:52:28AM +0200, Henning Schild wrote:

quoted

gnupg does print the keyid followed by a space and the signer comes
next. The same pattern is also used in gpgsm, but there the key
length would be 40 instead of 16. Instead of hardcoding the
expected length, find the first space and calculate it.  

Sounds good, but I think there's an off-by-one in the patch.

quoted

diff --git a/gpg-interface.c b/gpg-interface.c
index 0a8d1bff3..ac2df498d 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c

@@ -88,10 +88,11 @@ static void parse_gpg_output(struct

signature_check *sigc) sigc->result = sigcheck_gpg_status[i].result;
 		/* The trust messages are not followed by
key/signer information */ if (sigc->result != 'U') {
-			sigc->key = xmemdupz(found, 16);
+			next = strchrnul(found, ' ');
+			sigc->key = xmemdupz(found, next -
found);  

Here "next" may point to the trailing NUL of the string...

quoted

 			/* The ERRSIG message is not followed by
signer information */ if (sigc-> result != 'E') {
-				found += 17;
+				found = next + 1;
 				next = strchrnul(found, '\n');  

...in which case "found" points past the end of the string, and we
search random memory. That's presumably impossible with well-formed
gpg output (you don't get 'E' without an extra message), but we
should be robust against bogus input.

In the general case you need:

  found = *next ? next + 1 : next;

or similar. In this case, you can actually do:

  found = next;

because we know that it's OK to search over the literal space again.
But that's pretty subtle, so we're probably better off just doing the
conditional above.

(And yes, looking at the existing code, I think it's even worse, as
there does not seem to be a guarantee that we even have 16 characters
in the string).

The existing code works only on expected output and the same is true
for the version after this patch. Making the parser robust against
random input would imho be a sort of cleanup patch on top of my
series. .. or before, in which case i would become responsible for
making sure that still works after my modification.
This argument is twofold. I do not really want to fix that as well and
it might be a good idea to separate concerns anyways.

Henning

-Peff