From 107349ec2a8a5dcb1be5499d30045ebc37ecdac3 Mon Sep 17 00:00:00 2001 From: andreolf Date: Tue, 18 Aug 2026 12:55:13 +0200 Subject: [PATCH] fix(core): ignore invalid signatures in satisfies_threshold (#349) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RefUpdateCert::satisfies_threshold called verify_all, which fails closed on the first malformed or invalid signature entry. Signature entries live outside the signed body, so any third party can append a junk entry with no key material — appending one denied an otherwise valid certificate, a DoS on the threshold check. Add a lenient valid_signers() helper that skips entries failing to parse, decode, or verify, and have satisfies_threshold count distinct valid signers from it. verify_all stays strict (fail-closed) so its API and the tampered_signature_fails_verify_all test are unchanged. A forged entry naming a real maintainer DID with a bad signature is skipped, so it cannot inflate the count either. Adds tests: appended junk entries do not deny a valid 2-of-2 cert, and a forged maintainer signature is not counted toward the threshold. Closes #349 --- crates/gitlawb-core/src/cert.rs | 113 ++++++++++++++++++++++++++++++-- 1 file changed, 109 insertions(+), 4 deletions(-) diff --git a/crates/gitlawb-core/src/cert.rs b/crates/gitlawb-core/src/cert.rs index 5f30ce9c..a18fe213 100644 --- a/crates/gitlawb-core/src/cert.rs +++ b/crates/gitlawb-core/src/cert.rs @@ -108,9 +108,15 @@ impl RefUpdateCert { Ok(()) } - /// Verify all signatures on this certificate. + /// Verify all signatures on this certificate, failing closed. /// - /// Returns the list of DIDs whose signatures are valid. + /// On success returns the list of DIDs whose signatures are valid. This is + /// strict: it returns an error if *any* signature entry is malformed or + /// invalid. Because signature entries live outside the signed body (anyone + /// can append one without key material), do not use this to make a + /// threshold decision on an untrusted certificate — a single appended junk + /// entry would reject the whole cert. Use [`Self::satisfies_threshold`], + /// which ignores invalid entries and counts only the valid signers. pub fn verify_all(&self) -> Result> { use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine}; let signing_bytes = self.body.to_signing_bytes()?; @@ -135,13 +141,48 @@ impl RefUpdateCert { Ok(valid) } + /// The DIDs whose signature entries validate, skipping any that are + /// malformed or invalid rather than failing. + /// + /// Signature entries are appended outside the signed body, so any third + /// party can attach a junk entry without key material. Counting only the + /// entries that actually verify — instead of short-circuiting on the first + /// bad one, as [`Self::verify_all`] does — prevents that from becoming a + /// denial-of-service on an otherwise valid certificate (#349). A forged + /// entry that names a real maintainer DID but carries a bad signature is + /// skipped, so it cannot inflate the count either. + fn valid_signers(&self) -> Result> { + use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine}; + let signing_bytes = self.body.to_signing_bytes()?; + let mut valid = Vec::new(); + + for cert_sig in &self.signatures { + let Ok(vk) = cert_sig.signer.to_verifying_key() else { + continue; + }; + let Ok(sig_bytes_vec) = URL_SAFE_NO_PAD.decode(&cert_sig.sig) else { + continue; + }; + let Ok(sig_bytes) = <[u8; 64]>::try_from(sig_bytes_vec) else { + continue; + }; + if verify(&vk, &signing_bytes, &sig_bytes).is_ok() { + valid.push(cert_sig.signer.clone()); + } + } + + Ok(valid) + } + /// Check if this certificate satisfies a threshold of valid signatures /// from the provided set of authorized maintainer DIDs. /// /// Counts distinct signer DIDs, not signature entries: a repeated - /// signature from the same maintainer counts once. + /// signature from the same maintainer counts once. Malformed or invalid + /// signature entries are ignored (see [`Self::valid_signers`]), so an + /// attacker cannot deny a valid cert by appending junk signatures. pub fn satisfies_threshold(&self, maintainers: &[Did], threshold: usize) -> Result { - let valid = self.verify_all()?; + let valid = self.valid_signers()?; let distinct_signers: HashSet<&Did> = valid.iter().filter(|d| maintainers.contains(d)).collect(); Ok(distinct_signers.len() >= threshold) @@ -377,6 +418,70 @@ mod tests { assert!(!cert.satisfies_threshold(&maintainers, 2).unwrap()); } + /// An attacker can append junk signature entries to a cert (they live + /// outside the signed body, so no key material is needed). `satisfies_threshold` + /// must ignore them and still count the real signers — before the fix, + /// `verify_all`'s `?` on the junk made the whole check error out, denying a + /// valid 2-of-2 cert. + #[test] + fn satisfies_threshold_ignores_appended_junk_signatures() { + let kp1 = Keypair::generate(); + let kp2 = Keypair::generate(); + let mut cert = RefUpdateCert::new( + kp1.did(), + "refs/heads/main".to_string(), + dummy_hash('0'), + dummy_hash('a'), + 1, + &kp1, + ) + .unwrap(); + cert.countersign(&kp2).unwrap(); + + // Append entries that are unparseable / invalid in different ways. + cert.signatures.push(CertSignature { + signer: kp1.did(), + sig: "!!!not-base64!!!".to_string(), + }); + // Valid base64 but only 3 bytes — fails the 64-byte length check. + cert.signatures.push(CertSignature { + signer: kp2.did(), + sig: "AAAA".to_string(), + }); + + let maintainers = vec![kp1.did(), kp2.did()]; + assert!(cert.satisfies_threshold(&maintainers, 2).unwrap()); + } + + /// A junk entry that names a real maintainer DID but carries a signature + /// that does not verify must not be counted — otherwise appending garbage + /// under a maintainer's DID could forge a threshold. + #[test] + fn satisfies_threshold_does_not_count_a_forged_maintainer_signature() { + let kp1 = Keypair::generate(); + let kp2 = Keypair::generate(); + let mut cert = RefUpdateCert::new( + kp1.did(), + "refs/heads/main".to_string(), + dummy_hash('0'), + dummy_hash('a'), + 1, + &kp1, + ) + .unwrap(); + // Well-formed 64-byte signature from kp2, but over unrelated bytes, so + // it fails verification against the cert body. + cert.signatures.push(CertSignature { + signer: kp2.did(), + sig: kp2.sign_b64(b"unrelated bytes"), + }); + + let maintainers = vec![kp1.did(), kp2.did()]; + // Only kp1 actually signed the body: 2-of-2 must fail, 1-of-2 holds. + assert!(!cert.satisfies_threshold(&maintainers, 2).unwrap()); + assert!(cert.satisfies_threshold(&maintainers, 1).unwrap()); + } + #[test] fn threshold_zero_is_always_satisfied() { let kp = Keypair::generate();