Harden malformed key inputs → parameter-named ArgumentException (preview.3)

[moisesja]

Summary

Addresses a security review of the key-handling surface. Three findings:

1. Medium — malformed key inputs leaked backend exceptions. Several public paths passed caller bytes into NSec, Nethermind BLS, or the platform EC import before length validation, surfacing System.FormatException, Nethermind.Crypto.Bls+BlsException, and macOS AppleCommonCryptoCryptographicException instead of a parameter-named ArgumentException (NFR-3 violation). The fuzz suite pinned part of this in a KnownBackendDeviations allow-list.
2. Low/Medium — invalid Ed25519 → X25519 conversion. DeriveX25519PublicKeyFromEd25519 only checked length and y = 1; an all-zero (low-order) Ed25519 key minted X25519 public key 01 00 … 00.
3. Low — RUSTSEC-2026-0097 (transitive rand 0.8.5 via zkryptium 0.6.1).

Changes

• RawKeyGuard.RequireLength — up-front length validation at every backend hand-off:
  • Ed25519 Sign/Verify/Restore (32)
  • X25519 KeyAgreement/DeriveSharedSecret/Restore (private + public 32)
  • NIST EC private key via the ImportEcPrivateKey chokepoint (EcScalarByteLength: P-256=32, P-384=48, P-521=66)
  • BLS Sign/Restore (32) + try/catch mapping BlsException → ArgumentException(privateKey) (covers the valid-length/invalid-scalar case too)
• Low-order rejection in DeriveX25519PublicKeyFromEd25519 — rejects the five canonical Curve25519 low-order u-coordinates (the all-zero key maps to u=1), throwing ArgumentException(ed25519PublicKey).
• Deleted the KnownBackendDeviations allow-list in InputValidationFuzzTests — no more pinned NFR-3 breaches; any non-contract exception now fails the suite.
• New KeyInputValidationTests — .WithParameterName(...) assertions across every hardened path, low-order rejection, and a valid-key round-trip sanity check.
• Docs — tightened PRD NFR-3 AC; documented RUSTSEC-2026-0097 remediation is gated on a zkryptium release bumping rand past 0.8.x (no in-tree fix); recorded lesson L5.
• Version → 1.0.0-preview.3.

Consistency follow-up

A correctly-sized but out-of-range NIST scalar (D = 0 or D ≥ n) is now also normalized: ImportEcPrivateKey range-checks 0 < D < n against the verified P-256/384/521 curve orders and throws ArgumentException("privateKey"), instead of falling through to the opaque platform CryptographicException. This matches the BLS/secp256k1 invalid-scalar paths (curve orders verified against published FIPS 186-4 / SEC 2 values; boundary confirmed: D=n−1 accepted, D=n rejected).

Verification

• dotnet build NetCrypto.sln -c Release clean; native-present leg (--filter "Category!=BbsAbsent") → 786/786 passed.
• Adversarial review (per AGENTS.md): a sweep over 8 key types × 18 malformed lengths × {all-zero, all-0xFF, random} × every byte-oriented entry point found only in-contract exception types — no FormatException/BlsException/IndexOutOfRange/NullReference/Overflow leaks. Algebraically-constructed low-order-target inputs were all rejected (0 bypasses); 1000 real Ed25519 keys still convert; 200 P-521 keys confirm the 66-byte scalar length (no false rejection).

Note

Based on main after PR #3 merged (preview.2 → preview.3, linear). The tasks/todo20260613-105639.md audit-notes file is the security review's own record (with a resolution section appended); it was already untracked in the repo.

🤖 Generated with Claude Code

[moisesja]

Review — looks good to merge ✅

Reviewed the diff and spot-checked the security-sensitive parts against the source. This is a focused, well-scoped hardening change with strong test coverage. No blocking concerns.

What I verified:

• Low-order rejection is correct and complete. The LowOrderX25519U set matches the canonical Curve25519 small-subgroup blacklist — {0, 1, p−1} plus the two order-8 u-coordinates — after reduction mod p. The all-zero Ed25519 key (y=0) does map to u = (1+0)/(1−0) = 1, which is in the set, so it's correctly rejected. The pre-existing y = 1 (denominator-zero) guard and this new check together cover the degenerate cases.
• EC scalar lengths are right (P-256=32, P-384=48, P-521=66 bytes), and ImportEcPrivateKey is the correct single chokepoint for sign/derive/restore.
• BLS try/catch mapping covers both wrong-length and valid-length-but-invalid-scalar (e.g. all-zero) cases, normalizing BlsException → ArgumentException(privateKey). The dedicated zero-scalar test pins that.
• Deleting KnownBackendDeviations is the right call — the allow-list was exactly the kind of pinned-deviation footgun that lets a real NFR-3 gap stay green. With it gone, any leaked backend exception now fails the fuzz suite.

Things I noticed (non-blocking):

• ImportEcPublicKey isn't length-guarded the way the private-key path is, but with the allow-list removed the fuzz suite would now fail on any leaked exception there, and CI is green — so it's covered by contract. Worth a one-line confirmation that wrong-length EC public keys surface as ArgumentException/false rather than a backend type.
• The scope decision to leave valid-length-but-out-of-range NIST D as a platform CryptographicException is defensible and clearly documented — a genuine crypto failure vs. a malformed-length input. No objection.

CI: ubuntu, macOS, and the no-native (BBS-absent) legs all passed. Only build-test (windows-latest) is still in progress — worth confirming it goes green before merge, since the macOS-specific AppleCommonCryptoCryptographicException finding makes cross-platform exception behavior load-bearing here.

Nice work — the lesson capture (L5) and the tightened PRD AC make the stricter contract durable rather than a one-off fix.

---

Generated by Claude Code