{"thread":{"id":"49553","subject":"[PATCH v3] gpg-interface.c: detect and reject multiple signatures on commits","startedAt":"2018-10-12T21:17:45Z","lastAt":"2018-10-16T02:13:40Z","messageCount":5,"participants":["Michał Górny","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"360350","messageId":"20181012210928.18033-1-mgorny@gentoo.org","threadId":"49553","inReplyTo":null,"subject":"[PATCH v3] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-10-12T21:09:28Z","receivedAt":"2018-10-12T21:17:45Z","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 <mgorny@gentoo.org>\n---\n gpg-interface.c          | 94 +++++++++++++++++++++++++++-------------\n t/t7510-signed-commit.sh | 26 +++++++++++\n 2 files changed, 91 insertions(+), 29 deletions(-)\n\nChanges in v3: reworked the whole loop to iterate over lines rather\nthan scanning the whole buffer, as requested.  Now it also catches\nduplicate instances of the same status.\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex db17d65f8..480aab4ee 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -75,48 +75,84 @@ 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 had_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 (strncmp(line, \"[GNUPG:] \", 9))\n+\t\t\tcontinue;\n+\t\tline += 9;\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 (!strncmp(line, sigcheck_gpg_status[i].check,\n+\t\t\t\t\tstrlen(sigcheck_gpg_status[i].check))) {\n+\t\t\t\tline += strlen(sigcheck_gpg_status[i].check);\n+\n+\t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE)\n+\t\t\t\t\thad_exclusive_status++;\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+\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+\tif (had_exclusive_status > 1) {\n+\t\tsigc->result = 'E';\n+\t\t/* Clear partial data to avoid confusion */\n+\t\tif (sigc->signer)\n+\t\t\tFREE_AND_NULL(sigc->signer);\n+\t\tif (sigc->key)\n+\t\t\tFREE_AND_NULL(sigc->key);\n+\t}\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":"360476","messageId":"xmqq8t306ii2.fsf@gitster-ct.c.googlers.com","threadId":"49553","inReplyTo":"20181012210928.18033-1-mgorny@gentoo.org","subject":"Re: [PATCH v3] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-15T02:39:17Z","receivedAt":"2018-10-15T02:39:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, will take a look.\n"},{"id":"360477","messageId":"xmqqva636g2t.fsf@gitster-ct.c.googlers.com","threadId":"49553","inReplyTo":"20181012210928.18033-1-mgorny@gentoo.org","subject":"Re: [PATCH v3] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-15T03:31:38Z","receivedAt":"2018-10-15T03:31:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Górny <mgorny@gentoo.org> 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 <mgorny@gentoo.org>\n> ---\n>  gpg-interface.c          | 94 +++++++++++++++++++++++++++-------------\n>  t/t7510-signed-commit.sh | 26 +++++++++++\n>  2 files changed, 91 insertions(+), 29 deletions(-)\n>\n> Changes in v3: reworked the whole loop to iterate over lines rather\n> than scanning the whole buffer, as requested.  Now it also catches\n> duplicate instances of the same status.\n>\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index db17d65f8..480aab4ee 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -75,48 +75,84 @@ 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 had_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 (strncmp(line, \"[GNUPG:] \", 9))\n> +\t\t\tcontinue;\n> +\t\tline += 9;\n\nYou do not want to count to 9 yourself.  Instead\n\n\tif (!skip_prefix(line, \"[GNUPG:] \", &line))\n\t\tcontinue;\n\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 (!strncmp(line, sigcheck_gpg_status[i].check,\n> +\t\t\t\t\tstrlen(sigcheck_gpg_status[i].check))) {\n> +\t\t\t\tline += strlen(sigcheck_gpg_status[i].check);\n\nLikewise.\n\n> +\t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE)\n> +\t\t\t\t\thad_exclusive_status++;\n\n\"has\" is fine, but I think existing code elsewhere we use \"seen\" for\nthings like this.\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> +\t\t\t\tbreak;\n>  \t\t\t}\n>  \t\t}\n>  \t}\n\nSo unless U/E, we expect to see a key, and unless E, we also expect\nthere is a signer; we keep the last value we see in the sequence in\nsigc.  Because all of these that are not U are marked exclusive, if\nwe check if sigc->key already has value at the point you free the\nsigc->key field above, we can see if there is a duplicate record\nthat are of \"exclusive\" type?  I am not suggesting to lose the\naddition of \"flags = GPG_STATUS_EXCLUSIVE|0\" field, but trying to\nsee if I am getting the logic right.\n\nFor gpg_status that is !GPG_STATUS_EXCLUSIVE (i.e. \"U\"), we do not\ndo any replacement of already seen .key/.signer, and all the cases\nthat we do the replacement are GPG_STATUS_EXCLUSIVE, which we know\nwill become an error in the code below when we do see twice.  So it\nis fine not to check if .key/.signer we see twice are the same or\ndifferent.  It is an error even if we see the same .key/.signer\ntwice---having two records is already wrong no matter whose key/sign\nit is.\n\nOK, so the whole thing makes sense to me.\n\nHaving said that, if we wanted to short-circuit, I think\n\n                for (each line) {\n                        for (each sigcheck_gpg_status[]) {\n                                if (not the one on line)\n                                        continue;\n                                if (sigc->result != 'U') {\n                                        if (sigc->key)\n                                                goto found_dup;\n                                        sigc->key = make a copy;\n                                        if (*next && sigc->result != 'E') {\n                                                if (sigc->signer)\n                                                        goto found_dup;\n                                                sigc->signer = make a copy;\n                                        }\n                                }\n                                break;\n                        }\n                }\n                return;\n\n        found_dup:\n                sigc->result = 'E';\n                FREE_AND_NULL(sigc->signer);\n                FREE_AND_NULL(sigc->key);\n                return;\n\t\t\nwould also be fine.\n\n> +\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> +\tif (had_exclusive_status > 1) {\n> +\t\tsigc->result = 'E';\n> +\t\t/* Clear partial data to avoid confusion */\n> +\t\tif (sigc->signer)\n> +\t\t\tFREE_AND_NULL(sigc->signer);\n> +\t\tif (sigc->key)\n> +\t\t\tFREE_AND_NULL(sigc->key);\n\nI think it is OK to use FREE_AND_NULL() unconditionally (just like\nwe can use free(x) on x==NULL).\n\n> +\t}\n>  }\n\n"},{"id":"360554","messageId":"1539636266.1014.6.camel@gentoo.org","threadId":"49553","inReplyTo":"xmqqva636g2t.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-10-15T20:44:26Z","receivedAt":"2018-10-15T20:44:35Z","isPatch":true,"sender":{"key":"mgorny@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/110765?v=4"},"body":"On Mon, 2018-10-15 at 12:31 +0900, Junio C Hamano wrote:\n> Michał Górny <mgorny@gentoo.org> 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 <mgorny@gentoo.org>\n> > ---\n> >  gpg-interface.c          | 94 +++++++++++++++++++++++++++-------------\n> >  t/t7510-signed-commit.sh | 26 +++++++++++\n> >  2 files changed, 91 insertions(+), 29 deletions(-)\n> > \n> > Changes in v3: reworked the whole loop to iterate over lines rather\n> > than scanning the whole buffer, as requested.  Now it also catches\n> > duplicate instances of the same status.\n> > \n> > diff --git a/gpg-interface.c b/gpg-interface.c\n> > index db17d65f8..480aab4ee 100644\n> > --- a/gpg-interface.c\n> > +++ b/gpg-interface.c\n> > @@ -75,48 +75,84 @@ 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 had_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 (strncmp(line, \"[GNUPG:] \", 9))\n> > +\t\t\tcontinue;\n> > +\t\tline += 9;\n> \n> You do not want to count to 9 yourself.  Instead\n> \n> \tif (!skip_prefix(line, \"[GNUPG:] \", &line))\n> \t\tcontinue;\n> \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 (!strncmp(line, sigcheck_gpg_status[i].check,\n> > +\t\t\t\t\tstrlen(sigcheck_gpg_status[i].check))) {\n> > +\t\t\t\tline += strlen(sigcheck_gpg_status[i].check);\n> \n> Likewise.\n\nBoth done.\n\n> \n> > +\t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE)\n> > +\t\t\t\t\thad_exclusive_status++;\n> \n> \"has\" is fine, but I think existing code elsewhere we use \"seen\" for\n> things like this.\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> > +\t\t\t\tbreak;\n> >  \t\t\t}\n> >  \t\t}\n> >  \t}\n> \n> So unless U/E, we expect to see a key, and unless E, we also expect\n> there is a signer; we keep the last value we see in the sequence in\n> sigc.  Because all of these that are not U are marked exclusive, if\n> we check if sigc->key already has value at the point you free the\n> sigc->key field above, we can see if there is a duplicate record\n> that are of \"exclusive\" type?  I am not suggesting to lose the\n> addition of \"flags = GPG_STATUS_EXCLUSIVE|0\" field, but trying to\n> see if I am getting the logic right.\n> \n> For gpg_status that is !GPG_STATUS_EXCLUSIVE (i.e. \"U\"), we do not\n> do any replacement of already seen .key/.signer, and all the cases\n> that we do the replacement are GPG_STATUS_EXCLUSIVE, which we know\n> will become an error in the code below when we do see twice.  So it\n> is fine not to check if .key/.signer we see twice are the same or\n> different.  It is an error even if we see the same .key/.signer\n> twice---having two records is already wrong no matter whose key/sign\n> it is.\n> \n> OK, so the whole thing makes sense to me.\n> \n> Having said that, if we wanted to short-circuit, I think\n> \n>                 for (each line) {\n>                         for (each sigcheck_gpg_status[]) {\n>                                 if (not the one on line)\n>                                         continue;\n>                                 if (sigc->result != 'U') {\n>                                         if (sigc->key)\n>                                                 goto found_dup;\n>                                         sigc->key = make a copy;\n>                                         if (*next && sigc->result != 'E') {\n>                                                 if (sigc->signer)\n>                                                         goto found_dup;\n>                                                 sigc->signer = make a copy;\n>                                         }\n>                                 }\n>                                 break;\n>                         }\n>                 }\n>                 return;\n> \n>         found_dup:\n>                 sigc->result = 'E';\n>                 FREE_AND_NULL(sigc->signer);\n>                 FREE_AND_NULL(sigc->key);\n>                 return;\n> \t\t\n> would also be fine.\n\nDo I understand correctly that you mean to take advantage that 'seen\nexclusive status' cases match 'seen key' cases?  I think this would be\na little less readable.\n\nThat said, I was planning on next patch that replaced the \"!= 'U'\" test\nwith explicit flags for whether a particular status includes key\nand UID.  If you'd agree with this direction, I think having this one\nseparate as well would make sense.\n\n> > +\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> > +\tif (had_exclusive_status > 1) {\n> > +\t\tsigc->result = 'E';\n> > +\t\t/* Clear partial data to avoid confusion */\n> > +\t\tif (sigc->signer)\n> > +\t\t\tFREE_AND_NULL(sigc->signer);\n> > +\t\tif (sigc->key)\n> > +\t\t\tFREE_AND_NULL(sigc->key);\n> \n> I think it is OK to use FREE_AND_NULL() unconditionally (just like\n> we can use free(x) on x==NULL).\n\nDone as well.\n\n> \n> > +\t}\n> >  }\n> \n> \n\n-- \nBest regards,\nMichał Górny\n"},{"id":"360567","messageId":"xmqqva623agh.fsf@gitster-ct.c.googlers.com","threadId":"49553","inReplyTo":"1539636266.1014.6.camel@gentoo.org","subject":"Re: [PATCH v3] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-16T02:13:34Z","receivedAt":"2018-10-16T02:13:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Górny <mgorny@gentoo.org> writes:\n\n>> OK, so the whole thing makes sense to me.\n>> \n>> Having said that, if we wanted to short-circuit, I think\n>> \n>>                 for (each line) {\n>>                         for (each sigcheck_gpg_status[]) {\n>>                                 if (not the one on line)\n>>                                         continue;\n>>                                 if (sigc->result != 'U') {\n>>                                         if (sigc->key)\n>>                                                 goto found_dup;\n>>                                         sigc->key = make a copy;\n>>                                         if (*next && sigc->result != 'E') {\n>>                                                 if (sigc->signer)\n>>                                                         goto found_dup;\n>>                                                 sigc->signer = make a copy;\n>>                                         }\n>>                                 }\n>>                                 break;\n>>                         }\n>>                 }\n>>                 return;\n>> \n>>         found_dup:\n>>                 sigc->result = 'E';\n>>                 FREE_AND_NULL(sigc->signer);\n>>                 FREE_AND_NULL(sigc->key);\n>>                 return;\n>> \t\t\n>> would also be fine.\n>\n> Do I understand correctly that you mean to take advantage that 'seen\n> exclusive status' cases match 'seen key' cases?  I think this would be\n> a little less readable.\n\n\nYes, the above is taking advantage of: exclusive ones do give us\nkey and/or signer, so it is a sign that we've found collision\nbetween two exclusive status line if we need to free and replace.\n\nBut that was \"whole thing makes sense, but if we wanted to...\".  I\ndo not know if we want to short-circuit upon finding a single\nproblem, or parse the whole thing to the end.  I guess we could\nshort-circuit while still using the \"seen-exclusive\" variable (we\ncan just do so at the place seen-exclusive is incremented---if it is\nalready one, then we know we have seen one already and we are\nlooking at another one).\n\n> That said, I was planning on next patch that replaced the \"!= 'U'\" test\n> with explicit flags for whether a particular status includes key\n> and UID.  If you'd agree with this direction, I think having this one\n> separate as well would make sense.\n\nYup, it might be a bit over-engineered for this code, but we are\nadding the \"exclusive\" bit to the status[] array already, and I\nthink it makes sense to also have \"does this give us key?\" and \"does\nthis tell us signer?\" bit there.\n\nThanks.\n"}]}