Skip to content

One appended junk signature makes verify_all reject a threshold-satisfying certificate #349

Description

@beardthelion

RefUpdateCert::verify_all promises a filtered list and delivers an all-or-nothing result
(crates/gitlawb-core/src/cert.rs:113-135):

/// Returns the list of DIDs whose signatures are valid.
pub fn verify_all(&self) -> Result<Vec<Did>> {
    ...
    for cert_sig in &self.signatures {
        let vk = cert_sig.signer.to_verifying_key()?;
        let sig_bytes_vec = URL_SAFE_NO_PAD.decode(&cert_sig.sig)
            .map_err(|e| Error::RefCert(format!("invalid base64 sig: {e}")))?;
        let sig_bytes: [u8; 64] = sig_bytes_vec.try_into()
            .map_err(|_| Error::RefCert("signature must be 64 bytes".to_string()))?;
        verify(&vk, &signing_bytes, &sig_bytes)?;
        valid.push(cert_sig.signer.clone());
    }

Four ? on per-entry attacker-supplied data, under a docstring that says the opposite. The threshold
check consumes it unfiltered (cert.rs:143-148) and is itself written to tolerate irrelevant entries,
filtering non-maintainers and deduping, but never gets the chance.

The signature list is deliberately outside the signed bytes: RefUpdateBody::to_signing_bytes
(cert.rs:53-55) covers the body only, and gitlawb-attest strips signatures and attestations
before JCS encoding so countersignatures stay stable. Appending is the intended workflow
(countersign, cert.rs:98-109, a plain push). So an append is undetectable by construction and
requires no key material.

Measured

Workspace copied to scratch, integration test added there, wt-clawpatch untouched:

baseline  threshold(2)       = Ok(true)
junk-b64  threshold(2)       = Err(RefCert("invalid base64 sig: Invalid symbol 33, offset 0."))
bogus-sig threshold(2)       = Err(SignatureInvalid)
junk-b64  validate_structure = Ok(())

Both entry classes reproduce, and structure validation does not catch either.

Why this is low, and why it is still worth fixing now

RefUpdateCert, verify_all, and satisfies_threshold have zero production callers: grep across
gitlawb-node, gl, and git-remote-gitlawb returns nothing, and the node's own
crates/gitlawb-node/src/cert.rs is an unrelated single-signature receipt. Only tests and
gitlawb-attest/tests/with_gitlawb_core.rs touch it. It also fails closed, rejecting a good cert
rather than accepting a bad one, so nothing is bypassed.

It is a latent defect in a published library API (gitlawb-core 0.7.1) that becomes reachable the
moment a verifier lands, which is the cheap moment to fix it.

The intended semantics are not in doubt: crates/gitlawb-attest/src/verifier.rs:295-300 states the
same rule for the sibling verifier, "the attacker shouldn't be able to DoS the cert by attaching
attacker/spam/v1 before covenant/exec/v1", with a test asserting the batch still passes. Same
threat, already decided, one crate over.

This is a residual of #326: that change hardened the threshold count against duplicate valid
signatures and left the invalid-entry path in verify_all untouched.

Fix direction, with two cautions

Filter inside satisfies_threshold (or add a valid_signers() that skips bad entries) rather than
making verify_all swallow errors. verify_all is public on a published crate and a future caller
will reasonably read verify_all().is_ok() as "this cert is clean"; keeping both meanings available
is worth more than the smaller diff.

And to_verifying_key()? at cert.rs:121 is on the same footing as the other three, so a fix that
handles only the base64 decode and the verify call leaves an unparseable-DID append as a live
denial.

Metadata

Metadata

Assignees

No one assigned

    Labels

    crate:coregitlawb-core — identity, certs, encrypt, DID/UCANkind:bugDefect fix — wrong or unsafe behaviorsev:lowCosmetic, cleanup, or nice-to-havesubsystem:attestationCertificates, anchoring, per-ref attestation

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions