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

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

[Michał Górny]

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 <mgorny@gentoo.org>
---
 gpg-interface.c          | 41 ++++++++++++++++++++++++++++++++--------
 t/t7510-signed-commit.sh | 26 +++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 8 deletions(-)

Changes in v2:
* used generic 'flags' instead of boolean field,
* added test case for git-verify-commit & git-show.

diff --git a/gpg-interface.c b/gpg-interface.c
index 09ddfbc26..35c25106a 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -21,24 +21,29 @@ 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', "\n[GNUPG:] GOODSIG ", GPG_STATUS_EXCLUSIVE },
+	{ 'B', "\n[GNUPG:] BADSIG ", GPG_STATUS_EXCLUSIVE },
+	{ 'U', "\n[GNUPG:] TRUST_NEVER", 0 },
+	{ 'U', "\n[GNUPG:] TRUST_UNDEFINED", 0 },
+	{ 'E', "\n[GNUPG:] ERRSIG ", GPG_STATUS_EXCLUSIVE },
+	{ 'X', "\n[GNUPG:] EXPSIG ", GPG_STATUS_EXCLUSIVE },
+	{ 'Y', "\n[GNUPG:] EXPKEYSIG ", GPG_STATUS_EXCLUSIVE },
+	{ 'R', "\n[GNUPG:] REVKEYSIG ", GPG_STATUS_EXCLUSIVE },
 };
 
 static void parse_gpg_output(struct signature_check *sigc)
 {
 	const char *buf = sigc->gpg_status;
 	int i;
+	int had_exclusive_status = 0;
 
 	/* Iterate over all search strings */
 	for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
@@ -50,6 +55,10 @@ static void parse_gpg_output(struct signature_check *sigc)
 				continue;
 			found += strlen(sigcheck_gpg_status[i].check);
 		}
+
+		if (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE)
+			had_exclusive_status++;
+
 		sigc->result = sigcheck_gpg_status[i].result;
 		/* The trust messages are not followed by key/signer information */
 		if (sigc->result != 'U') {
@@ -62,6 +71,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_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);
+	}
 }
 
 int check_signature(const char *payload, size_t plen, const char *signature,
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 6e2015ed9..51fb92a72 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -227,4 +227,30 @@ test_expect_success GPG 'log.showsignature behaves like --show-signature' '
 	grep "gpg: Good signature" actual
 '
 
+test_expect_success GPG 'detect fudged commit with double signature' '
+	sed -e "/gpgsig/,/END PGP/d" forged1 >double-base &&
+	sed -n -e "/gpgsig/,/END PGP/p" forged1 | \
+		sed -e "s/^gpgsig//;s/^ //" | gpg --dearmor >double-sig1.sig &&
+	gpg -o double-sig2.sig -u 29472784 --detach-sign double-base &&
+	cat double-sig1.sig double-sig2.sig | gpg --enarmor >double-combined.asc &&
+	sed -e "s/^\(-.*\)ARMORED FILE/\1SIGNATURE/;1s/^/gpgsig /;2,\$s/^/ /" \
+		double-combined.asc > double-gpgsig &&
+	sed -e "/committer/r double-gpgsig" double-base >double-commit &&
+	git hash-object -w -t commit double-commit >double-commit.commit &&
+	test_must_fail git verify-commit $(cat double-commit.commit) &&
+	git show --pretty=short --show-signature $(cat double-commit.commit) >double-actual &&
+	grep "BAD signature from" double-actual &&
+	grep "Good signature from" double-actual
+'
+
+test_expect_success GPG 'show double signature with custom format' '
+	cat >expect <<-\EOF &&
+	E
+
+
+	EOF
+	git log -1 --format="%G?%n%GK%n%GS" $(cat double-commit.commit) >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.18.0

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

[Michał Górny]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

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.

-- 
Best regards,
Michał Górny

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

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

[Stefan Beller]

On Wed, Oct 3, 2018 at 1:29 AM Michał Górny <mgorny@gentoo.org> wrote:
>
> 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 am not an expert on GPG, but the patch (design, code, test) looks
reasonable to me.

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

[Tacitus Aedifex]

I think that there is a more simple way to catch multiple signatures see below.  
Other than that, I like this patch.

Signed-off-by: Tacitus Aedifex <aedifex@sdf.org>
---
 gpg-interface.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/gpg-interface.c b/gpg-interface.c
index db17d65f8..a4dba3361 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -93,6 +93,7 @@ static void parse_gpg_output(struct signature_check *sigc)
 {
 	const char *buf = sigc->gpg_status;
 	int i;
+	int multi_sig = 0;
 
 	/* Iterate over all search strings */
 	for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
@@ -115,6 +116,23 @@ static void parse_gpg_output(struct signature_check *sigc)
 				next = strchrnul(found, '\n');
 				sigc->signer = xmemdupz(found, next - found);
 			}
+		} else 
+			multi_sig++;
+
+		/*
+		 * GOODSIG, BADSIG, etc. can occure 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 are
+		 * rather hard to create, so something is likely probably not right
+		 * and we should reject them altogether.
+		 */
+		if (multi_sig > 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);
 		}
 	}
 }
--
2.18.0.129.ge333175
-- 

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

[Junio C Hamano]

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.