Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits

Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits

[Jonathan Nieder]

Michał Górny wrote:

> GnuPG supports creating signatures consisting of multiple signature
> packets.  If such a signature is verified, it outputs all the status
> messages for each signature separately.  However, git currently does not
> account for such scenario and gets terribly confused over getting
> multiple *SIG statuses.
>
> For example, if a malicious party alters a signed commit and appends
> a new untrusted signature, git is going to ignore the original bad
> signature and report untrusted commit instead.  However, %GK and %GS
> format strings may still expand to the data corresponding
> to the original signature, potentially tricking the scripts into
> trusting the malicious commit.
>
> Given that the use of multiple signatures is quite rare, git does not
> support creating them without jumping through a few hoops, and finally
> supporting them properly would require extensive API improvement, it
> seems reasonable to just reject them at the moment.
> ---

Thanks for the clear analysis and fix.

May we have your sign-off?  See
https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#sign-off
(or the equivalent section of your local copy of
Documentation/SubmittingPatches) for what this means.

>  gpg-interface.c | 38 ++++++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 09ddfbc26..4e03aec15 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -24,21 +24,23 @@ void signature_check_clear(struct signature_check *sigc)
>  static struct {
>  	char result;
>  	const char *check;
> +	int is_status;
>  } sigcheck_gpg_status[] = {
> -	{ 'G', "\n[GNUPG:] GOODSIG " },
> -	{ 'B', "\n[GNUPG:] BADSIG " },
> -	{ 'U', "\n[GNUPG:] TRUST_NEVER" },
> -	{ 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
> -	{ 'E', "\n[GNUPG:] ERRSIG "},
> -	{ 'X', "\n[GNUPG:] EXPSIG "},
> -	{ 'Y', "\n[GNUPG:] EXPKEYSIG "},
> -	{ 'R', "\n[GNUPG:] REVKEYSIG "},
> +	{ 'G', "\n[GNUPG:] GOODSIG ", 1 },
> +	{ 'B', "\n[GNUPG:] BADSIG ", 1 },
> +	{ 'U', "\n[GNUPG:] TRUST_NEVER", 0 },
> +	{ 'U', "\n[GNUPG:] TRUST_UNDEFINED", 0 },
> +	{ 'E', "\n[GNUPG:] ERRSIG ", 1},
> +	{ 'X', "\n[GNUPG:] EXPSIG ", 1},
> +	{ 'Y', "\n[GNUPG:] EXPKEYSIG ", 1},
> +	{ 'R', "\n[GNUPG:] REVKEYSIG ", 1},
>  };

nit: I wonder if making is_status into a flag field (like 'option' in
git.c's cmd_struct) and having an explicit SIGNATURE_STATUS value to
put there would make this easier to read.

It's not clear to me that the name is_status or SIGNATURE_STATUS
captures what this field represents.  Aren't these all sigcheck
statuses?  Can you describe briefly what distinguishes the cases where
this should be 0 versus 1?

>  
>  static void parse_gpg_output(struct signature_check *sigc)
>  {
>  	const char *buf = sigc->gpg_status;
>  	int i;
> +	int had_status = 0;
>  
>  	/* Iterate over all search strings */
>  	for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> @@ -50,6 +52,10 @@ static void parse_gpg_output(struct signature_check *sigc)
>  				continue;
>  			found += strlen(sigcheck_gpg_status[i].check);
>  		}
> +
> +		if (sigcheck_gpg_status[i].is_status)
> +			had_status++;
> +
>  		sigc->result = sigcheck_gpg_status[i].result;
>  		/* The trust messages are not followed by key/signer information */
>  		if (sigc->result != 'U') {
> @@ -62,6 +68,22 @@ static void parse_gpg_output(struct signature_check *sigc)
>  			}
>  		}
>  	}
> +
> +	/*
> +	 * GOODSIG, BADSIG etc. can occur only once for each signature.
> +	 * Therefore, if we had more than one then we're dealing with multiple
> +	 * signatures.  We don't support them currently, and they're rather
> +	 * hard to create, so something is likely fishy and we should reject
> +	 * them altogether.
> +	 */
> +	if (had_status > 1) {
> +		sigc->result = 'E';
> +		/* Clear partial data to avoid confusion */
> +		if (sigc->signer)
> +			FREE_AND_NULL(sigc->signer);
> +		if (sigc->key)
> +			FREE_AND_NULL(sigc->key);
> +	}

Makes sense to me.

