[PATCH 0/1] Limit search for primary key fingerprint

[PATCH 0/1] Limit search for primary key fingerprint

[Hans Jerry Illikainen]

As part of implementing signature verification for git clone, I decided
to refactor/unify the code for commit and merge verification to make it
reusable during clones.

This lead me to discover that git requires merge signatures to be
trusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike
the behavior of verify-tag and verify-commit.

So, I figured that I'd make the minimum trust level configurable to make
the behavior of merge/commit/tag consistent.  And while doing so, I
noticed that parse_gpg_output() in gpg-interface.c assumes that the
VALIDSIG status line has a field with a fingerprint for the primary key;
but that is only the case for OpenPGP signatures [1].

The consequence of that assumption is that the subsequent status line is
interpreted as the primary fingerprint for X509 signatures.  I'm not
sure if the order is hardcoded in GnuPG, but in my testing the TRUST_
status line always came after VALIDSIG -- and that breaks the config
option to set a minimum trust level (not part of this patch):

,----
| $ git log -n1 --format="primary key: %GP" signed-x509
| gpgsm: Signature made 2019-11-16 14:13:09 using certificate ID 0xFA23FD65
| gpgsm: Good signature from "/CN=C O Mitter/O=Example/SN=C O/GN=Mitter"
| gpgsm:                 aka "committer@example.com"
| primary key: TRUST_FULLY 0 shell
`----

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


Hans Jerry Illikainen (1):
  gpg-interface: limit search for primary key fingerprint

 gpg-interface.c | 20 +++++++++++++++-----
 t/t4202-log.sh  |  6 ++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

--
2.24.0.156.g69483321b9.dirty

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

[Hans Jerry Illikainen]

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.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 gpg-interface.c | 20 +++++++++++++++-----
 t/t4202-log.sh  |  6 ++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index d60115ca40..01c7ef42d4 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -148,21 +148,31 @@ static void parse_gpg_output(struct signature_check *sigc)
 				}
 				/* Do we have fingerprint? */
 				if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
+					const char *limit;
+
 					next = strchrnul(line, ' ');
 					free(sigc->fingerprint);
 					sigc->fingerprint = xmemdupz(line, next - line);
 
-					/* 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 || next >= limit)
 							break;
 						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);
+					}
 				}
 
 				break;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index e803ba402e..5d893b3137 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1580,6 +1580,12 @@ test_expect_success GPGSM 'setup signed branch x509' '
 	git commit -S -m signed_commit
 '
 
+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 GPG 'log --graph --show-signature' '
 	git log --graph --show-signature -n1 signed >actual &&
 	grep "^| gpg: Signature made" actual &&
-- 
2.24.0.156.g69483321b9.dirty

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

[Junio C Hamano]

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

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

[Hans Jerry Illikainen]

On Mon, Nov 18 2019, Junio C Hamano wrote:
> 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.

Done in v3 (sorry for the messed up versioning going from v0 to v2!).

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

I investigated the code paths to `primary_key_fingerprint` and deduced
that it's only ever touched when GPG_STATUS_FINGERPRINT is encountered
and a primary fingerprint is extracted.  However, v3 will NULL it even
when no primary fingerprint is found.

>> +							xmemdupz(line,
>> +								 next - line);
>> +					}
>
> Avoid such an unnatural line-wrapping that makes the result harder
> to read.

Sorry about that!  I figured that some projects prefer to always trust
in the code formatter; so I just left it be.  Now I know that human
decisions are allowed :)

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

Implemented.  I wasn't sure whether to do it in a separate commit or
not, but #git-devel suggested that I do; so that's what I did.

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

[Junio C Hamano]

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

> On Mon, Nov 18 2019, Junio C Hamano wrote:
> ...
>> 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.
>
> Implemented.  I wasn't sure whether to do it in a separate commit or
> not, but #git-devel suggested that I do; so that's what I did.

If such a refactoring helped the readability of _existing_ code that
can also use this helper, then I agree it is the right approach to
make that into a separate prelimimary commit.

Thanks for working on this.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Hans Jerry Illikainen <hji@dyntopia.com> writes:
>
>> On Mon, Nov 18 2019, Junio C Hamano wrote:
>> ...
>>> 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.
>>
>> Implemented.  I wasn't sure whether to do it in a separate commit or
>> not, but #git-devel suggested that I do; so that's what I did.
>
> If such a refactoring helped the readability of _existing_ code that
> can also use this helper, then I agree it is the right approach to
> make that into a separate prelimimary commit.

I did not expect the "how about doing it along this line...?"
suggestion written in my MUA without even compilation testing would
work well, and acually I do not think the above would work without
further tweaks on the types.  Wouldn't some of the fields this
helper works on be of type "char *"?

Perhaps something like this squashed in...

 gpg-interface.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 4269937b83..b481b0811b 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -105,7 +105,7 @@ static struct {
 	{ 0, "VALIDSIG ", GPG_STATUS_FINGERPRINT },
 };
 
-static void replace_cstring(const char **field, const char *line,
+static void replace_cstring(char **field, const char *line,
 			    const char *next)
 {
 	free(*field);
@@ -120,7 +120,6 @@ static void parse_gpg_output(struct signature_check *sigc)
 {
 	const char *buf = sigc->gpg_status;
 	const char *line, *next, *limit;
-	const char **field;
 	int i, j;
 	int seen_exclusive_status = 0;
 
@@ -158,6 +157,8 @@ static void parse_gpg_output(struct signature_check *sigc)
 				}
 				/* Do we have fingerprint? */
 				if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
+					char **field;
+
 					next = strchrnul(line, ' ');
 					replace_cstring(&sigc->fingerprint, line, next);
 

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

[Hans Jerry Illikainen]

On Fri, Nov 22 2019, Junio C Hamano wrote:
> Wouldn't some of the fields this helper works on be of type "char *"?

Wow, that's embarrassing.  I completely messed that one up after a
looong day.  Gah!  Fixed and re-built with DEVELOPER=1 and re-ran the
test suite for both commits in an attempt to avoid further fuckups.

I also fixed the criticism on 2/2 (even though you mentioned that
there's no need for that) and sent it as v4 because I'm not sure what
the right approach is for changing only 1/2.

For future reference; how does the project prefer fixups for a single
commit on a multi-patch submission?

--
hji

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

[Junio C Hamano]

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

> On Fri, Nov 22 2019, Junio C Hamano wrote:
>> Wouldn't some of the fields this helper works on be of type "char *"?
>
> Wow, that's embarrassing.  I completely messed that one up after a
> looong day.  Gah!  Fixed and re-built with DEVELOPER=1 and re-ran the
> test suite for both commits in an attempt to avoid further fuckups.

The embarrassment is mine ;-)

> I also fixed the criticism on 2/2 (even though you mentioned that
> there's no need for that) and sent it as v4 because I'm not sure what
> the right approach is for changing only 1/2.
>
> For future reference; how does the project prefer fixups for a single
> commit on a multi-patch submission?

Unless the series is insanely loooooooooooooooooooooooooooooooong,
resending the whole thing, optionally with summary of the changes
since the previous iteration for each step after the three-dash
lines (i.e. this allows readers to notice "unchanged since v3"
and skip individual ones marked as such while reviewing v4), would
be a good way to help both reviewers who saw the previous round and
those who have skipped the previous round.

Thanks.

Re: [PATCH 0/1] Limit search for primary key fingerprint

[Jonathan Nieder]

Hi,

Hans Jerry Illikainen wrote:

> As part of implementing signature verification for git clone, I decided
> to refactor/unify the code for commit and merge verification to make it
> reusable during clones.

Thanks for writing this.

Most of the text in this cover letter would be useful to have in the
commit message.  From the commit message alone, I could see that you
were fixing a bug, but I could not see the motivation or workflow it
is part of.  If I were to later discover an issue triggered by this
commit, I wouldn't have enough information to weigh tradeoffs about
the right way to address such an issue.

Thanks and hope that helps,
Jonathan

> This lead me to discover that git requires merge signatures to be
> trusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike
> the behavior of verify-tag and verify-commit.
>
> So, I figured that I'd make the minimum trust level configurable to make
> the behavior of merge/commit/tag consistent.  And while doing so, I
> noticed that parse_gpg_output() in gpg-interface.c assumes that the
> VALIDSIG status line has a field with a fingerprint for the primary key;
> but that is only the case for OpenPGP signatures [1].
>
> The consequence of that assumption is that the subsequent status line is
> interpreted as the primary fingerprint for X509 signatures.  I'm not
> sure if the order is hardcoded in GnuPG, but in my testing the TRUST_
> status line always came after VALIDSIG -- and that breaks the config
> option to set a minimum trust level (not part of this patch):
>
> ,----
> | $ git log -n1 --format="primary key: %GP" signed-x509
> | gpgsm: Signature made 2019-11-16 14:13:09 using certificate ID 0xFA23FD65
> | gpgsm: Good signature from "/CN=C O Mitter/O=Example/SN=C O/GN=Mitter"
> | gpgsm:                 aka "committer@example.com"
> | primary key: TRUST_FULLY 0 shell
> `----
>
> [1]: https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS

