git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH 0/5] Support for commits signed by multiple algorithms
@ 2021-01-11  0:37 brian m. carlson
  2021-01-11  0:37 ` [PATCH 1/5] commit: ignore additional signatures when parsing signed commits brian m. carlson
                   ` (7 more replies)
  0 siblings, 8 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  0:37 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

This series introduces support for verifying commits and tags signed by
multiple algorithms.

Originally, we had planned for SHA-256 tags to stuff the signature in a
header instead of using a trailing signature, and a patch to do that was
sent out in part 1/3.  Unfortunately, for whatever reason, that patch
didn't make it into the master branch, and so we use trailing signatures
there.

We can't change this now, because otherwise it would be ambiguous
whether the trailing signature on a SHA-256 object was for the SHA-256
contents or whether the contents were a rewritten SHA-1 object with no
SHA-256 signature at all.  To do the next best thing, let's use the
trailing signature for the preferred hash algorithm and use a header for
the other variant.  This permits round-tripping, but has the downside
that tags signed with multiple algorithms can't be verified with older
versions of Git.  However, signatures created with older versions of Git
continue to be accepted.

For commits, let's accept a commit that has two signatures.  We
previously created the commits correctly but didn't strip the extra
header off when verifying, so our verification indicated the signature
was bad.

Both these situations allow for signing commits and tags that can be
round-tripped through both SHA-1 and SHA-256.  We verify only the
signature using the current hash algorithm, since we currently don't
rewrite objects.

brian m. carlson (5):
  commit: ignore additional signatures when parsing signed commits
  gpg-interface: improve interface for parsing tags
  commit: allow parsing arbitrary buffers with headers
  ref-filter: hoist signature parsing
  gpg-interface: remove other signature headers before verifying

 builtin/receive-pack.c   |  4 +-
 builtin/tag.c            | 16 ++++++--
 commit.c                 | 82 +++++++++++++++++++++++++++-------------
 commit.h                 | 12 +++++-
 fmt-merge-msg.c          |  8 ++--
 gpg-interface.c          | 15 +++++++-
 gpg-interface.h          |  9 ++++-
 log-tree.c               | 15 ++++----
 ref-filter.c             | 23 +++++++----
 t/t7004-tag.sh           | 25 ++++++++++++
 t/t7510-signed-commit.sh | 43 ++++++++++++++++++++-
 tag.c                    | 15 ++++----
 12 files changed, 206 insertions(+), 61 deletions(-)


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 1/5] commit: ignore additional signatures when parsing signed commits
  2021-01-11  0:37 [PATCH 0/5] Support for commits signed by multiple algorithms brian m. carlson
@ 2021-01-11  0:37 ` brian m. carlson
  2021-01-11  0:37 ` [PATCH 2/5] gpg-interface: improve interface for parsing tags brian m. carlson
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  0:37 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

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                 | 52 ++++++++++++++++++++++++----------------
 commit.h                 |  3 ++-
 log-tree.c               |  2 +-
 t/t7510-signed-commit.sh | 43 ++++++++++++++++++++++++++++++++-
 4 files changed, 77 insertions(+), 23 deletions(-)

diff --git a/commit.c b/commit.c
index f128f18a9b..93faaad764 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,27 @@ 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] = { 0 }, *sigp = &sigs[0];
+	int i;
+	const char *orig_buf = buf->buf;
 
 	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 +1115,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 +1177,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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 2/5] gpg-interface: improve interface for parsing tags
  2021-01-11  0:37 [PATCH 0/5] Support for commits signed by multiple algorithms brian m. carlson
  2021-01-11  0:37 ` [PATCH 1/5] commit: ignore additional signatures when parsing signed commits brian m. carlson
@ 2021-01-11  0:37 ` brian m. carlson
  2021-01-12  4:58   ` Junio C Hamano
  2021-01-11  0:37 ` [PATCH 3/5] commit: allow parsing arbitrary buffers with headers brian m. carlson
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  0:37 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

We have a function which parses a buffer with a signature at the end,
parse_signature, and this function is used for signed tags.  However,
the transition plan has SHA-256 tags using a header, which is a
materially different syntax.  The current interface is not suitable for
parsing such tags.

Adjust the parse_signature interface to store the parsed data in two
strbufs and turn the existing function into parse_signed_buffer.  The
latter is still used in places where we want to strip off the signature
in a SHA-1 tag or in places where we know we always have a signed
buffer, such as push certs.

Adjust all the callers to deal with this new interface.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/receive-pack.c |  4 ++--
 builtin/tag.c          | 16 ++++++++++++----
 commit.c               |  9 ++++++---
 fmt-merge-msg.c        |  8 +++++---
 gpg-interface.c        | 13 ++++++++++++-
 gpg-interface.h        |  9 ++++++++-
 log-tree.c             | 13 +++++++------
 ref-filter.c           | 18 ++++++++++++++----
 tag.c                  | 15 ++++++++-------
 9 files changed, 74 insertions(+), 31 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d49d050e6e..b89ce31bf2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -764,7 +764,7 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 
 		memset(&sigcheck, '\0', sizeof(sigcheck));
 
-		bogs = parse_signature(push_cert.buf, push_cert.len);
+		bogs = parse_signed_buffer(push_cert.buf, push_cert.len);
 		check_signature(push_cert.buf, bogs, push_cert.buf + bogs,
 				push_cert.len - bogs, &sigcheck);
 
@@ -2050,7 +2050,7 @@ static void queue_commands_from_cert(struct command **tail,
 		die("malformed push certificate %.*s", 100, push_cert->buf);
 	else
 		boc += 2;
-	eoc = push_cert->buf + parse_signature(push_cert->buf, push_cert->len);
+	eoc = push_cert->buf + parse_signed_buffer(push_cert->buf, push_cert->len);
 
 	while (boc < eoc) {
 		const char *eol = memchr(boc, '\n', eoc - boc);
diff --git a/builtin/tag.c b/builtin/tag.c
index ecf011776d..7162f4ccc5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -174,11 +174,17 @@ static void write_tag_body(int fd, const struct object_id *oid)
 {
 	unsigned long size;
 	enum object_type type;
-	char *buf, *sp;
+	char *buf, *sp, *orig;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 
-	buf = read_object_file(oid, &type, &size);
+	orig = buf = read_object_file(oid, &type, &size);
 	if (!buf)
 		return;
+	if (parse_signature(buf, size, &payload, &signature)) {
+		buf = payload.buf;
+		size = payload.len;
+	}
 	/* skip header */
 	sp = strstr(buf, "\n\n");
 
@@ -187,9 +193,11 @@ static void write_tag_body(int fd, const struct object_id *oid)
 		return;
 	}
 	sp += 2; /* skip the 2 LFs */
-	write_or_die(fd, sp, parse_signature(sp, buf + size - sp));
+	write_or_die(fd, sp, buf + size - sp);
 
-	free(buf);
+	free(orig);
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 }
 
 static int build_tag_object(struct strbuf *buf, int sign, struct object_id *result)
diff --git a/commit.c b/commit.c
index 93faaad764..794dc8b593 100644
--- a/commit.c
+++ b/commit.c
@@ -1134,8 +1134,10 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 	struct merge_remote_desc *desc;
 	struct commit_extra_header *mergetag;
 	char *buf;
-	unsigned long size, len;
+	unsigned long size;
 	enum object_type type;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 
 	desc = merge_remote_util(parent);
 	if (!desc || !desc->obj)
@@ -1143,8 +1145,7 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 	buf = read_object_file(&desc->obj->oid, &type, &size);
 	if (!buf || type != OBJ_TAG)
 		goto free_return;
-	len = parse_signature(buf, size);
-	if (size == len)
+	if (!parse_signature(buf, size, &payload, &signature))
 		goto free_return;
 	/*
 	 * We could verify this signature and either omit the tag when
@@ -1163,6 +1164,8 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 
 	**tail = mergetag;
 	*tail = &mergetag->next;
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 	return;
 
 free_return:
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 9a664a4a58..b96e6e5926 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -512,15 +512,16 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 		unsigned long size, len;
 		char *buf = read_object_file(oid, &type, &size);
 		struct signature_check sigc = { NULL };
-		struct strbuf sig = STRBUF_INIT;
+		struct strbuf payload = STRBUF_INIT, sig = STRBUF_INIT;
 
 		if (!buf || type != OBJ_TAG)
 			goto next;
-		len = parse_signature(buf, size);
+		len = parse_signature(buf, size, &payload, &sig);
 
 		if (size == len)
 			; /* merely annotated */
-		else if (check_signature(buf, len, buf + len, size - len, &sigc) &&
+		else if (check_signature(payload.buf, payload.len, sig.buf,
+					 sig.len, &sigc) &&
 			!sigc.gpg_output)
 			strbuf_addstr(&sig, "gpg verification failed.\n");
 		else
@@ -547,6 +548,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 					strlen(origins.items[i].string));
 			fmt_tag_signature(&tagbuf, &sig, buf, len);
 		}
+		strbuf_release(&payload);
 		strbuf_release(&sig);
 	next:
 		free(buf);
diff --git a/gpg-interface.c b/gpg-interface.c
index b499270836..c6274c14af 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -345,7 +345,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }
 
-size_t parse_signature(const char *buf, size_t size)
+size_t parse_signed_buffer(const char *buf, size_t size)
 {
 	size_t len = 0;
 	size_t match = size;
@@ -361,6 +361,17 @@ size_t parse_signature(const char *buf, size_t size)
 	return match;
 }
 
