{"thread":{"id":"49625","subject":"[PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits","startedAt":"2018-10-20T19:30:30Z","lastAt":"2018-11-03T15:58:25Z","messageCount":11,"participants":["Michał Górny","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"361069","messageId":"20181020193020.28517-1-mgorny@gentoo.org","threadId":"49625","inReplyTo":null,"subject":"[PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-10-20T19:30:20Z","receivedAt":"2018-10-20T19:30:30Z","isPatch":true,"sender":{"key":"mgorny@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/110765?v=4"},"body":"GnuPG supports creating signatures consisting of multiple signature\npackets. If such a signature is verified, it outputs all the status\nmessages for each signature separately. However, git currently does not\naccount for such scenario and gets terribly confused over getting\nmultiple *SIG statuses.\n\nFor example, if a malicious party alters a signed commit and appends\na new untrusted signature, git is going to ignore the original bad\nsignature and report untrusted commit instead. However, %GK and %GS\nformat strings may still expand to the data corresponding\nto the original signature, potentially tricking the scripts into\ntrusting the malicious commit.\n\nGiven that the use of multiple signatures is quite rare, git does not\nsupport creating them without jumping through a few hoops, and finally\nsupporting them properly would require extensive API improvement, it\nseems reasonable to just reject them at the moment.\n\nSigned-off-by: Michał Górny \n---\n gpg-interface.c | 90 +++++++++++++++++++++++++++-------------\n t/t7510-signed-commit.sh | 26 ++++++++++++\n 2 files changed, 87 insertions(+), 29 deletions(-)\n\nChanges in v4:\n* switched to using skip_prefix(),\n* renamed the variable to seen_exclusive_status,\n* made the loop terminate early on first duplicate status seen.\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex db17d65f8..efe2c0d38 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -75,48 +75,80 @@ void signature_check_clear(struct signature_check *sigc)\n \tFREE_AND_NULL(sigc->key);\n }\n \n+/* An exclusive status -- only one of them can appear in output */\n+#define GPG_STATUS_EXCLUSIVE\t(1<<0)\n+\n static struct {\n \tchar result;\n \tconst char *check;\n+\tunsigned int flags;\n } sigcheck_gpg_status[] = {\n-\t{ 'G', \"\\n[GNUPG:] GOODSIG \" },\n-\t{ 'B', \"\\n[GNUPG:] BADSIG \" },\n-\t{ 'U', \"\\n[GNUPG:] TRUST_NEVER\" },\n-\t{ 'U', \"\\n[GNUPG:] TRUST_UNDEFINED\" },\n-\t{ 'E', \"\\n[GNUPG:] ERRSIG \"},\n-\t{ 'X', \"\\n[GNUPG:] EXPSIG \"},\n-\t{ 'Y', \"\\n[GNUPG:] EXPKEYSIG \"},\n-\t{ 'R', \"\\n[GNUPG:] REVKEYSIG \"},\n+\t{ 'G', \"GOODSIG \", GPG_STATUS_EXCLUSIVE },\n+\t{ 'B', \"BADSIG \", GPG_STATUS_EXCLUSIVE },\n+\t{ 'U', \"TRUST_NEVER\", 0 },\n+\t{ 'U', \"TRUST_UNDEFINED\", 0 },\n+\t{ 'E', \"ERRSIG \", GPG_STATUS_EXCLUSIVE },\n+\t{ 'X', \"EXPSIG \", GPG_STATUS_EXCLUSIVE },\n+\t{ 'Y', \"EXPKEYSIG \", GPG_STATUS_EXCLUSIVE },\n+\t{ 'R', \"REVKEYSIG \", GPG_STATUS_EXCLUSIVE },\n };\n \n static void parse_gpg_output(struct signature_check *sigc)\n {\n \tconst char *buf = sigc->gpg_status;\n+\tconst char *line, *next;\n \tint i;\n-\n-\t/* Iterate over all search strings */\n-\tfor (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n-\t\tconst char *found, *next;\n-\n-\t\tif (!skip_prefix(buf, sigcheck_gpg_status[i].check + 1, &found)) {\n-\t\t\tfound = strstr(buf, sigcheck_gpg_status[i].check);\n-\t\t\tif (!found)\n-\t\t\t\tcontinue;\n-\t\t\tfound += strlen(sigcheck_gpg_status[i].check);\n-\t\t}\n-\t\tsigc->result = sigcheck_gpg_status[i].result;\n-\t\t/* The trust messages are not followed by key/signer information */\n-\t\tif (sigc->result != 'U') {\n-\t\t\tnext = strchrnul(found, ' ');\n-\t\t\tsigc->key = xmemdupz(found, next - found);\n-\t\t\t/* The ERRSIG message is not followed by signer information */\n-\t\t\tif (*next && sigc-> result != 'E') {\n-\t\t\t\tfound = next + 1;\n-\t\t\t\tnext = strchrnul(found, '\\n');\n-\t\t\t\tsigc->signer = xmemdupz(found, next - found);\n+\tint seen_exclusive_status = 0;\n+\n+\t/* Iterate over all lines */\n+\tfor (line = buf; *line; line = strchrnul(line+1, '\\n')) {\n+\t\twhile (*line == '\\n')\n+\t\t\tline++;\n+\t\t/* Skip lines that don't start with GNUPG status */\n+\t\tif (!skip_prefix(line, \"[GNUPG:] \", &line))\n+\t\t\tcontinue;\n+\n+\t\t/* Iterate over all search strings */\n+\t\tfor (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n+\t\t\tif (skip_prefix(line, sigcheck_gpg_status[i].check, &line)) {\n+\t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE) {\n+\t\t\t\t\tif (++seen_exclusive_status > 1)\n+\t\t\t\t\t\tgoto found_duplicate_status;\n+\t\t\t\t}\n+\n+\t\t\t\tsigc->result = sigcheck_gpg_status[i].result;\n+\t\t\t\t/* The trust messages are not followed by key/signer information */\n+\t\t\t\tif (sigc->result != 'U') {\n+\t\t\t\t\tnext = strchrnul(line, ' ');\n+\t\t\t\t\tfree(sigc->key);\n+\t\t\t\t\tsigc->key = xmemdupz(line, next - line);\n+\t\t\t\t\t/* The ERRSIG message is not followed by signer information */\n+\t\t\t\t\tif (*next && sigc->result != 'E') {\n+\t\t\t\t\t\tline = next + 1;\n+\t\t\t\t\t\tnext = strchrnul(line, '\\n');\n+\t\t\t\t\t\tfree(sigc->signer);\n+\t\t\t\t\t\tsigc->signer = xmemdupz(line, next - line);\n+\t\t\t\t\t}\n+\t\t\t\t}\n+\n+\t\t\t\tbreak;\n \t\t\t}\n \t\t}\n \t}\n+\treturn;\n+\n+found_duplicate_status:\n+\t/*\n+\t * GOODSIG, BADSIG etc. can occur only once for each signature.\n+\t * Therefore, if we had more than one then we're dealing with multiple\n+\t * signatures. We don't support them currently, and they're rather\n+\t * hard to create, so something is likely fishy and we should reject\n+\t * them altogether.\n+\t */\n+\tsigc->result = 'E';\n+\t/* Clear partial data to avoid confusion */\n+\tFREE_AND_NULL(sigc->signer);\n+\tFREE_AND_NULL(sigc->key);\n }\n \n int check_signature(const char *payload, size_t plen, const char *signature,\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 4e37ff8f1..180f0be91 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -234,4 +234,30 @@ test_expect_success GPG 'check config gpg.format values' '\n \ttest_must_fail git commit -S --amend -m \"fail\"\n '\n \n+test_expect_success GPG 'detect fudged commit with double signature' '\n+\tsed -e \"/gpgsig/,/END PGP/d\" forged1 >double-base &&\n+\tsed -n -e \"/gpgsig/,/END PGP/p\" forged1 | \\\n+\t\tsed -e \"s/^gpgsig//;s/^ //\" | gpg --dearmor >double-sig1.sig &&\n+\tgpg -o double-sig2.sig -u 29472784 --detach-sign double-base &&\n+\tcat double-sig1.sig double-sig2.sig | gpg --enarmor >double-combined.asc &&\n+\tsed -e \"s/^\\(-.*\\)ARMORED FILE/\\1SIGNATURE/;1s/^/gpgsig /;2,\\$s/^/ /\" \\\n+\t\tdouble-combined.asc > double-gpgsig &&\n+\tsed -e \"/committer/r double-gpgsig\" double-base >double-commit &&\n+\tgit hash-object -w -t commit double-commit >double-commit.commit &&\n+\ttest_must_fail git verify-commit $(cat double-commit.commit) &&\n+\tgit show --pretty=short --show-signature $(cat double-commit.commit) >double-actual &&\n+\tgrep \"BAD signature from\" double-actual &&\n+\tgrep \"Good signature from\" double-actual\n+'\n+\n+test_expect_success GPG 'show double signature with custom format' '\n+\tcat >expect <<-\\EOF &&\n+\tE\n+\n+\n+\tEOF\n+\tgit log -1 --format=\"%G?%n%GK%n%GS\" $(cat double-commit.commit) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.19.1\n\n"},{"id":"361074","messageId":"xmqqin1wyxvz.fsf@gitster-ct.c.googlers.com","threadId":"49625","inReplyTo":"20181020193020.28517-1-mgorny@gentoo.org","subject":"Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-20T23:57:36Z","receivedAt":"2018-10-20T23:57:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Górny writes:\n\n> GnuPG supports creating signatures consisting of multiple signature\n> packets. If such a signature is verified, it outputs all the status\n> messages for each signature separately. However, git currently does not\n> account for such scenario and gets terribly confused over getting\n> multiple *SIG statuses.\n>\n> For example, if a malicious party alters a signed commit and appends\n> a new untrusted signature, git is going to ignore the original bad\n> signature and report untrusted commit instead. However, %GK and %GS\n> format strings may still expand to the data corresponding\n> to the original signature, potentially tricking the scripts into\n> trusting the malicious commit.\n>\n> Given that the use of multiple signatures is quite rare, git does not\n> support creating them without jumping through a few hoops, and finally\n> supporting them properly would require extensive API improvement, it\n> seems reasonable to just reject them at the moment.\n>\n> Signed-off-by: Michał Górny \n> ---\n> gpg-interface.c | 90 +++++++++++++++++++++++++++-------------\n> t/t7510-signed-commit.sh | 26 ++++++++++++\n> 2 files changed, 87 insertions(+), 29 deletions(-)\n>\n> Changes in v4:\n> * switched to using skip_prefix(),\n> * renamed the variable to seen_exclusive_status,\n> * made the loop terminate early on first duplicate status seen.\n\nThanks for sticking to the topic and polishing it further. Looks\nvery good. \n\nWill replace.\n\n> +\tint seen_exclusive_status = 0;\n> +\n> +\t/* Iterate over all lines */\n> +\tfor (line = buf; *line; line = strchrnul(line+1, '\\n')) {\n> +\t\twhile (*line == '\\n')\n> +\t\t\tline++;\n> +\t\t/* Skip lines that don't start with GNUPG status */\n> +\t\tif (!skip_prefix(line, \"[GNUPG:] \", &line))\n> +\t\t\tcontinue;\n> +\n> +\t\t/* Iterate over all search strings */\n> +\t\tfor (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n> +\t\t\tif (skip_prefix(line, sigcheck_gpg_status[i].check, &line)) {\n> +\t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE) {\n> +\t\t\t\t\tif (++seen_exclusive_status > 1)\n> +\t\t\t\t\t\tgoto found_duplicate_status;\n\nVery minor point but by not using pre-increment, i.e.\n\n\t\tif (seen_exclusive_status++)\n\t\t\tgoto found_duplicate_status;\n\nyou can use the expression as a \"have we already seen?\" boolean,\nwhic may probably be more idiomatic.\n\nThe patch is good in the way written as-is, and this is so minor\nthat it is not worth rerolling to only update this part.\n\nThanks.\n\n"},{"id":"361075","messageId":"1540105841.1174.1.camel@gentoo.org","threadId":"49625","inReplyTo":"xmqqin1wyxvz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-10-21T07:10:41Z","receivedAt":"2018-10-21T07:10:49Z","isPatch":true,"sender":{"key":"mgorny@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/110765?v=4"},"body":"On Sun, 2018-10-21 at 08:57 +0900, Junio C Hamano wrote:\n> Michał Górny writes:\n> \n> > GnuPG supports creating signatures consisting of multiple signature\n> > packets. If such a signature is verified, it outputs all the status\n> > messages for each signature separately. However, git currently does not\n> > account for such scenario and gets terribly confused over getting\n> > multiple *SIG statuses.\n> > \n> > For example, if a malicious party alters a signed commit and appends\n> > a new untrusted signature, git is going to ignore the original bad\n> > signature and report untrusted commit instead. However, %GK and %GS\n> > format strings may still expand to the data corresponding\n> > to the original signature, potentially tricking the scripts into\n> > trusting the malicious commit.\n> > \n> > Given that the use of multiple signatures is quite rare, git does not\n> > support creating them without jumping through a few hoops, and finally\n> > supporting them properly would require extensive API improvement, it\n> > seems reasonable to just reject them at the moment.\n> > \n> > Signed-off-by: Michał Górny \n> > ---\n> > gpg-interface.c | 90 +++++++++++++++++++++++++++-------------\n> > t/t7510-signed-commit.sh | 26 ++++++++++++\n> > 2 files changed, 87 insertions(+), 29 deletions(-)\n> > \n> > Changes in v4:\n> > * switched to using skip_prefix(),\n> > * renamed the variable to seen_exclusive_status,\n> > * made the loop terminate early on first duplicate status seen.\n> \n> Thanks for sticking to the topic and polishing it further. Looks\n> very good. \n> \n> Will replace.\n> \n> > +\tint seen_exclusive_status = 0;\n> > +\n> > +\t/* Iterate over all lines */\n> > +\tfor (line = buf; *line; line = strchrnul(line+1, '\\n')) {\n> > +\t\twhile (*line == '\\n')\n> > +\t\t\tline++;\n> > +\t\t/* Skip lines that don't start with GNUPG status */\n> > +\t\tif (!skip_prefix(line, \"[GNUPG:] \", &line))\n> > +\t\t\tcontinue;\n> > +\n> > +\t\t/* Iterate over all search strings */\n> > +\t\tfor (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n> > +\t\t\tif (skip_prefix(line, sigcheck_gpg_status[i].check, &line)) {\n> > +\t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE) {\n> > +\t\t\t\t\tif (++seen_exclusive_status > 1)\n> > +\t\t\t\t\t\tgoto found_duplicate_status;\n> \n> Very minor point but by not using pre-increment, i.e.\n> \n> \t\tif (seen_exclusive_status++)\n> \t\t\tgoto found_duplicate_status;\n> \n> you can use the expression as a \"have we already seen?\" boolean,\n> whic may probably be more idiomatic.\n> \n> The patch is good in the way written as-is, and this is so minor\n> that it is not worth rerolling to only update this part.\n> \n\nSure, thanks. For the record, I've been taught to use pre-increment\nwhenever possible to avoid copying the variable but I suppose it doesn't\nreally matter here. Just a habit.\n\nI'll start working on my next ideas once this is merged and I rebase.\n\n-- \nBest regards,\nMichał Górny\n"},{"id":"361103","messageId":"xmqqefciztit.fsf@gitster-ct.c.googlers.com","threadId":"49625","inReplyTo":"1540105841.1174.1.camel@gentoo.org","subject":"Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-22T00:58:50Z","receivedAt":"2018-10-22T01:01:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Górny writes:\n\n>> Very minor point but by not using pre-increment, i.e.\n>> \n>> \t\tif (seen_exclusive_status++)\n>> \t\t\tgoto found_duplicate_status;\n>> \n>> you can use the expression as a \"have we already seen?\" boolean,\n>> whic may probably be more idiomatic.\n>> \n>> The patch is good in the way written as-is, and this is so minor\n>> that it is not worth rerolling to only update this part.\n>> \n>\n> Sure, thanks. For the record, I've been taught to use pre-increment\n> whenever possible to avoid copying the variable but I suppose it doesn't\n> really matter here. Just a habit.\n\nYes, it's a habit many C++ trained people spread; it just looks\nweird to see a pre-increment of a \"have we done this once?\" variable\nand end up comparing to see if it is strictly greater than 1\n(i.e. have we reached 2 or more?).\n"},{"id":"361144","messageId":"7C705597-7AA9-4C4E-904B-CE8C4F01AD24@gentoo.org","threadId":"49625","inReplyTo":"xmqqin1wyxvz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-10-22T08:04:48Z","receivedAt":"2018-10-22T08:05:02Z","isPatch":true,"sender":{"key":"mgorny@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/110765?v=4"},"body":"Dnia October 20, 2018 11:57:36 PM UTC, Junio C Hamano napisał(a):\n>Michał Górny writes:\n>\n>> GnuPG supports creating signatures consisting of multiple signature\n>> packets. If such a signature is verified, it outputs all the status\n>> messages for each signature separately. However, git currently does\n>not\n>> account for such scenario and gets terribly confused over getting\n>> multiple *SIG statuses.\n>>\n>> For example, if a malicious party alters a signed commit and appends\n>> a new untrusted signature, git is going to ignore the original bad\n>> signature and report untrusted commit instead. However, %GK and %GS\n>> format strings may still expand to the data corresponding\n>> to the original signature, potentially tricking the scripts into\n>> trusting the malicious commit.\n>>\n>> Given that the use of multiple signatures is quite rare, git does not\n>> support creating them without jumping through a few hoops, and\n>finally\n>> supporting them properly would require extensive API improvement, it\n>> seems reasonable to just reject them at the moment.\n>>\n>> Signed-off-by: Michał Górny \n>> ---\n>> gpg-interface.c | 90\n>+++++++++++++++++++++++++++-------------\n>> t/t7510-signed-commit.sh | 26 ++++++++++++\n>> 2 files changed, 87 insertions(+), 29 deletions(-)\n>>\n>> Changes in v4:\n>> * switched to using skip_prefix(),\n>> * renamed the variable to seen_exclusive_status,\n>> * made the loop terminate early on first duplicate status seen.\n>\n>Thanks for sticking to the topic and polishing it further. Looks\n>very good. \n>\n>Will replace.\n>\n>> +\tint seen_exclusive_status = 0;\n>> +\n>> +\t/* Iterate over all lines */\n>> +\tfor (line = buf; *line; line = strchrnul(line+1, '\\n')) {\n>> +\t\twhile (*line == '\\n')\n>> +\t\t\tline++;\n>> +\t\t/* Skip lines that don't start with GNUPG status */\n>> +\t\tif (!skip_prefix(line, \"[GNUPG:] \", &line))\n>> +\t\t\tcontinue;\n>> +\n>> +\t\t/* Iterate over all search strings */\n>> +\t\tfor (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n>> +\t\t\tif (skip_prefix(line, sigcheck_gpg_status[i].check, &line)) {\n>> +\t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE) {\n>> +\t\t\t\t\tif (++seen_exclusive_status > 1)\n>> +\t\t\t\t\t\tgoto found_duplicate_status;\n>\n>Very minor point but by not using pre-increment, i.e.\n>\n>\t\tif (seen_exclusive_status++)\n>\t\t\tgoto found_duplicate_status;\n>\n>you can use the expression as a \"have we already seen?\" boolean,\n>whic may probably be more idiomatic.\n>\n>The patch is good in the way written as-is, and this is so minor\n>that it is not worth rerolling to only update this part.\n\nPlease don't merge it yet. I gave it some more thought and I think the loop refactoring may cause TRUST_* to override BADSIG (i.e. upgrade from 'bad' to 'untrusted'). I'm going to verify this when I get home.\n\n>\n>Thanks.\n\n\n--\nBest regards, \nMichał Górny\n"},{"id":"361167","messageId":"1540221901.1299.4.camel@gentoo.org","threadId":"49625","inReplyTo":"7C705597-7AA9-4C4E-904B-CE8C4F01AD24@gentoo.org","subject":"Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-10-22T15:25:01Z","receivedAt":"2018-10-22T15:25:09Z","isPatch":true,"sender":{"key":"mgorny@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/110765?v=4"},"body":"On Mon, 2018-10-22 at 08:04 +0000, Michał Górny wrote:\n> Dnia October 20, 2018 11:57:36 PM UTC, Junio C Hamano napisał(a):\n> > Michał Górny writes:\n> > \n> > > GnuPG supports creating signatures consisting of multiple signature\n> > > packets. If such a signature is verified, it outputs all the status\n> > > messages for each signature separately. However, git currently does\n> > \n> > not\n> > > account for such scenario and gets terribly confused over getting\n> > > multiple *SIG statuses.\n> > > \n> > > For example, if a malicious party alters a signed commit and appends\n> > > a new untrusted signature, git is going to ignore the original bad\n> > > signature and report untrusted commit instead. However, %GK and %GS\n> > > format strings may still expand to the data corresponding\n> > > to the original signature, potentially tricking the scripts into\n> > > trusting the malicious commit.\n> > > \n> > > Given that the use of multiple signatures is quite rare, git does not\n> > > support creating them without jumping through a few hoops, and\n> > \n> > finally\n> > > supporting them properly would require extensive API improvement, it\n> > > seems reasonable to just reject them at the moment.\n> > > \n> > > Signed-off-by: Michał Górny \n> > > ---\n> > > gpg-interface.c | 90\n> > \n> > +++++++++++++++++++++++++++-------------\n> > > t/t7510-signed-commit.sh | 26 ++++++++++++\n> > > 2 files changed, 87 insertions(+), 29 deletions(-)\n> > > \n> > > Changes in v4:\n> > > * switched to using skip_prefix(),\n> > > * renamed the variable to seen_exclusive_status,\n> > > * made the loop terminate early on first duplicate status seen.\n> > \n> > Thanks for sticking to the topic and polishing it further. Looks\n> > very good. \n> > \n> > Will replace.\n> > \n> > > +\tint seen_exclusive_status = 0;\n> > > +\n> > > +\t/* Iterate over all lines */\n> > > +\tfor (line = buf; *line; line = strchrnul(line+1, '\\n')) {\n> > > +\t\twhile (*line == '\\n')\n> > > +\t\t\tline++;\n> > > +\t\t/* Skip lines that don't start with GNUPG status */\n> > > +\t\tif (!skip_prefix(line, \"[GNUPG:] \", &line))\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\t/* Iterate over all search strings */\n> > > +\t\tfor (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n> > > +\t\t\tif (skip_prefix(line, sigcheck_gpg_status[i].check, &line)) {\n> > > +\t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE) {\n> > > +\t\t\t\t\tif (++seen_exclusive_status > 1)\n> > > +\t\t\t\t\t\tgoto found_duplicate_status;\n> > \n> > Very minor point but by not using pre-increment, i.e.\n> > \n> > \t\tif (seen_exclusive_status++)\n> > \t\t\tgoto found_duplicate_status;\n> > \n> > you can use the expression as a \"have we already seen?\" boolean,\n> > whic may probably be more idiomatic.\n> > \n> > The patch is good in the way written as-is, and this is so minor\n> > that it is not worth rerolling to only update this part.\n> \n> Please don't merge it yet. I gave it some more thought and I think the loop refactoring may cause TRUST_* to override BADSIG (i.e. upgrade from 'bad' to 'untrusted'). I'm going to verify this when I get home.\n> \n\nI was wrong. I'm sorry about the noise. I've reverified the logic,\nand it correct. That is:\n\n1) for trusted signature, only GOODSIG is emitted and 'G' is returned\ncorrectly,\n\n2) for untrusted signature, GOODSIG is followed by TRUST_* messages,\nso line-wise TRUST_* check replaces the 'G' with 'U',\n\n3) for bad signature, only BADSIG is emitted without TRUST_* messages.\n\nFurthermore, GnuPG documentation confirms that TRUST_* is only emitted\nfor good signatures [1].\n\n[1]:https://github.com/gpg/gnupg/blob/master/doc/DETAILS#trust_\n\n-- \nBest regards,\nMichał Górny\n"},{"id":"362364","messageId":"CACsJy8DKD3F3o74gTHW-WEL_hpB8x+oaWX8_SwN01Nmz3W9Z_w@mail.gmail.com","threadId":"49625","inReplyTo":"20181020193020.28517-1-mgorny@gentoo.org","subject":"Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-03T15:17:07Z","receivedAt":"2018-11-03T15:17:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Oct 20, 2018 at 9:31 PM Michał Górny wrote:\n> +test_expect_success GPG 'detect fudged commit with double signature' '\n> + sed -e \"/gpgsig/,/END PGP/d\" forged1 >double-base &&\n> + sed -n -e \"/gpgsig/,/END PGP/p\" forged1 | \\\n> + sed -e \"s/^gpgsig//;s/^ //\" | gpg --dearmor >double-sig1.sig &&\n> + gpg -o double-sig2.sig -u 29472784 --detach-sign double-base &&\n> + cat double-sig1.sig double-sig2.sig | gpg --enarmor >double-combined.asc &&\n> + sed -e \"s/^\\(-.*\\)ARMORED FILE/\\1SIGNATURE/;1s/^/gpgsig /;2,\\$s/^/ /\" \\\n> + double-combined.asc > double-gpgsig &&\n> + sed -e \"/committer/r double-gpgsig\" double-base >double-commit &&\n> + git hash-object -w -t commit double-commit >double-commit.commit &&\n> + test_must_fail git verify-commit $(cat double-commit.commit) &&\n> + git show --pretty=short --show-signature $(cat double-commit.commit) >double-actual &&\n> + grep \"BAD signature from\" double-actual &&\n> + grep \"Good signature from\" double-actual\n> +'\n\nThis test fails on 'master' today for me\n\ngpg: WARNING: multiple signatures detected. Only the first will be checked.\ngpg: Signature made Sat Nov 3 15:13:28 2018 UTC\ngpg: using DSA key 13B6F51ECDDE430D\ngpg: issuer \"committer@example.com\"\ngpg: BAD signature from \"C O Mitter \" [ultimate]\ngpg: BAD signature from \"C O Mitter \" [ultimate]\nnot ok 16 - detect fudged commit with double signature\n\nPerhaps my gpg is too old?\n\n$ gpg --version\ngpg (GnuPG) 2.1.15\nlibgcrypt 1.7.3\nCopyright (C) 2016 Free Software Foundation, Inc.\nLicense GPLv3+: GNU GPL version 3 or later \nThis is free software: you are free to change and redistribute it.\nThere is NO WARRANTY, to the extent permitted by law.\n\nHome: /home/pclouds/.gnupg\nSupported algorithms:\nPubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA\nCipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,\n CAMELLIA128, CAMELLIA192, CAMELLIA256\nHash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224\nCompression: Uncompressed, ZIP, ZLIB, BZIP2\n-- \nDuy\n"},{"id":"362366","messageId":"1541259137.1028.12.camel@gentoo.org","threadId":"49625","inReplyTo":"CACsJy8DKD3F3o74gTHW-WEL_hpB8x+oaWX8_SwN01Nmz3W9Z_w@mail.gmail.com","subject":"Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-11-03T15:32:17Z","receivedAt":"2018-11-03T15:32:25Z","isPatch":true,"sender":{"key":"mgorny@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/110765?v=4"},"body":"On Sat, 2018-11-03 at 16:17 +0100, Duy Nguyen wrote:\n> On Sat, Oct 20, 2018 at 9:31 PM Michał Górny wrote:\n> > +test_expect_success GPG 'detect fudged commit with double signature' '\n> > + sed -e \"/gpgsig/,/END PGP/d\" forged1 >double-base &&\n> > + sed -n -e \"/gpgsig/,/END PGP/p\" forged1 | \\\n> > + sed -e \"s/^gpgsig//;s/^ //\" | gpg --dearmor >double-sig1.sig &&\n> > + gpg -o double-sig2.sig -u 29472784 --detach-sign double-base &&\n> > + cat double-sig1.sig double-sig2.sig | gpg --enarmor >double-combined.asc &&\n> > + sed -e \"s/^\\(-.*\\)ARMORED FILE/\\1SIGNATURE/;1s/^/gpgsig /;2,\\$s/^/ /\" \\\n> > + double-combined.asc > double-gpgsig &&\n> > + sed -e \"/committer/r double-gpgsig\" double-base >double-commit &&\n> > + git hash-object -w -t commit double-commit >double-commit.commit &&\n> > + test_must_fail git verify-commit $(cat double-commit.commit) &&\n> > + git show --pretty=short --show-signature $(cat double-commit.commit) >double-actual &&\n> > + grep \"BAD signature from\" double-actual &&\n> > + grep \"Good signature from\" double-actual\n> > +'\n> \n> This test fails on 'master' today for me\n> \n> gpg: WARNING: multiple signatures detected. Only the first will be checked.\n> gpg: Signature made Sat Nov 3 15:13:28 2018 UTC\n> gpg: using DSA key 13B6F51ECDDE430D\n> gpg: issuer \"committer@example.com\"\n> gpg: BAD signature from \"C O Mitter \" [ultimate]\n> gpg: BAD signature from \"C O Mitter \" [ultimate]\n> not ok 16 - detect fudged commit with double signature\n> \n> Perhaps my gpg is too old?\n> \n> $ gpg --version\n> gpg (GnuPG) 2.1.15\n> libgcrypt 1.7.3\n> Copyright (C) 2016 Free Software Foundation, Inc.\n> License GPLv3+: GNU GPL version 3 or later \n> This is free software: you are free to change and redistribute it.\n> There is NO WARRANTY, to the extent permitted by law.\n> \n> Home: /home/pclouds/.gnupg\n> Supported algorithms:\n> Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA\n> Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,\n> CAMELLIA128, CAMELLIA192, CAMELLIA256\n> Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224\n> Compression: Uncompressed, ZIP, ZLIB, BZIP2\n\nPerhaps this is indeed specific to this version of GnuPG. The tests\npass for me with both 1.4.21 and 2.2.10. We don't have 2.1* in Gentoo\nanymore.\n\n-- \nBest regards,\nMichał Górny\n"},{"id":"362368","messageId":"CACsJy8DwGHWLWxLrC2XZ5XS+se2W6gYCXtFjzWghgkgYGi08ig@mail.gmail.com","threadId":"49625","inReplyTo":"1541259137.1028.12.camel@gentoo.org","subject":"Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-03T15:36:17Z","receivedAt":"2018-11-03T15:36:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Nov 3, 2018 at 4:32 PM Michał Górny wrote:\n> > Perhaps my gpg is too old?\n> >\n> > $ gpg --version\n> > gpg (GnuPG) 2.1.15\n> > libgcrypt 1.7.3\n> > Copyright (C) 2016 Free Software Foundation, Inc.\n> > License GPLv3+: GNU GPL version 3 or later \n> > This is free software: you are free to change and redistribute it.\n> > There is NO WARRANTY, to the extent permitted by law.\n> >\n> > Home: /home/pclouds/.gnupg\n> > Supported algorithms:\n> > Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA\n> > Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,\n> > CAMELLIA128, CAMELLIA192, CAMELLIA256\n> > Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224\n> > Compression: Uncompressed, ZIP, ZLIB, BZIP2\n>\n> Perhaps this is indeed specific to this version of GnuPG. The tests\n> pass for me with both 1.4.21 and 2.2.10. We don't have 2.1* in Gentoo\n> anymore.\n\nYeah I have not really used gpg and neglected updating it. Will try it\nnow. The question remains though whether we need to support 2.1* (I\ndon't know at all about gnupg status, maybe 2.1* is indeed too\nold/buggy that nobody should use it and so we don't need to support\nit).\n-- \nDuy\n"},{"id":"362369","messageId":"CACsJy8AU5fbb2JgRNj9NVvJ9OsmO03LjX4x1rKe-Ls8V9xVK0w@mail.gmail.com","threadId":"49625","inReplyTo":"1541259137.1028.12.camel@gentoo.org","subject":"Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-03T15:42:34Z","receivedAt":"2018-11-03T15:43:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Nov 3, 2018 at 4:32 PM Michał Górny wrote:\n> Perhaps this is indeed specific to this version of GnuPG. The tests\n> pass for me with both 1.4.21 and 2.2.10. We don't have 2.1* in Gentoo\n> anymore.\n\nUpdated to 2.2.8 and the test is passed.\n-- \nDuy\n"},{"id":"362370","messageId":"1541260696.1028.16.camel@gentoo.org","threadId":"49625","inReplyTo":"CACsJy8DwGHWLWxLrC2XZ5XS+se2W6gYCXtFjzWghgkgYGi08ig@mail.gmail.com","subject":"Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-11-03T15:58:16Z","receivedAt":"2018-11-03T15:58:25Z","isPatch":true,"sender":{"key":"mgorny@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/110765?v=4"},"body":"On Sat, 2018-11-03 at 16:36 +0100, Duy Nguyen wrote:\n> On Sat, Nov 3, 2018 at 4:32 PM Michał Górny wrote:\n> > > Perhaps my gpg is too old?\n> > > \n> > > $ gpg --version\n> > > gpg (GnuPG) 2.1.15\n> > > libgcrypt 1.7.3\n> > > Copyright (C) 2016 Free Software Foundation, Inc.\n> > > License GPLv3+: GNU GPL version 3 or later \n> > > This is free software: you are free to change and redistribute it.\n> > > There is NO WARRANTY, to the extent permitted by law.\n> > > \n> > > Home: /home/pclouds/.gnupg\n> > > Supported algorithms:\n> > > Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA\n> > > Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,\n> > > CAMELLIA128, CAMELLIA192, CAMELLIA256\n> > > Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224\n> > > Compression: Uncompressed, ZIP, ZLIB, BZIP2\n> > \n> > Perhaps this is indeed specific to this version of GnuPG. The tests\n> > pass for me with both 1.4.21 and 2.2.10. We don't have 2.1* in Gentoo\n> > anymore.\n> \n> Yeah I have not really used gpg and neglected updating it. Will try it\n> now. The question remains though whether we need to support 2.1* (I\n> don't know at all about gnupg status, maybe 2.1* is indeed too\n> old/buggy that nobody should use it and so we don't need to support\n> it).\n\nGnuPG upstream considers 2.2 as continuation/mature version of 2.1\nbranch. They currently support running either newest version of 1.4\n(legacy) or newest version of 2.2 [1]. In other words, this might have\nbeen a bug that was fixed in newer release (possibly 2.2.x).\n\n[1]:https://gnupg.org/download/index.html#text-end-of-life\n\n-- \nBest regards,\nMichał Górny\n"}]}