[PATCH v2 0/1] Limit search for primary key fingerprint

[Hans Jerry Illikainen]

As part of implementing signature verification for git clone, I decided
to refactor/unify the code for commit and merge verification to make it
reusable during clones.

This lead me to discover that git requires merge signatures to be
trusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike
the behavior of verify-tag and verify-commit.

So, I figured that I'd make the minimum trust level configurable to make
the behavior of merge/commit/tag consistent.  And while doing so, I
noticed that parse_gpg_output() in gpg-interface.c assumes that the
VALIDSIG status line has a field with a fingerprint for the primary key;
but that is only the case for OpenPGP signatures [1].

The consequence of that assumption is that the subsequent status line is
interpreted as the primary fingerprint for X509 signatures.  I'm not
sure if the order is hardcoded in GnuPG, but in my testing the TRUST_
status line always came after VALIDSIG -- and that breaks the config
option to set a minimum trust level (not part of this patch):

,----
| $ git log -n1 --format="primary key: %GP" signed-x509
| gpgsm: Signature made 2019-11-16 14:13:09 using certificate ID 0xFA23FD65
| gpgsm: Good signature from "/CN=C O Mitter/O=Example/SN=C O/GN=Mitter"
| gpgsm:                 aka "committer@example.com"
| primary key: TRUST_FULLY 0 shell
`----

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


Hans Jerry Illikainen (1):
  gpg-interface: limit search for primary key fingerprint

 gpg-interface.c | 20 +++++++++++++++-----
 t/t4202-log.sh  |  6 ++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

--
2.24.GIT

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

[Hans Jerry Illikainen]

As part of implementing signature verification for git clone, I decided
to refactor/unify the code for commit and merge verification to make it
reusable during clones.

This lead me to discover that git requires merge signatures to be
trusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike
the behavior of verify-tag and verify-commit.

So, I figured that I'd make the minimum trust level configurable to make
the behavior of merge/commit/tag consistent.  And while doing so, I
noticed that parse_gpg_output() in gpg-interface.c assumes that the
VALIDSIG status line has a field with a fingerprint for the primary key;
but that is only the case for OpenPGP signatures.

The consequence of that assumption is that the subsequent status line is
interpreted as the primary fingerprint for X509 signatures.  I'm not
sure if the order is hardcoded in GnuPG, but in my testing the TRUST_
status line always came after VALIDSIG -- and that breaks the
possibility of a configuration option to set a minimum trust level since
the TRUST_ line is consumed by VALIDSIG.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 gpg-interface.c | 20 +++++++++++++++-----
 t/t4202-log.sh  |  6 ++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index d60115ca40..01c7ef42d4 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -148,21 +148,31 @@ static void parse_gpg_output(struct signature_check *sigc)
 				}
 				/* Do we have fingerprint? */
 				if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
+					const char *limit;
+
 					next = strchrnul(line, ' ');
 					free(sigc->fingerprint);
 					sigc->fingerprint = xmemdupz(line, next - line);
 
-					/* 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 || next >= limit)
 							break;
 						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);
+					}
 				}
 
 				break;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index e803ba402e..5d893b3137 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1580,6 +1580,12 @@ test_expect_success GPGSM 'setup signed branch x509' '
 	git commit -S -m signed_commit
 '
 
+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 GPG 'log --graph --show-signature' '
 	git log --graph --show-signature -n1 signed >actual &&
 	grep "^| gpg: Signature made" actual &&
-- 
2.24.GIT

[PATCH v3 0/2] gpg-interface: fix search for primary key fingerprint

[Hans Jerry Illikainen]

As part of the process of implementing signature verification for git
clone, I decided to refactor/unify the code for commit and merge
verification to make it reusable during clones.

This lead me to discover that git requires merge signatures to be
trusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike
the behavior of verify-tag and verify-commit.

So, I figured that I'd make the minimum trust level configurable to make
the behavior of merge/commit/tag consistent.  And while doing so, I
noticed that parse_gpg_output() in gpg-interface.c assumes that the
VALIDSIG status line has a field with a fingerprint for the primary key;
but that is only the case for OpenPGP signatures [1].

The consequence of that assumption is that the subsequent status line is
interpreted as the primary fingerprint for X509 signatures.  I'm not
sure if the order is hardcoded in GnuPG, but in my testing the TRUST_
status line always came after VALIDSIG -- and that breaks the config
option to set a minimum trust level (not part of this patch):

,----
| $ git log -n1 --format="primary key: %GP" signed-x509
| gpgsm: Signature made 2019-11-16 14:13:09 using certificate ID 0xFA23FD65
| gpgsm: Good signature from "/CN=C O Mitter/O=Example/SN=C O/GN=Mitter"
| gpgsm:                 aka "committer@example.com"
| primary key: TRUST_FULLY 0 shell
`----