+int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct strbuf *signature)
+{
+	size_t match = parse_signed_buffer(buf, size);
+	if (match != size) {
+		strbuf_add(payload, buf, match);
+		strbuf_add(signature, buf + match, size - match);
+		return 1;
+	}
+	return 0;
+}
+
 void set_signing_key(const char *key)
 {
 	free(configured_signing_key);
diff --git a/gpg-interface.h b/gpg-interface.h
index f4e9b4f371..80567e4894 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -37,13 +37,20 @@ struct signature_check {
 
 void signature_check_clear(struct signature_check *sigc);
 
+/*
+ * Look at a GPG signed tag object.  If such a signature exists, store it in
+ * signature and the signed content in payload.  Return 1 if a signature was
+ * found, and 0 otherwise.
+ */
+int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct strbuf *signature);
+
 /*
  * Look at GPG signed content (e.g. a signed tag object), whose
  * payload is followed by a detached signature on it.  Return the
  * offset where the embedded detached signature begins, or the end of
  * the data when there is no such signature.
  */
-size_t parse_signature(const char *buf, size_t size);
+size_t parse_signed_buffer(const char *buf, size_t size);
 
 /*
  * Create a detached signature for the contents of "buffer" and append
diff --git a/log-tree.c b/log-tree.c
index 7e0335e548..b025c8da93 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -548,7 +548,8 @@ static int show_one_mergetag(struct commit *commit,
 	struct strbuf verify_message;
 	struct signature_check sigc = { 0 };
 	int status, nth;
-	size_t payload_size;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 
 	hash_object_file(the_hash_algo, extra->value, extra->len,
 			 type_name(OBJ_TAG), &oid);
@@ -571,13 +572,11 @@ static int show_one_mergetag(struct commit *commit,
 		strbuf_addf(&verify_message,
 			    "parent #%d, tagged '%s'\n", nth + 1, tag->tag);
 
-	payload_size = parse_signature(extra->value, extra->len);
 	status = -1;
-	if (extra->len > payload_size) {
+	if (parse_signature(extra->value, extra->len, &payload, &signature)) {
 		/* could have a good signature */
-		status = check_signature(extra->value, payload_size,
-					 extra->value + payload_size,
-					 extra->len - payload_size, &sigc);
+		status = check_signature(payload.buf, payload.len,
+					 signature.buf, signature.len, &sigc);
 		if (sigc.gpg_output)
 			strbuf_addstr(&verify_message, sigc.gpg_output);
 		else
@@ -588,6 +587,8 @@ static int show_one_mergetag(struct commit *commit,
 
 	show_sig_lines(opt, status, verify_message.buf);
 	strbuf_release(&verify_message);
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 	return 0;
 }
 
diff --git a/ref-filter.c b/ref-filter.c
index aa260bfd09..8d8baec1b5 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1215,7 +1215,13 @@ static void find_subpos(const char *buf,
 			unsigned long *nonsiglen,
 			const char **sig, unsigned long *siglen)
 {
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 	const char *eol;
+	const char *end = buf + strlen(buf);
+	const char *sigstart;
+
+
 	/* skip past header until we hit empty line */
 	while (*buf && *buf != '\n') {
 		eol = strchrnul(buf, '\n');
@@ -1228,14 +1234,15 @@ static void find_subpos(const char *buf,
 		buf++;
 
 	/* parse signature first; we might not even have a subject line */
-	*sig = buf + parse_signature(buf, strlen(buf));
-	*siglen = strlen(*sig);
+	parse_signature(buf, end - buf, &payload, &signature);
+	*sig = strbuf_detach(&signature, siglen);
+	sigstart = buf + parse_signed_buffer(buf, strlen(buf));
 
 	/* subject is first non-empty line */
 	*sub = buf;
 	/* subject goes to first empty line before signature begins */
 	if ((eol = strstr(*sub, "\n\n"))) {
-		eol = eol < *sig ? eol : *sig;
+		eol = eol < sigstart ? eol : sigstart;
 	/* check if message uses CRLF */
 	} else if (! (eol = strstr(*sub, "\r\n\r\n"))) {
 		/* treat whole message as subject */
@@ -1253,7 +1260,7 @@ static void find_subpos(const char *buf,
 		buf++;
 	*body = buf;
 	*bodylen = strlen(buf);
-	*nonsiglen = *sig - buf;
+	*nonsiglen = sigstart - buf;
 }
 
 /*
@@ -1291,6 +1298,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 		struct used_atom *atom = &used_atom[i];
 		const char *name = atom->name;
 		struct atom_value *v = &val[i];
+
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
@@ -1336,6 +1344,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 			v->s = strbuf_detach(&s, NULL);
 		} else if (atom->u.contents.option == C_BARE)
 			v->s = xstrdup(subpos);
+
+		free((void *)sigpos);
 	}
 }
 
diff --git a/tag.c b/tag.c
index 1ed2684e45..3e18a41841 100644
--- a/tag.c
+++ b/tag.c
@@ -13,26 +13,27 @@ const char *tag_type = "tag";
 static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 {
 	struct signature_check sigc;
-	size_t payload_size;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 	int ret;
 
 	memset(&sigc, 0, sizeof(sigc));
 
-	payload_size = parse_signature(buf, size);
-
-	if (size == payload_size) {
+	if (!parse_signature(buf, size, &payload, &signature)) {
 		if (flags & GPG_VERIFY_VERBOSE)
-			write_in_full(1, buf, payload_size);
+			write_in_full(1, buf, size);
 		return error("no signature found");
 	}
 
-	ret = check_signature(buf, payload_size, buf + payload_size,
-				size - payload_size, &sigc);
+	ret = check_signature(payload.buf, payload.len, signature.buf,
+				signature.len, &sigc);
 
 	if (!(flags & GPG_VERIFY_OMIT_STATUS))
 		print_signature_buffer(&sigc, flags);
 
 	signature_check_clear(&sigc);
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 	return ret;
 }
 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 3/5] commit: allow parsing arbitrary buffers with headers
  2021-01-11  0:37 [PATCH 0/5] Support for commits signed by multiple algorithms brian m. carlson
  2021-01-11  0:37 ` [PATCH 1/5] commit: ignore additional signatures when parsing signed commits brian m. carlson
  2021-01-11  0:37 ` [PATCH 2/5] gpg-interface: improve interface for parsing tags brian m. carlson
@ 2021-01-11  0:37 ` brian m. carlson
  2021-01-11  0:37 ` [PATCH 4/5] ref-filter: hoist signature parsing brian m. carlson
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  0:37 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

Currently only commits are signed with headers.  However, in the future,
we'll also sign tags with headers as well.  Let's refactor out a
function called parse_buffer_signed_by_header which does exactly that.
In addition, since we'll want to sign things other than commits this
way, let's call the function sign_with_header instead of do_sign_commit.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 commit.c | 21 +++++++++++++++++----
 commit.h |  9 +++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/commit.c b/commit.c
index 794dc8b593..7bbab5add4 100644
--- a/commit.c
+++ b/commit.c
@@ -995,7 +995,7 @@ static const char *gpg_sig_headers[] = {
 	"gpgsig-sha256",
 };
 
-static int do_sign_commit(struct strbuf *buf, const char *keyid)
+int sign_with_header(struct strbuf *buf, const char *keyid)
 {
 	struct strbuf sig = STRBUF_INIT;
 	int inspos, copypos;
@@ -1035,16 +1035,30 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)
 	return 0;
 }
 
+
+
 int parse_signed_commit(const struct commit *commit,
 			struct strbuf *payload, struct strbuf *signature,
 			const struct git_hash_algo *algop)
 {
-
 	unsigned long size;
 	const char *buffer = get_commit_buffer(commit, &size);
+	int ret = parse_buffer_signed_by_header(buffer, size, payload, signature, algop);
+
+	unuse_commit_buffer(commit, buffer);
+	return ret;
+}
+
+int parse_buffer_signed_by_header(const char *buffer,
+				  unsigned long size,
+				  struct strbuf *payload,
+				  struct strbuf *signature,
+				  const struct git_hash_algo *algop)
+{
 	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)];
+	int gpg_sig_header_len = strlen(gpg_sig_header);
 
 	line = buffer;
 	tail = buffer + size;
@@ -1078,7 +1092,6 @@ int parse_signed_commit(const struct commit *commit,
 		}
 		line = next;
 	}
-	unuse_commit_buffer(commit, buffer);
 	return saw_signature;
 }
 
@@ -1530,7 +1543,7 @@ int commit_tree_extended(const char *msg, size_t msg_len,
 	if (encoding_is_utf8 && !verify_utf8(&buffer))
 		fprintf(stderr, _(commit_utf8_warn));
 
-	if (sign_commit && do_sign_commit(&buffer, sign_commit)) {
+	if (sign_commit && sign_with_header(&buffer, sign_commit)) {
 		result = -1;
 		goto out;
 	}
diff --git a/commit.h b/commit.h
index 030aa65ab8..e2856ce8ef 100644
--- a/commit.h
+++ b/commit.h
@@ -360,4 +360,13 @@ int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void
 LAST_ARG_MUST_BE_NULL
 int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
 
+/* Sign a commit or tag buffer, storing the result in a header. */
+int sign_with_header(struct strbuf *buf, const char *keyid);
+/* Parse the signature out of a header. */
+int parse_buffer_signed_by_header(const char *buffer,
+				  unsigned long size,
+				  struct strbuf *payload,
+				  struct strbuf *signature,
+				  const struct git_hash_algo *algop);
+
 #endif /* COMMIT_H */

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 4/5] ref-filter: hoist signature parsing
  2021-01-11  0:37 [PATCH 0/5] Support for commits signed by multiple algorithms brian m. carlson
                   ` (2 preceding siblings ...)
  2021-01-11  0:37 ` [PATCH 3/5] commit: allow parsing arbitrary buffers with headers brian m. carlson
@ 2021-01-11  0:37 ` brian m. carlson
  2021-01-11  0:37 ` [PATCH 5/6] fixup! commit: ignore additional signatures when parsing signed commits brian m. carlson
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  0:37 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

When we parse a signature in the ref-filter code, we continually
increment the buffer pointer.  Hoist the signature parsing above the
blank line delimiting headers and body so we can find the signature when
using a header to sign the buffer.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 ref-filter.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8d8baec1b5..32ed4d5111 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1221,6 +1221,8 @@ static void find_subpos(const char *buf,
 	const char *end = buf + strlen(buf);
 	const char *sigstart;
 
+	/* parse signature first; we might not even have a subject line */
+	parse_signature(buf, end - buf, &payload, &signature);
 
 	/* skip past header until we hit empty line */
 	while (*buf && *buf != '\n') {
@@ -1232,9 +1234,6 @@ static void find_subpos(const char *buf,
 	/* skip any empty lines */
 	while (*buf == '\n')
 		buf++;
-
-	/* parse signature first; we might not even have a subject line */
-	parse_signature(buf, end - buf, &payload, &signature);
 	*sig = strbuf_detach(&signature, siglen);
 	sigstart = buf + parse_signed_buffer(buf, strlen(buf));
 
@@ -1330,7 +1329,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 			v->s = xmemdupz(sigpos, siglen);
 		else if (atom->u.contents.option == C_LINES) {
 			struct strbuf s = STRBUF_INIT;
-			const char *contents_end = bodylen + bodypos - siglen;
+			const char *contents_end = bodypos + nonsiglen;
 
 			/*  Size is the length of the message after removing the signature */
 			append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines);

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 5/6] fixup! commit: ignore additional signatures when parsing signed commits
  2021-01-11  0:37 [PATCH 0/5] Support for commits signed by multiple algorithms brian m. carlson
                   ` (3 preceding siblings ...)
  2021-01-11  0:37 ` [PATCH 4/5] ref-filter: hoist signature parsing brian m. carlson
@ 2021-01-11  0:37 ` brian m. carlson
  2021-01-11  0:43   ` brian m. carlson
  2021-01-11  0:37 ` [PATCH 5/5] gpg-interface: remove other signature headers before verifying brian m. carlson
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  0:37 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

---
 commit.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/commit.c b/commit.c
index 7bbab5add4..23020c0bca 100644
--- a/commit.c
+++ b/commit.c
@@ -1058,7 +1058,6 @@ int parse_buffer_signed_by_header(const char *buffer,
 	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)];
-	int gpg_sig_header_len = strlen(gpg_sig_header);
 
 	line = buffer;
 	tail = buffer + size;

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 5/5] gpg-interface: remove other signature headers before verifying
  2021-01-11  0:37 [PATCH 0/5] Support for commits signed by multiple algorithms brian m. carlson
                   ` (4 preceding siblings ...)
  2021-01-11  0:37 ` [PATCH 5/6] fixup! commit: ignore additional signatures when parsing signed commits brian m. carlson
@ 2021-01-11  0:37 ` brian m. carlson
  2021-01-11  0:37 ` [PATCH 6/6] " brian m. carlson
  2021-01-11  3:58 ` [PATCH v2 0/5] Support for commits signed by multiple algorithms brian m. carlson
  7 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  0:37 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

When we have a multiply signed commit, we need to remove the signature
in the header before verifying the object, since the trailing signature
will not be over both pieces of data.  Do so, and verify that we
validate the signature appropriately.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 gpg-interface.c |  2 ++
 t/t7004-tag.sh  | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/gpg-interface.c b/gpg-interface.c
index c6274c14af..127aecfc2b 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "commit.h"
 #include "config.h"
 #include "run-command.h"
 #include "strbuf.h"
@@ -366,6 +367,7 @@ int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct
 	size_t match = parse_signed_buffer(buf, size);
 	if (match != size) {
 		strbuf_add(payload, buf, match);
+		remove_signature(payload);
 		strbuf_add(signature, buf + match, size - match);
 		return 1;
 	}
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 05f411c821..6fb4e3cf11 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -17,6 +17,13 @@ tag_exists () {
 	git show-ref --quiet --verify refs/tags/"$1"
 }
 
+test_expect_success 'setup' '
+	test_oid_cache <<-EOM
+	othersigheader sha1:gpgsig-sha256
+	othersigheader sha256:gpgsig
+	EOM
+'
+
 test_expect_success 'listing all tags in an empty tree should succeed' '
 	git tag -l &&
 	git tag
@@ -1371,6 +1378,24 @@ test_expect_success GPG \
 	'test_config gpg.program echo &&
 	 test_must_fail git tag -s -m tail tag-gpg-failure'
 
+# try to produce invalid signature
+test_expect_success GPG 'git verifies tag is valid with double signature' '
+	git tag -s -m tail tag-gpg-double-sig &&
+	git cat-file tag tag-gpg-double-sig >tag &&
+	othersigheader=$(test_oid othersigheader) &&
+	sed -ne "/^\$/q;p" tag >new-tag &&
+	cat <<-EOM >>new-tag &&
+	$othersigheader -----BEGIN PGP SIGNATURE-----
+	 someinvaliddata
+	 -----END PGP SIGNATURE-----
+	EOM
+	sed -e "1,/^tagger/d" tag >>new-tag &&
+	new_tag=$(git hash-object -t tag -w new-tag) &&
+	git update-ref refs/tags/tag-gpg-double-sig $new_tag &&
+	git verify-tag tag-gpg-double-sig &&
+	git fsck
+'
+
 # try to sign with bad user.signingkey
 test_expect_success GPGSM \
 	'git tag -s fails if gpgsm is misconfigured (bad key)' \

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 6/6] gpg-interface: remove other signature headers before verifying
  2021-01-11  0:37 [PATCH 0/5] Support for commits signed by multiple algorithms brian m. carlson
                   ` (5 preceding siblings ...)
  2021-01-11  0:37 ` [PATCH 5/5] gpg-interface: remove other signature headers before verifying brian m. carlson
@ 2021-01-11  0:37 ` brian m. carlson
  2021-01-11  3:58 ` [PATCH v2 0/5] Support for commits signed by multiple algorithms brian m. carlson
  7 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  0:37 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

When we have a multiply signed commit, we need to remove the signature
in the header before verifying the object, since the trailing signature
will not be over both pieces of data.  Do so, and verify that we
validate the signature appropriately.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 gpg-interface.c |  2 ++
 t/t7004-tag.sh  | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/gpg-interface.c b/gpg-interface.c
index c6274c14af..127aecfc2b 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "commit.h"
 #include "config.h"
 #include "run-command.h"
 #include "strbuf.h"
@@ -366,6 +367,7 @@ int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct
 	size_t match = parse_signed_buffer(buf, size);
 	if (match != size) {
 		strbuf_add(payload, buf, match);
+		remove_signature(payload);
 		strbuf_add(signature, buf + match, size - match);
 		return 1;
 	}
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 05f411c821..6fb4e3cf11 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -17,6 +17,13 @@ tag_exists () {
 	git show-ref --quiet --verify refs/tags/"$1"
 }
 
+test_expect_success 'setup' '
+	test_oid_cache <<-EOM
+	othersigheader sha1:gpgsig-sha256
+	othersigheader sha256:gpgsig
+	EOM
+'
+
 test_expect_success 'listing all tags in an empty tree should succeed' '
 	git tag -l &&
 	git tag
@@ -1371,6 +1378,24 @@ test_expect_success GPG \
 	'test_config gpg.program echo &&
 	 test_must_fail git tag -s -m tail tag-gpg-failure'
 
+# try to produce invalid signature
+test_expect_success GPG 'git verifies tag is valid with double signature' '
+	git tag -s -m tail tag-gpg-double-sig &&
+	git cat-file tag tag-gpg-double-sig >tag &&
+	othersigheader=$(test_oid othersigheader) &&
+	sed -ne "/^\$/q;p" tag >new-tag &&
+	cat <<-EOM >>new-tag &&
+	$othersigheader -----BEGIN PGP SIGNATURE-----
+	 someinvaliddata
+	 -----END PGP SIGNATURE-----
+	EOM
+	sed -e "1,/^tagger/d" tag >>new-tag &&
+	new_tag=$(git hash-object -t tag -w new-tag) &&
+	git update-ref refs/tags/tag-gpg-double-sig $new_tag &&
+	git verify-tag tag-gpg-double-sig &&
+	git fsck
+'
+
 # try to sign with bad user.signingkey
 test_expect_success GPGSM \
 	'git tag -s fails if gpgsm is misconfigured (bad key)' \

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 5/6] fixup! commit: ignore additional signatures when parsing signed commits
  2021-01-11  0:37 ` [PATCH 5/6] fixup! commit: ignore additional signatures when parsing signed commits brian m. carlson
@ 2021-01-11  0:43   ` brian m. carlson
  0 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  0:43 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

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

On 2021-01-11 at 00:37:38, brian m. carlson wrote:
> ---
>  commit.c | 1 -
>  1 file changed, 1 deletion(-)
> 

Please ignore this patch.  I squashed it in, but neglected to clean my
patches directory before sending.  My apologies for the error.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 0/5] Support for commits signed by multiple algorithms
  2021-01-11  0:37 [PATCH 0/5] Support for commits signed by multiple algorithms brian m. carlson
                   ` (6 preceding siblings ...)
  2021-01-11  0:37 ` [PATCH 6/6] " brian m. carlson
@ 2021-01-11  3:58 ` brian m. carlson
  2021-01-11  3:58   ` [PATCH v2 1/5] commit: ignore additional signatures when parsing signed commits brian m. carlson
                     ` (6 more replies)
  7 siblings, 7 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  3:58 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

This series introduces support for verifying commits and tags signed by
multiple algorithms.

Originally, we had planned for SHA-256 tags to stuff the signature in a
header instead of using a trailing signature, and a patch to do that was
sent out in part 1/3.  Unfortunately, for whatever reason, that patch
didn't make it into the master branch, and so we use trailing signatures
there.

We can't change this now, because otherwise it would be ambiguous
whether the trailing signature on a SHA-256 object was for the SHA-256
contents or whether the contents were a rewritten SHA-1 object with no
SHA-256 signature at all.  To do the next best thing, let's use the
trailing signature for the preferred hash algorithm and use a header for
the other variant.  This permits round-tripping, but has the downside
that tags signed with multiple algorithms can't be verified with older
versions of Git.  However, signatures created with older versions of Git
continue to be accepted.

For commits, let's accept a commit that has two signatures.  We
previously created the commits correctly but didn't strip the extra
header off when verifying, so our verification indicated the signature
was bad.

Both these situations allow for signing commits and tags that can be
round-tripped through both SHA-1 and SHA-256.  We verify only the
signature using the current hash algorithm, since we currently don't
rewrite objects.

Changes from v1:
* Fix brown paper bag bug where some tests failed due to a bad rebase.

brian m. carlson (5):
  commit: ignore additional signatures when parsing signed commits
  gpg-interface: improve interface for parsing tags
  commit: allow parsing arbitrary buffers with headers
  ref-filter: hoist signature parsing
  gpg-interface: remove other signature headers before verifying

 builtin/receive-pack.c   |  4 +-
 builtin/tag.c            | 16 ++++++--
 commit.c                 | 83 ++++++++++++++++++++++++++--------------
 commit.h                 | 12 +++++-
 fmt-merge-msg.c          | 29 ++++++++------
 gpg-interface.c          | 15 +++++++-
 gpg-interface.h          |  9 ++++-
 log-tree.c               | 15 ++++----
 ref-filter.c             | 23 +++++++----
 t/t7004-tag.sh           | 25 ++++++++++++
 t/t7510-signed-commit.sh | 43 ++++++++++++++++++++-
 tag.c                    | 15 ++++----
 12 files changed, 219 insertions(+), 70 deletions(-)


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 1/5] commit: ignore additional signatures when parsing signed commits
  2021-01-11  3:58 ` [PATCH v2 0/5] Support for commits signed by multiple algorithms brian m. carlson
@ 2021-01-11  3:58   ` brian m. carlson
  2021-01-12 17:03     ` SZEDER Gábor
  2021-01-11  3:58   ` [PATCH v2 2/5] gpg-interface: improve interface for parsing tags brian m. carlson
                     ` (5 subsequent siblings)
  6 siblings, 1 reply; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  3:58 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

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                 | 52 ++++++++++++++++++++++++----------------
 commit.h                 |  3 ++-
 log-tree.c               |  2 +-
 t/t7510-signed-commit.sh | 43 ++++++++++++++++++++++++++++++++-
 4 files changed, 77 insertions(+), 23 deletions(-)

diff --git a/commit.c b/commit.c
index f128f18a9b..93faaad764 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,27 @@ 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] = { 0 }, *sigp = &sigs[0];
+	int i;
+	const char *orig_buf = buf->buf;
 
 	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 +1115,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 +1177,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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 2/5] gpg-interface: improve interface for parsing tags
  2021-01-11  3:58 ` [PATCH v2 0/5] Support for commits signed by multiple algorithms brian m. carlson
  2021-01-11  3:58   ` [PATCH v2 1/5] commit: ignore additional signatures when parsing signed commits brian m. carlson
@ 2021-01-11  3:58   ` brian m. carlson
  2021-01-12  5:24     ` Junio C Hamano
  2021-01-11  3:58   ` [PATCH v2 3/5] commit: allow parsing arbitrary buffers with headers brian m. carlson
                     ` (4 subsequent siblings)
  6 siblings, 1 reply; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  3:58 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

We have a function which parses a buffer with a signature at the end,
parse_signature, and this function is used for signed tags.  However,
we'll need to store values for multiple algorithms, and we'll do this by
using a header for the non-default algorithm.

Adjust the parse_signature interface to store the parsed data in two
strbufs and turn the existing function into parse_signed_buffer.  The
latter is still used in places where we know we always have a signed
buffer, such as push certs.

Adjust all the callers to deal with this new interface.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/receive-pack.c |  4 ++--
 builtin/tag.c          | 16 ++++++++++++----
 commit.c               |  9 ++++++---
 fmt-merge-msg.c        | 29 ++++++++++++++++++-----------
 gpg-interface.c        | 13 ++++++++++++-
 gpg-interface.h        |  9 ++++++++-
 log-tree.c             | 13 +++++++------
 ref-filter.c           | 18 ++++++++++++++----
 tag.c                  | 15 ++++++++-------
 9 files changed, 87 insertions(+), 39 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d49d050e6e..b89ce31bf2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -764,7 +764,7 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 
 		memset(&sigcheck, '\0', sizeof(sigcheck));
 
-		bogs = parse_signature(push_cert.buf, push_cert.len);
+		bogs = parse_signed_buffer(push_cert.buf, push_cert.len);
 		check_signature(push_cert.buf, bogs, push_cert.buf + bogs,
 				push_cert.len - bogs, &sigcheck);
 
@@ -2050,7 +2050,7 @@ static void queue_commands_from_cert(struct command **tail,
 		die("malformed push certificate %.*s", 100, push_cert->buf);
 	else
 		boc += 2;
-	eoc = push_cert->buf + parse_signature(push_cert->buf, push_cert->len);
+	eoc = push_cert->buf + parse_signed_buffer(push_cert->buf, push_cert->len);
 
 	while (boc < eoc) {
 		const char *eol = memchr(boc, '\n', eoc - boc);
diff --git a/builtin/tag.c b/builtin/tag.c
index ecf011776d..7162f4ccc5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -174,11 +174,17 @@ static void write_tag_body(int fd, const struct object_id *oid)
 {
 	unsigned long size;
 	enum object_type type;
-	char *buf, *sp;
+	char *buf, *sp, *orig;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 
-	buf = read_object_file(oid, &type, &size);
+	orig = buf = read_object_file(oid, &type, &size);
 	if (!buf)
 		return;
+	if (parse_signature(buf, size, &payload, &signature)) {
+		buf = payload.buf;
+		size = payload.len;
+	}
 	/* skip header */
 	sp = strstr(buf, "\n\n");
 
@@ -187,9 +193,11 @@ static void write_tag_body(int fd, const struct object_id *oid)
 		return;
 	}
 	sp += 2; /* skip the 2 LFs */
-	write_or_die(fd, sp, parse_signature(sp, buf + size - sp));
+	write_or_die(fd, sp, buf + size - sp);
 
-	free(buf);
+	free(orig);
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 }
 
 static int build_tag_object(struct strbuf *buf, int sign, struct object_id *result)
diff --git a/commit.c b/commit.c
index 93faaad764..794dc8b593 100644
--- a/commit.c
+++ b/commit.c
@@ -1134,8 +1134,10 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 	struct merge_remote_desc *desc;
 	struct commit_extra_header *mergetag;
 	char *buf;
-	unsigned long size, len;
+	unsigned long size;
 	enum object_type type;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 
 	desc = merge_remote_util(parent);
 	if (!desc || !desc->obj)
@@ -1143,8 +1145,7 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 	buf = read_object_file(&desc->obj->oid, &type, &size);
 	if (!buf || type != OBJ_TAG)
 		goto free_return;
-	len = parse_signature(buf, size);
-	if (size == len)
+	if (!parse_signature(buf, size, &payload, &signature))
 		goto free_return;
 	/*
 	 * We could verify this signature and either omit the tag when
@@ -1163,6 +1164,8 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 
 	**tail = mergetag;
 	*tail = &mergetag->next;
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 	return;
 
 free_return:
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 9a664a4a58..7fd99f0ac1 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -509,22 +509,28 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 	for (i = 0; i < origins.nr; i++) {
 		struct object_id *oid = origins.items[i].util;
 		enum object_type type;
-		unsigned long size, len;
+		unsigned long size;
 		char *buf = read_object_file(oid, &type, &size);
+		char *origbuf = buf;
+		unsigned long len = size;
 		struct signature_check sigc = { NULL };
-		struct strbuf sig = STRBUF_INIT;
+		struct strbuf payload = STRBUF_INIT, sig = STRBUF_INIT;
 
 		if (!buf || type != OBJ_TAG)
 			goto next;
-		len = parse_signature(buf, size);
 
-		if (size == len)
-			; /* merely annotated */
-		else if (check_signature(buf, len, buf + len, size - len, &sigc) &&
-			!sigc.gpg_output)
-			strbuf_addstr(&sig, "gpg verification failed.\n");
-		else
-			strbuf_addstr(&sig, sigc.gpg_output);
+		if (!parse_signature(buf, size, &payload, &sig))
+			;/* merely annotated */
+		else {
+			buf = payload.buf;
+			len = payload.len;
+			if (check_signature(payload.buf, payload.len, sig.buf,
+					 sig.len, &sigc) &&
+				!sigc.gpg_output)
+				strbuf_addstr(&sig, "gpg verification failed.\n");
+			else
+				strbuf_addstr(&sig, sigc.gpg_output);
+		}
 		signature_check_clear(&sigc);
 
 		if (!tag_number++) {
@@ -547,9 +553,10 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 					strlen(origins.items[i].string));
 			fmt_tag_signature(&tagbuf, &sig, buf, len);
 		}
+		strbuf_release(&payload);
 		strbuf_release(&sig);
 	next:
-		free(buf);
+		free(origbuf);
 	}
 	if (tagbuf.len) {
 		strbuf_addch(out, '\n');
diff --git a/gpg-interface.c b/gpg-interface.c
index b499270836..c6274c14af 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -345,7 +345,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }
 
