Jonathan NiederMichał Górny wrote:
Show 18 quoted lines
> 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.
Show 30 quoted lines
> 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?
Show 40 quoted lines
>
> 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.
Show 5 quoted 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