As per suggestion from Hamano, I also introduced a helper function,
replace_cstring().  I wasn't sure whether to add it in a separate commit
or not, but the kind folks in #git-devel suggested I do.

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

Hans Jerry Illikainen (2):
  gpg-interface: refactor the free-and-xmemdupz pattern
  gpg-interface: limit search for primary key fingerprint

 gpg-interface.c | 45 ++++++++++++++++++++++++++++++++-------------
 t/t4202-log.sh  | 20 ++++++++++++++++++++
 2 files changed, 52 insertions(+), 13 deletions(-)

--
2.24.0.157.gba9f894af8

[PATCH v3 1/2] gpg-interface: refactor the free-and-xmemdupz pattern

[Hans Jerry Illikainen]

This commit introduces a static replace_cstring() function.  This
function simplifies the continuous pattern of free-and-xmemdupz() for
GPG status line parsing.

The benefit of having it in a single helper function is that it helps
avoid the need for duplicate code that does the same thing.  It also
helps avoid potential memleaks if parsing of new status lines are
introduced in the future.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 gpg-interface.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index d60115ca40..b4c4443287 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -105,6 +105,17 @@ static struct {
 	{ 0, "VALIDSIG ", GPG_STATUS_FINGERPRINT },
 };
 
+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;
+}
+
 static void parse_gpg_output(struct signature_check *sigc)
 {
 	const char *buf = sigc->gpg_status;
@@ -136,21 +147,18 @@ static void parse_gpg_output(struct signature_check *sigc)
 				/* Do we have key information? */
 				if (sigcheck_gpg_status[i].flags & GPG_STATUS_KEYID) {
 					next = strchrnul(line, ' ');
-					free(sigc->key);
-					sigc->key = xmemdupz(line, next - line);
+					replace_cstring(&sigc->key, line, next);
 					/* Do we have signer information? */
 					if (*next && (sigcheck_gpg_status[i].flags & GPG_STATUS_UID)) {
 						line = next + 1;
 						next = strchrnul(line, '\n');
-						free(sigc->signer);
-						sigc->signer = xmemdupz(line, next - line);
+						replace_cstring(&sigc->signer, line, next);
 					}
 				}
 				/* Do we have fingerprint? */
 				if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
 					next = strchrnul(line, ' ');
-					free(sigc->fingerprint);
-					sigc->fingerprint = xmemdupz(line, next - line);
+					replace_cstring(&sigc->fingerprint, line, next);
 
 					/* Skip interim fields */
 					for (j = 9; j > 0; j--) {
@@ -162,7 +170,8 @@ static void parse_gpg_output(struct signature_check *sigc)
 
 					next = strchrnul(line, '\n');
 					free(sigc->primary_key_fingerprint);
-					sigc->primary_key_fingerprint = xmemdupz(line, next - line);
+					replace_cstring(&sigc->primary_key_fingerprint,
+							line, next);
 				}
 
 				break;
-- 
2.24.0.157.gba9f894af8

Re: [PATCH v3 1/2] gpg-interface: refactor the free-and-xmemdupz pattern

[Junio C Hamano]

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

> This commit introduces a static replace_cstring() function.  This
> function simplifies the continuous pattern of free-and-xmemdupz() for
> GPG status line parsing.
>
> The benefit of having it in a single helper function is that it helps
> avoid the need for duplicate code that does the same thing.  It also
> helps avoid potential memleaks if parsing of new status lines are
> introduced in the future.
>
> Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
> ---
>  gpg-interface.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)

