git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 2/6] commit: ignore additional signatures when parsing signed commits

From
brian m. carlson <sandals@crustytoothpaste.net>
Date
Jan 18, 2021, 23:49 UTC
Message-ID
<20210118234915.2036197-3-sandals@crustytoothpaste.net>
In-Reply-To
<20210118234915.2036197-1-sandals@crustytoothpaste.net>

When we create a commit with multiple signatures, neither of these signatures includes the other. Consequently, when we produce the payload which has been signed so we can verify the commit, we must strip off any other signatures, or the payload will differ from what was signed. Do so, and in preparation for verifying with multiple algorithms, pass the algorithm we want to verify into parse_signed_commit.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 commit.c                 | 54 +++++++++++++++++++++++++---------------
 commit.h                 |  3 ++-
 log-tree.c               |  2 +-
 t/t7510-signed-commit.sh | 43 +++++++++++++++++++++++++++++++-
 4 files changed, 79 insertions(+), 23 deletions(-)
diff --git a/commit.c b/commit.c
index bab8d5ab07..1006c85ca8 100644
--- a/commit.c
+++ b/commit.c
@@ -1036,20 +1036,18 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)
 }
 
 int parse_signed_commit(const struct commit *commit,
-			struct strbuf *payload, struct strbuf *signature)
+			struct strbuf *payload, struct strbuf *signature,
+			const struct git_hash_algo *algop)
 {
 
 	unsigned long size;
 	const char *buffer = get_commit_buffer(commit, &size);
-	int in_signature, saw_signature = -1;
-	const char *line, *tail;
-	const char *gpg_sig_header = gpg_sig_headers[hash_algo_by_ptr(the_hash_algo)];
-	int gpg_sig_header_len = strlen(gpg_sig_header);
+	int in_signature = 0, saw_signature = 0, other_signature = 0;
+	const char *line, *tail, *p;
+	const char *gpg_sig_header = gpg_sig_headers[hash_algo_by_ptr(algop)];
 
 	line = buffer;
 	tail = buffer + size;
-	in_signature = 0;
-	saw_signature = 0;
 	while (line < tail) {
 		const char *sig = NULL;
 		const char *next = memchr(line, '\n', tail - line);
@@ -1057,9 +1055,15 @@ int parse_signed_commit(const struct commit *commit,
 		next = next ? next + 1 : tail;
 		if (in_signature && line[0] == ' ')
 			sig = line + 1;
-		else if (starts_with(line, gpg_sig_header) &&
-			 line[gpg_sig_header_len] == ' ')
-			sig = line + gpg_sig_header_len + 1;
+		else if (skip_prefix(line, gpg_sig_header, &p) &&
+			 *p == ' ') {
+			sig = line + strlen(gpg_sig_header) + 1;
+			other_signature = 0;
+		}
+		else if (starts_with(line, "gpgsig"))
+			other_signature = 1;
+		else if (other_signature && line[0] != ' ')
+			other_signature = 0;
 		if (sig) {
 			strbuf_add(signature, sig, next - sig);
 			saw_signature = 1;
@@ -1068,7 +1072,8 @@ int parse_signed_commit(const struct commit *commit,
 			if (*line == '\n')
 				/* dump the whole remainder of the buffer */
 				next = tail;
-			strbuf_add(payload, line, next - line);
+			if (!other_signature)
+				strbuf_add(payload, line, next - line);
 			in_signature = 0;
 		}
 		line = next;
@@ -1082,23 +1087,29 @@ int remove_signature(struct strbuf *buf)
 	const char *line = buf->buf;
 	const char *tail = buf->buf + buf->len;
 	int in_signature = 0;
-	const char *sig_start = NULL;
-	const char *sig_end = NULL;
+	struct sigbuf {
+		const char *start;
+		const char *end;
+	} sigs[2], *sigp = &sigs[0];
+	int i;
+	const char *orig_buf = buf->buf;
+
+	memset(sigs, 0, sizeof(sigs));
 
 	while (line < tail) {
 		const char *next = memchr(line, '\n', tail - line);
 		next = next ? next + 1 : tail;
 
 		if (in_signature && line[0] == ' ')
-			sig_end = next;
+			sigp->end = next;
 		else if (starts_with(line, "gpgsig")) {
 			int i;
 			for (i = 1; i < GIT_HASH_NALGOS; i++) {
 				const char *p;
 				if (skip_prefix(line, gpg_sig_headers[i], &p) &&
 				    *p == ' ') {
-					sig_start = line;
-					sig_end = next;
+					sigp->start = line;
+					sigp->end = next;
 					in_signature = 1;
 				}
 			}
@@ -1106,15 +1117,18 @@ int remove_signature(struct strbuf *buf)
 			if (*line == '\n')
 				/* dump the whole remainder of the buffer */
 				next = tail;
+			if (in_signature && sigp - sigs != ARRAY_SIZE(sigs))
+				sigp++;
 			in_signature = 0;
 		}
 		line = next;
 	}
 
-	if (sig_start)
-		strbuf_remove(buf, sig_start - buf->buf, sig_end - sig_start);
+	for (i = ARRAY_SIZE(sigs) - 1; i >= 0; i--)
+		if (sigs[i].start)
+			strbuf_remove(buf, sigs[i].start - orig_buf, sigs[i].end - sigs[i].start);
 
-	return sig_start != NULL;
+	return sigs[0].start != NULL;
 }
 
 static void handle_signed_tag(struct commit *parent, struct commit_extra_header ***tail)
@@ -1165,7 +1179,7 @@ int check_commit_signature(const struct commit *commit, struct signature_check *
 
 	sigc->result = 'N';
 
-	if (parse_signed_commit(commit, &payload, &signature) <= 0)
+	if (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0)
 		goto out;
 	ret = check_signature(payload.buf, payload.len, signature.buf,
 		signature.len, sigc);
diff --git a/commit.h b/commit.h
index f4e7b0158e..030aa65ab8 100644
--- a/commit.h
+++ b/commit.h
@@ -317,7 +317,8 @@ void set_merge_remote_desc(struct commit *commit,
 struct commit *get_merge_parent(const char *name);
 
 int parse_signed_commit(const struct commit *commit,
-			struct strbuf *message, struct strbuf *signature);
+			struct strbuf *message, struct strbuf *signature,
+			const struct git_hash_algo *algop);
 int remove_signature(struct strbuf *buf);
 
 /*
diff --git a/log-tree.c b/log-tree.c
index fd0dde97ec..7e0335e548 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -502,7 +502,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)
 	struct signature_check sigc = { 0 };
 	int status;
 
-	if (parse_signed_commit(commit, &payload, &signature) <= 0)
+	if (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0)
 		goto out;
 
 	status = check_signature(payload.buf, payload.len, signature.buf,
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 6baaa1ad91..d78319d5c8 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -172,7 +172,7 @@ test_expect_success GPG 'show signed commit with signature' '
 	git cat-file commit initial >cat &&
 	grep -v -e "gpg: " -e "Warning: " show >show.commit &&
 	grep -e "gpg: " -e "Warning: " show >show.gpg &&
-	grep -v "^ " cat | grep -v "^$(test_oid header) " >cat.commit &&
+	grep -v "^ " cat | grep -v "^gpgsig.* " >cat.commit &&
 	test_cmp show.commit commit &&
 	test_cmp show.gpg verify.2 &&
 	test_cmp cat.commit verify.1
@@ -334,4 +334,45 @@ test_expect_success GPG 'show double signature with custom format' '
 	test_cmp expect actual
 '
 
+
+test_expect_success GPG 'verify-commit verifies multiply signed commits' '
+	git init multiply-signed &&
+	cd multiply-signed &&
+	test_commit first &&
+	echo 1 >second &&
+	git add second &&
+	tree=$(git write-tree) &&
+	parent=$(git rev-parse HEAD^{commit}) &&
+	git commit --gpg-sign -m second &&
+	git cat-file commit HEAD &&
+	# Avoid trailing whitespace.
+	sed -e "s/^Q//" -e "s/^Z/ /" >commit <<-EOF &&
+	Qtree $tree
+	Qparent $parent
+	Qauthor A U Thor <author@example.com> 1112912653 -0700
+	Qcommitter C O Mitter <committer@example.com> 1112912653 -0700
+	Qgpgsig -----BEGIN PGP SIGNATURE-----
+	QZ
+	Q iHQEABECADQWIQRz11h0S+chaY7FTocTtvUezd5DDQUCX/uBDRYcY29tbWl0dGVy
+	Q QGV4YW1wbGUuY29tAAoJEBO29R7N3kMNd+8AoK1I8mhLHviPH+q2I5fIVgPsEtYC
+	Q AKCTqBh+VabJceXcGIZuF0Ry+udbBQ==
+	Q =tQ0N
+	Q -----END PGP SIGNATURE-----
+	Qgpgsig-sha256 -----BEGIN PGP SIGNATURE-----
+	QZ
+	Q iHQEABECADQWIQRz11h0S+chaY7FTocTtvUezd5DDQUCX/uBIBYcY29tbWl0dGVy
+	Q QGV4YW1wbGUuY29tAAoJEBO29R7N3kMN/NEAn0XO9RYSBj2dFyozi0JKSbssYMtO
+	Q AJwKCQ1BQOtuwz//IjU8TiS+6S4iUw==
+	Q =pIwP
+	Q -----END PGP SIGNATURE-----
+	Q
+	Qsecond
+	EOF
+	head=$(git hash-object -t commit -w commit) &&
+	git reset --hard $head &&
+	git verify-commit $head 2>actual &&
+	grep "Good signature from" actual &&
+	! grep "BAD signature from" actual
+'
+
 test_done
Previous: brian m. carlsonNext: brian m. carlson
Message 27 of 39 in “Support for commits signed by multiple algorithms”
  1. 0/5 Support for commits signed by multiple algorithmsbrian m. carlson, Jan 11, 2021
  2. 1/5 commit: ignore additional signatures when parsing signed commitsbrian m. carlson, Jan 11, 2021
  3. 3/5 commit: allow parsing arbitrary buffers with headersbrian m. carlson, Jan 11, 2021
  4. 2/5 gpg-interface: improve interface for parsing tagsbrian m. carlson, Jan 11, 2021
  5. Junio C HamanoJan 12, 2021
  6. brian m. carlsonJan 14, 2021
  7. Junio C HamanoJan 15, 2021
  8. 4/5 ref-filter: hoist signature parsingbrian m. carlson, Jan 11, 2021
  9. 5/6 fixup! commit: ignore additional signatures when parsing signed commitsbrian m. carlson, Jan 11, 2021
  10. brian m. carlsonJan 11, 2021
  11. 5/5 gpg-interface: remove other signature headers before verifyingbrian m. carlson, Jan 11, 2021
  12. 6/6 gpg-interface: remove other signature headers before verifyingbrian m. carlson, Jan 11, 2021
  13. 0/5 Support for commits signed by multiple algorithmsbrian m. carlson, Jan 11, 2021
  14. 3/5 commit: allow parsing arbitrary buffers with headersbrian m. carlson, Jan 11, 2021
  15. 4/5 ref-filter: hoist signature parsingbrian m. carlson, Jan 11, 2021
  16. 2/5 gpg-interface: improve interface for parsing tagsbrian m. carlson, Jan 11, 2021
  17. Junio C HamanoJan 12, 2021
  18. 5/5 gpg-interface: remove other signature headers before verifyingbrian m. carlson, Jan 11, 2021
  19. 1/5 commit: ignore additional signatures when parsing signed commitsbrian m. carlson, Jan 11, 2021
  20. SZEDER GáborJan 12, 2021
  21. Junio C HamanoJan 11, 2021
  22. brian m. carlsonJan 11, 2021
  23. Junio C HamanoJan 12, 2021
  24. brian m. carlsonJan 12, 2021
  25. 0/6 Support for commits signed by multiple algorithmsbrian m. carlson, Jan 18, 2021
  26. 1/6 ref-filter: switch some uses of unsigned long to size_tbrian m. carlson, Jan 18, 2021
  27. 2/6 commit: ignore additional signatures when parsing signed commitsbrian m. carlson, Jan 18, 2021
  28. 4/6 commit: allow parsing arbitrary buffers with headersbrian m. carlson, Jan 18, 2021
  29. 3/6 gpg-interface: improve interface for parsing tagsbrian m. carlson, Jan 18, 2021
  30. 6/6 gpg-interface: remove other signature headers before verifyingbrian m. carlson, Jan 18, 2021
  31. 5/6 ref-filter: hoist signature parsingbrian m. carlson, Jan 18, 2021
  32. 0/6 Support for commits signed by multiple algorithmsbrian m. carlson, Feb 11, 2021
  33. 4/6 commit: allow parsing arbitrary buffers with headersbrian m. carlson, Feb 11, 2021
  34. 1/6 ref-filter: switch some uses of unsigned long to size_tbrian m. carlson, Feb 11, 2021
  35. 2/6 commit: ignore additional signatures when parsing signed commitsbrian m. carlson, Feb 11, 2021
  36. 3/6 gpg-interface: improve interface for parsing tagsbrian m. carlson, Feb 11, 2021
  37. 5/6 ref-filter: hoist signature parsingbrian m. carlson, Feb 11, 2021
  38. 6/6 gpg-interface: remove other signature headers before verifyingbrian m. carlson, Feb 11, 2021
  39. Junio C HamanoFeb 11, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.