{"thread":{"id":"49151","subject":"[PATCH v2] gpg-interface.c: detect and reject multiple signatures on commits","startedAt":"2018-08-17T07:34:54Z","lastAt":"2018-10-05T06:14:21Z","messageCount":5,"participants":["Michał Górny","Stefan Beller","Tacitus Aedifex","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"355895","messageId":"20180817073441.5247-1-mgorny@gentoo.org","threadId":"49151","inReplyTo":null,"subject":"[PATCH v2] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-08-17T07:34:41Z","receivedAt":"2018-08-17T07:34:54Z","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          | 41 ++++++++++++++++++++++++++++++++--------\n t/t7510-signed-commit.sh | 26 +++++++++++++++++++++++++\n 2 files changed, 59 insertions(+), 8 deletions(-)\n\nChanges in v2:\n* used generic 'flags' instead of boolean field,\n* added test case for git-verify-commit & git-show.\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 09ddfbc26..35c25106a 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -21,24 +21,29 @@ 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', \"\\n[GNUPG:] GOODSIG \", GPG_STATUS_EXCLUSIVE },\n+\t{ 'B', \"\\n[GNUPG:] BADSIG \", GPG_STATUS_EXCLUSIVE },\n+\t{ 'U', \"\\n[GNUPG:] TRUST_NEVER\", 0 },\n+\t{ 'U', \"\\n[GNUPG:] TRUST_UNDEFINED\", 0 },\n+\t{ 'E', \"\\n[GNUPG:] ERRSIG \", GPG_STATUS_EXCLUSIVE },\n+\t{ 'X', \"\\n[GNUPG:] EXPSIG \", GPG_STATUS_EXCLUSIVE },\n+\t{ 'Y', \"\\n[GNUPG:] EXPKEYSIG \", GPG_STATUS_EXCLUSIVE },\n+\t{ 'R', \"\\n[GNUPG:] 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 \tint i;\n+\tint had_exclusive_status = 0;\n \n \t/* Iterate over all search strings */\n \tfor (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n@@ -50,6 +55,10 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\t\t\tcontinue;\n \t\t\tfound += strlen(sigcheck_gpg_status[i].check);\n \t\t}\n+\n+\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE)\n+\t\t\thad_exclusive_status++;\n+\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@@ -62,6 +71,22 @@ static void parse_gpg_output(struct signature_check *sigc)\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 6e2015ed9..51fb92a72 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -227,4 +227,30 @@ test_expect_success GPG 'log.showsignature behaves like --show-signature' '\n \tgrep \"gpg: Good signature\" actual\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.18.0\n\n"},{"id":"359501","messageId":"1538555376.1042.3.camel@gentoo.org","threadId":"49151","inReplyTo":"20180817073441.5247-1-mgorny@gentoo.org","subject":"Re: [PATCH v2] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-10-03T08:29:36Z","receivedAt":"2018-10-03T08:29:45Z","isPatch":true,"sender":{"key":"mgorny@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/110765?v=4"},"body":"On Fri, 2018-08-17 at 09:34 +0200, Michał Górny wrote:\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\nGentle ping.\n\n-- \nBest regards,\nMichał Górny\n"},{"id":"359565","messageId":"CAGZ79kZ97saPgp55mY8CPqrxQMo8cmQ+oB7moVTk3hgyRn7tZw@mail.gmail.com","threadId":"49151","inReplyTo":"1538555376.1042.3.camel@gentoo.org","subject":"Re: [PATCH v2] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-03T18:57:13Z","receivedAt":"2018-10-03T18:57:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 3, 2018 at 1:29 AM Michał Górny <mgorny@gentoo.org> wrote:\n>\n> On Fri, 2018-08-17 at 09:34 +0200, Michał Górny wrote:\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>\n> Gentle ping.\n\nI am not an expert on GPG, but the patch (design, code, test) looks\nreasonable to me.\n"},{"id":"359643","messageId":"20181004225229.GA15236@SDF.ORG","threadId":"49151","inReplyTo":"20180817073441.5247-1-mgorny@gentoo.org","subject":"Re: [PATCH v2] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Tacitus Aedifex","fromEmail":"aedifex@sdf.org","sentAt":"2018-10-04T22:52:29Z","receivedAt":"2018-10-04T22:52:45Z","isPatch":true,"sender":{"key":"aedifex@sdf.org","avatar":"https://gravatar.com/avatar/7d5361117c8a59b99af67c0282a2785183f7cee0f2a3294d8d3dcb7a6c00387d?d=mp&s=160"},"body":"I think that there is a more simple way to catch multiple signatures see below.  \nOther than that, I like this patch.\n\nSigned-off-by: Tacitus Aedifex <aedifex@sdf.org>\n---\n gpg-interface.c | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex db17d65f8..a4dba3361 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -93,6 +93,7 @@ static void parse_gpg_output(struct signature_check *sigc)\n {\n \tconst char *buf = sigc->gpg_status;\n \tint i;\n+\tint multi_sig = 0;\n \n \t/* Iterate over all search strings */\n \tfor (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n@@ -115,6 +116,23 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\t\t\tnext = strchrnul(found, '\\n');\n \t\t\t\tsigc->signer = xmemdupz(found, next - found);\n \t\t\t}\n+\t\t} else \n+\t\t\tmulti_sig++;\n+\n+\t\t/*\n+\t\t * GOODSIG, BADSIG, etc. can occure only once for each signature.\n+\t\t * Therefore, if we had more than one then we're dealing with\n+\t\t * multiple signatures. We don't support them currently and they are\n+\t\t * rather hard to create, so something is likely probably not right\n+\t\t * and we should reject them altogether.\n+\t\t */\n+\t\tif (multi_sig > 1) {\n+\t\t\tsigc->result = 'E';\n+\t\t\t/* clear partial data to avoid confusion */\n+\t\t\tif (sigc->signer)\n+\t\t\t\tFREE_AND_NULL(sigc->signer);\n+\t\t\tif (sigc->key)\n+\t\t\t\tFREE_AND_NULL(sigc->key);\n \t\t}\n \t}\n }\n--\n2.18.0.129.ge333175\n-- \n"},{"id":"359652","messageId":"xmqqin2g3op4.fsf@gitster-ct.c.googlers.com","threadId":"49151","inReplyTo":"1538555376.1042.3.camel@gentoo.org","subject":"Re: [PATCH v2] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-05T06:14:15Z","receivedAt":"2018-10-05T06:14:21Z","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> On Fri, 2018-08-17 at 09:34 +0200, Michał Górny wrote:\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>\n> Gentle ping.\n\nI think among the three issues raised in the review of v1 [*1*], one\nof them remain unaddressed.  Other than that the addition relative\nto v2 looks reasonable (but I only skimmed the patch).\n\n[Reference] *1* https://public-inbox.org/git/xmqq1saxc5gu.fsf@gitster-ct.c.googlers.com/ \n\nRelevant part reproduced here.\n\n>>>  \t/* Iterate over all search strings */\n>>>  \tfor (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n>>> @@ -50,6 +52,10 @@ static void parse_gpg_output(struct signature_check *sigc)\n>>>  \t\t\t\tcontinue;\n>>>  \t\t\tfound += strlen(sigcheck_gpg_status[i].check);\n>>> ...\n>>> +\tif (had_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>> Makes sense to me.\n>\n> I was wondering if we have to revamp the loop altogether.  The\n> current code runs through the list of all the possible \"status\"\n> lines, and find the first occurrence for each type in the buffer\n> that has GPG output.  Second and subsequent occurrence of the same\n> type, if existed, will not be noticed by the original loop\n> structure, and this patch does not change it, even though the topic\n> of the patch is about rejecting the signature block with elements\n> taken from multiple signatures.\n\nWhich still smells to me that it points out a grave (made grave by\nwhat the patch claims to address) issue in the implementation of v1;\ndid v2 get substantially updated to address the concern?\n\n> One way to fix it may be to keep\n> the current loop structure to go over the sigcheck_gpg_status[],\n> but make the logic inside the loop into an inner loop that finds all\n> occurrences of the same type, instead of stopping after finding the\n> first instance.  But once we go to that length, I suspect that it\n> may be cleaner to iterate over the lines in the buffer, checking\n> each line if it matches one of the recognized \"[GNUPG:] FOOSIG\"\n> lines and acting on it (while ignoring unrecognized lines).\n\n\nP.S. I'd be either offline or otherwise occupied until the next\nweek, so there is no need to hastily prepare an updated patch\nseries.\n\nThanks.\n\n"}]}