Looks quite cleanly done and clearly explained.  Thanks.

> diff --git a/gpg-interface.c b/gpg-interface.c
> index d60115ca40..b4c4443287 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -105,6 +105,17 @@ static struct {
>  	{ 0, "VALIDSIG ", GPG_STATUS_FINGERPRINT },
>  };
>  
> +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;
> +}
> +
>  static void parse_gpg_output(struct signature_check *sigc)
>  {
>  	const char *buf = sigc->gpg_status;
> @@ -136,21 +147,18 @@ static void parse_gpg_output(struct signature_check *sigc)
>  				/* Do we have key information? */
>  				if (sigcheck_gpg_status[i].flags & GPG_STATUS_KEYID) {
>  					next = strchrnul(line, ' ');
> -					free(sigc->key);
> -					sigc->key = xmemdupz(line, next - line);
> +					replace_cstring(&sigc->key, line, next);
>  					/* Do we have signer information? */
>  					if (*next && (sigcheck_gpg_status[i].flags & GPG_STATUS_UID)) {
>  						line = next + 1;
>  						next = strchrnul(line, '\n');
> -						free(sigc->signer);
> -						sigc->signer = xmemdupz(line, next - line);
> +						replace_cstring(&sigc->signer, line, next);
>  					}
>  				}
>  				/* Do we have fingerprint? */
>  				if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
>  					next = strchrnul(line, ' ');
> -					free(sigc->fingerprint);
> -					sigc->fingerprint = xmemdupz(line, next - line);
> +					replace_cstring(&sigc->fingerprint, line, next);
>  
>  					/* Skip interim fields */
>  					for (j = 9; j > 0; j--) {
> @@ -162,7 +170,8 @@ static void parse_gpg_output(struct signature_check *sigc)
>  
>  					next = strchrnul(line, '\n');
>  					free(sigc->primary_key_fingerprint);
> -					sigc->primary_key_fingerprint = xmemdupz(line, next - line);
> +					replace_cstring(&sigc->primary_key_fingerprint,
> +							line, next);
>  				}
>  
>  				break;

[PATCH v3 2/2] gpg-interface: limit search for primary key fingerprint

[Hans Jerry Illikainen]

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.

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

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.

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.

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

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 gpg-interface.c | 24 +++++++++++++++++-------
 t/t4202-log.sh  | 20 ++++++++++++++++++++
 2 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index b4c4443287..4269937b83 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -119,7 +119,8 @@ static void replace_cstring(const char **field, const char *line,
 static void parse_gpg_output(struct signature_check *sigc)
 {
 	const char *buf = sigc->gpg_status;
-	const char *line, *next;
+	const char *line, *next, *limit;
+	const char **field;
 	int i, j;
 	int seen_exclusive_status = 0;
 
@@ -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, ' ');
 					}
 
-					next = strchrnul(line, '\n');
-					free(sigc->primary_key_fingerprint);
-					replace_cstring(&sigc->primary_key_fingerprint,
-							line, next);
+					field = &sigc->primary_key_fingerprint;
+					if (!j) {
+						next = strchrnul(line, '\n');
+						replace_cstring(field, line, next);
+					} else {
+						replace_cstring(field, NULL, NULL);
+					}
 				}
 
 				break;
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
+'
+
 test_expect_success GPGSM 'setup signed branch x509' '
 	test_when_finished "git reset --hard && git checkout master" &&
 	git checkout -b signed-x509 master &&
