Re: [PATCH 1/1] gpg-interface: limit search for primary key fingerprint

[Junio C Hamano <gitster@pobox.com>]

Hans Jerry Illikainen <hji@dyntopia.com> writes:

> The VALIDSIG status line from GnuPG with --status-fd has a field that
> specifies the fingerprint of the primary key that made the signature.
> However, that field is only available for OpenPGP signatures; not for
> CMS/X.509.
>
> An unbounded search for a non-existent primary key fingerprint for X509
> signatures results in the following status line being interpreted as the
> fingerprint.

The above two paragraphs give us an excellent explanation of what
happens in today's code.  What needs to follow is a description of
the approach taken to solve the problem in such a way that helps
readers to agree or disagree with the patch.  It needs to convince
them of at least two things:

 - The change is necessary to avoid a wrong line from getting
   mistaken as the fingerprint with CMS/X.509 sigs, and instead we
   say "there is no fingerprint" on X.509 sig (or whatever happens
   with this change---I cannot tell what ramifications lack of the
   fingerprint has to the callers of this function offhand, or how
   the lack of fingerprint is reported back to the callers, but the
   proposed log message must talk about it).

 - The change safely keeps identifying the right fingerprint with
   OpenPGP sigs because the primary fingerprint, if shown, must be
   on the same line (or whatever reason why it makes it safe---I do
   not offhand know if this is guaranteed how and by whom [*1*], but
   you must have researched it before sending this patch, so please
   write it down to help readers).

>  				/* Do we have fingerprint? */
>  				if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
> +					const char *limit;
> +

I wonder if it is simpler to define it next to 'next'.  Yes, this
new variable is used only within this block, but it also gets used
only in conjunction with that other variable.

>  					next = strchrnul(line, ' ');
>  					free(sigc->fingerprint);
>  					sigc->fingerprint = xmemdupz(line, next - line);

So, we skipped "VALIDSIG " and grabbed the first field <fingerprint>
in sigc->fingerprint.  Then we used to unconditionally skip 9 SP
separated fields.  But there may only be 8 fields on the line, which
is why this patch is needed.

> -					/* Skip interim fields */
> +					/* Skip interim fields.  The search is
> +					 * limited to the same line since only
> +					 * OpenPGP signatures has a field with
> +					 * the primary fingerprint. */

	/*
	 * A multi-line comment of ours looks like this; the
	 * slash-asterisk that begins it, and the asterisk-slash
	 * that ends it, are on their own lines, without anything
	 * else but the indentation.
	 */

> +					limit = strchrnul(line, '\n');
>  					for (j = 9; j > 0; j--) {
> -						if (!*next)
> +						if (!*next || next >= limit)
>  							break;

This makes sure that a premature exit (i.e. "0 < j") means we ran
out of the fields.  

I'd prefer to write it "limit <= next" to help visualizing the two
values (on a single line, lower values come left, higher ones come
right), by the way.  That is a minor point.

A bigger question is, when this happens, what value do we want to
leave in sigc->primary_key_fingerprint?  As we can see from the
original code that makes sure the old value in the field will not
leak by first free()ing, it seems that it is possible in this code
that the field may not be NULL, but we just saw that on _our_
signature verification system, the primary key is not available.
Shouldn't we be nulling it out, after free()ing possibly leftover
value in the field?

>  						line = next + 1;
>  						next = strchrnul(line, ' ');
>  					}
>  
> -					next = strchrnul(line, '\n');
> -					free(sigc->primary_key_fingerprint);
> -					sigc->primary_key_fingerprint = xmemdupz(line, next - line);
> +					if (j == 0) {
> +						next = strchrnul(line, '\n');
> +						free(sigc->primary_key_fingerprint);
> +						sigc->primary_key_fingerprint =
> +							xmemdupz(line,
> +								 next - line);
> +					}

Avoid such an unnatural line-wrapping that makes the result harder
to read.  A short helper

	static void replace_cstring(const char **field,
				    const char *line, const char *next)
	{
		free(*field);
		if (line && next)
			*field = xmemdupz(line, next - line);
		else
			*field = NULL;
	}

may have quite a lot of uses in this function, not only for this
field.

This is a tangent, but I think "skip 9 fields" loop by itself
deserves to become a small static helper function.

With such a helper, it would become

		if (!j) {
			next = strchrnul(line, '\n');
			replace_cstring(&sigc->primary_key_fingerprint, line, next);
		} else {
			replace_cstring(&sigc->primary_key_fingerprint,	NULL, NULL);
		}

or if you do not like the overlong lines (I don't), perhaps

		field = &sigc->primary_key_fingerprint;
		if (!j)
			replace_cstring(field, line, strchrnul(line, '\n'));
		else
			replace_cstring(field, NULL, NULL);

Note that sigc->key, sigc->signer, sigc->fingerprint fields all
share the same pattern and will benefit from the use of such a
helper function.

Thanks.

[Reference]

*1* Perhaps this one?  https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=dea9d426351e043f872007696e841afb22fe9689;hb=591523ec94b6279b8b39a01501d78cf980de8722#l480