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

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

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

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

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

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

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

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

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

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

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

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