@@ -1580,6 +1588,18 @@ test_expect_success GPGSM 'setup signed branch x509' '
 	git commit -S -m signed_commit
 '
 
+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
+'
+
 test_expect_success GPG 'log --graph --show-signature' '
 	git log --graph --show-signature -n1 signed >actual &&
 	grep "^| gpg: Signature made" actual &&
-- 
2.24.0.157.gba9f894af8

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

[Junio C Hamano]

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.

[PATCH v4 0/2] Limit search for primary fingerprint

[Hans Jerry Illikainen]

As part of the process of implementing signature verification for git
clone, I decided to refactor/unify the code for commit and merge
verification to make it reusable during clones.

This lead me to discover that git requires merge signatures to be
trusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike
the behavior of verify-tag and verify-commit.

So, I figured that I'd make the minimum trust level configurable to make
the behavior of merge/commit/tag consistent.  And while doing so, I
noticed that parse_gpg_output() in gpg-interface.c assumes that the
VALIDSIG status line has a field with a fingerprint for the primary key;
but that is only the case for OpenPGP signatures [1].

The consequence of that assumption is that the subsequent status line is
interpreted as the primary fingerprint for X509 signatures.  I'm not
sure if the order is hardcoded in GnuPG, but in my testing the TRUST_
status line always came after VALIDSIG -- and that breaks the config
option to set a minimum trust level (not part of this patch):

