Re: [PATCH v3 2/2] 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 is documented to
> have 9 required and 1 optional fields [1].  The final, and optional,
> field is used to specify the fingerprint of the primary key that made
> the signature in case it was made by a subkey.  However, this field is
> only available for OpenPGP signatures; not for CMS/X.509.
>
> The current code assumes that the VALIDSIG status line always has 10
> fields.  Furthermore, the current code assumes that each field is
> separated by a space (0x20) character.

This make it sound as if the "assumption" of splitting at SP is a wrong
one, so I went back to the source.  The DETAILS document curiously
forgets to say that these are "usual space delimited arguments" for the
VALIDSIG, unlike it does for a few other entries in the "General status
codes" section.  But with the presense of "usual space delimited" in
another seciton with the lack of explicit delimiter definition in this
section, I would say it is safe to assume that the authors meant these
fields to be separated by SPs. 

In any case, I think this paragraph can safely disappear and would make
the overall clarity of the problem analysis even better.  We already
said that assuming 10th field is wrong in the earlier paragraph, and we
say what happens when the is only 9 fields in the next paragraph.

> If the VALIDSIG status line does not have the optional 10th field, the
> current code will continue reading onto the next status line -- because
> only 0x20 is considered a field separator, not 0xa.  And this is the
> case for non-OpenPGP signatures [1].

I actually think stresing on 0x20 vs 0x0a misleads readers in a wrong
direction.  If the code were written in such a way that both can be used
as a field separator, we would still continue reading into the next
line.  IOW, treating only SP as field delimiter is correct; what is
wrong is we do not limit the search to the current/single line.

	... because the code does not pay attention to the end of the
	current line when it looks for the end of the 10th (and
	optional) field that it incorrectly assumes to exist.

> The consequence is that a subsequent status line may be considered as
> the "primary key" for signatures that does not have an actual primary
> key.

Overall, all of the description in the above paragraphs are nicely
analysed and explained.  I wish all contributors' wrote their proposed
log messages clearly like you.

> The solution introduced by this commit is to add 0xa as a bound for the
> search for a primary key.  The search for the 10th VALIDSIG field is
> aborted as soon as it sees a newline character.  This keeps the parser
> from interpreting subsequent lines as the primary key.

Rather than "... to add 0xa as ... primary key.", I would probably write
it as "... to limit the search for the primary key on the single line."
The end of line being represented (internally as C string) with a byte
whose value is 0x0a or a newline character is an implementation detail.

Also we tend to describe the change we make as if the author is ordering
the codebase to "become like so" in imperative mood, so, perhaps

	Limit the search of these 9 or 10 fields to the single line
	to avoid this problem.  If the 10th field is missing, report
	that there is no primary key fingerprint.

would be sufficient, as the last sentence, i.e. "The consequence
is...", has already hinted what needs to be fixed clearly and quite
directly.


> [1] https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS#l483

I actually wrote the URL with the exact revision in my earlier example
because the URL written without is a moving target.  IOW, "Find the tip
commit of whichever the default branch is, and then look at the path
doc/DETAILS in it and hope that line 483 will stay forever what we want
to refer to" was what I wanted to avoid.  Perhaps in addition to the
above URL (which can go stale), leave a textual reference so that
readers can notice that the link is stale and look for what you meant to
point them at?  Like so:

	[Reference]

        *1* The description for VALIDSIG in the "General status codes"
	section of GnuPG document that is at

	https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS#l483

	says:

        VALIDSIG <args>

        The args are:

        - <fingerprint_in_hex>
        - <sig_creation_date>
        - <sig-timestamp>
        - <expire-timestamp>
        - <sig-version>
        - <reserved>
        - <pubkey-algo>
        - <hash-algo>
        - <sig-class>
        - [ <primary-key-fpr> ]

        This status indicates that the signature is cryptographically
        valid. ... PRIMARY-KEY-FPR is the fingerprint of the primary key
        or identical to the first argument.  This is useful to get back
        to the primary key without running gpg again for this purpose.

        The primary-key-fpr parameter is used for OpenPGP and not
        available for CMS signatures. ...

> @@ -160,18 +161,27 @@ static void parse_gpg_output(struct signature_check *sigc)
>  					next = strchrnul(line, ' ');
>  					replace_cstring(&sigc->fingerprint, line, next);
>  
> -					/* 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.
> +					 */
> +					limit = strchrnul(line, '\n');
>  					for (j = 9; j > 0; j--) {
> -						if (!*next)
> +						if (!*next || limit <= next)
>  							break;
>  						line = next + 1;
>  						next = strchrnul(line, ' ');
>  					}

Nice.  We try not to go beyond the end of the current line, and
otherwise break out which leaves j non-zero.

> -					next = strchrnul(line, '\n');
> -					free(sigc->primary_key_fingerprint);
> -					replace_cstring(&sigc->primary_key_fingerprint,
> -							line, next);

And doing the above unconditionally was wrong, but ...

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

... we correct it by doing the replacing only when we did find the
10th field.

Nicely done.

By the way, now you have another new variable "field" together with
"limit" both of whose uses are limited to this narrower scope, I no
longer mind seeing declarations of them inside this scope, as opposed to
make them function-wide.  That's a fairly minor point.

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index e803ba402e..17ec2401ec 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -1570,6 +1570,14 @@ test_expect_success GPG 'setup signed branch' '
>  	git commit -S -m signed_commit
>  '
>  
> +test_expect_success GPG 'setup signed branch with subkey' '
> +        test_when_finished "git reset --hard && git checkout master" &&
> +        git checkout -b signed-subkey master &&
> +        echo foo >foo &&
> +        git add foo &&
> +        git commit -SB7227189 -m signed_commit
> +'
> +

The new tests (not only this one) are indented with 8 SPs---will fix
to use HT while queuing (no need to resend only to fix these).

> +test_expect_success GPGSM 'log x509 fingerprint' '
> +        echo "F8BF62E0693D0694816377099909C779FA23FD65 | " >expect &&
> +        git log -n1 --format="%GF | %GP" signed-x509 >actual &&
> +        test_cmp expect actual
> +'
> +
> +test_expect_success GPGSM 'log OpenPGP fingerprint' '
> +        echo "D4BE22311AD3131E5EDA29A461092E85B7227189" > expect &&
> +        git log -n1 --format="%GP" signed-subkey >actual &&
> +        test_cmp expect actual
> +'

These two tests are really to the point.  Nice.

Thanks.