From ba95eb1a16c5c7964e34e8ec08adaad9de01fcf7 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:49:03 -0500 Subject: [PATCH 01/12] fix(core): reject small-order Ed25519 keys at DID resolution and before X25519 A did:key can encode a small-order (weak) Ed25519 point. VerifyingKey::from_bytes only decompresses, so such a key resolved cleanly, and x25519_public converted it to the all-zero Montgomery u. X25519 against u = 0 is the all-zero shared secret for every scalar, so the per-recipient wrap in seal_blob could be rebuilt with no secret at all. Since one content key is wrapped per recipient, a single weak recipient exposed the blob for every recipient. Reject in Did::to_verifying_key, which is the choke point every consumer that resolves a DID shares, so a weak key becomes unresolvable and the existing fail-closed recipient handling catches it without a new code path. Guard x25519_public as well so the primitive is safe for a caller that obtained the key some other way. The attacker test performs the real attack rather than asserting an absence: with either guard removed it recovers the plaintext and fails printing it. --- crates/gitlawb-core/src/did.rs | 48 ++++++++++- crates/gitlawb-core/src/encrypt.rs | 130 +++++++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 1 deletion(-) diff --git a/crates/gitlawb-core/src/did.rs b/crates/gitlawb-core/src/did.rs index e3775845..0a61c770 100644 --- a/crates/gitlawb-core/src/did.rs +++ b/crates/gitlawb-core/src/did.rs @@ -106,7 +106,21 @@ impl Did { .try_into() .map_err(|_| Error::InvalidDid("ed25519 key must be 32 bytes".to_string()))?; - VerifyingKey::from_bytes(&key_bytes).map_err(|e| Error::InvalidDid(e.to_string())) + let key = + VerifyingKey::from_bytes(&key_bytes).map_err(|e| Error::InvalidDid(e.to_string()))?; + + // `from_bytes` only decompresses, so it accepts a small-order point. + // Such a key satisfies the verification equation for any message, and + // its Montgomery form is the all-zero X25519 u-coordinate, which makes + // any X25519 shared secret derived from it the all-zero key. Rejecting + // at resolution is what makes this the choke point: every consumer that + // resolves a DID through here inherits the rejection, and a recipient + // that cannot resolve already fails closed downstream. + if key.is_weak() { + return Err(Error::InvalidDid("small-order ed25519 key".to_string())); + } + + Ok(key) } /// Return the full DID string as a `&str`. @@ -369,4 +383,36 @@ mod tests { ); did.to_verifying_key().expect("a real did:key must resolve"); } + + /// A `did:key` can encode a small-order (weak) Ed25519 point. Such a key + /// satisfies the verification equation for any message and, converted to + /// Montgomery form for X25519, yields the all-zero shared secret, so an + /// envelope sealed to it is decryptable by anyone. Resolution is the choke + /// point every `Did`-based consumer shares, so it fails closed here. + #[test] + fn to_verifying_key_rejects_a_small_order_key() { + // The compressed identity point, the canonical small-order key. + let mut weak = [0u8; 32]; + weak[0] = 1; + let mut prefixed = Vec::with_capacity(ED25519_MULTICODEC.len() + 32); + prefixed.extend_from_slice(ED25519_MULTICODEC); + prefixed.extend_from_slice(&weak); + let did: Did = format!( + "did:key:{}", + multibase::encode(multibase::Base::Base58Btc, &prefixed) + ) + .parse() + .expect("a small-order did:key is still well-formed"); + + let err = did + .to_verifying_key() + .expect_err("a small-order did:key must not resolve"); + match err { + Error::InvalidDid(msg) => assert!( + msg.contains("small-order"), + "rejection must name the small-order key, got: {msg}" + ), + other => panic!("expected Error::InvalidDid, got {other:?}"), + } + } } diff --git a/crates/gitlawb-core/src/encrypt.rs b/crates/gitlawb-core/src/encrypt.rs index d8659be7..c1b60a16 100644 --- a/crates/gitlawb-core/src/encrypt.rs +++ b/crates/gitlawb-core/src/encrypt.rs @@ -11,6 +11,16 @@ use zeroize::Zeroizing; /// X25519 public key (Montgomery u) for an Ed25519 verifying key. fn x25519_public(vk: &VerifyingKey) -> Result<[u8; 32]> { use curve25519_dalek::edwards::CompressedEdwardsY; + + // A small-order point converts to the all-zero Montgomery u, and X25519 + // against u = 0 yields the all-zero shared secret for every scalar, so the + // per-recipient wrap could be rebuilt by anyone. Resolution already refuses + // such a key (see Did::to_verifying_key); this guard makes the primitive + // safe on its own terms for a caller that obtained the key some other way. + if vk.is_weak() { + anyhow::bail!("verifying key is a small-order point"); + } + let edwards = CompressedEdwardsY::from_slice(vk.as_bytes()) .ok() .and_then(|c| c.decompress()) @@ -293,4 +303,124 @@ mod tests { header["nonce"] = bad_nonce; assert!(open_blob(&reframe(&header), &reader).is_err()); } + + /// A small-order recipient key converts to Montgomery u = 0, and X25519 + /// against u = 0 is the all-zero shared secret for ANY scalar, so the + /// wrapping box is reconstructable with no secret at all. Reject at the + /// primitive so the seal side is safe regardless of how the caller + /// obtained the key. + #[test] + fn x25519_public_rejects_a_small_order_key() { + let mut weak = [0u8; 32]; + weak[0] = 1; // compressed identity point + let weak_vk = VerifyingKey::from_bytes(&weak).expect("identity point decompresses"); + assert!(weak_vk.is_weak(), "precondition: key is small-order"); + + assert!( + x25519_public(&weak_vk).is_err(), + "a small-order key must not yield an x25519 public key" + ); + } + + /// Control: a legitimate key still converts, and to a non-zero u. + #[test] + fn x25519_public_still_accepts_a_real_key() { + let kp = Keypair::generate(); + let u = x25519_public(&kp.verifying_key()).expect("a real key must convert"); + assert_ne!(u, [0u8; 32], "a real key must not map to the zero point"); + } + + /// The guard has to propagate: no envelope may be produced for a weak + /// recipient even when the DID choke point is bypassed entirely. + #[test] + fn seal_blob_refuses_a_small_order_recipient() { + let mut weak = [0u8; 32]; + weak[0] = 1; + let weak_vk = VerifyingKey::from_bytes(&weak).unwrap(); + + assert!( + seal_blob(b"withheld", &[weak_vk]).is_err(), + "sealing to a weak recipient must fail" + ); + + // And a mixed set must fail too: one weak recipient exposes the shared + // content key, so a partial envelope is not an acceptable outcome. + let honest = Keypair::generate(); + assert!( + seal_blob(b"withheld", &[honest.verifying_key(), weak_vk]).is_err(), + "a mixed honest+weak recipient set must fail closed, not seal partially" + ); + } + + /// Control for the two above: the ordinary seal/open round trip is intact. + #[test] + fn legit_seal_open_round_trip_still_works() { + let reader = Keypair::generate(); + let env = seal_blob(b"withheld blob", &[reader.verifying_key()]).expect("seal"); + assert_eq!(open_blob(&env, &reader).expect("open"), b"withheld blob"); + } + + /// The defect this fix exists to close, kept as an executable regression + /// rather than an assertion about absence. It runs the real attack: craft a + /// small-order did:key, get it into a recipient set alongside an honest + /// reader, then rebuild the wrapping box from the all-zero shared secret + /// that a small-order recipient forces. If any guard regresses, this does + /// not merely fail, it fails printing the plaintext it recovered. + #[test] + fn attacker_cannot_recover_plaintext_via_a_weak_recipient() { + use crate::did::Did; + use std::str::FromStr; + + let mut weak_bytes = [0u8; 32]; + weak_bytes[0] = 1; // compressed identity point + let weak_vk = VerifyingKey::from_bytes(&weak_bytes).expect("decompresses"); + + // Layer 1: the attacker's did:key string must not resolve at all. + let weak_did = Did::from_verifying_key(&weak_vk).to_string(); + assert!( + Did::from_str(&weak_did) + .expect("still a well-formed did:key") + .to_verifying_key() + .is_err(), + "a small-order did:key must not resolve" + ); + + // Layer 2: even handed the key directly, sealing must refuse. One weak + // recipient would expose the single shared content key, so the honest + // reader's blob would be readable by anyone. + let honest = Keypair::generate(); + let secret_plaintext = b"WITHHELD BLOB CONTENTS"; + let envelope = match seal_blob(secret_plaintext, &[honest.verifying_key(), weak_vk]) { + Err(_) => return, // no envelope exists, nothing to attack + Ok(env) => env, + }; + + // Only reachable if a guard regressed. Run the attack and report what + // it got, so the failure names the actual exposure. + let mut p = MAGIC.len() + 1; + let hlen = u32::from_le_bytes(envelope[p..p + 4].try_into().unwrap()) as usize; + p += 4; + let header: serde_json::Value = serde_json::from_slice(&envelope[p..p + hlen]).unwrap(); + let body = &envelope[p + hlen..]; + let body_nonce = B64.decode(header["nonce"].as_str().unwrap()).unwrap(); + + // X25519 against u = 0 is the all-zero shared secret for any scalar, so + // the sealer's box is reconstructable with a key the attacker picks. + let zero_box = ChaChaBox::new(&XPublic::from([0u8; 32]), &XSecret::from([7u8; 32])); + for r in header["recipients"].as_array().unwrap() { + let n = B64.decode(r["nonce"].as_str().unwrap()).unwrap(); + let w = B64.decode(r["wrap"].as_str().unwrap()).unwrap(); + if let Ok(content_key) = zero_box.decrypt(n.as_slice().into(), w.as_slice()) { + let cipher = XChaCha20Poly1305::new_from_slice(&content_key).unwrap(); + let recovered = cipher + .decrypt(XNonce::from_slice(&body_nonce), body) + .expect("body decrypts once the content key is out"); + panic!( + "REGRESSION: attacker recovered plaintext with no private key: {:?}", + String::from_utf8_lossy(&recovered) + ); + } + } + panic!("an envelope was sealed to a small-order recipient; the seal guard regressed"); + } } From 490bb30aecfc0c5381e005cdb8e415a5c7c0ca61 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:49:12 -0500 Subject: [PATCH 02/12] fix(attest): mirror the small-order key rejection in its own did:key parser gitlawb-attest parses did:key itself in verifying_key_from_did_key and never routes through gitlawb-core's Did. It cannot: gitlawb-core is a dev-dependency here, so the choke-point rejection is structurally unreachable from this crate and has to be mirrored rather than inherited. Signature verification is already strict, so this is defense in depth against a future consumer of this parser that does not verify strictly, not the closing of an exploitable path. One consequence worth naming: verify_rejects_weak_key_signature now observes Error::Did rather than Error::Signature, because the weak signer is refused at resolution before verify_strict is reached. That test is retargeted accordingly. It no longer exercises verify_strict, which remains the second layer for malleability that does not involve a weak public key (a small-order R under an honest key); this fixture cannot construct that case. --- crates/gitlawb-attest/src/attestation.rs | 79 +++++++++++++++++++++--- 1 file changed, 72 insertions(+), 7 deletions(-) diff --git a/crates/gitlawb-attest/src/attestation.rs b/crates/gitlawb-attest/src/attestation.rs index 0e29e3a9..d1213ea6 100644 --- a/crates/gitlawb-attest/src/attestation.rs +++ b/crates/gitlawb-attest/src/attestation.rs @@ -217,8 +217,20 @@ fn verifying_key_from_did_key(did: &str) -> Result { let key_bytes: [u8; 32] = bytes[ED25519_MULTICODEC.len()..] .try_into() .expect("length checked above"); - VerifyingKey::from_bytes(&key_bytes) - .map_err(|e| Error::Did(format!("invalid ed25519 key: {e}"))) + let key = VerifyingKey::from_bytes(&key_bytes) + .map_err(|e| Error::Did(format!("invalid ed25519 key: {e}")))?; + + // `from_bytes` only decompresses, so it accepts a small-order point. This + // crate parses did:key itself rather than going through gitlawb-core's + // `Did`, and it cannot go through it (gitlawb-core is a dev-dependency + // here), so the rejection is mirrored rather than inherited. Signature + // verification is already strict, so this is defense in depth against a + // future consumer of this parser that does not verify strictly. + if key.is_weak() { + return Err(Error::Did("small-order ed25519 key".to_string())); + } + + Ok(key) } #[cfg(test)] @@ -559,6 +571,50 @@ mod tests { /// weak (small-order) public key satisfies the verification equation /// but must be rejected. The identity point is such a key: with R the /// identity and S = 0, [S]B - [k]A is the identity for any message. + /// gitlawb-attest parses did:key itself and never routes through + /// gitlawb-core's `Did`, which it cannot: gitlawb-core is a dev-dependency + /// here. So the choke-point rejection has to be mirrored in this parser or + /// this crate keeps handing out keys the rest of the system rejects. + #[test] + fn verifying_key_from_did_key_rejects_a_small_order_key() { + let mut weak = [0u8; 32]; + weak[0] = 1; // compressed identity point + let mut buf = Vec::with_capacity(ED25519_MULTICODEC.len() + 32); + buf.extend_from_slice(&ED25519_MULTICODEC); + buf.extend_from_slice(&weak); + let did = format!( + "did:key:{}", + multibase::encode(multibase::Base::Base58Btc, &buf) + ); + + let err = + verifying_key_from_did_key(&did).expect_err("a small-order did:key must not resolve"); + match err { + Error::Did(msg) => assert!( + msg.contains("small-order"), + "rejection must name the small-order key, got: {msg}" + ), + other => panic!("expected Error::Did, got {other:?}"), + } + } + + /// Control for the guard above: a real did:key must still resolve. + #[test] + fn verifying_key_from_did_key_still_accepts_a_real_key() { + let sk = SigningKey::from_bytes(&[3u8; 32]); + let vk = sk.verifying_key(); + let mut buf = Vec::with_capacity(ED25519_MULTICODEC.len() + 32); + buf.extend_from_slice(&ED25519_MULTICODEC); + buf.extend_from_slice(&vk.to_bytes()); + let did = format!( + "did:key:{}", + multibase::encode(multibase::Base::Base58Btc, &buf) + ); + + let got = verifying_key_from_did_key(&did).expect("a real did:key must resolve"); + assert_eq!(got.to_bytes(), vk.to_bytes()); + } + #[test] fn verify_rejects_weak_key_signature() { let mut weak_key_bytes = [0u8; 32]; @@ -572,7 +628,13 @@ mod tests { let forged_sig = B64U.encode(forged); // Build an attestation with the weak key as signer and the forged - // signature. The weak-key check happens at verify time. + // signature. Rejection now happens at DID RESOLUTION rather than at + // verify time: `verifying_key_from_did_key` refuses a small-order key + // before `verify_strict` is reached, so the observed error is + // `Error::Did`, not `Error::Signature`. `verify_strict` remains the + // second layer for malleability cases that do not involve a weak + // public key (a small-order R under an honest key), which this fixture + // cannot construct and therefore no longer covers. let mut att = dummy_attestation(&SigningKey::generate(&mut OsRng), cert_hash); let mut buf = Vec::with_capacity(ED25519_MULTICODEC.len() + 32); buf.extend_from_slice(&ED25519_MULTICODEC); @@ -584,9 +646,12 @@ mod tests { att.sig = forged_sig; let err = att.verify_signature(cert_hash).unwrap_err(); - assert!( - matches!(err, Error::Signature(_)), - "signature under a weak (small-order) public key must be rejected" - ); + match err { + Error::Did(msg) => assert!( + msg.contains("small-order"), + "weak signer must be refused as a small-order key, got: {msg}" + ), + other => panic!("expected Error::Did for a small-order signer, got {other:?}"), + } } } From bab7a63530b6d063dd558c0ebdc957b3eac83f21 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:49:25 -0500 Subject: [PATCH 03/12] test(node): pin that a weak recipient DID falls into the existing fail-closed arm Because a small-order did:key no longer resolves, it lands in the unresolved set and plan_seal returns the #47 SkipUnresolvable arm that already refuses to seal to a partial recipient set. Nothing is sealed for the blob and the existing bounded operator warning fires. No new branch, no new SealPlan variant, no new log site. The must-not-over-reject control asserts an all-legitimate set still seals. --- crates/gitlawb-node/src/encrypted_pin.rs | 43 ++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/crates/gitlawb-node/src/encrypted_pin.rs b/crates/gitlawb-node/src/encrypted_pin.rs index 19b80651..16e4761e 100644 --- a/crates/gitlawb-node/src/encrypted_pin.rs +++ b/crates/gitlawb-node/src/encrypted_pin.rs @@ -206,6 +206,49 @@ mod tests { use super::*; use ed25519_dalek::SigningKey; + /// A did:key encoding a small-order point. Well-formed, indistinguishable + /// from a real one by eye, and refused at resolution since the choke-point + /// guard landed. + fn weak_did_key() -> String { + let mut weak = [0u8; 32]; + weak[0] = 1; // compressed identity point + let vk = ed25519_dalek::VerifyingKey::from_bytes(&weak).expect("decompresses"); + Did::from_verifying_key(&vk).to_string() + } + + /// The #47 fail-closed arm already refuses to seal to a partial recipient + /// set. Because a small-order did:key no longer RESOLVES, it lands in the + /// unresolved set and that existing arm catches it: nothing is sealed, and + /// no new code path was added to make that happen. + #[test] + fn weak_recipient_falls_into_the_existing_fail_closed_arm() { + let weak = weak_did_key(); + let mut dids = BTreeSet::new(); + dids.insert(did_key(9)); + dids.insert(weak.clone()); + + match plan_seal(&[0u8; 32], &dids, None) { + SealPlan::SkipUnresolvable(unresolved) => assert!( + unresolved.contains(&weak), + "the weak DID must be named in the unresolved set, got {unresolved:?}" + ), + other => panic!("expected SkipUnresolvable, got {other:?}"), + } + } + + /// Must-not-over-reject control: an all-legitimate recipient set still seals. + #[test] + fn legitimate_recipient_set_still_seals() { + let mut dids = BTreeSet::new(); + dids.insert(did_key(9)); + dids.insert(did_key(11)); + + match plan_seal(&[0u8; 32], &dids, None) { + SealPlan::Seal { keys, .. } => assert_eq!(keys.len(), 2, "both recipients resolve"), + other => panic!("expected Seal, got {other:?}"), + } + } + fn did_key(seed: u8) -> String { let vk = SigningKey::from_bytes(&[seed; 32]).verifying_key(); Did::from_verifying_key(&vk).to_string() From e209908a2e74c8343e55387288f110a86664294a Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:49:25 -0500 Subject: [PATCH 04/12] fix(gl): stop telling a did:key that did:key is unsupported did_resolve reported every resolution failure as "only did:key is supported without a resolver". That is right for a method with no local resolver and misleading for a did:key that parsed fine and failed for its own reason, which now includes a small-order key. Split the message on whether the DID is a did:key and surface the underlying error when it is. Extracted as a pure helper so both branches are unit-testable without an MCP client harness. --- crates/gl/src/mcp.rs | 61 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 54 insertions(+), 7 deletions(-) diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index ae319c73..6a47160a 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -775,13 +775,15 @@ async fn call_tool( let did: gitlawb_core::did::Did = did_str .parse() .map_err(|e: gitlawb_core::Error| anyhow::anyhow!("{e}"))?; - if let Ok(vk) = did.to_verifying_key() { - let doc = gitlawb_core::did::DidDocument::new(did, &vk); - Ok(serde_json::to_string_pretty(&doc)?) - } else { - Err(anyhow::anyhow!( - "cannot resolve '{did_str}' locally — only did:key is supported without a resolver" - )) + let is_did_key = did.is_did_key(); + match did.to_verifying_key() { + Ok(vk) => { + let doc = gitlawb_core::did::DidDocument::new(did, &vk); + Ok(serde_json::to_string_pretty(&doc)?) + } + Err(e) => Err(anyhow::anyhow!(did_resolve_failure_message( + did_str, is_did_key, &e + ))), } } @@ -1341,6 +1343,51 @@ fn write_message(writer: &mut impl Write, value: &Value) -> Result<()> { Ok(()) } +/// Message for a `did_resolve` failure. A `did:key` that fails to resolve is a +/// different situation from a DID method we have no resolver for, so saying +/// "only did:key is supported" about something that IS a did:key sends the +/// reader looking for the wrong problem. +fn did_resolve_failure_message( + did_str: &str, + is_did_key: bool, + err: &gitlawb_core::Error, +) -> String { + if is_did_key { + format!("cannot resolve '{did_str}': {err}") + } else { + format!("cannot resolve '{did_str}' locally — only did:key is supported without a resolver") + } +} + +#[cfg(test)] +mod did_resolve_message_tests { + use super::did_resolve_failure_message; + + #[test] + fn a_did_key_that_fails_to_resolve_does_not_claim_did_key_is_unsupported() { + let err = gitlawb_core::Error::InvalidDid("small-order ed25519 key".to_string()); + let msg = did_resolve_failure_message("did:key:z6MkWeak", true, &err); + assert!( + !msg.contains("only did:key is supported"), + "a did:key must not be told did:key is unsupported: {msg}" + ); + assert!( + msg.contains("small-order"), + "the real cause must survive into the message: {msg}" + ); + } + + #[test] + fn a_non_key_method_keeps_the_no_resolver_message() { + let err = gitlawb_core::Error::InvalidDid("expected did:key, got did:web".to_string()); + let msg = did_resolve_failure_message("did:web:example.com", false, &err); + assert!( + msg.contains("only did:key is supported"), + "an unresolvable method keeps its existing explanation: {msg}" + ); + } +} + #[cfg(test)] mod tests { use super::*; From 7b833b18890ad9a31de1adf629e77eb4ee624f3e Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 10 Aug 2026 11:45:33 -0500 Subject: [PATCH 05/12] fix(review): restore verify_strict coverage the parser guard displaced Refusing a small-order key at DID resolution means a weak signer can no longer reach attestation.rs's verify_strict through verify_signature, which left that line with no test at all: downgrading it to the non-strict verify kept the whole attest suite green. Split the signature check into verify_sig_with_key so a test can drive it directly with a weak key. Check order in verify_signature is unchanged; only the seam is new. Also from review: - auth/mod.rs sent a did:key that failed on its own key material away with "only did:key is supported in alpha", the same misleading hint the gl MCP tool was just fixed for. Made it conditional on is_did_key so the two parallel surfaces say the same thing. - The guard comments claimed a small-order point converts to the all-zero Montgomery u. That holds for order 1 and 2 only; order 4 and 8 are annihilated by the scalar clamping instead. is_weak covers the whole torsion set, and mixed-order points are correctly still accepted. - did.rs doc comment now states the new rejection. - encrypt.rs: extracted the repeated weak-key fixture into a test helper and switched to the file's existing Err(anyhow!(..)) rejection idiom. - gl tests derive the did:key discriminator from a real parsed Did instead of hardcoding the bool, so inverting it at the call site now fails. --- crates/gitlawb-attest/src/attestation.rs | 60 +++++++++++++++++++++++- crates/gitlawb-core/src/did.rs | 11 +++-- crates/gitlawb-core/src/encrypt.rs | 38 ++++++++------- crates/gitlawb-node/src/auth/mod.rs | 13 ++++- crates/gl/src/mcp.rs | 31 +++++++++--- 5 files changed, 124 insertions(+), 29 deletions(-) diff --git a/crates/gitlawb-attest/src/attestation.rs b/crates/gitlawb-attest/src/attestation.rs index d1213ea6..91274f98 100644 --- a/crates/gitlawb-attest/src/attestation.rs +++ b/crates/gitlawb-attest/src/attestation.rs @@ -105,16 +105,30 @@ impl Attestation { let bytes = canonical_signing_bytes(&self.type_, &self.payload, &self.cert_hash)?; let vk = verifying_key_from_did_key(&self.signer)?; + self.verify_sig_with_key(&vk, &bytes)?; + + Ok(vk) + } + + /// The signature check itself, against a key the caller supplies. + /// + /// Split out so the STRICTNESS of the check stays provable on its own. + /// `verifying_key_from_did_key` now refuses a small-order key, so a weak + /// signer can no longer reach this line through `verify_signature`. Without + /// this seam there would be nothing left that fails when `verify_strict` is + /// downgraded to the non-strict `verify`, and that regression would land + /// silently. Check order in `verify_signature` is unchanged. + fn verify_sig_with_key(&self, vk: &VerifyingKey, bytes: &[u8]) -> Result<()> { let sig_bytes: [u8; 64] = B64U .decode(&self.sig) .map_err(|e| Error::Signature(format!("base64url: {e}")))? .try_into() .map_err(|_| Error::Signature("signature must be 64 bytes".to_string()))?; let sig = Signature::from_bytes(&sig_bytes); - vk.verify_strict(&bytes, &sig) + vk.verify_strict(bytes, &sig) .map_err(|e| Error::Signature(format!("ed25519: {e}")))?; - Ok(vk) + Ok(()) } /// Reparse `payload` as `P`. Errors if the type discriminator does not @@ -598,6 +612,48 @@ mod tests { } } + /// Strictness regression guard. The parser now refuses a small-order key, + /// so `verify_signature` can no longer carry a weak signer down to the + /// signature check; this drives that check directly to prove it is still + /// STRICT. Downgrading `verify_strict` to `verify` makes this go red, + /// which nothing else in the suite would catch. + #[test] + fn verify_sig_with_key_is_strict_about_a_weak_key() { + let mut weak_key_bytes = [0u8; 32]; + weak_key_bytes[0] = 1; // compressed identity point + let weak_vk = VerifyingKey::from_bytes(&weak_key_bytes).unwrap(); + assert!(weak_vk.is_weak()); + + let cert_hash = sample_cert_hash(); + let mut att = dummy_attestation(&SigningKey::generate(&mut OsRng), cert_hash); + // R = identity, S = 0: satisfies the non-strict verification equation + // for any message under a small-order key. + let mut forged = [0u8; 64]; + forged[0] = 1; + att.sig = B64U.encode(forged); + + let bytes = canonical_signing_bytes(&att.type_, &att.payload, &att.cert_hash).expect("jcs"); + let err = att + .verify_sig_with_key(&weak_vk, &bytes) + .expect_err("strict verification must reject a small-order key"); + assert!( + matches!(err, Error::Signature(_)), + "expected a signature error, got {err:?}" + ); + } + + /// Accept control for the seam: a genuine signature still verifies through + /// the same helper, so the guard above is not simply rejecting everything. + #[test] + fn verify_sig_with_key_accepts_a_genuine_signature() { + let cert_hash = sample_cert_hash(); + let sk = SigningKey::generate(&mut OsRng); + let att = dummy_attestation(&sk, cert_hash); + let bytes = canonical_signing_bytes(&att.type_, &att.payload, &att.cert_hash).expect("jcs"); + att.verify_sig_with_key(&sk.verifying_key(), &bytes) + .expect("a real signature must verify"); + } + /// Control for the guard above: a real did:key must still resolve. #[test] fn verifying_key_from_did_key_still_accepts_a_real_key() { diff --git a/crates/gitlawb-core/src/did.rs b/crates/gitlawb-core/src/did.rs index 0a61c770..29bbdb04 100644 --- a/crates/gitlawb-core/src/did.rs +++ b/crates/gitlawb-core/src/did.rs @@ -71,7 +71,8 @@ impl Did { /// Resolve the Ed25519 verifying key from a `did:key`. /// - /// Returns `Err` if the DID is not a `did:key` or the key bytes are invalid. + /// Returns `Err` if the DID is not a `did:key`, the key bytes are malformed, + /// or the key is a small-order (weak) point. pub fn to_verifying_key(&self) -> Result { if !self.is_did_key() { return Err(Error::InvalidDid(format!( @@ -111,8 +112,12 @@ impl Did { // `from_bytes` only decompresses, so it accepts a small-order point. // Such a key satisfies the verification equation for any message, and - // its Montgomery form is the all-zero X25519 u-coordinate, which makes - // any X25519 shared secret derived from it the all-zero key. Rejecting + // every X25519 shared secret derived from it is the all-zero key: the + // order-1 and order-2 points convert to the all-zero Montgomery u + // directly, and the order-4 and order-8 points are annihilated by the + // scalar clamping instead. `is_weak` covers the whole torsion set. + // Mixed-order points are correctly NOT rejected, since clamping clears + // the cofactor component. Rejecting // at resolution is what makes this the choke point: every consumer that // resolves a DID through here inherits the rejection, and a recipient // that cannot resolve already fails closed downstream. diff --git a/crates/gitlawb-core/src/encrypt.rs b/crates/gitlawb-core/src/encrypt.rs index c1b60a16..cfc354a7 100644 --- a/crates/gitlawb-core/src/encrypt.rs +++ b/crates/gitlawb-core/src/encrypt.rs @@ -12,13 +12,16 @@ use zeroize::Zeroizing; fn x25519_public(vk: &VerifyingKey) -> Result<[u8; 32]> { use curve25519_dalek::edwards::CompressedEdwardsY; - // A small-order point converts to the all-zero Montgomery u, and X25519 - // against u = 0 yields the all-zero shared secret for every scalar, so the - // per-recipient wrap could be rebuilt by anyone. Resolution already refuses - // such a key (see Did::to_verifying_key); this guard makes the primitive - // safe on its own terms for a caller that obtained the key some other way. + // Every X25519 shared secret derived from a small-order point is the + // all-zero key: order-1 and order-2 convert to the all-zero Montgomery u + // directly, order-4 and order-8 are annihilated by the scalar clamping in + // x25519_secret_from_seed. Either way the per-recipient wrap could be + // rebuilt by anyone. Resolution already refuses such a key (see + // Did::to_verifying_key); this guard covers the SEAL side on its own terms + // for a caller that obtained the key some other way. The open side + // (open_blob's attacker-supplied `eph`) is NOT covered here. if vk.is_weak() { - anyhow::bail!("verifying key is a small-order point"); + return Err(anyhow::anyhow!("verifying key is a small-order point")); } let edwards = CompressedEdwardsY::from_slice(vk.as_bytes()) @@ -304,6 +307,16 @@ mod tests { assert!(open_blob(&reframe(&header), &reader).is_err()); } + /// The compressed identity point: a well-formed Ed25519 encoding that is + /// small-order, so every shared secret derived from it is all-zero. + fn weak_verifying_key() -> VerifyingKey { + let mut weak = [0u8; 32]; + weak[0] = 1; + let vk = VerifyingKey::from_bytes(&weak).expect("identity point decompresses"); + assert!(vk.is_weak(), "fixture precondition: key is small-order"); + vk + } + /// A small-order recipient key converts to Montgomery u = 0, and X25519 /// against u = 0 is the all-zero shared secret for ANY scalar, so the /// wrapping box is reconstructable with no secret at all. Reject at the @@ -311,10 +324,7 @@ mod tests { /// obtained the key. #[test] fn x25519_public_rejects_a_small_order_key() { - let mut weak = [0u8; 32]; - weak[0] = 1; // compressed identity point - let weak_vk = VerifyingKey::from_bytes(&weak).expect("identity point decompresses"); - assert!(weak_vk.is_weak(), "precondition: key is small-order"); + let weak_vk = weak_verifying_key(); assert!( x25519_public(&weak_vk).is_err(), @@ -334,9 +344,7 @@ mod tests { /// recipient even when the DID choke point is bypassed entirely. #[test] fn seal_blob_refuses_a_small_order_recipient() { - let mut weak = [0u8; 32]; - weak[0] = 1; - let weak_vk = VerifyingKey::from_bytes(&weak).unwrap(); + let weak_vk = weak_verifying_key(); assert!( seal_blob(b"withheld", &[weak_vk]).is_err(), @@ -371,9 +379,7 @@ mod tests { use crate::did::Did; use std::str::FromStr; - let mut weak_bytes = [0u8; 32]; - weak_bytes[0] = 1; // compressed identity point - let weak_vk = VerifyingKey::from_bytes(&weak_bytes).expect("decompresses"); + let weak_vk = weak_verifying_key(); // Layer 1: the attacker's did:key string must not resolve at all. let weak_did = Did::from_verifying_key(&weak_vk).to_string(); diff --git a/crates/gitlawb-node/src/auth/mod.rs b/crates/gitlawb-node/src/auth/mod.rs index eee8fd8b..1b023bfb 100644 --- a/crates/gitlawb-node/src/auth/mod.rs +++ b/crates/gitlawb-node/src/auth/mod.rs @@ -141,15 +141,24 @@ pub async fn require_signature(request: Request, next: Next) -> Response { let verifying_key = match sig.key_id.to_verifying_key() { Ok(vk) => vk, Err(e) => { + // The hint is only true for a method we have no resolver for. + // A did:key that parsed and then failed on its key material (a + // small-order key, wrong multicodec, wrong length) would be sent + // looking for the wrong problem by it. + let hint = if sig.key_id.is_did_key() { + "the DID is a did:key whose key material did not resolve" + } else { + "only did:key is supported in alpha" + }; return ( StatusCode::BAD_REQUEST, Json(json!({ "error": "unresolvable_did", "message": format!("cannot resolve DID '{}': {e}", sig.key_id), - "hint": "only did:key is supported in alpha", + "hint": hint, })), ) - .into_response() + .into_response(); } }; diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index 6a47160a..c94c4d54 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -1362,11 +1362,25 @@ fn did_resolve_failure_message( #[cfg(test)] mod did_resolve_message_tests { use super::did_resolve_failure_message; + use gitlawb_core::did::Did; + use std::str::FromStr; + + /// A well-formed did:key encoding the compressed identity point, which is + /// small-order. Indistinguishable by eye from a real key: same `z6Mk` + /// prefix, same fixed 48-character method-id. + const WEAK_DID_KEY: &str = "did:key:z6MkeXATEjyXENzBXBxgC5EHk2JE5aqd7qMGGtDpLUH1e2Sj"; #[test] - fn a_did_key_that_fails_to_resolve_does_not_claim_did_key_is_unsupported() { - let err = gitlawb_core::Error::InvalidDid("small-order ed25519 key".to_string()); - let msg = did_resolve_failure_message("did:key:z6MkWeak", true, &err); + fn test_did_key_failure_does_not_claim_did_key_is_unsupported() { + // Drive the real parse and the real resolution error rather than + // hardcoding the discriminator, so flipping it at the call site fails. + let did_str = WEAK_DID_KEY; + let did = Did::from_str(did_str).expect("well-formed did:key"); + let err = did + .to_verifying_key() + .expect_err("a small-order did:key must not resolve"); + + let msg = did_resolve_failure_message(did_str, did.is_did_key(), &err); assert!( !msg.contains("only did:key is supported"), "a did:key must not be told did:key is unsupported: {msg}" @@ -1378,9 +1392,14 @@ mod did_resolve_message_tests { } #[test] - fn a_non_key_method_keeps_the_no_resolver_message() { - let err = gitlawb_core::Error::InvalidDid("expected did:key, got did:web".to_string()); - let msg = did_resolve_failure_message("did:web:example.com", false, &err); + fn test_non_key_method_keeps_the_no_resolver_message() { + let did_str = "did:web:example.com"; + let did = Did::from_str(did_str).expect("did:web parses"); + let err = did + .to_verifying_key() + .expect_err("did:web has no local resolver"); + + let msg = did_resolve_failure_message(did_str, did.is_did_key(), &err); assert!( msg.contains("only did:key is supported"), "an unresolvable method keeps its existing explanation: {msg}" From b9ab8c447ca4c5522d25429ef6f38f8dfe8dc4a1 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 10 Aug 2026 12:09:34 -0500 Subject: [PATCH 06/12] fix(attest): cap the did:key method-id before decoding, mirroring core The parser here mirrors gitlawb-core's small-order rejection because it cannot call into it (gitlawb-core is a dev-dependency of this crate). It was mirroring only one of the two guards that sit three lines apart in the function being copied: core also caps the method-id at 64 bytes BEFORE the multibase decode, because base58 decoding is quadratic in its input. signer comes off an attacker-supplied attestation and verify_signature reaches this parser after computing the JCS bytes, so an oversized method-id bought a large decode for a short request. An ed25519 did:key method-id is a fixed 48 characters, so the bound is slack rather than a behavior change. The test asserts the length error specifically rather than any error: a multibase error there would mean the cap ran after the decode and bought nothing. --- crates/gitlawb-attest/src/attestation.rs | 32 ++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/crates/gitlawb-attest/src/attestation.rs b/crates/gitlawb-attest/src/attestation.rs index 91274f98..9638b236 100644 --- a/crates/gitlawb-attest/src/attestation.rs +++ b/crates/gitlawb-attest/src/attestation.rs @@ -216,6 +216,19 @@ fn verifying_key_from_did_key(did: &str) -> Result { "did:key must use base58btc (z-prefix): {did}" ))); } + // Refuse an oversized id before decoding. base58 decoding is quadratic in + // its input and `signer` comes off an attacker-supplied attestation, so a + // short request could otherwise buy a large decode. An ed25519 did:key + // method-id is a fixed 48 characters, so this bound is slack rather than a + // behavior change, and it only helps ahead of the decode. Mirrors the same + // cap in gitlawb-core's Did::to_verifying_key, which this crate cannot call + // (gitlawb-core is a dev-dependency here). + const MAX_METHOD_ID_LEN: usize = 64; + if method_id.len() > MAX_METHOD_ID_LEN { + return Err(Error::Did( + "did:key method-specific id too long".to_string(), + )); + } let (base, bytes) = multibase::decode(method_id).map_err(|e| Error::Did(format!("multibase: {e}")))?; if base != multibase::Base::Base58Btc { @@ -612,6 +625,25 @@ mod tests { } } + /// base58 decoding is quadratic in its input and `signer` is attacker + /// controlled, so an oversized method-id must be refused BEFORE the decode + /// or a hostile attestation buys a large decode for a short request. The + /// assertion is on the length error specifically: a multibase error here + /// would mean the cap ran too late to matter. + #[test] + fn verifying_key_from_did_key_refuses_an_oversized_method_id_before_decoding() { + let oversized = format!("did:key:z{}", "1".repeat(65_536)); + let err = verifying_key_from_did_key(&oversized) + .expect_err("an oversized method-id must be refused"); + match err { + Error::Did(msg) => assert!( + msg.contains("too long"), + "must fail on the length cap, not after decoding: {msg}" + ), + other => panic!("expected Error::Did, got {other:?}"), + } + } + /// Strictness regression guard. The parser now refuses a small-order key, /// so `verify_signature` can no longer carry a weak signer down to the /// signature check; this drives that check directly to prove it is still From c24a9d5342af1268df8a3c29de7d2287a0abd4e3 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 17 Aug 2026 11:52:26 -0500 Subject: [PATCH 07/12] fix(node): surface stored envelopes sealed to a now-unresolvable recipient plan_seal short-circuited on a matching stored tag before resolving the recipient set, so a blob sealed to a small-order did:key before the choke-point guard landed kept its old CID and was silently treated as unchanged. Resolve the set on the match and surface a now-unresolvable reader as SkipUnresolvableStored so the operator sees the exposure. Co-authored-by: CommandCodeBot --- crates/gitlawb-node/src/encrypted_pin.rs | 85 ++++++++++++++++++++++-- 1 file changed, 79 insertions(+), 6 deletions(-) diff --git a/crates/gitlawb-node/src/encrypted_pin.rs b/crates/gitlawb-node/src/encrypted_pin.rs index 16e4761e..088bd69e 100644 --- a/crates/gitlawb-node/src/encrypted_pin.rs +++ b/crates/gitlawb-node/src/encrypted_pin.rs @@ -69,6 +69,12 @@ fn resolve_all_recipients(dids: &BTreeSet) -> Result, enum SealPlan { /// An existing envelope already covers exactly this recipient set; nothing to do. SkipUnchanged, + /// The stored tag matches, but the stored reader set contains a DID that + /// must not resolve (e.g. a small-order did:key sealed before the choke + /// point landed). The old envelope is already public and cannot be + /// withdrawn, but it must be surfaced for operator action rather than + /// silently skipped as unchanged. + SkipUnresolvableStored(Vec), /// No recipient DID resolved to a key, so there is nothing to seal to. SkipNoRecipients, /// At least one recipient DID is unresolvable. Fail closed: never seal to a @@ -90,15 +96,30 @@ enum SealPlan { /// recover the blob; reader removal is not retroactive (the old envelope is /// already public). The comparison is on the opaque node-keyed tag, never the /// DID list. +/// +/// A matching stored tag is normally a skip, but the set is still resolved +/// before skipping: a reader that used to resolve and now must not (a +/// small-order did:key sealed before the choke point landed) is surfaced as +/// [`SealPlan::SkipUnresolvableStored`] instead of being silently treated as +/// unchanged. The old envelope is already public and cannot be withdrawn, but +/// the operator must be able to see that a persisted envelope was sealed to a +/// weak recipient. fn plan_seal(node_seed: &[u8; 32], dids: &BTreeSet, stored_tag: Option<&str>) -> SealPlan { let tag = recipients_tag(node_seed, dids); if stored_tag == Some(tag.as_str()) { - return SealPlan::SkipUnchanged; - } - match resolve_all_recipients(dids) { - Ok(keys) if keys.is_empty() => SealPlan::SkipNoRecipients, - Ok(keys) => SealPlan::Seal { keys, tag }, - Err(unresolved) => SealPlan::SkipUnresolvable(unresolved), + // The stored envelope already covers exactly this set, so there is + // nothing to re-seal. Resolve anyway so a now-unresolvable reader in an + // unchanged set is surfaced rather than hidden by the tag match. + match resolve_all_recipients(dids) { + Ok(_) => SealPlan::SkipUnchanged, + Err(unresolved) => SealPlan::SkipUnresolvableStored(unresolved), + } + } else { + match resolve_all_recipients(dids) { + Ok(keys) if keys.is_empty() => SealPlan::SkipNoRecipients, + Ok(keys) => SealPlan::Seal { keys, tag }, + Err(unresolved) => SealPlan::SkipUnresolvable(unresolved), + } } } @@ -131,6 +152,21 @@ pub async fn encrypt_and_pin( // covered (which would permanently lock out the dropped readers, #47). let (keys, tag) = match plan_seal(node_seed, dids, stored_tag.as_deref()) { SealPlan::SkipUnchanged => continue, + SealPlan::SkipUnresolvableStored(unresolved) => { + // A stored envelope already covers this exact reader set, and + // the set now contains a DID that must not resolve (a + // small-order did:key sealed before the choke point landed). + // The old envelope is already public and cannot be withdrawn + // or re-sealed; surface it so the operator can act. + let sample: Vec<&String> = unresolved.iter().take(3).collect(); + tracing::warn!( + oid = %oid, + unresolved_count = unresolved.len(), + unresolved_sample = ?sample, + "stored envelope was sealed to a recipient that must not resolve; operator action may be required" + ); + continue; + } SealPlan::SkipNoRecipients => { tracing::warn!(oid = %oid, "no resolvable recipient keys; skipping encrypted pin"); continue; @@ -391,6 +427,43 @@ mod tests { )); } + /// P1 remediation (review): an unchanged reader set is normally a skip, but + /// the skip must not hide a recipient that now fails to resolve. A blob + /// sealed to a small-order did:key before the choke-point guard landed keeps + /// its old CID and stays decryptable-without-a-private-key unless the + /// matching-tag path surfaces it. This pins that the stored-tag match still + /// resolves the set instead of short-circuiting. + #[test] + fn plan_seal_surfaces_unresolvable_recipient_in_unchanged_reader_set() { + let weak = weak_did_key(); + let mut dids = BTreeSet::new(); + dids.insert(did_key(9)); + dids.insert(weak.clone()); + let stored = recipients_tag(&SEED, &dids); + + match plan_seal(&SEED, &dids, Some(&stored)) { + SealPlan::SkipUnresolvableStored(unresolved) => assert!( + unresolved.contains(&weak), + "the weak DID must be named in the unresolved set, got {unresolved:?}" + ), + other => panic!("expected SkipUnresolvableStored, got {other:?}"), + } + } + + /// Degenerate control: an empty set with a matching stored tag stays a + /// silent skip. The state is unreachable in practice (a seal is never + /// recorded for an empty reader set), so this pins only that the + /// resolve-on-match path must not turn it into a re-seal or a warning. + #[test] + fn plan_seal_unchanged_empty_set_still_skips() { + let empty = BTreeSet::new(); + let empty_tag = recipients_tag(&SEED, &empty); + assert!(matches!( + plan_seal(&SEED, &empty, Some(&empty_tag)), + SealPlan::SkipUnchanged + )); + } + #[test] fn plan_seal_reseals_when_recipient_set_changed() { // A stored tag for a DIFFERENT set is a miss: a newly added reader must From 0871dd521b83c8c6fadec956357de024c389c334 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 17 Aug 2026 11:56:29 -0500 Subject: [PATCH 08/12] fix(attest): anchor VerifiedAttestation.signer to the key that verified Registry::verify discarded the VerifyingKey returned by verify_signature and copied the artifact's signer string into the result. The parser is canonical so the strings agree for honest artifacts and a mismatched field already fails verification, but the consumer-facing value now provably comes from the key that verified rather than a raw artifact field. Co-authored-by: CommandCodeBot --- crates/gitlawb-attest/src/attestation.rs | 2 +- crates/gitlawb-attest/src/verifier.rs | 59 ++++++++++++++++++++++-- 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/crates/gitlawb-attest/src/attestation.rs b/crates/gitlawb-attest/src/attestation.rs index 9638b236..5b55ab77 100644 --- a/crates/gitlawb-attest/src/attestation.rs +++ b/crates/gitlawb-attest/src/attestation.rs @@ -195,7 +195,7 @@ fn canonical_signing_bytes( Ok(out) } -fn did_key_from_verifying_key(key: &VerifyingKey) -> String { +pub(crate) fn did_key_from_verifying_key(key: &VerifyingKey) -> String { let mut buf = Vec::with_capacity(ED25519_MULTICODEC.len() + 32); buf.extend_from_slice(&ED25519_MULTICODEC); buf.extend_from_slice(&key.to_bytes()); diff --git a/crates/gitlawb-attest/src/verifier.rs b/crates/gitlawb-attest/src/verifier.rs index c0790c59..f6a2601d 100644 --- a/crates/gitlawb-attest/src/verifier.rs +++ b/crates/gitlawb-attest/src/verifier.rs @@ -121,7 +121,12 @@ impl Registry { attestation: &Attestation, expected_cert_hash: [u8; 32], ) -> Result { - attestation.verify_signature(expected_cert_hash)?; + // The key that actually verified is the trust anchor for the signer a + // consumer sees. `verify_signature` recovers it from the artifact's + // `signer` field and rejects a field that does not match the signing + // key, so this is the canonical DID of the key that signed — never a + // raw artifact string that could disagree with it. + let verified_key = attestation.verify_signature(expected_cert_hash)?; let fully = match self.by_type.get(attestation.type_.as_str()) { Some(v) => { @@ -138,7 +143,7 @@ impl Registry { Ok(VerifiedAttestation { type_: attestation.type_.clone(), - signer: attestation.signer.clone(), + signer: crate::attestation::did_key_from_verifying_key(&verified_key), cert_hash: attestation.cert_hash.clone(), fully_verified: fully, }) @@ -171,7 +176,7 @@ impl Registry { #[cfg(test)] mod tests { use super::*; - use crate::attestation::{Attestation, AttestationPayload}; + use crate::attestation::{did_key_from_verifying_key, Attestation, AttestationPayload}; use ed25519_dalek::SigningKey; use rand::rngs::OsRng; use serde::{Deserialize, Serialize}; @@ -374,4 +379,52 @@ mod tests { let err = reg.verify(&att, cert_hash).unwrap_err(); assert!(matches!(err, Error::Payload(_))); } + + /// The signer a consumer sees must be the canonical DID of the key that + /// actually verified, never a raw artifact string that could disagree. + /// `verify_signature` derives the key from the artifact's `signer` field + /// and the parser is canonical (base58btc, exact length, ed25519 + /// multicodec), so a forged field fails verification outright: the + /// must-not here is that no `VerifiedAttestation` ever carries a signer + /// that did not verify. + #[test] + fn verified_signer_is_anchored_to_the_key_that_verified() { + let signer = fresh(); + let other = fresh(); + assert_ne!( + signer.verifying_key().to_bytes(), + other.verifying_key().to_bytes(), + "fixture precondition: distinct signing keys" + ); + + let cert_hash = sample_hash(); + let reg = Registry::new(); + + // Control: an honest artifact reports the signer's canonical DID, and + // it agrees with the artifact field. + let att = signed_demo(&signer, cert_hash, "ok"); + let v = reg.verify(&att, cert_hash).unwrap(); + let expected = did_key_from_verifying_key(&signer.verifying_key()); + assert_eq!(v.signer, expected, "signer must be the verified key's DID"); + assert_eq!(v.signer, att.signer, "honest artifact: field and key agree"); + + // Must-not, both directions: a signer field naming a DIFFERENT key + // than the one that signed must fail verification, so the artifact + // string can never redirect the reported signer. + let mut forged = signed_demo(&signer, cert_hash, "ok"); + forged.signer = did_key_from_verifying_key(&other.verifying_key()); + let err = reg.verify(&forged, cert_hash).unwrap_err(); + assert!( + matches!(err, Error::Signature(_)), + "a signer field that does not match the signing key must fail, got {err:?}" + ); + + let mut forged_rev = signed_demo(&other, cert_hash, "ok"); + forged_rev.signer = did_key_from_verifying_key(&signer.verifying_key()); + let err_rev = reg.verify(&forged_rev, cert_hash).unwrap_err(); + assert!( + matches!(err_rev, Error::Signature(_)), + "mismatched signer field must fail verification, got {err_rev:?}" + ); + } } From 2896a00bec1ede6e23cc51dc9df72e77944223d6 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 17 Aug 2026 12:01:09 -0500 Subject: [PATCH 09/12] fix(node): bound the echoed DID in the unresolvable-did error and pin the hint branches The keyid is unauthenticated request input and was reflected into the response message at full length. Bound the echo to 96 chars and extract the two-branch hint selection into a testable helper with both branches pinned. Co-authored-by: CommandCodeBot --- crates/gitlawb-node/src/auth/mod.rs | 89 +++++++++++++++++++++++++---- 1 file changed, 78 insertions(+), 11 deletions(-) diff --git a/crates/gitlawb-node/src/auth/mod.rs b/crates/gitlawb-node/src/auth/mod.rs index 1b023bfb..3beb7f14 100644 --- a/crates/gitlawb-node/src/auth/mod.rs +++ b/crates/gitlawb-node/src/auth/mod.rs @@ -34,6 +34,27 @@ use gitlawb_core::http_sig::{ }; use gitlawb_core::identity::verify; +/// The hint for an unresolvable DID, chosen by method. The hint is only true +/// for a method we have no resolver for: a did:key that parsed and then failed +/// on its key material (a small-order key, wrong multicodec, wrong length) +/// would be sent looking for the wrong problem by the alpha-support wording. +fn unresolvable_did_hint(key_id: &Did) -> &'static str { + if key_id.is_did_key() { + "the DID is a did:key whose key material did not resolve" + } else { + "only did:key is supported in alpha" + } +} + +/// Bound the echoed keyid in the unresolvable-DID message. `key_id` is +/// unauthenticated request input, so reflecting it back at whatever length the +/// caller chose would amplify attacker input into the response body. The bound +/// is slack (a did:key method-id is a fixed 48 characters) and only defends +/// the echo, not the resolution. +fn bounded_did_echo(key_id: &str) -> String { + key_id.chars().take(96).collect() +} + /// Axum middleware that enforces HTTP Signature authentication (RFC 9421). /// /// Every write request must carry: @@ -141,21 +162,15 @@ pub async fn require_signature(request: Request, next: Next) -> Response { let verifying_key = match sig.key_id.to_verifying_key() { Ok(vk) => vk, Err(e) => { - // The hint is only true for a method we have no resolver for. - // A did:key that parsed and then failed on its key material (a - // small-order key, wrong multicodec, wrong length) would be sent - // looking for the wrong problem by it. - let hint = if sig.key_id.is_did_key() { - "the DID is a did:key whose key material did not resolve" - } else { - "only did:key is supported in alpha" - }; return ( StatusCode::BAD_REQUEST, Json(json!({ "error": "unresolvable_did", - "message": format!("cannot resolve DID '{}': {e}", sig.key_id), - "hint": hint, + "message": format!( + "cannot resolve DID '{}': {e}", + bounded_did_echo(&sig.key_id.to_string()) + ), + "hint": unresolvable_did_hint(&sig.key_id), })), ) .into_response(); @@ -432,6 +447,58 @@ mod tests { assert!(validate_ucan_chain(&token, &node_did, &agent_did).is_ok()); } + /// The two-branch hint selection is the point of the unresolvable-DID + /// change: a did:key that failed on its key material must be told the key + /// material did not resolve, and any other method must be told only did:key + /// is supported. A regression that drops the is_did_key() check would send + /// a did:key caller after the wrong problem, so both branches are pinned. + #[test] + fn unresolvable_did_hint_distinguishes_key_material_from_method() { + // A real did:key whose key material fails to resolve (small-order): + // key-material hint. + let mut weak = [0u8; 32]; + weak[0] = 1; // compressed identity point + let weak_vk = ed25519_dalek::VerifyingKey::from_bytes(&weak).unwrap(); + let weak_did = Did::from_verifying_key(&weak_vk); + assert!( + weak_did.to_verifying_key().is_err(), + "fixture precondition: small-order did:key must not resolve" + ); + assert_eq!( + unresolvable_did_hint(&weak_did), + "the DID is a did:key whose key material did not resolve" + ); + + // A method with no resolver: alpha-support hint. + let web = Did::web("example.com"); + assert!( + web.to_verifying_key().is_err(), + "fixture precondition: did:web must not resolve locally" + ); + assert_eq!( + unresolvable_did_hint(&web), + "only did:key is supported in alpha" + ); + } + + /// The unresolvable-DID message echoes the keyid, which is unauthenticated + /// request input. The echoed value must be bounded so a hostile caller + /// cannot amplify arbitrary-length input into the response body. + #[test] + fn unresolvable_did_message_bounds_the_echoed_keyid() { + let long_keyid = format!("did:key:z{}", "A".repeat(2000)); + let shown = bounded_did_echo(&long_keyid); + assert!( + shown.len() <= 96, + "echoed keyid must be bounded, got {} chars", + shown.len() + ); + assert!( + !shown.contains(&"A".repeat(1000)), + "the full keyid must not survive into the message" + ); + } + #[test] fn validate_ucan_chain_wrong_issuer() { let node = Keypair::generate(); From 6f2c0724b5a88ee07cd2d1aa09c7d09692c19b55 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 17 Aug 2026 12:05:05 -0500 Subject: [PATCH 10/12] test(gl): build the weak did:key fixture from its identity-point bytes The hardcoded WEAK_DID_KEY constant trips secret scanners, which cannot tell a public DID from an API key. Deriving the DID from the compressed identity point states why the fixture is weak and matches the house pattern used in the core, attest, and node crates. ed25519-dalek is added as a dev-dependency (it was already a workspace dependency). Co-authored-by: CommandCodeBot --- Cargo.lock | 1 + crates/gl/Cargo.toml | 1 + crates/gl/src/mcp.rs | 14 ++++++++++---- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b7050bc6..e8409e18 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3497,6 +3497,7 @@ dependencies = [ "chrono", "clap", "dirs", + "ed25519-dalek", "gitlawb-core", "icaptcha-client", "mockito", diff --git a/crates/gl/Cargo.toml b/crates/gl/Cargo.toml index 3d7ddb9f..9c6e4f5e 100644 --- a/crates/gl/Cargo.toml +++ b/crates/gl/Cargo.toml @@ -42,3 +42,4 @@ alloy = { version = "1", default-features = false, features = [ mockito = "1" tempfile = "3" tokio = { workspace = true } +ed25519-dalek = { workspace = true } diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index c94c4d54..6ca3514e 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -1366,15 +1366,21 @@ mod did_resolve_message_tests { use std::str::FromStr; /// A well-formed did:key encoding the compressed identity point, which is - /// small-order. Indistinguishable by eye from a real key: same `z6Mk` - /// prefix, same fixed 48-character method-id. - const WEAK_DID_KEY: &str = "did:key:z6MkeXATEjyXENzBXBxgC5EHk2JE5aqd7qMGGtDpLUH1e2Sj"; + /// small-order. Derived from the point bytes so the fixture states WHY it + /// is weak rather than pinning an opaque string (the literal also trips + /// secret scanners, which cannot tell a public DID from an API key). + fn weak_did_key() -> String { + let mut weak = [0u8; 32]; + weak[0] = 1; // compressed identity point + let vk = ed25519_dalek::VerifyingKey::from_bytes(&weak).expect("decompresses"); + Did::from_verifying_key(&vk).to_string() + } #[test] fn test_did_key_failure_does_not_claim_did_key_is_unsupported() { // Drive the real parse and the real resolution error rather than // hardcoding the discriminator, so flipping it at the call site fails. - let did_str = WEAK_DID_KEY; + let did_str = &weak_did_key(); let did = Did::from_str(did_str).expect("well-formed did:key"); let err = did .to_verifying_key() From 8164dbad7876a1fa98367589f1a70a0d46eb8564 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 17 Aug 2026 12:10:42 -0500 Subject: [PATCH 11/12] fix(core): skip a low-order ephemeral on the envelope open side open_blob built the X25519 box from the header-supplied eph with no small-order check, so a crafted entry forced the all-zero shared secret and unwrapped for any reader. Skip entries whose eph decompresses to a small-order point; honest entries in the same envelope still open. Co-authored-by: CommandCodeBot --- crates/gitlawb-core/src/encrypt.rs | 152 ++++++++++++++++++++++++++++- 1 file changed, 150 insertions(+), 2 deletions(-) diff --git a/crates/gitlawb-core/src/encrypt.rs b/crates/gitlawb-core/src/encrypt.rs index cfc354a7..621fb628 100644 --- a/crates/gitlawb-core/src/encrypt.rs +++ b/crates/gitlawb-core/src/encrypt.rs @@ -19,7 +19,8 @@ fn x25519_public(vk: &VerifyingKey) -> Result<[u8; 32]> { // rebuilt by anyone. Resolution already refuses such a key (see // Did::to_verifying_key); this guard covers the SEAL side on its own terms // for a caller that obtained the key some other way. The open side - // (open_blob's attacker-supplied `eph`) is NOT covered here. + // (open_blob's attacker-supplied `eph`) is covered by + // is_low_order_montgomery. if vk.is_weak() { return Err(anyhow::anyhow!("verifying key is a small-order point")); } @@ -31,6 +32,23 @@ fn x25519_public(vk: &VerifyingKey) -> Result<[u8; 32]> { Ok(edwards.to_montgomery().to_bytes()) } +/// True when a 32-byte Montgomery u-coordinate is a small-order point, i.e. a +/// point whose X25519 shared secret is the all-zero key for any scalar. +/// The header-supplied `eph` in an envelope is attacker-controlled, so the +/// open side must not derive a shared secret from one: an entry built on a +/// low-order ephemeral unwraps for any reader. u = 0 and u = 1 are the +/// u-coordinates that decompress to a small-order Edwards point (u = 0 is the +/// all-zero Montgomery u); the remaining small-order u values fail to +/// decompress and are already skipped by the decode path in `open_blob`. +fn is_low_order_montgomery(u: &[u8; 32]) -> bool { + use curve25519_dalek::montgomery::MontgomeryPoint; + + MontgomeryPoint(*u) + .to_edwards(0u8) + .map(|p| p.is_small_order()) + .unwrap_or(false) +} + /// X25519 secret scalar for an Ed25519 seed (SHA-512 of seed, lower 32, clamped). /// Returns the scalar wrapped in `Zeroizing`, and scrubs the intermediate /// SHA-512 digest, so no copy of this secret material lingers in freed memory. @@ -153,7 +171,12 @@ pub fn open_blob(envelope: &[u8], keypair: &Keypair) -> Result> { .ok() .and_then(|b| <[u8; 32]>::try_from(b.as_slice()).ok()) { - Some(b) => XPublic::from(b), + // A small-order ephemeral public forces the all-zero shared + // secret, so the entry would unwrap for anyone. Skip it. + // A small-order ephemeral public forces the all-zero shared + // secret, so the entry would unwrap for anyone. Skip it. + Some(b) if !is_low_order_montgomery(&b) => XPublic::from(b), + Some(_) => continue, None => continue, }; // from_slice panics on a wrong length, and the envelope is attacker @@ -307,6 +330,131 @@ mod tests { assert!(open_blob(&reframe(&header), &reader).is_err()); } + /// A low-order ephemeral public forces the all-zero shared secret, so an + /// entry built on it would unwrap for anyone. The header's `eph` is + /// attacker-controlled, so the open side must skip it. u = 0 and u = 1 are + /// the Montgomery u-coordinates that decompress to a small-order Edwards + /// point (u = 0 is the one that produces the all-zero shared secret); the + /// other small-order u values fail to decompress and are already skipped + /// by the decode path. + #[test] + fn is_low_order_montgomery_flags_small_order_and_accepts_real() { + let zero = [0u8; 32]; + assert!(is_low_order_montgomery(&zero), "u=0 is the all-zero secret"); + let mut one = [0u8; 32]; + one[0] = 1; + assert!(is_low_order_montgomery(&one), "u=1 is small-order"); + + // A real X25519 public key is not low-order. + let kp = Keypair::generate(); + let real_u = x25519_public(&kp.verifying_key()).unwrap(); + assert!( + !is_low_order_montgomery(&real_u), + "a real key must not be flagged low-order" + ); + } + + /// The open side must skip an entry whose header-supplied `eph` is a + /// low-order point: such an entry forces the all-zero shared secret and + /// would unwrap for any reader. A poisoned entry among honest ones must + /// not DoS the envelope — the honest entry still opens. + #[test] + fn open_blob_skips_a_low_order_ephemeral_and_still_opens_honest_entry() { + use curve25519_dalek::montgomery::MontgomeryPoint; + let reader = Keypair::generate(); + let env = seal_blob(b"private blob contents", &[reader.verifying_key()]).unwrap(); + + // Split the envelope framing into header JSON and body. + let mut p = MAGIC.len() + 1; + let hlen = u32::from_le_bytes(env[p..p + 4].try_into().unwrap()) as usize; + p += 4; + let header_bytes = &env[p..p + hlen]; + let body = &env[p + hlen..]; + + // A low-order Montgomery u (u = 0) encoded as the entry's eph. + let low_order_u = MontgomeryPoint([0u8; 32]).to_bytes(); + let poisoned = serde_json::json!({ + "eph": B64.encode(low_order_u), + "nonce": B64.encode([0u8; 24]), + "wrap": B64.encode([0u8; 32]), + }); + + let reframe = |header: &serde_json::Value| -> Vec { + let hj = serde_json::to_vec(header).unwrap(); + let mut out = Vec::new(); + out.extend_from_slice(MAGIC); + out.push(VERSION); + out.extend_from_slice(&(hj.len() as u32).to_le_bytes()); + out.extend_from_slice(&hj); + out.extend_from_slice(body); + out + }; + + let mut header: serde_json::Value = serde_json::from_slice(header_bytes).unwrap(); + // Prepend the poisoned entry. The reader's honest entry must still + // unwrap, so the envelope opens despite the low-order entry. + let recipients = header["recipients"].as_array_mut().unwrap(); + recipients.insert(0, poisoned); + assert_eq!( + open_blob(&reframe(&header), &reader).unwrap(), + b"private blob contents", + "the honest entry must still open despite a low-order eph entry" + ); + } + + /// Must-not for the open-side guard: an envelope whose ONLY entry is built + /// on a low-order ephemeral must not open for any reader. Without the + /// guard the all-zero shared secret unwraps for everyone, so the attacker + /// controls the plaintext and the envelope's confidentiality is void. + #[test] + fn open_blob_rejects_a_poisoned_only_envelope() { + use chacha20poly1305::{aead::Aead, KeyInit, XChaCha20Poly1305, XNonce}; + use crypto_box::aead::AeadCore; + use curve25519_dalek::montgomery::MontgomeryPoint; + + // Build a poisoned envelope from scratch: one recipient entry whose + // eph is the low-order u = 0. The wrap is the encryption of a content + // key under the all-zero shared secret, so without the guard any + // reader's ChaChaBox against u = 0 decrypts it. + let content_key = [0x42u8; 32]; + let zero_box = ChaChaBox::new(&XPublic::from([0u8; 32]), &XSecret::from([7u8; 32])); + let nonce = ChaChaBox::generate_nonce(&mut OsRng); + let wrap = zero_box.encrypt(&nonce, &content_key[..]).unwrap(); + + let body_nonce = [0x24u8; 24]; + let body_cipher = XChaCha20Poly1305::new_from_slice(&content_key).unwrap(); + let body = body_cipher + .encrypt( + XNonce::from_slice(&body_nonce), + b"attacker-chosen plaintext".as_slice(), + ) + .unwrap(); + + let header = serde_json::json!({ + "alg": "xchacha20poly1305", + "nonce": B64.encode(body_nonce), + "recipients": [{ + "eph": B64.encode(MontgomeryPoint([0u8; 32]).to_bytes()), + "nonce": B64.encode(nonce), + "wrap": B64.encode(wrap), + }], + }); + let header_json = serde_json::to_vec(&header).unwrap(); + let mut env = Vec::new(); + env.extend_from_slice(MAGIC); + env.push(VERSION); + env.extend_from_slice(&(header_json.len() as u32).to_le_bytes()); + env.extend_from_slice(&header_json); + env.extend_from_slice(&body); + + let reader = Keypair::generate(); + let err = open_blob(&env, &reader).unwrap_err(); + assert!( + err.to_string().contains("not a recipient"), + "a poisoned-only envelope must not open for anyone, got: {err}" + ); + } + /// The compressed identity point: a well-formed Ed25519 encoding that is /// small-order, so every shared secret derived from it is all-zero. fn weak_verifying_key() -> VerifyingKey { From bbab74f2fa63ef6adcab4b7ba4bf957b1b4f43a5 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:31:54 -0500 Subject: [PATCH 12/12] fix(core): test the exchange result, not the ephemeral's encoding The open-side guard decompressed the ephemeral to Edwards and asked is_small_order, treating a failure to decompress as safe. That reports safe for any low-order input that is not a valid Edwards encoding. Measured across the seven standard low-order encodings: six caught, u = p-1 missed. Its exchange with any reader is still the all-zero shared secret, so an entry built on it unwraps for everyone and the attacker controls the plaintext. Check the result instead: an ephemeral is rejected when the exchange with this reader's own scalar is all-zero. That is the property the guard exists to enforce, and it holds for every low-order input regardless of encoding. The new must-not test needed two attempts. The first passed with the guard, without it, and with the original pre-fix guard alike, because a blanket `use chacha20poly1305::aead::Aead` in the test resolved the wrap's encrypt through a different aead version than open_blob decrypts with, building a ciphertext that could never open for any reason. Every call in it now names its trait. --- crates/gitlawb-core/src/encrypt.rs | 170 ++++++++++++++++++++++++----- 1 file changed, 143 insertions(+), 27 deletions(-) diff --git a/crates/gitlawb-core/src/encrypt.rs b/crates/gitlawb-core/src/encrypt.rs index 621fb628..6f930b4f 100644 --- a/crates/gitlawb-core/src/encrypt.rs +++ b/crates/gitlawb-core/src/encrypt.rs @@ -32,21 +32,24 @@ fn x25519_public(vk: &VerifyingKey) -> Result<[u8; 32]> { Ok(edwards.to_montgomery().to_bytes()) } -/// True when a 32-byte Montgomery u-coordinate is a small-order point, i.e. a -/// point whose X25519 shared secret is the all-zero key for any scalar. -/// The header-supplied `eph` in an envelope is attacker-controlled, so the -/// open side must not derive a shared secret from one: an entry built on a -/// low-order ephemeral unwraps for any reader. u = 0 and u = 1 are the -/// u-coordinates that decompress to a small-order Edwards point (u = 0 is the -/// all-zero Montgomery u); the remaining small-order u values fail to -/// decompress and are already skipped by the decode path in `open_blob`. -fn is_low_order_montgomery(u: &[u8; 32]) -> bool { +/// True when the X25519 exchange between an attacker-supplied `u` and this +/// reader's scalar yields the all-zero shared secret. +/// +/// Asked of the RESULT, not of the encoding, and that distinction is the whole +/// guard. The previous version decompressed `u` to an Edwards point and asked +/// `is_small_order`, which reports "safe" for any low-order encoding that does +/// not decompress at all. Measured across the seven standard low-order +/// encodings, six were caught and `u = p - 1` was not: it fails to decompress, +/// so the check returned false, and the exchange is still the all-zero secret. +/// An entry built on it unwraps for every reader. +/// +/// Enumerating encodings cannot be exhaustive, because the attack depends on the +/// shared secret rather than on the bytes that produced it, and twist and +/// non-canonical inputs keep arriving. Asking the result needs no list. +fn yields_all_zero_shared_secret(u: &[u8; 32], scalar: &[u8; 32]) -> bool { use curve25519_dalek::montgomery::MontgomeryPoint; - MontgomeryPoint(*u) - .to_edwards(0u8) - .map(|p| p.is_small_order()) - .unwrap_or(false) + MontgomeryPoint(*u).mul_clamped(*scalar).to_bytes() == [0u8; 32] } /// X25519 secret scalar for an Ed25519 seed (SHA-512 of seed, lower 32, clamped). @@ -159,7 +162,10 @@ pub fn open_blob(envelope: &[u8], keypair: &Keypair) -> Result> { .context("decode header")?; let body = &envelope[p + hlen..]; - let my_x = XSecret::from(*x25519_secret_from_seed(&keypair.to_seed())); + // The raw scalar is kept alongside the box secret so the exchange can be + // tested BEFORE a box is built from an attacker-supplied ephemeral. + let my_x_scalar = x25519_secret_from_seed(&keypair.to_seed()); + let my_x = XSecret::from(*my_x_scalar); // Identities are blinded: no entry says which recipient it belongs to, so // try each one. The ChaChaBox AEAD tag authenticates, so exactly the @@ -171,11 +177,11 @@ pub fn open_blob(envelope: &[u8], keypair: &Keypair) -> Result> { .ok() .and_then(|b| <[u8; 32]>::try_from(b.as_slice()).ok()) { - // A small-order ephemeral public forces the all-zero shared - // secret, so the entry would unwrap for anyone. Skip it. - // A small-order ephemeral public forces the all-zero shared - // secret, so the entry would unwrap for anyone. Skip it. - Some(b) if !is_low_order_montgomery(&b) => XPublic::from(b), + // An ephemeral whose exchange with this reader is the all-zero + // shared secret would unwrap for anyone, so the entry is skipped. + // Tested on the exchange itself: an encoding-shaped check misses + // every low-order input that does not decompress to Edwards. + Some(b) if !yields_all_zero_shared_secret(&b, &my_x_scalar) => XPublic::from(b), Some(_) => continue, None => continue, }; @@ -338,19 +344,58 @@ mod tests { /// other small-order u values fail to decompress and are already skipped /// by the decode path. #[test] - fn is_low_order_montgomery_flags_small_order_and_accepts_real() { - let zero = [0u8; 32]; - assert!(is_low_order_montgomery(&zero), "u=0 is the all-zero secret"); + fn all_zero_shared_secret_is_detected_for_every_standard_low_order_encoding() { + // The seven standard X25519 low-order encodings, driven as a set rather + // than as the two that happen to decompress. The encoding-shaped check + // this replaced reported `p - 1` as safe, and its exchange is all-zero. let mut one = [0u8; 32]; one[0] = 1; - assert!(is_low_order_montgomery(&one), "u=1 is small-order"); - - // A real X25519 public key is not low-order. + let hex = |h: &str| -> [u8; 32] { + let mut out = [0u8; 32]; + for i in 0..32 { + out[i] = u8::from_str_radix(&h[i * 2..i * 2 + 2], 16).unwrap(); + } + out + }; + let vectors = [ + ("u=0", [0u8; 32]), + ("u=1", one), + ( + "order8-a", + hex("e0eb7a7c3b41b8ae1656e3faf19fc46ada098deb9c32b1fd866205165f49b800"), + ), + ( + "order8-b", + hex("5f9c95bca3508c24b1d0b1559c83ef5b04445cc4581c8e86d8224eddd09f1157"), + ), + ( + "p-1", + hex("ecffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f"), + ), + ( + "p", + hex("edffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f"), + ), + ( + "p+1", + hex("eeffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f"), + ), + ]; let kp = Keypair::generate(); + let scalar = x25519_secret_from_seed(&kp.to_seed()); + for (name, u) in vectors { + assert!( + yields_all_zero_shared_secret(&u, &scalar), + "{name} is a low-order encoding and must be detected" + ); + } + + // And the other direction, or a guard that returned true always would + // pass everything above while making the envelope unopenable. let real_u = x25519_public(&kp.verifying_key()).unwrap(); assert!( - !is_low_order_montgomery(&real_u), - "a real key must not be flagged low-order" + !yields_all_zero_shared_secret(&real_u, &scalar), + "a real key must not be treated as low-order" ); } @@ -455,6 +500,77 @@ mod tests { ); } + /// The same must-not, driven with the low-order encoding that does NOT map + /// to an Edwards point. + /// + /// This is the case an encoding-shaped guard cannot see. `u = p - 1` is a + /// legal X25519 input whose exchange is the all-zero secret for any scalar, + /// and `MontgomeryPoint::to_edwards` returns None for it, so a check written + /// as "decompress, then ask is_small_order" reports it as safe. Measured + /// across the seven standard low-order encodings: six are caught that way + /// and this one is not. + /// + /// Found by a cross-family review of the head that first added the guard, + /// and the comment beside that guard asserted this case was already handled + /// by a decode path that does not exist. That is why the fix rejects on the + /// RESULT of the exchange rather than on a list of encodings: the result is + /// what the attack actually depends on, and it needs no enumeration to be + /// exhaustive. + #[test] + fn open_blob_rejects_a_non_edwards_low_order_ephemeral() { + // No blanket `use ...::Aead` here. Two aead versions are reachable, and a + // blanket import silently resolved the wrap through the one open_blob does + // NOT decrypt with, producing a ciphertext that could never open. The test + // then passed with the guard, without it, and with the original pre-fix + // guard alike, proving nothing. Every call below names its trait. + use chacha20poly1305::{KeyInit, XChaCha20Poly1305, XNonce}; + use crypto_box::aead::AeadCore; + + // u = p - 1 = 2^255 - 20, little endian. + let mut eph = [0xffu8; 32]; + eph[0] = 0xec; + eph[31] = 0x7f; + + let content_key = [0x42u8; 32]; + let zero_box = ChaChaBox::new(&XPublic::from([0u8; 32]), &XSecret::from([7u8; 32])); + let nonce = ChaChaBox::generate_nonce(&mut OsRng); + let wrap = crypto_box::aead::Aead::encrypt(&zero_box, &nonce, &content_key[..]).unwrap(); + + let body_nonce = [0x24u8; 24]; + let body_cipher = XChaCha20Poly1305::new_from_slice(&content_key).unwrap(); + let body = chacha20poly1305::aead::Aead::encrypt( + &body_cipher, + XNonce::from_slice(&body_nonce), + b"attacker-chosen plaintext".as_slice(), + ) + .unwrap(); + + let header = serde_json::json!({ + "alg": "xchacha20poly1305", + "nonce": B64.encode(body_nonce), + "recipients": [{ + "eph": B64.encode(eph), + "nonce": B64.encode(nonce), + "wrap": B64.encode(wrap), + }], + }); + let header_json = serde_json::to_vec(&header).unwrap(); + let mut env = Vec::new(); + env.extend_from_slice(MAGIC); + env.push(VERSION); + env.extend_from_slice(&(header_json.len() as u32).to_le_bytes()); + env.extend_from_slice(&header_json); + env.extend_from_slice(&body); + + let reader = Keypair::generate(); + let err = open_blob(&env, &reader).unwrap_err(); + assert!( + err.to_string().contains("not a recipient"), + "an envelope whose only entry uses a non-Edwards low-order eph must not \ + open for anyone, got: {err}" + ); + } + /// The compressed identity point: a well-formed Ed25519 encoding that is /// small-order, so every shared secret derived from it is all-zero. fn weak_verifying_key() -> VerifyingKey {