>  }
>  
>  int check_signature(const char *payload, size_t plen, const char *signature,
> -- 
> 2.18.0

Can we have a test to make sure this behavior doesn't regress?  See
t/README for an overview of the test framework and "git grep -e gpg t/"
for some examples.

The result looks good.  Thanks again for writing it.

Sincerely,
Jonathan

Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits

[Michał Górny]

[-- Attachment #1: Type: text/plain, Size: 5247 bytes --]

On Wed, 2018-08-15 at 14:31 -0700, Jonathan Nieder wrote:
> Michał Górny wrote:
> 
> > GnuPG supports creating signatures consisting of multiple signature
> > packets.  If such a signature is verified, it outputs all the status
> > messages for each signature separately.  However, git currently does not
> > account for such scenario and gets terribly confused over getting
> > multiple *SIG statuses.
> > 
> > For example, if a malicious party alters a signed commit and appends
> > a new untrusted signature, git is going to ignore the original bad
> > signature and report untrusted commit instead.  However, %GK and %GS
> > format strings may still expand to the data corresponding
> > to the original signature, potentially tricking the scripts into
> > trusting the malicious commit.
> > 
> > Given that the use of multiple signatures is quite rare, git does not
> > support creating them without jumping through a few hoops, and finally
> > supporting them properly would require extensive API improvement, it
> > seems reasonable to just reject them at the moment.
> > ---
> 
> Thanks for the clear analysis and fix.
> 
> May we have your sign-off?  See
> https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#sign-off
> (or the equivalent section of your local copy of
> Documentation/SubmittingPatches) for what this means.

Of course, I'm sorry for missing it in the original submission.

> 
> >  gpg-interface.c | 38 ++++++++++++++++++++++++++++++--------
> >  1 file changed, 30 insertions(+), 8 deletions(-)
> > 
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 09ddfbc26..4e03aec15 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -24,21 +24,23 @@ void signature_check_clear(struct signature_check *sigc)
> >  static struct {
> >  	char result;
> >  	const char *check;
> > +	int is_status;
> >  } sigcheck_gpg_status[] = {
> > -	{ 'G', "\n[GNUPG:] GOODSIG " },
> > -	{ 'B', "\n[GNUPG:] BADSIG " },
> > -	{ 'U', "\n[GNUPG:] TRUST_NEVER" },
> > -	{ 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
> > -	{ 'E', "\n[GNUPG:] ERRSIG "},
> > -	{ 'X', "\n[GNUPG:] EXPSIG "},
> > -	{ 'Y', "\n[GNUPG:] EXPKEYSIG "},
> > -	{ 'R', "\n[GNUPG:] REVKEYSIG "},
> > +	{ 'G', "\n[GNUPG:] GOODSIG ", 1 },
> > +	{ 'B', "\n[GNUPG:] BADSIG ", 1 },
> > +	{ 'U', "\n[GNUPG:] TRUST_NEVER", 0 },
> > +	{ 'U', "\n[GNUPG:] TRUST_UNDEFINED", 0 },
> > +	{ 'E', "\n[GNUPG:] ERRSIG ", 1},
> > +	{ 'X', "\n[GNUPG:] EXPSIG ", 1},
> > +	{ 'Y', "\n[GNUPG:] EXPKEYSIG ", 1},
> > +	{ 'R', "\n[GNUPG:] REVKEYSIG ", 1},
> >  };
> 
> nit: I wonder if making is_status into a flag field (like 'option' in
> git.c's cmd_struct) and having an explicit SIGNATURE_STATUS value to
> put there would make this easier to read.

I think that makes sense.

> 
> It's not clear to me that the name is_status or SIGNATURE_STATUS
> captures what this field represents.  Aren't these all sigcheck
> statuses?  Can you describe briefly what distinguishes the cases where
> this should be 0 versus 1?

Yes, the name really does suck.  Maybe it should be EXCLUSIVE_STATUS
or something like that, to distinguish from things that can occur
simultaneously to them.

> 
> >  
> >  static void parse_gpg_output(struct signature_check *sigc)
> >  {
> >  	const char *buf = sigc->gpg_status;
> >  	int i;
> > +	int had_status = 0;
> >  
> >  	/* Iterate over all search strings */
> >  	for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> > @@ -50,6 +52,10 @@ static void parse_gpg_output(struct signature_check *sigc)
> >  				continue;
> >  			found += strlen(sigcheck_gpg_status[i].check);
> >  		}
> > +
> > +		if (sigcheck_gpg_status[i].is_status)
> > +			had_status++;
> > +
> >  		sigc->result = sigcheck_gpg_status[i].result;
> >  		/* The trust messages are not followed by key/signer information */
> >  		if (sigc->result != 'U') {
> > @@ -62,6 +68,22 @@ static void parse_gpg_output(struct signature_check *sigc)
> >  			}
> >  		}
> >  	}
> > +
> > +	/*
> > +	 * GOODSIG, BADSIG etc. can occur only once for each signature.
> > +	 * Therefore, if we had more than one then we're dealing with multiple
> > +	 * signatures.  We don't support them currently, and they're rather
> > +	 * hard to create, so something is likely fishy and we should reject
> > +	 * them altogether.
> > +	 */
> > +	if (had_status > 1) {
> > +		sigc->result = 'E';
> > +		/* Clear partial data to avoid confusion */
> > +		if (sigc->signer)
> > +			FREE_AND_NULL(sigc->signer);
> > +		if (sigc->key)
> > +			FREE_AND_NULL(sigc->key);
> > +	}
> 
> Makes sense to me.
> 
> >  }
> >  
> >  int check_signature(const char *payload, size_t plen, const char *signature,
> > -- 
> > 2.18.0
> 
> Can we have a test to make sure this behavior doesn't regress?  See
> t/README for an overview of the test framework and "git grep -e gpg t/"
> for some examples.

Will try.  Do I presume correctly that I should include the commit
object with the double signature instead of hacking git to construct it?
;-)

> 
> The result looks good.  Thanks again for writing it.
> 
> Sincerely,
> Jonathan

-- 
Best regards,
Michał Górny

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits

[Jonathan Nieder]

Michał Górny wrote:
> On Wed, 2018-08-15 at 14:31 -0700, Jonathan Nieder wrote:

>> It's not clear to me that the name is_status or SIGNATURE_STATUS
>> captures what this field represents.  Aren't these all sigcheck
>> statuses?  Can you describe briefly what distinguishes the cases where
>> this should be 0 versus 1?
[...]
>                                  Maybe it should be EXCLUSIVE_STATUS
> or something like that, to distinguish from things that can occur
> simultaneously to them.

Thanks.  Makes sense.

[...]
>> Can we have a test to make sure this behavior doesn't regress?  See
>> t/README for an overview of the test framework and "git grep -e gpg t/"
>> for some examples.
>
> Will try.  Do I presume correctly that I should include the commit
> object with the double signature instead of hacking git to construct it?
> ;-)