-size_t parse_signature(const char *buf, size_t size)
+size_t parse_signed_buffer(const char *buf, size_t size)
 {
 	size_t len = 0;
 	size_t match = size;
@@ -361,6 +361,17 @@ size_t parse_signature(const char *buf, size_t size)
 	return match;
 }
 
+int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct strbuf *signature)
+{
+	size_t match = parse_signed_buffer(buf, size);
+	if (match != size) {
+		strbuf_add(payload, buf, match);
+		strbuf_add(signature, buf + match, size - match);
+		return 1;
+	}
+	return 0;
+}
+
 void set_signing_key(const char *key)
 {
 	free(configured_signing_key);
diff --git a/gpg-interface.h b/gpg-interface.h
index f4e9b4f371..80567e4894 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -37,13 +37,20 @@ struct signature_check {
 
 void signature_check_clear(struct signature_check *sigc);
 
+/*
+ * Look at a GPG signed tag object.  If such a signature exists, store it in
+ * signature and the signed content in payload.  Return 1 if a signature was
+ * found, and 0 otherwise.
+ */
+int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct strbuf *signature);
+
 /*
  * Look at GPG signed content (e.g. a signed tag object), whose
  * payload is followed by a detached signature on it.  Return the
  * offset where the embedded detached signature begins, or the end of
  * the data when there is no such signature.
  */
-size_t parse_signature(const char *buf, size_t size);
+size_t parse_signed_buffer(const char *buf, size_t size);
 
 /*
  * Create a detached signature for the contents of "buffer" and append
diff --git a/log-tree.c b/log-tree.c
index 7e0335e548..b025c8da93 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -548,7 +548,8 @@ static int show_one_mergetag(struct commit *commit,
 	struct strbuf verify_message;
 	struct signature_check sigc = { 0 };
 	int status, nth;
-	size_t payload_size;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 
 	hash_object_file(the_hash_algo, extra->value, extra->len,
 			 type_name(OBJ_TAG), &oid);
@@ -571,13 +572,11 @@ static int show_one_mergetag(struct commit *commit,
 		strbuf_addf(&verify_message,
 			    "parent #%d, tagged '%s'\n", nth + 1, tag->tag);
 
-	payload_size = parse_signature(extra->value, extra->len);
 	status = -1;
-	if (extra->len > payload_size) {
+	if (parse_signature(extra->value, extra->len, &payload, &signature)) {
 		/* could have a good signature */
-		status = check_signature(extra->value, payload_size,
-					 extra->value + payload_size,
-					 extra->len - payload_size, &sigc);
+		status = check_signature(payload.buf, payload.len,
+					 signature.buf, signature.len, &sigc);
 		if (sigc.gpg_output)
 			strbuf_addstr(&verify_message, sigc.gpg_output);
 		else
@@ -588,6 +587,8 @@ static int show_one_mergetag(struct commit *commit,
 
 	show_sig_lines(opt, status, verify_message.buf);
 	strbuf_release(&verify_message);
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 	return 0;
 }
 
diff --git a/ref-filter.c b/ref-filter.c
index aa260bfd09..8d8baec1b5 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1215,7 +1215,13 @@ static void find_subpos(const char *buf,
 			unsigned long *nonsiglen,
 			const char **sig, unsigned long *siglen)
 {
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 	const char *eol;
+	const char *end = buf + strlen(buf);
+	const char *sigstart;
+
+
 	/* skip past header until we hit empty line */
 	while (*buf && *buf != '\n') {
 		eol = strchrnul(buf, '\n');
@@ -1228,14 +1234,15 @@ static void find_subpos(const char *buf,
 		buf++;
 
 	/* parse signature first; we might not even have a subject line */
-	*sig = buf + parse_signature(buf, strlen(buf));
-	*siglen = strlen(*sig);
+	parse_signature(buf, end - buf, &payload, &signature);
+	*sig = strbuf_detach(&signature, siglen);
+	sigstart = buf + parse_signed_buffer(buf, strlen(buf));
 
 	/* subject is first non-empty line */
 	*sub = buf;
 	/* subject goes to first empty line before signature begins */
 	if ((eol = strstr(*sub, "\n\n"))) {
-		eol = eol < *sig ? eol : *sig;
+		eol = eol < sigstart ? eol : sigstart;
 	/* check if message uses CRLF */
 	} else if (! (eol = strstr(*sub, "\r\n\r\n"))) {
 		/* treat whole message as subject */
@@ -1253,7 +1260,7 @@ static void find_subpos(const char *buf,
 		buf++;
 	*body = buf;
 	*bodylen = strlen(buf);
-	*nonsiglen = *sig - buf;
+	*nonsiglen = sigstart - buf;
 }
 
 /*
@@ -1291,6 +1298,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 		struct used_atom *atom = &used_atom[i];
 		const char *name = atom->name;
 		struct atom_value *v = &val[i];
+
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
@@ -1336,6 +1344,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 			v->s = strbuf_detach(&s, NULL);
 		} else if (atom->u.contents.option == C_BARE)
 			v->s = xstrdup(subpos);
+
+		free((void *)sigpos);
 	}
 }
 
diff --git a/tag.c b/tag.c
index 1ed2684e45..3e18a41841 100644
--- a/tag.c
+++ b/tag.c
@@ -13,26 +13,27 @@ const char *tag_type = "tag";
 static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 {
 	struct signature_check sigc;
-	size_t payload_size;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 	int ret;
 
 	memset(&sigc, 0, sizeof(sigc));
 
-	payload_size = parse_signature(buf, size);
-
-	if (size == payload_size) {
+	if (!parse_signature(buf, size, &payload, &signature)) {
 		if (flags & GPG_VERIFY_VERBOSE)
-			write_in_full(1, buf, payload_size);
+			write_in_full(1, buf, size);
 		return error("no signature found");
 	}
 
-	ret = check_signature(buf, payload_size, buf + payload_size,
-				size - payload_size, &sigc);
+	ret = check_signature(payload.buf, payload.len, signature.buf,
+				signature.len, &sigc);
 
 	if (!(flags & GPG_VERIFY_OMIT_STATUS))
 		print_signature_buffer(&sigc, flags);
 
 	signature_check_clear(&sigc);
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 	return ret;
 }
 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 3/5] commit: allow parsing arbitrary buffers with headers
  2021-01-11  3:58 ` [PATCH v2 0/5] Support for commits signed by multiple algorithms brian m. carlson
  2021-01-11  3:58   ` [PATCH v2 1/5] commit: ignore additional signatures when parsing signed commits brian m. carlson
  2021-01-11  3:58   ` [PATCH v2 2/5] gpg-interface: improve interface for parsing tags brian m. carlson
@ 2021-01-11  3:58   ` brian m. carlson
  2021-01-11  3:58   ` [PATCH v2 4/5] ref-filter: hoist signature parsing brian m. carlson
                     ` (3 subsequent siblings)
  6 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  3:58 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

Currently only commits are signed with headers.  However, in the future,
we'll also sign tags with headers as well.  Let's refactor out a
function called parse_buffer_signed_by_header which does exactly that.
In addition, since we'll want to sign things other than commits this
way, let's call the function sign_with_header instead of do_sign_commit.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 commit.c | 20 ++++++++++++++++----
 commit.h |  9 +++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/commit.c b/commit.c
index 794dc8b593..23020c0bca 100644
--- a/commit.c
+++ b/commit.c
@@ -995,7 +995,7 @@ static const char *gpg_sig_headers[] = {
 	"gpgsig-sha256",
 };
 
-static int do_sign_commit(struct strbuf *buf, const char *keyid)
+int sign_with_header(struct strbuf *buf, const char *keyid)
 {
 	struct strbuf sig = STRBUF_INIT;
 	int inspos, copypos;
@@ -1035,13 +1035,26 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)
 	return 0;
 }
 
+
+
 int parse_signed_commit(const struct commit *commit,
 			struct strbuf *payload, struct strbuf *signature,
 			const struct git_hash_algo *algop)
 {
-
 	unsigned long size;
 	const char *buffer = get_commit_buffer(commit, &size);
+	int ret = parse_buffer_signed_by_header(buffer, size, payload, signature, algop);
+
+	unuse_commit_buffer(commit, buffer);
+	return ret;
+}
+
+int parse_buffer_signed_by_header(const char *buffer,
+				  unsigned long size,
+				  struct strbuf *payload,
+				  struct strbuf *signature,
+				  const struct git_hash_algo *algop)
+{
 	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)];
@@ -1078,7 +1091,6 @@ int parse_signed_commit(const struct commit *commit,
 		}
 		line = next;
 	}
-	unuse_commit_buffer(commit, buffer);
 	return saw_signature;
 }
 
@@ -1530,7 +1542,7 @@ int commit_tree_extended(const char *msg, size_t msg_len,
 	if (encoding_is_utf8 && !verify_utf8(&buffer))
 		fprintf(stderr, _(commit_utf8_warn));
 
-	if (sign_commit && do_sign_commit(&buffer, sign_commit)) {
+	if (sign_commit && sign_with_header(&buffer, sign_commit)) {
 		result = -1;
 		goto out;
 	}
diff --git a/commit.h b/commit.h
index 030aa65ab8..e2856ce8ef 100644
--- a/commit.h
+++ b/commit.h
@@ -360,4 +360,13 @@ int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void
 LAST_ARG_MUST_BE_NULL
 int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
 
+/* Sign a commit or tag buffer, storing the result in a header. */
+int sign_with_header(struct strbuf *buf, const char *keyid);
+/* Parse the signature out of a header. */
+int parse_buffer_signed_by_header(const char *buffer,
+				  unsigned long size,
+				  struct strbuf *payload,
+				  struct strbuf *signature,
+				  const struct git_hash_algo *algop);
+
 #endif /* COMMIT_H */

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 4/5] ref-filter: hoist signature parsing
  2021-01-11  3:58 ` [PATCH v2 0/5] Support for commits signed by multiple algorithms brian m. carlson
                     ` (2 preceding siblings ...)
  2021-01-11  3:58   ` [PATCH v2 3/5] commit: allow parsing arbitrary buffers with headers brian m. carlson
@ 2021-01-11  3:58   ` brian m. carlson
  2021-01-11  3:58   ` [PATCH v2 5/5] gpg-interface: remove other signature headers before verifying brian m. carlson
                     ` (2 subsequent siblings)
  6 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  3:58 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

When we parse a signature in the ref-filter code, we continually
increment the buffer pointer.  Hoist the signature parsing above the
blank line delimiting headers and body so we can find the signature when
using a header to sign the buffer.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 ref-filter.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8d8baec1b5..32ed4d5111 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1221,6 +1221,8 @@ static void find_subpos(const char *buf,
 	const char *end = buf + strlen(buf);
 	const char *sigstart;
 
+	/* parse signature first; we might not even have a subject line */
+	parse_signature(buf, end - buf, &payload, &signature);
 
 	/* skip past header until we hit empty line */
 	while (*buf && *buf != '\n') {
@@ -1232,9 +1234,6 @@ static void find_subpos(const char *buf,
 	/* skip any empty lines */
 	while (*buf == '\n')
 		buf++;
-
-	/* parse signature first; we might not even have a subject line */
-	parse_signature(buf, end - buf, &payload, &signature);
 	*sig = strbuf_detach(&signature, siglen);
 	sigstart = buf + parse_signed_buffer(buf, strlen(buf));
 
@@ -1330,7 +1329,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 			v->s = xmemdupz(sigpos, siglen);
 		else if (atom->u.contents.option == C_LINES) {
 			struct strbuf s = STRBUF_INIT;
-			const char *contents_end = bodylen + bodypos - siglen;
+			const char *contents_end = bodypos + nonsiglen;
 
 			/*  Size is the length of the message after removing the signature */
 			append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines);

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 5/5] gpg-interface: remove other signature headers before verifying
  2021-01-11  3:58 ` [PATCH v2 0/5] Support for commits signed by multiple algorithms brian m. carlson
                     ` (3 preceding siblings ...)
  2021-01-11  3:58   ` [PATCH v2 4/5] ref-filter: hoist signature parsing brian m. carlson
@ 2021-01-11  3:58   ` brian m. carlson
  2021-01-11 22:16   ` [PATCH v2 0/5] Support for commits signed by multiple algorithms Junio C Hamano
  2021-01-18 23:49   ` [PATCH v3 0/6] " brian m. carlson
  6 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-11  3:58 UTC (permalink / raw)
  To: git; +Cc: Eric Sunshine, Denton Liu, Jeff King

When we have a multiply signed commit, we need to remove the signature
in the header before verifying the object, since the trailing signature
will not be over both pieces of data.  Do so, and verify that we
validate the signature appropriately.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 gpg-interface.c |  2 ++
 t/t7004-tag.sh  | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/gpg-interface.c b/gpg-interface.c
index c6274c14af..127aecfc2b 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "commit.h"
 #include "config.h"
 #include "run-command.h"
 #include "strbuf.h"
@@ -366,6 +367,7 @@ int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct
 	size_t match = parse_signed_buffer(buf, size);
 	if (match != size) {
 		strbuf_add(payload, buf, match);
+		remove_signature(payload);
 		strbuf_add(signature, buf + match, size - match);
 		return 1;
 	}
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 05f411c821..6fb4e3cf11 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -17,6 +17,13 @@ tag_exists () {
 	git show-ref --quiet --verify refs/tags/"$1"
 }
 
+test_expect_success 'setup' '
+	test_oid_cache <<-EOM
+	othersigheader sha1:gpgsig-sha256
+	othersigheader sha256:gpgsig
+	EOM
+'
+
 test_expect_success 'listing all tags in an empty tree should succeed' '
 	git tag -l &&
 	git tag
@@ -1371,6 +1378,24 @@ test_expect_success GPG \
 	'test_config gpg.program echo &&
 	 test_must_fail git tag -s -m tail tag-gpg-failure'
 
+# try to produce invalid signature
+test_expect_success GPG 'git verifies tag is valid with double signature' '
+	git tag -s -m tail tag-gpg-double-sig &&
+	git cat-file tag tag-gpg-double-sig >tag &&
+	othersigheader=$(test_oid othersigheader) &&
+	sed -ne "/^\$/q;p" tag >new-tag &&
+	cat <<-EOM >>new-tag &&
+	$othersigheader -----BEGIN PGP SIGNATURE-----
+	 someinvaliddata
+	 -----END PGP SIGNATURE-----
+	EOM
+	sed -e "1,/^tagger/d" tag >>new-tag &&
+	new_tag=$(git hash-object -t tag -w new-tag) &&
+	git update-ref refs/tags/tag-gpg-double-sig $new_tag &&
+	git verify-tag tag-gpg-double-sig &&
+	git fsck
+'
+
 # try to sign with bad user.signingkey
 test_expect_success GPGSM \
 	'git tag -s fails if gpgsm is misconfigured (bad key)' \

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 0/5] Support for commits signed by multiple algorithms
  2021-01-11  3:58 ` [PATCH v2 0/5] Support for commits signed by multiple algorithms brian m. carlson
                     ` (4 preceding siblings ...)
  2021-01-11  3:58   ` [PATCH v2 5/5] gpg-interface: remove other signature headers before verifying brian m. carlson
@ 2021-01-11 22:16   ` Junio C Hamano
  2021-01-11 23:29     ` brian m. carlson
  2021-01-18 23:49   ` [PATCH v3 0/6] " brian m. carlson
  6 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-01-11 22:16 UTC (permalink / raw)
  To: brian m. carlson; +Cc: git, Eric Sunshine, Denton Liu, Jeff King

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> This series introduces support for verifying commits and tags signed by
> multiple algorithms.
>
> Originally, we had planned for SHA-256 tags to stuff the signature in a
> header instead of using a trailing signature, and a patch to do that was
> sent out in part 1/3.  Unfortunately, for whatever reason, that patch
> didn't make it into the master branch, and so we use trailing signatures
> there.
>
> We can't change this now, because otherwise it would be ambiguous
> whether the trailing signature on a SHA-256 object was for the SHA-256
> contents or whether the contents were a rewritten SHA-1 object with no
> SHA-256 signature at all.

How widely are SHA-256 tags in use in the real world, though?  Is it
really too late to fix that already?

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 0/5] Support for commits signed by multiple algorithms
  2021-01-11 22:16   ` [PATCH v2 0/5] Support for commits signed by multiple algorithms Junio C Hamano
@ 2021-01-11 23:29     ` brian m. carlson
  2021-01-12  2:03       ` Junio C Hamano
  0 siblings, 1 reply; 31+ messages in thread
From: brian m. carlson @ 2021-01-11 23:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Eric Sunshine, Denton Liu, Jeff King

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

On 2021-01-11 at 22:16:33, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > This series introduces support for verifying commits and tags signed by
> > multiple algorithms.
> >
> > Originally, we had planned for SHA-256 tags to stuff the signature in a
> > header instead of using a trailing signature, and a patch to do that was
> > sent out in part 1/3.  Unfortunately, for whatever reason, that patch
> > didn't make it into the master branch, and so we use trailing signatures
> > there.
> >
> > We can't change this now, because otherwise it would be ambiguous
> > whether the trailing signature on a SHA-256 object was for the SHA-256
> > contents or whether the contents were a rewritten SHA-1 object with no
> > SHA-256 signature at all.
> 
> How widely are SHA-256 tags in use in the real world, though?  Is it
> really too late to fix that already?

I don't know.  I don't know of any major hosting platform that supports
them, but of course many people may be using them independently on
self-hosted instances.

I don't think it matters one way or the other, honestly, because the
functionality is the same either way, whether we always put SHA-256 in a
header or whether we put the non-default algorithm in the header.
Multiply signed commits and tags are still unverifiable on older
versions because the older versions consider the header to be part of
the payload and not something to be stripped.

I just noticed this because I'm now getting to the case where we write
(and sign) both SHA-1 and SHA-256 versions of commits and I thought I'd
better send in a patch sooner rather than later, since there's a lot
more prep work (surprise) before we get to anything interesting.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 0/5] Support for commits signed by multiple algorithms
  2021-01-11 23:29     ` brian m. carlson
@ 2021-01-12  2:03       ` Junio C Hamano
  2021-01-12  2:24         ` brian m. carlson
  0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-01-12  2:03 UTC (permalink / raw)
  To: brian m. carlson; +Cc: git, Eric Sunshine, Denton Liu, Jeff King

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I just noticed this because I'm now getting to the case where we write
> (and sign) both SHA-1 and SHA-256 versions of commits and I thought I'd
> better send in a patch sooner rather than later, since there's a lot
> more prep work (surprise) before we get to anything interesting.

Uncomfortably excited to hear this ;-)

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 0/5] Support for commits signed by multiple algorithms
  2021-01-12  2:03       ` Junio C Hamano
@ 2021-01-12  2:24         ` brian m. carlson
  0 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-12  2:24 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Eric Sunshine, Denton Liu, Jeff King

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

On 2021-01-12 at 02:03:08, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > I just noticed this because I'm now getting to the case where we write
> > (and sign) both SHA-1 and SHA-256 versions of commits and I thought I'd
> > better send in a patch sooner rather than later, since there's a lot
> > more prep work (surprise) before we get to anything interesting.
> 
> Uncomfortably excited to hear this ;-)

Here's a brief summary of what's ahead:

* struct object_id is going to have an algorithm member.
* Consequently, there will be per-algorithm null OIDs.
* To efficiently compare and copy OIDs of all sizes (notably in khash
  tables, where things otherwise become tricky), we'll zero-pad SHA-1
  OIDs and always compare the full buffer.
* For all of these reasons, oidread (or similar) will become practically
  required.
* Objects will be mapped into the loose object store when written, and
  each type of object will have a function to convert it between formats
  if required.
* The testsuite will learn a mode not to stuff invalid OIDs into things,
  since those will no longer work (because those OIDs can't be mapped
  and so we can't create SHA-1 versions of them).  That will necessitate
  another large set of prerequisite additions in the testsuite.

In progress work can be seen on the transition-interop branch at
https://github.com/bk2204/git.git.  I should point out that it is very
in progress; the tip will definitely fail the testsuite in certain
cases.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/5] gpg-interface: improve interface for parsing tags
  2021-01-11  0:37 ` [PATCH 2/5] gpg-interface: improve interface for parsing tags brian m. carlson
@ 2021-01-12  4:58   ` Junio C Hamano
  2021-01-14 23:18     ` brian m. carlson
  0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-01-12  4:58 UTC (permalink / raw)
  To: brian m. carlson; +Cc: git, Eric Sunshine, Denton Liu, Jeff King

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> diff --git a/ref-filter.c b/ref-filter.c
> index aa260bfd09..8d8baec1b5 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1215,7 +1215,13 @@ static void find_subpos(const char *buf,
>  			unsigned long *nonsiglen,
>  			const char **sig, unsigned long *siglen)
>  {
> +	struct strbuf payload = STRBUF_INIT;
> +	struct strbuf signature = STRBUF_INIT;
>  	const char *eol;
> +	const char *end = buf + strlen(buf);
> +	const char *sigstart;
> +
> +
>  	/* skip past header until we hit empty line */
>  	while (*buf && *buf != '\n') {
>  		eol = strchrnul(buf, '\n');
> @@ -1228,14 +1234,15 @@ static void find_subpos(const char *buf,
>  		buf++;
>  
>  	/* parse signature first; we might not even have a subject line */
> -	*sig = buf + parse_signature(buf, strlen(buf));
> -	*siglen = strlen(*sig);
> +	parse_signature(buf, end - buf, &payload, &signature);
> +	*sig = strbuf_detach(&signature, siglen);

"unsigned long *siglen" may not be the same as "size_t *siglen", and
the latter is what strbuf_detach() expects to see.  This breaks
32-bit builds e.g. [*1*].

I suspect that all these ${foo}len, including the parameter to this
function but also the four local variables in its sole caller, would
want to become size_t.

Thanks.


[Reference]

*1* https://github.com/git/git/runs/1685453231?check_suite_focus=true#step:5:519

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/5] gpg-interface: improve interface for parsing tags
  2021-01-11  3:58   ` [PATCH v2 2/5] gpg-interface: improve interface for parsing tags brian m. carlson
@ 2021-01-12  5:24     ` Junio C Hamano
  0 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-01-12  5:24 UTC (permalink / raw)
  To: brian m. carlson; +Cc: git, Eric Sunshine, Denton Liu, Jeff King

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> diff --git a/ref-filter.c b/ref-filter.c
> index aa260bfd09..8d8baec1b5 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1215,7 +1215,13 @@ static void find_subpos(const char *buf,
>  			unsigned long *nonsiglen,
>  			const char **sig, unsigned long *siglen)
>  {

I think a preliminary clean-up should look like this.

Tonight's tip of 'seen' contains this at the tip of your topic.

  https://github.com/git/git/actions/runs/479421349

Thanks.

diff --git a/ref-filter.c b/ref-filter.c
index 32ed4d5111..e6c8106377 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1210,10 +1210,10 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
 }
 
 static void find_subpos(const char *buf,
-			const char **sub, unsigned long *sublen,
-			const char **body, unsigned long *bodylen,
-			unsigned long *nonsiglen,
-			const char **sig, unsigned long *siglen)
+			const char **sub, size_t *sublen,
+			const char **body, size_t *bodylen,
+			size_t *nonsiglen,
+			const char **sig, size_t *siglen)
 {
 	struct strbuf payload = STRBUF_INIT;
 	struct strbuf signature = STRBUF_INIT;
@@ -1291,7 +1291,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 {
 	int i;
 	const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;
-	unsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;
+	size_t sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct used_atom *atom = &used_atom[i];

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/5] commit: ignore additional signatures when parsing signed commits
  2021-01-11  3:58   ` [PATCH v2 1/5] commit: ignore additional signatures when parsing signed commits brian m. carlson
@ 2021-01-12 17:03     ` SZEDER Gábor
  0 siblings, 0 replies; 31+ messages in thread
From: SZEDER Gábor @ 2021-01-12 17:03 UTC (permalink / raw)
  To: brian m. carlson; +Cc: git, Eric Sunshine, Denton Liu, Jeff King

On Mon, Jan 11, 2021 at 03:58:36AM +0000, brian m. carlson wrote:
> diff --git a/commit.c b/commit.c
> index f128f18a9b..93faaad764 100644
> --- a/commit.c
> +++ b/commit.c

> @@ -1082,23 +1087,27 @@ 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] = { 0 }, *sigp = &sigs[0];

Various Clang versions issue the following warning about this
initialization:

    CC commit.o
commit.c:1105:16: error: suggest braces around initialization of subobject
      [-Werror,-Wmissing-braces]
        } sigs[2] = { 0 }, *sigp = &sigs[0];
                      ^
                      {}
1 error generated.


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/5] gpg-interface: improve interface for parsing tags
  2021-01-12  4:58   ` Junio C Hamano
@ 2021-01-14 23:18     ` brian m. carlson
  2021-01-15  1:47       ` Junio C Hamano
  0 siblings, 1 reply; 31+ messages in thread
From: brian m. carlson @ 2021-01-14 23:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Eric Sunshine, Denton Liu, Jeff King

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

On 2021-01-12 at 04:58:57, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > diff --git a/ref-filter.c b/ref-filter.c
> > index aa260bfd09..8d8baec1b5 100644
> > --- a/ref-filter.c
> > +++ b/ref-filter.c
> > @@ -1215,7 +1215,13 @@ static void find_subpos(const char *buf,
> >  			unsigned long *nonsiglen,
> >  			const char **sig, unsigned long *siglen)
> >  {
> > +	struct strbuf payload = STRBUF_INIT;
> > +	struct strbuf signature = STRBUF_INIT;
> >  	const char *eol;
> > +	const char *end = buf + strlen(buf);
> > +	const char *sigstart;
> > +
> > +
> >  	/* skip past header until we hit empty line */
> >  	while (*buf && *buf != '\n') {
> >  		eol = strchrnul(buf, '\n');
> > @@ -1228,14 +1234,15 @@ static void find_subpos(const char *buf,
> >  		buf++;
> >  
> >  	/* parse signature first; we might not even have a subject line */
> > -	*sig = buf + parse_signature(buf, strlen(buf));
> > -	*siglen = strlen(*sig);
> > +	parse_signature(buf, end - buf, &payload, &signature);
> > +	*sig = strbuf_detach(&signature, siglen);
> 
> "unsigned long *siglen" may not be the same as "size_t *siglen", and
> the latter is what strbuf_detach() expects to see.  This breaks
> 32-bit builds e.g. [*1*].
> 
> I suspect that all these ${foo}len, including the parameter to this
> function but also the four local variables in its sole caller, would
> want to become size_t.
> 
> Thanks.

I'll reroll with the fixes for size_t and unsigned long.  Feel free to
drop the series if it's causing CI to fail until I get around to that
(likely this weekend).
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/5] gpg-interface: improve interface for parsing tags
  2021-01-14 23:18     ` brian m. carlson
@ 2021-01-15  1:47       ` Junio C Hamano
  0 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-01-15  1:47 UTC (permalink / raw)
  To: brian m. carlson; +Cc: git, Eric Sunshine, Denton Liu, Jeff King

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I'll reroll with the fixes for size_t and unsigned long.  Feel free to
> drop the series if it's causing CI to fail until I get around to that
> (likely this weekend).

No worries.  You would have found my tentative fix-up applied in
'seen' already if you updated from me when you got the message you
are responding to.

Thanks.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 0/6] Support for commits signed by multiple algorithms
  2021-01-11  3:58 ` [PATCH v2 0/5] Support for commits signed by multiple algorithms brian m. carlson
                     ` (5 preceding siblings ...)
  2021-01-11 22:16   ` [PATCH v2 0/5] Support for commits signed by multiple algorithms Junio C Hamano
@ 2021-01-18 23:49   ` brian m. carlson
  2021-01-18 23:49     ` [PATCH v3 1/6] ref-filter: switch some uses of unsigned long to size_t brian m. carlson
                       ` (5 more replies)
  6 siblings, 6 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-18 23:49 UTC (permalink / raw)
  To: git; +Cc: SZEDER Gábor

This series introduces support for verifying commits and tags signed by
multiple algorithms.

These commits allow for signing commits and tags that can be
round-tripped through both SHA-1 and SHA-256.  We verify only the
signature using the current hash algorithm, since we currently don't
rewrite objects.

Changes from v2:
* Fix problems with size_t and unsigned long on 32-bit systems by
  creating a cleanup patch at the beginning of the series.
* Fix a clang warning with initialization by using memset.

Changes from v1:
* Fix brown paper bag bug where some tests failed due to a bad rebase.

brian m. carlson (6):
  ref-filter: switch some uses of unsigned long to size_t
  commit: ignore additional signatures when parsing signed commits
  gpg-interface: improve interface for parsing tags
  commit: allow parsing arbitrary buffers with headers
  ref-filter: hoist signature parsing
  gpg-interface: remove other signature headers before verifying

 builtin/receive-pack.c   |  4 +-
 builtin/tag.c            | 16 ++++++--
 commit.c                 | 85 +++++++++++++++++++++++++++-------------
 commit.h                 | 12 +++++-
 fmt-merge-msg.c          | 29 ++++++++------
 gpg-interface.c          | 15 ++++++-
 gpg-interface.h          |  9 ++++-
 log-tree.c               | 15 +++----
 ref-filter.c             | 33 ++++++++++------
 t/t7004-tag.sh           | 25 ++++++++++++
 t/t7510-signed-commit.sh | 43 +++++++++++++++++++-
 tag.c                    | 15 +++----
 12 files changed, 226 insertions(+), 75 deletions(-)


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 1/6] ref-filter: switch some uses of unsigned long to size_t
  2021-01-18 23:49   ` [PATCH v3 0/6] " brian m. carlson
@ 2021-01-18 23:49     ` brian m. carlson
  2021-01-18 23:49     ` [PATCH v3 2/6] commit: ignore additional signatures when parsing signed commits brian m. carlson
                       ` (4 subsequent siblings)
  5 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-18 23:49 UTC (permalink / raw)
  To: git; +Cc: SZEDER Gábor

In the future, we'll want to pass some of the arguments of find_subpos
to strbuf_detach, which takes a size_t.  This is fine on systems where
that's the same size as unsigned long, but that isn't the case on all
systems.  Moreover, size_t makes sense since it's not possible to use a
buffer here that's larger than memory anyway.

Let's switch each use to size_t for these lengths in
grab_sub_body_contents and find_subpos.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 ref-filter.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index aa260bfd09..606f638ab1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1210,10 +1210,10 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
 }
 
 static void find_subpos(const char *buf,
-			const char **sub, unsigned long *sublen,
-			const char **body, unsigned long *bodylen,
-			unsigned long *nonsiglen,
-			const char **sig, unsigned long *siglen)
+			const char **sub, size_t *sublen,
+			const char **body, size_t *bodylen,
+			size_t *nonsiglen,
+			const char **sig, size_t *siglen)
 {
 	const char *eol;
 	/* skip past header until we hit empty line */
@@ -1285,7 +1285,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 {
 	int i;
 	const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;
-	unsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;
+	size_t sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct used_atom *atom = &used_atom[i];

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 2/6] commit: ignore additional signatures when parsing signed commits
  2021-01-18 23:49   ` [PATCH v3 0/6] " brian m. carlson
  2021-01-18 23:49     ` [PATCH v3 1/6] ref-filter: switch some uses of unsigned long to size_t brian m. carlson
@ 2021-01-18 23:49     ` brian m. carlson
  2021-01-18 23:49     ` [PATCH v3 3/6] gpg-interface: improve interface for parsing tags brian m. carlson
                       ` (3 subsequent siblings)
  5 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-18 23:49 UTC (permalink / raw)
  To: git; +Cc: SZEDER Gábor

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 3/6] gpg-interface: improve interface for parsing tags
  2021-01-18 23:49   ` [PATCH v3 0/6] " brian m. carlson
  2021-01-18 23:49     ` [PATCH v3 1/6] ref-filter: switch some uses of unsigned long to size_t brian m. carlson
  2021-01-18 23:49     ` [PATCH v3 2/6] commit: ignore additional signatures when parsing signed commits brian m. carlson
@ 2021-01-18 23:49     ` brian m. carlson
  2021-01-18 23:49     ` [PATCH v3 4/6] commit: allow parsing arbitrary buffers with headers brian m. carlson
                       ` (2 subsequent siblings)
  5 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-18 23:49 UTC (permalink / raw)
  To: git; +Cc: SZEDER Gábor

We have a function which parses a buffer with a signature at the end,
parse_signature, and this function is used for signed tags.  However,
we'll need to store values for multiple algorithms, and we'll do this by
using a header for the non-default algorithm.

Adjust the parse_signature interface to store the parsed data in two
strbufs and turn the existing function into parse_signed_buffer.  The
latter is still used in places where we know we always have a signed
buffer, such as push certs.

Adjust all the callers to deal with this new interface.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/receive-pack.c |  4 ++--
 builtin/tag.c          | 16 ++++++++++++----
 commit.c               |  9 ++++++---
 fmt-merge-msg.c        | 29 ++++++++++++++++++-----------
 gpg-interface.c        | 13 ++++++++++++-
 gpg-interface.h        |  9 ++++++++-
 log-tree.c             | 13 +++++++------
 ref-filter.c           | 18 ++++++++++++++----
 tag.c                  | 15 ++++++++-------
 9 files changed, 87 insertions(+), 39 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d49d050e6e..b89ce31bf2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -764,7 +764,7 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 
 		memset(&sigcheck, '\0', sizeof(sigcheck));
 
-		bogs = parse_signature(push_cert.buf, push_cert.len);
+		bogs = parse_signed_buffer(push_cert.buf, push_cert.len);
 		check_signature(push_cert.buf, bogs, push_cert.buf + bogs,
 				push_cert.len - bogs, &sigcheck);
 
@@ -2050,7 +2050,7 @@ static void queue_commands_from_cert(struct command **tail,
 		die("malformed push certificate %.*s", 100, push_cert->buf);
 	else
 		boc += 2;
-	eoc = push_cert->buf + parse_signature(push_cert->buf, push_cert->len);
+	eoc = push_cert->buf + parse_signed_buffer(push_cert->buf, push_cert->len);
 
 	while (boc < eoc) {
 		const char *eol = memchr(boc, '\n', eoc - boc);
diff --git a/builtin/tag.c b/builtin/tag.c
index ecf011776d..7162f4ccc5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -174,11 +174,17 @@ static void write_tag_body(int fd, const struct object_id *oid)
 {
 	unsigned long size;
 	enum object_type type;
-	char *buf, *sp;
+	char *buf, *sp, *orig;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 
-	buf = read_object_file(oid, &type, &size);
+	orig = buf = read_object_file(oid, &type, &size);
 	if (!buf)
 		return;
+	if (parse_signature(buf, size, &payload, &signature)) {
+		buf = payload.buf;
+		size = payload.len;
+	}
 	/* skip header */
 	sp = strstr(buf, "\n\n");
 
@@ -187,9 +193,11 @@ static void write_tag_body(int fd, const struct object_id *oid)
 		return;
 	}
 	sp += 2; /* skip the 2 LFs */
-	write_or_die(fd, sp, parse_signature(sp, buf + size - sp));
+	write_or_die(fd, sp, buf + size - sp);
 
-	free(buf);
+	free(orig);
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 }
 
 static int build_tag_object(struct strbuf *buf, int sign, struct object_id *result)
diff --git a/commit.c b/commit.c
index 1006c85ca8..ccb912b9b5 100644
--- a/commit.c
+++ b/commit.c
@@ -1136,8 +1136,10 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 	struct merge_remote_desc *desc;
 	struct commit_extra_header *mergetag;
 	char *buf;
-	unsigned long size, len;
+	unsigned long size;
 	enum object_type type;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 
 	desc = merge_remote_util(parent);
 	if (!desc || !desc->obj)
@@ -1145,8 +1147,7 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 	buf = read_object_file(&desc->obj->oid, &type, &size);
 	if (!buf || type != OBJ_TAG)
 		goto free_return;
-	len = parse_signature(buf, size);
-	if (size == len)
+	if (!parse_signature(buf, size, &payload, &signature))
 		goto free_return;
 	/*
 	 * We could verify this signature and either omit the tag when
@@ -1165,6 +1166,8 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 
 	**tail = mergetag;
 	*tail = &mergetag->next;
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 	return;
 
 free_return:
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 9a664a4a58..7fd99f0ac1 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -509,22 +509,28 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 	for (i = 0; i < origins.nr; i++) {
 		struct object_id *oid = origins.items[i].util;
 		enum object_type type;
-		unsigned long size, len;
+		unsigned long size;
 		char *buf = read_object_file(oid, &type, &size);
+		char *origbuf = buf;
+		unsigned long len = size;
 		struct signature_check sigc = { NULL };
-		struct strbuf sig = STRBUF_INIT;
+		struct strbuf payload = STRBUF_INIT, sig = STRBUF_INIT;
 
 		if (!buf || type != OBJ_TAG)
 			goto next;
-		len = parse_signature(buf, size);
 
-		if (size == len)
-			; /* merely annotated */
-		else if (check_signature(buf, len, buf + len, size - len, &sigc) &&
-			!sigc.gpg_output)
-			strbuf_addstr(&sig, "gpg verification failed.\n");
-		else
-			strbuf_addstr(&sig, sigc.gpg_output);
+		if (!parse_signature(buf, size, &payload, &sig))
+			;/* merely annotated */
+		else {
+			buf = payload.buf;
+			len = payload.len;
+			if (check_signature(payload.buf, payload.len, sig.buf,
+					 sig.len, &sigc) &&
+				!sigc.gpg_output)
+				strbuf_addstr(&sig, "gpg verification failed.\n");
+			else
+				strbuf_addstr(&sig, sigc.gpg_output);
+		}
 		signature_check_clear(&sigc);
 
 		if (!tag_number++) {
@@ -547,9 +553,10 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 					strlen(origins.items[i].string));
 			fmt_tag_signature(&tagbuf, &sig, buf, len);
 		}
+		strbuf_release(&payload);
 		strbuf_release(&sig);
 	next:
-		free(buf);
+		free(origbuf);
 	}
 	if (tagbuf.len) {
 		strbuf_addch(out, '\n');
diff --git a/gpg-interface.c b/gpg-interface.c
index b499270836..c6274c14af 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -345,7 +345,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }
 
-size_t parse_signature(const char *buf, size_t size)
+size_t parse_signed_buffer(const char *buf, size_t size)
 {
 	size_t len = 0;
 	size_t match = size;
@@ -361,6 +361,17 @@ size_t parse_signature(const char *buf, size_t size)
 	return match;
 }
 
+int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct strbuf *signature)
+{
+	size_t match = parse_signed_buffer(buf, size);
+	if (match != size) {
+		strbuf_add(payload, buf, match);
+		strbuf_add(signature, buf + match, size - match);
+		return 1;
+	}
+	return 0;
+}
+
 void set_signing_key(const char *key)
 {
 	free(configured_signing_key);
diff --git a/gpg-interface.h b/gpg-interface.h
index f4e9b4f371..80567e4894 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -37,13 +37,20 @@ struct signature_check {
 
 void signature_check_clear(struct signature_check *sigc);
 
+/*
+ * Look at a GPG signed tag object.  If such a signature exists, store it in
+ * signature and the signed content in payload.  Return 1 if a signature was
+ * found, and 0 otherwise.
+ */
+int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct strbuf *signature);
+
 /*
  * Look at GPG signed content (e.g. a signed tag object), whose
  * payload is followed by a detached signature on it.  Return the
  * offset where the embedded detached signature begins, or the end of
  * the data when there is no such signature.
  */
-size_t parse_signature(const char *buf, size_t size);
+size_t parse_signed_buffer(const char *buf, size_t size);
 
 /*
  * Create a detached signature for the contents of "buffer" and append
diff --git a/log-tree.c b/log-tree.c
index 7e0335e548..b025c8da93 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -548,7 +548,8 @@ static int show_one_mergetag(struct commit *commit,
 	struct strbuf verify_message;
 	struct signature_check sigc = { 0 };
 	int status, nth;
-	size_t payload_size;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 
 	hash_object_file(the_hash_algo, extra->value, extra->len,
 			 type_name(OBJ_TAG), &oid);
@@ -571,13 +572,11 @@ static int show_one_mergetag(struct commit *commit,
 		strbuf_addf(&verify_message,
 			    "parent #%d, tagged '%s'\n", nth + 1, tag->tag);
 
-	payload_size = parse_signature(extra->value, extra->len);
 	status = -1;
-	if (extra->len > payload_size) {
+	if (parse_signature(extra->value, extra->len, &payload, &signature)) {
 		/* could have a good signature */
-		status = check_signature(extra->value, payload_size,
-					 extra->value + payload_size,
-					 extra->len - payload_size, &sigc);
+		status = check_signature(payload.buf, payload.len,
+					 signature.buf, signature.len, &sigc);
 		if (sigc.gpg_output)
 			strbuf_addstr(&verify_message, sigc.gpg_output);
 		else
@@ -588,6 +587,8 @@ static int show_one_mergetag(struct commit *commit,
 
 	show_sig_lines(opt, status, verify_message.buf);
 	strbuf_release(&verify_message);
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 	return 0;
 }
 
diff --git a/ref-filter.c b/ref-filter.c
index 606f638ab1..bba4877745 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1215,7 +1215,13 @@ static void find_subpos(const char *buf,
 			size_t *nonsiglen,
 			const char **sig, size_t *siglen)
 {
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 	const char *eol;
+	const char *end = buf + strlen(buf);
+	const char *sigstart;
+
+
 	/* skip past header until we hit empty line */
 	while (*buf && *buf != '\n') {
 		eol = strchrnul(buf, '\n');
@@ -1228,14 +1234,15 @@ static void find_subpos(const char *buf,
 		buf++;
 
 	/* parse signature first; we might not even have a subject line */
-	*sig = buf + parse_signature(buf, strlen(buf));
-	*siglen = strlen(*sig);
+	parse_signature(buf, end - buf, &payload, &signature);
+	*sig = strbuf_detach(&signature, siglen);
+	sigstart = buf + parse_signed_buffer(buf, strlen(buf));
 
 	/* subject is first non-empty line */
 	*sub = buf;
 	/* subject goes to first empty line before signature begins */
 	if ((eol = strstr(*sub, "\n\n"))) {
-		eol = eol < *sig ? eol : *sig;
+		eol = eol < sigstart ? eol : sigstart;
 	/* check if message uses CRLF */
 	} else if (! (eol = strstr(*sub, "\r\n\r\n"))) {
 		/* treat whole message as subject */
@@ -1253,7 +1260,7 @@ static void find_subpos(const char *buf,
 		buf++;
 	*body = buf;
 	*bodylen = strlen(buf);
-	*nonsiglen = *sig - buf;
+	*nonsiglen = sigstart - buf;
 }
 
 /*
@@ -1291,6 +1298,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 		struct used_atom *atom = &used_atom[i];
 		const char *name = atom->name;
 		struct atom_value *v = &val[i];
+
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
@@ -1336,6 +1344,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 			v->s = strbuf_detach(&s, NULL);
 		} else if (atom->u.contents.option == C_BARE)
 			v->s = xstrdup(subpos);
+
+		free((void *)sigpos);
 	}
 }
 
diff --git a/tag.c b/tag.c
index 1ed2684e45..3e18a41841 100644
--- a/tag.c
+++ b/tag.c
@@ -13,26 +13,27 @@ const char *tag_type = "tag";
 static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 {
 	struct signature_check sigc;
-	size_t payload_size;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 	int ret;
 
 	memset(&sigc, 0, sizeof(sigc));
 
-	payload_size = parse_signature(buf, size);
-
-	if (size == payload_size) {
+	if (!parse_signature(buf, size, &payload, &signature)) {
 		if (flags & GPG_VERIFY_VERBOSE)
-			write_in_full(1, buf, payload_size);
+			write_in_full(1, buf, size);
 		return error("no signature found");
 	}
 
-	ret = check_signature(buf, payload_size, buf + payload_size,
-				size - payload_size, &sigc);
+	ret = check_signature(payload.buf, payload.len, signature.buf,
+				signature.len, &sigc);
 
 	if (!(flags & GPG_VERIFY_OMIT_STATUS))
 		print_signature_buffer(&sigc, flags);
 
 	signature_check_clear(&sigc);
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 	return ret;
 }
 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 4/6] commit: allow parsing arbitrary buffers with headers
  2021-01-18 23:49   ` [PATCH v3 0/6] " brian m. carlson
                       ` (2 preceding siblings ...)
  2021-01-18 23:49     ` [PATCH v3 3/6] gpg-interface: improve interface for parsing tags brian m. carlson
@ 2021-01-18 23:49     ` brian m. carlson
  2021-01-18 23:49     ` [PATCH v3 5/6] ref-filter: hoist signature parsing brian m. carlson
  2021-01-18 23:49     ` [PATCH v3 6/6] gpg-interface: remove other signature headers before verifying brian m. carlson
  5 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-18 23:49 UTC (permalink / raw)
  To: git; +Cc: SZEDER Gábor

Currently only commits are signed with headers.  However, in the future,
we'll also sign tags with headers as well.  Let's refactor out a
function called parse_buffer_signed_by_header which does exactly that.
In addition, since we'll want to sign things other than commits this
way, let's call the function sign_with_header instead of do_sign_commit.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 commit.c | 20 ++++++++++++++++----
 commit.h |  9 +++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/commit.c b/commit.c
index ccb912b9b5..2c57b6ded9 100644
--- a/commit.c
+++ b/commit.c
@@ -995,7 +995,7 @@ static const char *gpg_sig_headers[] = {
 	"gpgsig-sha256",
 };
 
-static int do_sign_commit(struct strbuf *buf, const char *keyid)
+int sign_with_header(struct strbuf *buf, const char *keyid)
 {
 	struct strbuf sig = STRBUF_INIT;
 	int inspos, copypos;
@@ -1035,13 +1035,26 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)
 	return 0;
 }
 
+
+
 int parse_signed_commit(const struct commit *commit,
 			struct strbuf *payload, struct strbuf *signature,
 			const struct git_hash_algo *algop)
 {
-
 	unsigned long size;
 	const char *buffer = get_commit_buffer(commit, &size);
+	int ret = parse_buffer_signed_by_header(buffer, size, payload, signature, algop);
+
+	unuse_commit_buffer(commit, buffer);
+	return ret;
+}
+
+int parse_buffer_signed_by_header(const char *buffer,
+				  unsigned long size,
+				  struct strbuf *payload,
+				  struct strbuf *signature,
+				  const struct git_hash_algo *algop)
+{
 	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)];
@@ -1078,7 +1091,6 @@ int parse_signed_commit(const struct commit *commit,
 		}
 		line = next;
 	}
-	unuse_commit_buffer(commit, buffer);
 	return saw_signature;
 }
 
@@ -1532,7 +1544,7 @@ int commit_tree_extended(const char *msg, size_t msg_len,
 	if (encoding_is_utf8 && !verify_utf8(&buffer))
 		fprintf(stderr, _(commit_utf8_warn));
 
-	if (sign_commit && do_sign_commit(&buffer, sign_commit)) {
+	if (sign_commit && sign_with_header(&buffer, sign_commit)) {
 		result = -1;
 		goto out;
 	}
diff --git a/commit.h b/commit.h
index 030aa65ab8..e2856ce8ef 100644
--- a/commit.h
+++ b/commit.h
@@ -360,4 +360,13 @@ int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void
 LAST_ARG_MUST_BE_NULL
 int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
 
+/* Sign a commit or tag buffer, storing the result in a header. */
+int sign_with_header(struct strbuf *buf, const char *keyid);
+/* Parse the signature out of a header. */
+int parse_buffer_signed_by_header(const char *buffer,
+				  unsigned long size,
+				  struct strbuf *payload,
+				  struct strbuf *signature,
+				  const struct git_hash_algo *algop);
+
 #endif /* COMMIT_H */

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 5/6] ref-filter: hoist signature parsing
  2021-01-18 23:49   ` [PATCH v3 0/6] " brian m. carlson
                       ` (3 preceding siblings ...)
  2021-01-18 23:49     ` [PATCH v3 4/6] commit: allow parsing arbitrary buffers with headers brian m. carlson
@ 2021-01-18 23:49     ` brian m. carlson
  2021-01-18 23:49     ` [PATCH v3 6/6] gpg-interface: remove other signature headers before verifying brian m. carlson
  5 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-18 23:49 UTC (permalink / raw)
  To: git; +Cc: SZEDER Gábor

When we parse a signature in the ref-filter code, we continually
increment the buffer pointer.  Hoist the signature parsing above the
blank line delimiting headers and body so we can find the signature when
using a header to sign the buffer.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 ref-filter.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index bba4877745..e6c8106377 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1221,6 +1221,8 @@ static void find_subpos(const char *buf,
 	const char *end = buf + strlen(buf);
 	const char *sigstart;
 
+	/* parse signature first; we might not even have a subject line */
+	parse_signature(buf, end - buf, &payload, &signature);
 
 	/* skip past header until we hit empty line */
 	while (*buf && *buf != '\n') {
@@ -1232,9 +1234,6 @@ static void find_subpos(const char *buf,
 	/* skip any empty lines */
 	while (*buf == '\n')
 		buf++;
-
-	/* parse signature first; we might not even have a subject line */
-	parse_signature(buf, end - buf, &payload, &signature);
 	*sig = strbuf_detach(&signature, siglen);
 	sigstart = buf + parse_signed_buffer(buf, strlen(buf));
 
@@ -1330,7 +1329,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 			v->s = xmemdupz(sigpos, siglen);
 		else if (atom->u.contents.option == C_LINES) {
 			struct strbuf s = STRBUF_INIT;
-			const char *contents_end = bodylen + bodypos - siglen;
+			const char *contents_end = bodypos + nonsiglen;
 
 			/*  Size is the length of the message after removing the signature */
 			append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines);

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 6/6] gpg-interface: remove other signature headers before verifying
  2021-01-18 23:49   ` [PATCH v3 0/6] " brian m. carlson
                       ` (4 preceding siblings ...)
  2021-01-18 23:49     ` [PATCH v3 5/6] ref-filter: hoist signature parsing brian m. carlson
@ 2021-01-18 23:49     ` brian m. carlson
  5 siblings, 0 replies; 31+ messages in thread
From: brian m. carlson @ 2021-01-18 23:49 UTC (permalink / raw)
  To: git; +Cc: SZEDER Gábor

When we have a multiply signed commit, we need to remove the signature
in the header before verifying the object, since the trailing signature
will not be over both pieces of data.  Do so, and verify that we
validate the signature appropriately.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 gpg-interface.c |  2 ++
 t/t7004-tag.sh  | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/gpg-interface.c b/gpg-interface.c
index c6274c14af..127aecfc2b 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "commit.h"
 #include "config.h"
 #include "run-command.h"
 #include "strbuf.h"
@@ -366,6 +367,7 @@ int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct
 	size_t match = parse_signed_buffer(buf, size);
 	if (match != size) {
 		strbuf_add(payload, buf, match);
+		remove_signature(payload);
 		strbuf_add(signature, buf + match, size - match);
 		return 1;
 	}
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 05f411c821..6fb4e3cf11 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -17,6 +17,13 @@ tag_exists () {
 	git show-ref --quiet --verify refs/tags/"$1"
 }
 
+test_expect_success 'setup' '
+	test_oid_cache <<-EOM
+	othersigheader sha1:gpgsig-sha256
+	othersigheader sha256:gpgsig
+	EOM
+'
+
 test_expect_success 'listing all tags in an empty tree should succeed' '
 	git tag -l &&
 	git tag
@@ -1371,6 +1378,24 @@ test_expect_success GPG \
 	'test_config gpg.program echo &&
 	 test_must_fail git tag -s -m tail tag-gpg-failure'
 
+# try to produce invalid signature
+test_expect_success GPG 'git verifies tag is valid with double signature' '
+	git tag -s -m tail tag-gpg-double-sig &&
+	git cat-file tag tag-gpg-double-sig >tag &&
+	othersigheader=$(test_oid othersigheader) &&
+	sed -ne "/^\$/q;p" tag >new-tag &&
+	cat <<-EOM >>new-tag &&
+	$othersigheader -----BEGIN PGP SIGNATURE-----
+	 someinvaliddata
+	 -----END PGP SIGNATURE-----
+	EOM
+	sed -e "1,/^tagger/d" tag >>new-tag &&
+	new_tag=$(git hash-object -t tag -w new-tag) &&
+	git update-ref refs/tags/tag-gpg-double-sig $new_tag &&
+	git verify-tag tag-gpg-double-sig &&
+	git fsck
+'
+
 # try to sign with bad user.signingkey
 test_expect_success GPGSM \
 	'git tag -s fails if gpgsm is misconfigured (bad key)' \

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2021-01-18 23:53 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-11  0:37 [PATCH 0/5] Support for commits signed by multiple algorithms brian m. carlson
2021-01-11  0:37 ` [PATCH 1/5] commit: ignore additional signatures when parsing signed commits brian m. carlson
2021-01-11  0:37 ` [PATCH 2/5] gpg-interface: improve interface for parsing tags brian m. carlson
2021-01-12  4:58   ` Junio C Hamano
2021-01-14 23:18     ` brian m. carlson
2021-01-15  1:47       ` Junio C Hamano
2021-01-11  0:37 ` [PATCH 3/5] commit: allow parsing arbitrary buffers with headers brian m. carlson
2021-01-11  0:37 ` [PATCH 4/5] ref-filter: hoist signature parsing brian m. carlson
2021-01-11  0:37 ` [PATCH 5/6] fixup! commit: ignore additional signatures when parsing signed commits brian m. carlson
2021-01-11  0:43   ` brian m. carlson
2021-01-11  0:37 ` [PATCH 5/5] gpg-interface: remove other signature headers before verifying brian m. carlson
2021-01-11  0:37 ` [PATCH 6/6] " brian m. carlson
2021-01-11  3:58 ` [PATCH v2 0/5] Support for commits signed by multiple algorithms brian m. carlson
2021-01-11  3:58   ` [PATCH v2 1/5] commit: ignore additional signatures when parsing signed commits brian m. carlson
2021-01-12 17:03     ` SZEDER Gábor
2021-01-11  3:58   ` [PATCH v2 2/5] gpg-interface: improve interface for parsing tags brian m. carlson
2021-01-12  5:24     ` Junio C Hamano
2021-01-11  3:58   ` [PATCH v2 3/5] commit: allow parsing arbitrary buffers with headers brian m. carlson
2021-01-11  3:58   ` [PATCH v2 4/5] ref-filter: hoist signature parsing brian m. carlson
2021-01-11  3:58   ` [PATCH v2 5/5] gpg-interface: remove other signature headers before verifying brian m. carlson
2021-01-11 22:16   ` [PATCH v2 0/5] Support for commits signed by multiple algorithms Junio C Hamano
2021-01-11 23:29     ` brian m. carlson
2021-01-12  2:03       ` Junio C Hamano
2021-01-12  2:24         ` brian m. carlson
2021-01-18 23:49   ` [PATCH v3 0/6] " brian m. carlson
2021-01-18 23:49     ` [PATCH v3 1/6] ref-filter: switch some uses of unsigned long to size_t brian m. carlson
2021-01-18 23:49     ` [PATCH v3 2/6] commit: ignore additional signatures when parsing signed commits brian m. carlson
2021-01-18 23:49     ` [PATCH v3 3/6] gpg-interface: improve interface for parsing tags brian m. carlson
2021-01-18 23:49     ` [PATCH v3 4/6] commit: allow parsing arbitrary buffers with headers brian m. carlson
2021-01-18 23:49     ` [PATCH v3 5/6] ref-filter: hoist signature parsing brian m. carlson
2021-01-18 23:49     ` [PATCH v3 6/6] gpg-interface: remove other signature headers before verifying brian m. carlson

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git