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

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

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

On Mon, 2018-10-15 at 12:31 +0900, Junio C Hamano wrote:
> Michał Górny <mgorny@gentoo.org> writes:
> 
> > 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.
> > 
> > Signed-off-by: Michał Górny <mgorny@gentoo.org>
> > ---
> >  gpg-interface.c          | 94 +++++++++++++++++++++++++++-------------
> >  t/t7510-signed-commit.sh | 26 +++++++++++
> >  2 files changed, 91 insertions(+), 29 deletions(-)
> > 
> > Changes in v3: reworked the whole loop to iterate over lines rather
> > than scanning the whole buffer, as requested.  Now it also catches
> > duplicate instances of the same status.
> > 
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index db17d65f8..480aab4ee 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -75,48 +75,84 @@ void signature_check_clear(struct signature_check *sigc)
> >  	FREE_AND_NULL(sigc->key);
> >  }
> >  
> > +/* An exclusive status -- only one of them can appear in output */
> > +#define GPG_STATUS_EXCLUSIVE	(1<<0)
> > +
> >  static struct {
> >  	char result;
> >  	const char *check;
> > +	unsigned int flags;
> >  } 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', "GOODSIG ", GPG_STATUS_EXCLUSIVE },
> > +	{ 'B', "BADSIG ", GPG_STATUS_EXCLUSIVE },
> > +	{ 'U', "TRUST_NEVER", 0 },
> > +	{ 'U', "TRUST_UNDEFINED", 0 },
> > +	{ 'E', "ERRSIG ", GPG_STATUS_EXCLUSIVE },
> > +	{ 'X', "EXPSIG ", GPG_STATUS_EXCLUSIVE },
> > +	{ 'Y', "EXPKEYSIG ", GPG_STATUS_EXCLUSIVE },
> > +	{ 'R', "REVKEYSIG ", GPG_STATUS_EXCLUSIVE },
> >  };
> >  
> >  static void parse_gpg_output(struct signature_check *sigc)
> >  {
> >  	const char *buf = sigc->gpg_status;
> > +	const char *line, *next;
> >  	int i;
> > -
> > -	/* Iterate over all search strings */
> > -	for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> > -		const char *found, *next;
> > -
> > -		if (!skip_prefix(buf, sigcheck_gpg_status[i].check + 1, &found)) {
> > -			found = strstr(buf, sigcheck_gpg_status[i].check);
> > -			if (!found)
> > -				continue;
> > -			found += strlen(sigcheck_gpg_status[i].check);
> > -		}
> > -		sigc->result = sigcheck_gpg_status[i].result;
> > -		/* The trust messages are not followed by key/signer information */
> > -		if (sigc->result != 'U') {
> > -			next = strchrnul(found, ' ');
> > -			sigc->key = xmemdupz(found, next - found);
> > -			/* The ERRSIG message is not followed by signer information */
> > -			if (*next && sigc-> result != 'E') {
> > -				found = next + 1;
> > -				next = strchrnul(found, '\n');
> > -				sigc->signer = xmemdupz(found, next - found);
> > +	int had_exclusive_status = 0;
> > +
> > +	/* Iterate over all lines */
> > +	for (line = buf; *line; line = strchrnul(line+1, '\n')) {
> > +		while (*line == '\n')
> > +			line++;
> > +		/* Skip lines that don't start with GNUPG status */
> > +		if (strncmp(line, "[GNUPG:] ", 9))
> > +			continue;
> > +		line += 9;
> 
> You do not want to count to 9 yourself.  Instead
> 
> 	if (!skip_prefix(line, "[GNUPG:] ", &line))
> 		continue;
> 
> 
> > +		/* Iterate over all search strings */
> > +		for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> > +			if (!strncmp(line, sigcheck_gpg_status[i].check,
> > +					strlen(sigcheck_gpg_status[i].check))) {
> > +				line += strlen(sigcheck_gpg_status[i].check);
> 
> Likewise.

Both done.

> 
> > +				if (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE)
> > +					had_exclusive_status++;
> 
> "has" is fine, but I think existing code elsewhere we use "seen" for
> things like this.
> 
> > +				sigc->result = sigcheck_gpg_status[i].result;
> > +				/* The trust messages are not followed by key/signer information */
> > +				if (sigc->result != 'U') {
> > +					next = strchrnul(line, ' ');
> > +					free(sigc->key);
> > +					sigc->key = xmemdupz(line, next - line);
> > +					/* The ERRSIG message is not followed by signer information */
> > +					if (*next && sigc->result != 'E') {
> > +						line = next + 1;
> > +						next = strchrnul(line, '\n');
> > +						free(sigc->signer);
> > +						sigc->signer = xmemdupz(line, next - line);
> > +					}
> > +				}
> > +				break;
> >  			}
> >  		}
> >  	}
> 
> So unless U/E, we expect to see a key, and unless E, we also expect
> there is a signer; we keep the last value we see in the sequence in
> sigc.  Because all of these that are not U are marked exclusive, if
> we check if sigc->key already has value at the point you free the
> sigc->key field above, we can see if there is a duplicate record
> that are of "exclusive" type?  I am not suggesting to lose the
> addition of "flags = GPG_STATUS_EXCLUSIVE|0" field, but trying to
> see if I am getting the logic right.
> 
> For gpg_status that is !GPG_STATUS_EXCLUSIVE (i.e. "U"), we do not
> do any replacement of already seen .key/.signer, and all the cases
> that we do the replacement are GPG_STATUS_EXCLUSIVE, which we know
> will become an error in the code below when we do see twice.  So it
> is fine not to check if .key/.signer we see twice are the same or
> different.  It is an error even if we see the same .key/.signer
> twice---having two records is already wrong no matter whose key/sign
> it is.
> 
> OK, so the whole thing makes sense to me.
> 
> Having said that, if we wanted to short-circuit, I think
> 
>                 for (each line) {
>                         for (each sigcheck_gpg_status[]) {
>                                 if (not the one on line)
>                                         continue;
>                                 if (sigc->result != 'U') {
>                                         if (sigc->key)
>                                                 goto found_dup;
>                                         sigc->key = make a copy;
>                                         if (*next && sigc->result != 'E') {
>                                                 if (sigc->signer)
>                                                         goto found_dup;
>                                                 sigc->signer = make a copy;
>                                         }
>                                 }
>                                 break;
>                         }
>                 }
>                 return;
> 
>         found_dup:
>                 sigc->result = 'E';
>                 FREE_AND_NULL(sigc->signer);
>                 FREE_AND_NULL(sigc->key);
>                 return;
> 		
> would also be fine.

Do I understand correctly that you mean to take advantage that 'seen
exclusive status' cases match 'seen key' cases?  I think this would be
a little less readable.

That said, I was planning on next patch that replaced the "!= 'U'" test
with explicit flags for whether a particular status includes key
and UID.  If you'd agree with this direction, I think having this one
separate as well would make sense.

> > +
> > +	/*
> > +	 * 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_exclusive_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);
> 
> I think it is OK to use FREE_AND_NULL() unconditionally (just like
> we can use free(x) on x==NULL).

Done as well.

> 
> > +	}
> >  }
> 
> 

-- 
Best regards,
Michał Górny

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