gpg-interface: make signature-prefix matching length-aware
After merging v2.56.0-rc0 into Git for Windows, its Coverity run reported the following issue: The `parse_signed_buffer()` function accepts object buffers with an explicit size, while `get_format_by_sig()` uses `starts_with()`, i.e. it expects a NUL-terminated buffer. A tag object with a non-NUL-terminated payload ending in a partial signature prefix, such as a final '-' byte, could therefore cause an invalid read past the object buffer. The observable consequences are limited to reading past the allocation. In practice it can crash Git if the read enters an unmapped page. It can also misplace the payload/signature split, corrupting the compat-hash object being written. The older unbounded matcher predates this path, butc8762c30df(object-file-convert: convert tag objects when writing, 2023-10-01) exposed the defect by passing exact-sized converted tag buffers to `parse_signed_buffer()`. That commit first shipped in v2.45.0, so the defect has been latent in every release since. This pattern was noticed on the mailing list in February 2024. Reviewing a patch for a very similar issue in commit.c's find_header_mem(), Jeff King observed in https://lore.kernel.org/git/20240208214137.GB1090198@coredump.intra.peff.net/: But more interestingly: even though we pass a buf/len pair to parse_signed_buffer(), it then calls get_format_by_sig() which takes only a NUL-terminated string. [...] That raises the question of whether parse_signed_buffer() has a similar walk-too-far problem. ;) The answer is no, because we feed it from a strbuf. But it's not a great pattern overall. That reasoning surveyed the callers that existed at the time and missedc8762c30df(object-file-convert: convert tag objects when writing, 2023-10-01), which was four months old at that time, and does not feed from a strbuf; `convert_tag_object()` hands `parse_signed_buffer()` an exact-sized `xmalloc()` buffer, and the concern flagged and dismissed in that thread is exactly the defect Coverity now reports. Jeff went on to add `starts_with_mem()` a month later, in https://lore.kernel.org/git/20240307092638.GK2080210@coredump.intra.peff.net/, precisely for "cases where the buffer is not NUL-terminated (and we instead have an explicit size or end pointer)", so the tool for this fix has been in the tree since v2.45.0. Even though the issue had been latent, it most likely surfaced via Coverity because of215d305f45(odb: compute compat object ID in `odb_write_object_ext()`, 2026-07-17), which moved `convert_object_file()` out of the `source->write_object` function pointer into a direct call in `odb_write_object_ext()`. Preserve the existing NUL-terminated behavior for callers that provide strings while making signature-prefix matching honor the known buffer lengths, via the `starts_with_mem()` helper. This keeps reads within the object data without implying exploitability beyond the observed invalid read. Assisted-by: GPT-5.6 Luna Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
This commit is contained in:
1 parent
506a21b312
commit
723dd6ca3e
1 file changed
+5
-5
+5
-5
@@ -133,20 +133,20 @@ static struct gpg_format *get_format_by_name(const char *str)
|
||||
return NULL;
|
||||
}
|
||||
|
||||
static struct gpg_format *get_format_by_sig(const char *sig)
|
||||
static struct gpg_format *get_format_by_sig(const char *sig, size_t len)
|
||||
{
|
||||
int j;
|
||||
|
||||
for (size_t i = 0; i < ARRAY_SIZE(gpg_format); i++)
|
||||
for (j = 0; gpg_format[i].sigs[j]; j++)
|
||||
if (starts_with(sig, gpg_format[i].sigs[j]))
|
||||
if (starts_with_mem(sig, len, gpg_format[i].sigs[j]))
|
||||
return gpg_format + i;
|
||||
return NULL;
|
||||
}
|
||||
|
||||
const char *get_signature_format(const char *buf)
|
||||
{
|
||||
struct gpg_format *format = get_format_by_sig(buf);
|
||||
struct gpg_format *format = get_format_by_sig(buf, strlen(buf));
|
||||
return format ? format->name : "unknown";
|
||||
}
|
||||
|
||||
@@ -669,7 +669,7 @@ int check_signature(struct signature_check *sigc,
|
||||
sigc->result = 'N';
|
||||
sigc->trust_level = TRUST_UNDEFINED;
|
||||
|
||||
fmt = get_format_by_sig(signature);
|
||||
fmt = get_format_by_sig(signature, slen);
|
||||
if (!fmt)
|
||||
die(_("bad/incompatible signature '%s'"), signature);
|
||||
|
||||
@@ -706,7 +706,7 @@ size_t parse_signed_buffer(const char *buf, size_t size)
|
||||
while (len < size) {
|
||||
const char *eol;
|
||||
|
||||
if (get_format_by_sig(buf + len))
|
||||
if (get_format_by_sig(buf + len, size - len))
|
||||
match = len;
|
||||
|
||||
eol = memchr(buf + len, '\n', size - len);
|
||||
|
||||
Reference in new issue
Block a user