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

[Junio C Hamano <gitster@pobox.com>]

Michał Górny <mgorny@gentoo.org> writes:

> On Fri, 2018-08-17 at 09:34 +0200, 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.
>> 
>
> Gentle ping.

I think among the three issues raised in the review of v1 [*1*], one
of them remain unaddressed.  Other than that the addition relative
to v2 looks reasonable (but I only skimmed the patch).

[Reference] *1* https://public-inbox.org/git/xmqq1saxc5gu.fsf@gitster-ct.c.googlers.com/ 

Relevant part reproduced here.

>>>  	/* 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 (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.

Which still smells to me that it points out a grave (made grave by
what the patch claims to address) issue in the implementation of v1;
did v2 get substantially updated to address the concern?

> 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).


P.S. I'd be either offline or otherwise occupied until the next
week, so there is no need to hastily prepare an updated patch
series.

Thanks.