Good question.  You can hack away with a new program in t/helper/, or
you can make your test do object manipulation with "git cat-file
commit <object>" and "git hash-object -t commit -w --stdin".  If you
run into trouble, just let the list know and I'm happy to try to help.
(Or if you would like real-time help, I'm usually in #git-devel on
freenode.)

Jonathan

Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Michał Górny wrote:
>
>> GnuPG supports creating signatures consisting of multiple signature
>> packets.  If such a signature is verified, it outputs all the status
>> messages for each signature separately.  However, git currently does not
>> account for such scenario and gets terribly confused over getting
>> multiple *SIG statuses.
>>
>> For example, if a malicious party alters a signed commit and appends
>> a new untrusted signature, git is going to ignore the original bad
>> signature and report untrusted commit instead.  However, %GK and %GS
>> format strings may still expand to the data corresponding
>> to the original signature, potentially tricking the scripts into
>> trusting the malicious commit.
>>
>> Given that the use of multiple signatures is quite rare, git does not
>> support creating them without jumping through a few hoops, and finally
>> supporting them properly would require extensive API improvement, it
>> seems reasonable to just reject them at the moment.
>> ---
>
> Thanks for the clear analysis and fix.
>
> May we have your sign-off?  See
> https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#sign-off
> (or the equivalent section of your local copy of
> Documentation/SubmittingPatches) for what this means.

I do not see the original message you are writing response to on
public-inbox archive.  As long as an update with sign-off will be
sent to the git@vger.kernel.org list, that is OK.

