{"thread":{"id":"49140","subject":"Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits","startedAt":"2018-08-15T21:31:12Z","lastAt":"2018-08-17T16:39:56Z","messageCount":4,"participants":["Jonathan Nieder","Michał Górny","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"355771","messageId":"20180815213108.GM181377@aiede.svl.corp.google.com","threadId":"49140","inReplyTo":"20180814151142.13960-1-mgorny@gentoo.org","subject":"Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-15T21:31:08Z","receivedAt":"2018-08-15T21:31:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michał Górny wrote:\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\nThanks for the clear analysis and fix.\n\nMay we have your sign-off?  See\nhttps://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#sign-off\n(or the equivalent section of your local copy of\nDocumentation/SubmittingPatches) for what this means.\n\n>  gpg-interface.c | 38 ++++++++++++++++++++++++++++++--------\n>  1 file changed, 30 insertions(+), 8 deletions(-)\n>\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 09ddfbc26..4e03aec15 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -24,21 +24,23 @@ void signature_check_clear(struct signature_check *sigc)\n>  static struct {\n>  \tchar result;\n>  \tconst char *check;\n> +\tint is_status;\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 \", 1 },\n> +\t{ 'B', \"\\n[GNUPG:] BADSIG \", 1 },\n> +\t{ 'U', \"\\n[GNUPG:] TRUST_NEVER\", 0 },\n> +\t{ 'U', \"\\n[GNUPG:] TRUST_UNDEFINED\", 0 },\n> +\t{ 'E', \"\\n[GNUPG:] ERRSIG \", 1},\n> +\t{ 'X', \"\\n[GNUPG:] EXPSIG \", 1},\n> +\t{ 'Y', \"\\n[GNUPG:] EXPKEYSIG \", 1},\n> +\t{ 'R', \"\\n[GNUPG:] REVKEYSIG \", 1},\n>  };\n\nnit: I wonder if making is_status into a flag field (like 'option' in\ngit.c's cmd_struct) and having an explicit SIGNATURE_STATUS value to\nput there would make this easier to read.\n\nIt's not clear to me that the name is_status or SIGNATURE_STATUS\ncaptures what this field represents.  Aren't these all sigcheck\nstatuses?  Can you describe briefly what distinguishes the cases where\nthis should be 0 versus 1?\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_status = 0;\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>  \t\t}\n> +\n> +\t\tif (sigcheck_gpg_status[i].is_status)\n> +\t\t\thad_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 +68,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_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\nMakes sense to me.\n\n>  }\n>  \n>  int check_signature(const char *payload, size_t plen, const char *signature,\n> -- \n> 2.18.0\n\nCan we have a test to make sure this behavior doesn't regress?  See\nt/README for an overview of the test framework and \"git grep -e gpg t/\"\nfor some examples.\n\nThe result looks good.  Thanks again for writing it.\n\nSincerely,\nJonathan\n"},{"id":"355891","messageId":"1534488137.1262.2.camel@gentoo.org","threadId":"49140","inReplyTo":"20180815213108.GM181377@aiede.svl.corp.google.com","subject":"Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-08-17T06:42:17Z","receivedAt":"2018-08-17T06:42:25Z","isPatch":true,"sender":{"key":"mgorny@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/110765?v=4"},"body":"On Wed, 2018-08-15 at 14:31 -0700, Jonathan Nieder wrote:\n> Michał Górny wrote:\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> \n> Thanks for the clear analysis and fix.\n> \n> May we have your sign-off?  See\n> https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#sign-off\n> (or the equivalent section of your local copy of\n> Documentation/SubmittingPatches) for what this means.\n\nOf course, I'm sorry for missing it in the original submission.\n\n> \n> >  gpg-interface.c | 38 ++++++++++++++++++++++++++++++--------\n> >  1 file changed, 30 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/gpg-interface.c b/gpg-interface.c\n> > index 09ddfbc26..4e03aec15 100644\n> > --- a/gpg-interface.c\n> > +++ b/gpg-interface.c\n> > @@ -24,21 +24,23 @@ void signature_check_clear(struct signature_check *sigc)\n> >  static struct {\n> >  \tchar result;\n> >  \tconst char *check;\n> > +\tint is_status;\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 \", 1 },\n> > +\t{ 'B', \"\\n[GNUPG:] BADSIG \", 1 },\n> > +\t{ 'U', \"\\n[GNUPG:] TRUST_NEVER\", 0 },\n> > +\t{ 'U', \"\\n[GNUPG:] TRUST_UNDEFINED\", 0 },\n> > +\t{ 'E', \"\\n[GNUPG:] ERRSIG \", 1},\n> > +\t{ 'X', \"\\n[GNUPG:] EXPSIG \", 1},\n> > +\t{ 'Y', \"\\n[GNUPG:] EXPKEYSIG \", 1},\n> > +\t{ 'R', \"\\n[GNUPG:] REVKEYSIG \", 1},\n> >  };\n> \n> nit: I wonder if making is_status into a flag field (like 'option' in\n> git.c's cmd_struct) and having an explicit SIGNATURE_STATUS value to\n> put there would make this easier to read.\n\nI think that makes sense.\n\n> \n> It's not clear to me that the name is_status or SIGNATURE_STATUS\n> captures what this field represents.  Aren't these all sigcheck\n> statuses?  Can you describe briefly what distinguishes the cases where\n> this should be 0 versus 1?\n\nYes, the name really does suck.  Maybe it should be EXCLUSIVE_STATUS\nor something like that, to distinguish from things that can occur\nsimultaneously to them.\n\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_status = 0;\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> >  \t\t}\n> > +\n> > +\t\tif (sigcheck_gpg_status[i].is_status)\n> > +\t\t\thad_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 +68,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_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> >  }\n> >  \n> >  int check_signature(const char *payload, size_t plen, const char *signature,\n> > -- \n> > 2.18.0\n> \n> Can we have a test to make sure this behavior doesn't regress?  See\n> t/README for an overview of the test framework and \"git grep -e gpg t/\"\n> for some examples.\n\nWill try.  Do I presume correctly that I should include the commit\nobject with the double signature instead of hacking git to construct it?\n;-)\n\n> \n> The result looks good.  Thanks again for writing it.\n> \n> Sincerely,\n> Jonathan\n\n-- \nBest regards,\nMichał Górny\n"},{"id":"355894","messageId":"20180817065444.GC131749@aiede.svl.corp.google.com","threadId":"49140","inReplyTo":"1534488137.1262.2.camel@gentoo.org","subject":"Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-17T06:54:44Z","receivedAt":"2018-08-17T06:54:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michał Górny wrote:\n> On Wed, 2018-08-15 at 14:31 -0700, Jonathan Nieder wrote:\n\n>> It's not clear to me that the name is_status or SIGNATURE_STATUS\n>> captures what this field represents.  Aren't these all sigcheck\n>> statuses?  Can you describe briefly what distinguishes the cases where\n>> this should be 0 versus 1?\n[...]\n>                                  Maybe it should be EXCLUSIVE_STATUS\n> or something like that, to distinguish from things that can occur\n> simultaneously to them.\n\nThanks.  Makes sense.\n\n[...]\n>> Can we have a test to make sure this behavior doesn't regress?  See\n>> t/README for an overview of the test framework and \"git grep -e gpg t/\"\n>> for some examples.\n>\n> Will try.  Do I presume correctly that I should include the commit\n> object with the double signature instead of hacking git to construct it?\n> ;-)\n\nGood question.  You can hack away with a new program in t/helper/, or\nyou can make your test do object manipulation with \"git cat-file\ncommit <object>\" and \"git hash-object -t commit -w --stdin\".  If you\nrun into trouble, just let the list know and I'm happy to try to help.\n(Or if you would like real-time help, I'm usually in #git-devel on\nfreenode.)\n\nJonathan\n"},{"id":"355919","messageId":"xmqq1saxc5gu.fsf@gitster-ct.c.googlers.com","threadId":"49140","inReplyTo":"20180815213108.GM181377@aiede.svl.corp.google.com","subject":"Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-17T16:39:45Z","receivedAt":"2018-08-17T16:39:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Michał Górny wrote:\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>\n> Thanks for the clear analysis and fix.\n>\n> May we have your sign-off?  See\n> https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#sign-off\n> (or the equivalent section of your local copy of\n> Documentation/SubmittingPatches) for what this means.\n\nI do not see the original message you are writing response to on\npublic-inbox archive.  As long as an update with sign-off will be\nsent to the git@vger.kernel.org list, that is OK.\n\n>>  gpg-interface.c | 38 ++++++++++++++++++++++++++++++--------\n>>  1 file changed, 30 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/gpg-interface.c b/gpg-interface.c\n>> index 09ddfbc26..4e03aec15 100644\n>> --- a/gpg-interface.c\n>> +++ b/gpg-interface.c\n>> @@ -24,21 +24,23 @@ void signature_check_clear(struct signature_check *sigc)\n>>  static struct {\n>>  \tchar result;\n>>  \tconst char *check;\n>> +\tint is_status;\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 \", 1 },\n>> +\t{ 'B', \"\\n[GNUPG:] BADSIG \", 1 },\n>> +\t{ 'U', \"\\n[GNUPG:] TRUST_NEVER\", 0 },\n>> +\t{ 'U', \"\\n[GNUPG:] TRUST_UNDEFINED\", 0 },\n>> +\t{ 'E', \"\\n[GNUPG:] ERRSIG \", 1},\n>> +\t{ 'X', \"\\n[GNUPG:] EXPSIG \", 1},\n>> +\t{ 'Y', \"\\n[GNUPG:] EXPKEYSIG \", 1},\n>> +\t{ 'R', \"\\n[GNUPG:] REVKEYSIG \", 1},\n>>  };\n>\n> nit: I wonder if making is_status into a flag field (like 'option' in\n> git.c's cmd_struct) and having an explicit SIGNATURE_STATUS value to\n> put there would make this easier to read.\n>\n> It's not clear to me that the name is_status or SIGNATURE_STATUS\n> captures what this field represents.  Aren't these all sigcheck\n> statuses?  Can you describe briefly what distinguishes the cases where\n> this should be 0 versus 1?\n\nGood suggestion.\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_status = 0;\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>>  \t\t}\n>> +\n>> +\t\tif (sigcheck_gpg_status[i].is_status)\n>> +\t\t\thad_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 +68,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_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\nI was wondering if we have to revamp the loop altogether.  The\ncurrent code runs through the list of all the possible \"status\"\nlines, and find the first occurrence for each type in the buffer\nthat has GPG output.  Second and subsequent occurrence of the same\ntype, if existed, will not be noticed by the original loop\nstructure, and this patch does not change it, even though the topic\nof the patch is about rejecting the signature block with elements\ntaken from multiple signatures.  One way to fix it may be to keep\nthe current loop structure to go over the sigcheck_gpg_status[],\nbut make the logic inside the loop into an inner loop that finds all\noccurrences of the same type, instead of stopping after finding the\nfirst instance.  But once we go to that length, I suspect that it\nmay be cleaner to iterate over the lines in the buffer, checking\neach line if it matches one of the recognized \"[GNUPG:] FOOSIG\"\nlines and acting on it (while ignoring unrecognized lines).\n\n>>  }\n>>  \n>>  int check_signature(const char *payload, size_t plen, const char *signature,\n>> -- \n>> 2.18.0\n>\n> Can we have a test to make sure this behavior doesn't regress?  See\n> t/README for an overview of the test framework and \"git grep -e gpg t/\"\n> for some examples.\n>\n> The result looks good.  Thanks again for writing it.\n>\n> Sincerely,\n> Jonathan\n"}]}