Jeff King <peff@xxxxxxxx> writes:
> It's not that the commit is bad or the source of problems. My point is
> that the assumption that commit messages are NUL-terminated has been
> there for a really long time, so there are lots of spots in the code
> that sloppily run string functions on them. Every one of those needs to
> be found and fixed (e.g., I remember seeing this in
> for-each-ref.c:find_subpos recently).
>
> It's not impossible, of course, or even really that hard. It's just a
> giant pain, and I wonder if the effort is worth it.
True.
It probably is not worth it for most applications, but this fix-up to a
fairly recent one is worth doing, I would suspect.
-- >8 --
Subject: parse_signed_commit: really use the entire commit log message
... even beyond the first NUL in the buffer, when checking the commit
against the detached signature in the header.
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
commit.c | 11 +++++------
t/t7510-signed-commit.sh | 21 ++++++++++++++++-----
2 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/commit.c b/commit.c
index 93045a2..6ec49fa 100644
--- a/commit.c
+++ b/commit.c
@@ -854,28 +854,27 @@ int parse_signed_commit(const unsigned char *sha1,
unsigned long size;
enum object_type type;
char *buffer = read_sha1_file(sha1, &type, &size);
- int in_header, saw_signature = -1;
+ int saw_signature = -1;
char *line;
if (!buffer || type != OBJ_COMMIT)
goto cleanup;
line = buffer;
- in_header = 1;
saw_signature = 0;
- while (*line) {
+ while (line < buffer + size) {
char *next = strchrnul(line, '\n');
if (*next)
next++;
- if (in_header && !prefixcmp(line, gpg_sig_header)) {
+ if (!prefixcmp(line, gpg_sig_header)) {
const char *sig = line + gpg_sig_header_len;
strbuf_add(signature, sig, next - sig);
saw_signature = 1;
} else {
+ if (*line == '\n')
+ next = buffer + size; /* dump the whole remainder */
strbuf_add(payload, line, next - line);
}
- if (*line == '\n')
- in_header = 0;
line = next;
}
cleanup:
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 5c7475d..30401ce 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -50,11 +50,22 @@ test_expect_success GPG 'show signatures' '
test_expect_success GPG 'detect fudged signature' '
git cat-file commit master >raw &&
- sed -e "s/fourth signed/4th forged/" raw >forged &&
- git hash-object -w -t commit forged >forged.commit &&
- git show --pretty=short --show-signature $(cat forged.commit) >actual &&
- grep "BAD signature from" actual &&
- ! grep "Good signature from" actual
+
+ sed -e "s/fourth signed/4th forged/" raw >forged1 &&
+ git hash-object -w -t commit forged1 >forged1.commit &&
+ git show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&
+ grep "BAD signature from" actual1 &&
+ ! grep "Good signature from" actual1
+'
+
+test_expect_success GPG 'detect fudged signature with NUL' '
+ git cat-file commit master >raw &&
+ cat raw >forged2 &&
+ echo Qwik | tr "Q" "\000" >>forged2 &&
+ git hash-object -w -t commit forged2 >forged2.commit &&
+ git show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&
+ grep "BAD signature from" actual2 &&
+ ! grep "Good signature from" actual2
'
test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html