Re: [PATCH 2/7] gpg-interface: make signature-prefix matching length-aware

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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

Hi Junio,

On Thu, 17 Sep 2026, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > @@ -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);

This hunk is a direct consequence of `get_format_by_sig()` gaining a
length parameter earlier in the same patch: it now has three callers,
`get_signature_format()`, `check_signature()` here, and the loop inside
`parse_signed_buffer()` (the actual site of the bug this series fixes,
Coverity issue with CID 1678690 if you want to double-check).

Once the function takes a (sig, len) pair uniformly, every one of them has
to pass a length, so this call site is not optional scaffolding; it is
what makes all three callers correct by construction instead of leaving
two of them trusting NUL-termination and one bounds-checked.

> >  	if (!fmt)
> >  		die(_("bad/incompatible signature '%s'"), signature);
> 
> All the existing callers of check_signature() pass a NUL-terminated
> buffer which is <buf, len> pair of a strbuf.

That holds for six of the seven call sites: `commit.c`, `tag.c`,
`builtin/fast-import.c`, `fmt-merge-msg.c`, and `log-tree.c` (twice).
`builtin/receive-pack.c` is a minor wrinkle worth flagging: it passes
`push_cert.buf + bogs` ("bogs" = "beginning_of_gpg_sig") and
`push_cert.len - bogs`, an offset sub-buffer of `push_cert`, not that
strbuf's own buf/len pair verbatim. It still ends on `push_cert`'s own
terminating NUL, so the observation holds in spirit, but strictly the
pattern is "ends at some strbuf's own NUL", which is more a matter of
code-review convention across call sites than something
`check_signature()`'s own signature guarantees.

> Another approach that may be simpler is to drop the slen parameter from
> check_signature().

I would rather keep it right where it is, for (at least 😊) two reasons.

First, `slen` isn't new here: `check_signature()` has taken a `(sigc,
signature, slen)` signature since 02769437e142 (ssh signing: use sigc
struct to pass payload, 2021-12-09), three years before this series, so
dropping it now would fold an unrelated API change into a bug fix.

Second, and this is the one that actually worries me: `slen` is used twice
inside `check_signature()`, not once. Besides the `get_format_by_sig()`
call above, the pre-existing `fmt->verify_signed_buffer(sigc, fmt,
signature, slen)` a few lines down depends on it too (there it is named
`signature_size`). Both concrete implementations of that vtable member,
`verify_gpg_signed_buffer()` and `verify_ssh_signed_buffer()`, use
`signature_size` to decide exactly how many bytes to `write_in_full()`
into the temporary file that then gets handed to `gpg`/`ssh-keygen` as the
detached signature to verify. That is the authoritative byte count of the
blob being verified, not a defensive nicety. If we dropped `slen` and let
`check_signature()` fall back on `strlen(signature)`, a signature blob
with an embedded NUL before its logical end would get truncated before it
ever reaches the external verifier: a correctness regression in the actual
cryptographic verification path, not merely in the prefix-matching helper
this series fixes. `check_signature()` has no doc comment promising
`signature` is free of embedded NULs, so dropping `slen` would trade an
explicit length for an implicit assumption.

As the commit message notes, we have been down this road with this exact
function chain before. In February 2024, Peff concluded there was no
walk-too-far problem in `parse_signed_buffer()` "because we feed it from a
strbuf":
https://lore.kernel.org/git/20240208214137.GB1090198@coredump.intra.peff.net/

But that conclusion was already four months stale: c8762c30df5b
(object-file-convert: convert tag objects when writing, 2023-10-01) had
already added `convert_tag_object()` as a caller that does _not_ feed from
a strbuf: the same gap this series closes. Applying the same "audit
today's callers and assume it holds" reasoning to `check_signature()` now
risks reproducing that failure mode a second time.

So I would like to keep `slen` and the `get_format_by_sig(signature,
slen)` call as in the patch.

Ciao,
Johannes