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

["Michał Górny" <mgorny@gentoo.org>]

[-- 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 --]