,----
| $ git log -n1 --format="primary key: %GP" signed-x509
| gpgsm: Signature made 2019-11-16 14:13:09 using certificate ID 0xFA23FD65
| gpgsm: Good signature from "/CN=C O Mitter/O=Example/SN=C O/GN=Mitter"
| gpgsm:                 aka "committer@example.com"
| primary key: TRUST_FULLY 0 shell
`----

As per suggestion from Hamano, I also introduced a helper function,
replace_cstring().

This patch revision fixes the indentation in the test cases as well as
an erroneous const qualification.

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


Hans Jerry Illikainen (2):
  gpg-interface: refactor the free-and-xmemdupz pattern
  gpg-interface: limit search for primary key fingerprint

 gpg-interface.c | 44 ++++++++++++++++++++++++++++++++------------
 t/t4202-log.sh  | 20 ++++++++++++++++++++
 2 files changed, 52 insertions(+), 12 deletions(-)

--
2.24.GIT

[PATCH v4 1/2] gpg-interface: refactor the free-and-xmemdupz pattern

[Hans Jerry Illikainen]

This commit introduces a static replace_cstring() function.  This
function simplifies the continuous pattern of free-and-xmemdupz() for
GPG status line parsing.

The benefit of having it in a single helper function is that it helps
avoid the need for duplicate code that does the same thing.  It also
helps avoid potential memleaks if parsing of new status lines are
introduced in the future.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 gpg-interface.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index d60115ca40..37162c9a43 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -105,6 +105,16 @@ static struct {
 	{ 0, "VALIDSIG ", GPG_STATUS_FINGERPRINT },
 };
 
+static void replace_cstring(char **field, const char *line, const char *next)
+{
+	free(*field);
+
+	if (line && next)
+		*field = xmemdupz(line, next - line);
+	else
+		*field = NULL;
+}
+
 static void parse_gpg_output(struct signature_check *sigc)
 {
 	const char *buf = sigc->gpg_status;
@@ -136,21 +146,18 @@ static void parse_gpg_output(struct signature_check *sigc)
 				/* Do we have key information? */
 				if (sigcheck_gpg_status[i].flags & GPG_STATUS_KEYID) {
 					next = strchrnul(line, ' ');
-					free(sigc->key);
-					sigc->key = xmemdupz(line, next - line);
+					replace_cstring(&sigc->key, line, next);
 					/* Do we have signer information? */
 					if (*next && (sigcheck_gpg_status[i].flags & GPG_STATUS_UID)) {
 						line = next + 1;
 						next = strchrnul(line, '\n');
-						free(sigc->signer);
-						sigc->signer = xmemdupz(line, next - line);
+						replace_cstring(&sigc->signer, line, next);
 					}
 				}
 				/* Do we have fingerprint? */
 				if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
 					next = strchrnul(line, ' ');
-					free(sigc->fingerprint);
-					sigc->fingerprint = xmemdupz(line, next - line);
+					replace_cstring(&sigc->fingerprint, line, next);
 
 					/* Skip interim fields */
 					for (j = 9; j > 0; j--) {
@@ -162,7 +169,8 @@ static void parse_gpg_output(struct signature_check *sigc)
 
 					next = strchrnul(line, '\n');
 					free(sigc->primary_key_fingerprint);
-					sigc->primary_key_fingerprint = xmemdupz(line, next - line);
+					replace_cstring(&sigc->primary_key_fingerprint,
+							line, next);
 				}
 
 				break;
-- 
2.24.GIT

[PATCH v4 2/2] gpg-interface: limit search for primary key fingerprint

[Hans Jerry Illikainen]

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.

If the VALIDSIG status line does not have the optional 10th field, the
current code will continue reading onto the next status line.  And this
is the case for non-OpenPGP signatures [1].

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.

The solution introduced by this commit is to limit the search for a
primary key to a single line.  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.

[Reference]
[1] GnuPG Details, General status codes
https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=6ce340e8c04794add995e84308bb3091450bd28f;hb=HEAD#l483

The documentation say:

    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.

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

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 gpg-interface.c | 24 ++++++++++++++++++------
 t/t4202-log.sh  | 20 ++++++++++++++++++++
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 37162c9a43..131e7d529e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -156,21 +156,33 @@ static void parse_gpg_output(struct signature_check *sigc)
 				}
 				/* Do we have fingerprint? */
 				if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
+					const char *limit;
+					char **field;
+
 					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, ' ');
 					}
 
-					next = strchrnul(line, '\n');
-					free(sigc->primary_key_fingerprint);
-					replace_cstring(&sigc->primary_key_fingerprint,
-							line, next);
+					field = &sigc->primary_key_fingerprint;
+					if (!j) {
+						next = strchrnul(line, '\n');
+						replace_cstring(field, line, next);
+					} else {
+						replace_cstring(field, NULL, NULL);
+					}
 				}
 
 				break;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index e803ba402e..d18613dad1 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
+'
+
 test_expect_success GPGSM 'setup signed branch x509' '
 	test_when_finished "git reset --hard && git checkout master" &&
 	git checkout -b signed-x509 master &&
@@ -1580,6 +1588,18 @@ test_expect_success GPGSM 'setup signed branch x509' '
 	git commit -S -m signed_commit
 '
 
+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
+'
+
 test_expect_success GPG 'log --graph --show-signature' '
 	git log --graph --show-signature -n1 signed >actual &&
 	grep "^| gpg: Signature made" actual &&
-- 
2.24.GIT

Re: [PATCH v4 0/2] Limit search for primary fingerprint

[Junio C Hamano]

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

> ...
> This patch revision fixes the indentation in the test cases as well as
> an erroneous const qualification.

I made minor tweaks to the log message, but otherwise I think this
is ready for 'next'.

Thanks for working on this.

Re: [PATCH 0/1] Limit search for primary key fingerprint

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hans Jerry Illikainen wrote:
>
>> As part of implementing signature verification for git clone, I decided
>> to refactor/unify the code for commit and merge verification to make it
>> reusable during clones.
>
> Thanks for writing this.
>
> Most of the text in this cover letter would be useful to have in the
> commit message.  From the commit message alone, I could see that you
> were fixing a bug, but I could not see the motivation or workflow it
> is part of.  If I were to later discover an issue triggered by this
> commit, I wouldn't have enough information to weigh tradeoffs about
> the right way to address such an issue.

After reading the proposed log message of [PATCH v1 1/1], I have to
disagree.  It does not matter if we will later see new code in the
clone codepath that would use the gpg-interface API.  Whether it
happens or not, this change to look for the key fingerprint only on
the same line is something we should consider independently.

On the other hand, why the author of this change thought that it may
be necessary thing to do is an excellent material to tell the story
behind the patch in the cover letter.

Thanks.