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

> On Tue, Jul 10, 2018 at 10:52:28AM +0200, Henning Schild wrote:
> 
> > 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.
> 
> > 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...
> 
> >  			/* 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).