>>  gpg-interface.c | 38 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 30 insertions(+), 8 deletions(-)
>>
>> diff --git a/gpg-interface.c b/gpg-interface.c
>> index 09ddfbc26..4e03aec15 100644
>> --- a/gpg-interface.c
>> +++ b/gpg-interface.c
>> @@ -24,21 +24,23 @@ void signature_check_clear(struct signature_check *sigc)
>>  static struct {
>>  	char result;
>>  	const char *check;
>> +	int is_status;
>>  } sigcheck_gpg_status[] = {
>> -	{ 'G', "\n[GNUPG:] GOODSIG " },
>> -	{ 'B', "\n[GNUPG:] BADSIG " },
>> -	{ 'U', "\n[GNUPG:] TRUST_NEVER" },
>> -	{ 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
>> -	{ 'E', "\n[GNUPG:] ERRSIG "},
>> -	{ 'X', "\n[GNUPG:] EXPSIG "},
>> -	{ 'Y', "\n[GNUPG:] EXPKEYSIG "},
>> -	{ 'R', "\n[GNUPG:] REVKEYSIG "},
>> +	{ 'G', "\n[GNUPG:] GOODSIG ", 1 },
>> +	{ 'B', "\n[GNUPG:] BADSIG ", 1 },
>> +	{ 'U', "\n[GNUPG:] TRUST_NEVER", 0 },
>> +	{ 'U', "\n[GNUPG:] TRUST_UNDEFINED", 0 },
>> +	{ 'E', "\n[GNUPG:] ERRSIG ", 1},
>> +	{ 'X', "\n[GNUPG:] EXPSIG ", 1},
>> +	{ 'Y', "\n[GNUPG:] EXPKEYSIG ", 1},
>> +	{ 'R', "\n[GNUPG:] REVKEYSIG ", 1},
>>  };
>
> nit: I wonder if making is_status into a flag field (like 'option' in
> git.c's cmd_struct) and having an explicit SIGNATURE_STATUS value to
> put there would make this easier to read.
>
> It's not clear to me that the name is_status or SIGNATURE_STATUS
> captures what this field represents.  Aren't these all sigcheck
> statuses?  Can you describe briefly what distinguishes the cases where
> this should be 0 versus 1?

Good suggestion.

>>  static void parse_gpg_output(struct signature_check *sigc)
>>  {
>>  	const char *buf = sigc->gpg_status;
>>  	int i;
>> +	int had_status = 0;
>>  
>>  	/* Iterate over all search strings */
>>  	for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
>> @@ -50,6 +52,10 @@ static void parse_gpg_output(struct signature_check *sigc)
>>  				continue;
>>  			found += strlen(sigcheck_gpg_status[i].check);
>>  		}
>> +
>> +		if (sigcheck_gpg_status[i].is_status)
>> +			had_status++;
>> +
>>  		sigc->result = sigcheck_gpg_status[i].result;
>>  		/* The trust messages are not followed by key/signer information */
>>  		if (sigc->result != 'U') {
>> @@ -62,6 +68,22 @@ static void parse_gpg_output(struct signature_check *sigc)
>>  			}
>>  		}
>>  	}
>> +
>> +	/*
>> +	 * GOODSIG, BADSIG etc. can occur only once for each signature.
>> +	 * Therefore, if we had more than one then we're dealing with multiple
>> +	 * signatures.  We don't support them currently, and they're rather
>> +	 * hard to create, so something is likely fishy and we should reject
>> +	 * them altogether.
>> +	 */
>> +	if (had_status > 1) {
>> +		sigc->result = 'E';
>> +		/* Clear partial data to avoid confusion */
>> +		if (sigc->signer)
>> +			FREE_AND_NULL(sigc->signer);
>> +		if (sigc->key)
>> +			FREE_AND_NULL(sigc->key);
>> +	}
>
> Makes sense to me.

I was wondering if we have to revamp the loop altogether.  The
current code runs through the list of all the possible "status"
lines, and find the first occurrence for each type in the buffer
that has GPG output.  Second and subsequent occurrence of the same
type, if existed, will not be noticed by the original loop
structure, and this patch does not change it, even though the topic
of the patch is about rejecting the signature block with elements
taken from multiple signatures.  One way to fix it may be to keep
the current loop structure to go over the sigcheck_gpg_status[],
but make the logic inside the loop into an inner loop that finds all
occurrences of the same type, instead of stopping after finding the
first instance.  But once we go to that length, I suspect that it
may be cleaner to iterate over the lines in the buffer, checking
each line if it matches one of the recognized "[GNUPG:] FOOSIG"
lines and acting on it (while ignoring unrecognized lines).

>>  }
>>  
>>  int check_signature(const char *payload, size_t plen, const char *signature,
>> -- 
>> 2.18.0
>
> Can we have a test to make sure this behavior doesn't regress?  See
> t/README for an overview of the test framework and "git grep -e gpg t/"
> for some examples.
>
> The result looks good.  Thanks again for writing it.
>
> Sincerely,
> Jonathan