Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits
Jonathan Nieder writes:
> 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.
I do not see the original message you are writing response to on
public-inbox archive. As long as an update with sign-off will be
sent to the git@vger.kernel.org list, that is OK.
>> 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?
Good suggestion.
>> 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.
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
Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits
Michał Górny wrote: > On Wed, 2018-08-15 at 14:31 -0700, Jonathan Nieder wrote: >> 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? [...] > Maybe it should be EXCLUSIVE_STATUS > or something like that, to distinguish from things that can occur > simultaneously to them. Thanks. Makes sense. [...] >> 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? > ;-) Good question. You can hack away with a new program in t/helper/, or you can make your test do object manipulation with "git cat-file commit " and "git hash-object -t commit -w --stdin". If you run into trouble, just let the list know and I'm happy to try to help. (Or if you would like real-time help, I'm usually in #git-devel on freenode.) Jonathan
Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits
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
Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits
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.
> 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?
>
> 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.
The result looks good. Thanks again for writing it.
Sincerely,
Jonathan
