On Mon, 2018-10-15 at 12:31 +0900, Junio C Hamano wrote: > Michał Górny <[email protected]> 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 <[email protected]> > > --- > > 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
signature.asc
Description: This is a digitally signed message part
