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.

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?

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

On Wed, 2018-08-15 at 14:31 -0700, Jonathan Nieder wrote:

Michał Górny wrote:

quoted

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.

quoted

 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.

quoted

 
 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.

quoted

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

Will try.  Do I presume correctly that I should include the commit
object with the double signature instead of hacking git to construct it?
;-)

The result looks good.  Thanks again for writing it.

Sincerely,
Jonathan

-- 
Best regards,
Michał Górny

Michał Górny wrote:

On Wed, 2018-08-15 at 14:31 -0700, Jonathan Nieder wrote:

quoted

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.

[...]

quoted

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 <object>" 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