From 392b862e9aea69acf43532527e27644c97e3ea56 Mon Sep 17 00:00:00 2001 From: Hans Jerry Illikainen Date: Thu, 21 Nov 2019 23:43:35 +0000 Subject: [PATCH 1/2] gpg-interface: refactor the free-and-xmemdupz pattern Introduce a static replace_cstring() function to simplify repeated pattern of free-and-xmemdupz() for GPG status line parsing. This also helps us avoid potential memleaks if parsing of new status lines are introduced in the future. Signed-off-by: Hans Jerry Illikainen Signed-off-by: Junio C Hamano --- 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; From 67a6ea63008bcee32a239934ad29eb5c5a554509 Mon Sep 17 00:00:00 2001 From: Hans Jerry Illikainen Date: Fri, 22 Nov 2019 20:23:12 +0000 Subject: [PATCH 2/2] gpg-interface: limit search for primary key fingerprint 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. 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. [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 says: VALIDSIG The args are: - - - - - - - - - - [ ] 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 Signed-off-by: Junio C Hamano --- 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 819c24d10e..da8cb06f9b 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -1555,6 +1555,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 && @@ -1565,6 +1573,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 &&