diff --git a/CHANGELOG.md b/CHANGELOG.md index d0cec54..692d3e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,33 @@ All notable changes to didcomm-dotnet are documented here. Format follows ## [Unreleased] +### Fixed — receive-path correctness (`feat/security-receive-correctness`) + +Resolves three Low-severity audit findings on the unpack/JOSE receive surface (GitHub issues +#22, #23, #24). Each ships with tests and an adversarial red-team pass; full suite (615 tests) green. + +- **Unpack honors its exception contract for non-canonical field lengths (#22).** `EnvelopeReader` + gains a defensive `ArgumentException → MalformedMessageException` boundary catch at both the JWS and + JWE delegation points (neutral message; `InnerException` preserved). Note: the originally-reported + escape is **already resolved upstream** — the delegated `DataProofsDotnet.Jose` parser now wraps the + AEAD's wrong-length-iv/tag `ArgumentException` (and base64 `FormatException`) as `MalformedJoseException`. + Covered by a regression test on the unpack contract plus a unit test that drives the boundary catch + directly (a throwing consumer lookup → `MalformedMessageException`). +- **Strict JOSE base64url decoding (#24).** `Base64Url.Decode` now rejects `=` padding, embedded ASCII + whitespace, and standard-base64 `+`/`/` (anything outside `[A-Za-z0-9-_]`) with `FormatException`, + per RFC 7515 §2 / RFC 4648 §3.2-3.3, closing a parser-differential / non-canonical-encoding gap on + the strict-no-pad paths: `from_prior` JWTs, OOB URLs (FR-OOB-02), and JWK material. **Attachment + `data.base64` is deliberately kept lenient** (new `Base64Url.DecodeRelaxed`): FR-ATT-02 imposes no + no-pad requirement and the mediator only relays bytes the recipient re-parses strictly, so a peer's + padded forward attachment is still relayed rather than refused (PR #38 review). Either way a + genuinely-invalid forward attachment maps to `MalformedMessageException` rather than escaping the + mediator as a raw `FormatException` (red-team follow-up). +- **`UnpackResult.AnonymousSender` derived from the outermost encrypt layer (#23).** The flag is now + computed from the outermost encrypt layer's alg (matching its documented semantics) instead of + OR-accumulating across layers. The contradiction the finding described (`authcrypt(anoncrypt)` → + both flags true) was already unreachable after the #17 composition gate; this makes the code match + its contract by construction. + ### Security — Medium-severity audit remediation (`feat/security-medium-cluster`) Remediates the five Medium-severity findings from the multi-agent security & compliance audit diff --git a/src/DidComm.Core/Composition/EnvelopeReader.cs b/src/DidComm.Core/Composition/EnvelopeReader.cs index 594e920..e2ed3d2 100644 --- a/src/DidComm.Core/Composition/EnvelopeReader.cs +++ b/src/DidComm.Core/Composition/EnvelopeReader.cs @@ -147,10 +147,10 @@ public static UnpackResult Unpack( Authenticated: authenticated, NonRepudiation: nonRepudiation, // The two flags are independent (matches SICPA reference impl): - // anonymous reflects whether the *outermost* encrypt layer was - // anoncrypt (true) or authcrypt (false); authenticated reflects - // whether any layer (signed or authcrypt) bound a sender identity. - // anon-encrypt + inner-sign sets both flags. + // anonymous is derived strictly from the *outermost* encrypt layer's alg — + // anoncrypt (true) or authcrypt (false), set once when that layer is unwrapped + // (#23) — while authenticated reflects whether any layer (signed or authcrypt) + // bound a sender identity. anoncrypt-encrypt + inner-sign sets both flags. AnonymousSender: anonymous, ContentEncryption: contentEnc, KeyWrap: keyWrap, @@ -180,6 +180,14 @@ public static UnpackResult Unpack( { throw new CryptoException(ex.Message, ex); } + catch (ArgumentException ex) + { + // Defensive boundary guard (#22, FR-API-07): map any ArgumentException surfacing + // from the delegated JWS parse (a non-canonical field length, or a throwing + // consumer signer lookup) to the documented unpack contract, so a raw + // ArgumentException never escapes UnpackAsync. InnerException is preserved. + throw new MalformedMessageException("Malformed JWS.", ex); + } // Fail closed: a verified JWS MUST surface a signer kid. Otherwise FR-CONSIST-03 // (signed 'from' ↔ signer) and FR-CONSIST-05 (inner signer ↔ skid) would silently @@ -224,13 +232,34 @@ public static UnpackResult Unpack( { throw new CryptoException(ex.Message, ex); } + catch (ArgumentException ex) + { + // Defensive boundary guard (#22, FR-API-07). The delegated DataProofsDotnet.Jose + // parser currently wraps the AEAD's wrong-length-iv/tag ArgumentException as + // MalformedJoseException (caught above), but EnvelopeReader's contract promises + // only MalformedMessageException/CryptoException — so map ANY ArgumentException + // surfacing from the delegated parse to the contract type rather than let a raw + // one escape UnpackAsync. Neutral message (the cause may be a field length or a + // throwing consumer lookup); the original is preserved as InnerException. + throw new MalformedMessageException("Malformed JWE.", ex); + } + // The loop unwraps outermost→inner, so the first encrypt layer is the outermost one. + // AnonymousSender is documented as "the outermost encrypt layer was anoncrypt", so + // derive it from THIS layer's alg only when it's the outermost encrypt — not by + // OR-accumulating across layers (#23). For legal shapes the #17 gate already + // guarantees anoncrypt-if-present is outermost; deriving here keeps the flag correct + // by construction regardless. + var isOutermostEncryptLayer = !encrypted; encrypted = true; contentEnc = jweResult.ContentEncryption; keyWrap = jweResult.Algorithm; recipientKid = jweResult.RecipientKid; allRecipientKids = jweResult.AllRecipientKids; + if (isOutermostEncryptLayer) + anonymous = !jweResult.IsAuthenticated; + if (jweResult.IsAuthenticated) { authenticated = true; @@ -239,7 +268,6 @@ public static UnpackResult Unpack( } else { - anonymous = true; shape.Add(LayerShape.AnonEncrypt); } diff --git a/src/DidComm.Core/Jose/Base64Url.cs b/src/DidComm.Core/Jose/Base64Url.cs index 94a488b..65ab9e2 100644 --- a/src/DidComm.Core/Jose/Base64Url.cs +++ b/src/DidComm.Core/Jose/Base64Url.cs @@ -27,8 +27,44 @@ public static string EncodeUtf8(string utf8String) } /// Decode from base64url without padding to bytes. - /// Base64url string (no padding). + /// Strict base64url string: no padding, no whitespace, alphabet [A-Za-z0-9-_]. + /// When contains a non-base64url character or non-canonical trailing bits. public static byte[] Decode(string value) + { + ArgumentException.ThrowIfNullOrEmpty(value); + + // Strict JOSE base64url (RFC 7515 §2 / RFC 4648 §3.2-3.3): no padding, no line breaks, no + // whitespace, and not standard-base64 '+'/'/'. The BCL codec tolerates trailing '=' padding + // and silently strips embedded ASCII whitespace, so pre-validate the alphabet here and reject + // anything outside [A-Za-z0-9-_] before delegating (#24). The BCL still enforces the + // non-canonical-trailing-bits check. + foreach (var c in value) + { + if (!IsBase64UrlChar(c)) + { + throw new FormatException( + "Input is not strict base64url: a character outside the [A-Za-z0-9-_] alphabet was found " + + "(RFC 7515 §2 forbids padding, whitespace, and '+'/'/')."); + } + } + + return SystemBase64Url.DecodeFromChars(value.AsSpan()); + } + + private static bool IsBase64UrlChar(char c) + => (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || c == '-' || c == '_'; + + /// + /// Decode tolerating '=' padding and ASCII whitespace (the BCL + /// codec's lenient behavior). Use ONLY for transport/attachment payloads where padding is + /// historically permitted and not spec-forbidden — DIDComm attachment data.base64 + /// (FR-ATT-02; cf. Aries RFC 0017), which the mediator merely relays for the recipient to + /// re-parse. JOSE fields (JWS/JWE segments, JWK material), from_prior JWTs, and OOB URLs + /// have an explicit no-pad requirement and MUST use the strict . + /// + /// Base64url string, possibly padded. + /// When the input is not valid base64url even allowing padding/whitespace. + public static byte[] DecodeRelaxed(string value) { ArgumentException.ThrowIfNullOrEmpty(value); return SystemBase64Url.DecodeFromChars(value.AsSpan()); diff --git a/src/DidComm.Core/Protocols/Routing/ForwardProcessor.cs b/src/DidComm.Core/Protocols/Routing/ForwardProcessor.cs index a401016..5ea9e99 100644 --- a/src/DidComm.Core/Protocols/Routing/ForwardProcessor.cs +++ b/src/DidComm.Core/Protocols/Routing/ForwardProcessor.cs @@ -116,7 +116,22 @@ private static byte[] ExtractAttachmentBytes(Messages.Attachment attachment) return Encoding.UTF8.GetBytes(json.ToJsonString()); if (!string.IsNullOrEmpty(attachment.Data.Base64)) - return Base64Url.Decode(attachment.Data.Base64); + { + try + { + // Relaxed decode: attachment data.base64 has no spec no-pad requirement (FR-ATT-02, + // cf. Aries RFC 0017), and a mediator only relays the bytes for the recipient to + // re-parse strictly — so tolerate '=' padding rather than refuse a peer's forward. + // Genuinely-invalid base64 still maps to the documented malformed-input type (a raw + // FormatException must not escape ProcessAsync). (#24 + PR #38 review.) + return Base64Url.DecodeRelaxed(attachment.Data.Base64); + } + catch (FormatException ex) + { + throw new MalformedMessageException( + "Forward attachment 'data.base64' is not valid base64url.", ex); + } + } throw new ConsistencyException( "Forward attachment is missing both 'data.json' and 'data.base64'. The mediator has nothing to relay."); diff --git a/tasks/lessons.md b/tasks/lessons.md index 422a260..43976b2 100644 --- a/tasks/lessons.md +++ b/tasks/lessons.md @@ -482,3 +482,34 @@ Format per entry: argument for shared mutable state must be re-derived under concurrency; guard expensive rebuild/evict passes with single-flight. (3) Always run a break-it adversarial pass on a security fix before declaring done — the fix that closes the headline issue often leaves a residual. + +## L-026 — When you make a shared primitive throw a new/wider exception, audit EVERY call site's exception contract — don't trust the issue's "call sites already handle it" premise. + +- **Lesson:** Making `Base64Url.Decode` strict (#24) widened when it throws `FormatException`. The issue + body asserted the call sites "map it to MalformedMessageException" — but the adversarial pass found + `ForwardProcessor.ExtractAttachmentBytes` had NO try/catch, so a malformed forward attachment threw a + raw `FormatException` out of the mediator's `ProcessAsync`. The from_prior site mapped it (#19); the + forward site did not. +- **Why:** A primitive's exception is part of its contract; changing/widening it ripples to every + caller. The audit-era issue text described call-site behavior that was only partially true, and the + pre-existing raw-`FormatException` escape was simply made more reachable. +- **How to apply:** Before changing what a shared decoder/parser throws, `grep` every call site and + verify each maps the (new or widened) exception to the module's documented contract type — add the + map where missing, with a regression test per reachable site. Treat the issue's stated premise as a + hypothesis to verify, not a fact. (This is exactly what the mandatory adversarial pass, AGENTS.md + §2, is for — run it on every security fix and fix what it finds.) + +## L-027 — Scope encoding strictness to where the spec actually mandates it; a global tightening can refuse spec-valid peer data. + +- **Lesson:** Making `Base64Url.Decode` strict (no-pad) globally also tightened the mediator's + forward-attachment relay. The reviewer flagged that attachment `data.base64` (FR-ATT-02; Aries RFC + 0017) has NO no-pad requirement — only JOSE/JWT segments and OOB URLs do (FR-ENC-13/14, FR-SIG-04, + FR-OOB-02 "whitespace-stripped"). A global strict decode would refuse a peer's padded forward, an + interop regression with ~zero security upside (the mediator just relays bytes the recipient re-parses + strictly). Fix: keep `Decode` strict for the JOSE/JWT/OOB paths, add `DecodeRelaxed` for the + attachment relay. +- **Why:** "Strict is safer" is true for fields the spec pins (signed bytes, key material) but wrong + where the spec is permissive and the data is pass-through — there it just breaks interop. +- **How to apply:** Before tightening a shared codec/parser, enumerate each call site and check the + *spec's* strictness requirement for THAT field. Apply strictness only where mandated; give permissive + fields a clearly-named relaxed path. Verify the spec, don't assume one rule fits all call sites. diff --git a/tasks/todo20260617T021817Z.md b/tasks/todo20260617T021817Z.md new file mode 100644 index 0000000..961f957 --- /dev/null +++ b/tasks/todo20260617T021817Z.md @@ -0,0 +1,119 @@ +# Plan — Batch A: receive-path correctness (#22, #23, #24) + +**Branch:** `feat/security-receive-correctness` (off `main`) +**One PR** closing **#22, #24**, and **#23** (see disposition below). + +## ⚠️ Scoping correction vs the issue text + +The issues were filed during the original audit, **before** the JOSE/crypto layer was delegated to +`DataProofsDotnet.Jose` (now on `main`). Two of them reference code that no longer exists or has moved: + +- **#22** cites `src/DidComm.Core/Jose/Encryption/JweParser.cs` and `Crypto/Aead/*` — **those files are + deleted on `main`.** JWE parsing is now `DataProofsDotnet.Jose.Encryption.JweParser`, whose AEAD throws + `ArgumentException` on a wrong-length iv/tag (verified in the dataproofs tests). `EnvelopeReader` wraps + that call in `catch (MalformedJoseException)` / `catch (JoseCryptoException)` only — so the + `ArgumentException` **still escapes `UnpackAsync`** (bug is real; fix location moved to the delegation + boundary). Cross-repo note: the upstream parser ideally should validate iv/tag length itself; the + in-repo defensive catch fixes it for didcomm-dotnet regardless. +- **#23**'s exploit (`authcrypt(anoncrypt)` → both flags true) is **already unreachable** — the merged + #17 composition gate rejects that ordering. For every *legal* shape the accumulated flags already + equal the documented "outermost layer" semantics. Residual = make the flag *derivation* literally + match its doc (defense-in-depth), or close as resolved-by-#17. +- **#24**'s `Base64Url.Decode` is genuinely in-repo and reachable (from_prior, forward attachment, OOB, + JWK) — clean fix, as written. + +--- + +## Fixes + +### [ ] #22 — wrong-length iv/tag `ArgumentException` escapes the unpack contract +- **File:** `src/DidComm.Core/Composition/EnvelopeReader.cs` (JWE parse block ~214-227; JWS block ~170-182). +- **Fix:** add `catch (ArgumentException ex) { throw new MalformedMessageException(ex.Message, ex); }` to + **both** the `DpEnc.JweParser.Parse` and `DpSig.JwsParser.Parse` try/catch blocks (after the existing + `MalformedJoseException`/`JoseCryptoException` catches). The lookups passed in are non-null + (constructed internally), so the only `ArgumentException` from these calls on attacker bytes is + malformed input — safe to map to the contract type. +- **Test:** end-to-end — craft a valid anoncrypt envelope, splice a wrong-length `iv` (e.g. 5 bytes + base64url) into the JWE JSON, `EnvelopeReader.Unpack`/`UnpackAsync` ⇒ `MalformedMessageException` + (not `ArgumentException`). Cover A256GCM iv and, if cheap, a wrong-length tag. +- **Optional follow-up (cross-repo, NOT this PR):** file an upstream issue to have + `DataProofsDotnet.Jose.JweParser` validate iv/tag length → `MalformedJoseException`. + +### [ ] #24 — strict base64url (reject `=` padding + embedded whitespace + `+`/`/`) +- **File:** `src/DidComm.Core/Jose/Base64Url.cs` (`Decode`). +- **Fix:** before delegating to the BCL codec, scan the input and reject any char outside + `[A-Za-z0-9-_]` by throwing `FormatException` (catches `=`, whitespace, `+`, `/`). Keep `Encode` + unchanged (already no-pad). Apply in `Decode` so `DecodeUtf8` inherits it. +- **Call-site contract (verify, no behavior regression):** `FromPriorValidator` already maps + `FormatException` → `ProtocolException` (#19); OOB `FromUrl` documents/raises `FormatException`; + `ForwardProcessor.ExtractAttachmentBytes` (`:119`) already could surface `FormatException` from a + malformed attachment today (BCL rejects `+`/`/`) — #24 only widens the rejected set, not the + exception *kind*. JWK material (`JwsSignerFactory`) is pack-side/local, not attacker-reachable. + → confirm each reachable receive site surfaces a `DidCommException`; add a minimal map if a latent + raw-`FormatException` escape is found (note it, keep scope tight). +- **Tests:** `Base64Url` unit tests — `"Mw=="`, `"SGVsbG8="` (padding), `"M w"`, `"SGVs\tbG8"`, + `"SGVs\r\nbG8"` (whitespace), `"a+b/"` (std-base64) all ⇒ `FormatException`; clean no-pad input still + round-trips. Plus an end-to-end: a `from_prior` JWT segment with trailing `=` ⇒ `ProtocolException`. + +### [ ] #23 — derive `AnonymousSender` from the outermost encrypt layer (match the doc) +- **Disposition:** the security contradiction is **already fixed by #17**; this is a small + defense-in-depth / contract-honesty change so the code literally matches `UnpackResult.cs:16` + ("outermost encrypt layer"). *If you'd rather just close #23 as resolved-by-#17, say so and I'll drop + this from the PR.* +- **File:** `src/DidComm.Core/Composition/EnvelopeReader.cs`. +- **Fix:** set `AnonymousSender` from the **first** (outermost) encrypt layer's anon/auth flag only, + instead of OR-accumulating across all layers; tidy the doc comment to state the #17 gate guarantees + anoncrypt-if-present is outermost. (`Authenticated` stays "any layer bound a sender" — that already + matches its documented meaning.) +- **Test:** assert flags for the legal shapes (`anoncrypt`, `authcrypt`, `anoncrypt(authcrypt)`, + `anoncrypt(sign)`, `anoncrypt(authcrypt(sign))`) are correct, and note the illegal contradiction is + now rejected by the gate (already covered by #17 tests). + +--- + +## Cross-cutting +- [ ] Branch `feat/security-receive-correctness` off `main`. +- [ ] Implement the three fixes (additive; no public API change). +- [ ] `dotnet build` (warnings-as-errors) + full `dotnet test` green, incl. new tests; watch that no + existing test relied on lenient base64url decoding. +- [ ] Update `CHANGELOG.md` (Security/Fixed) with the three issues + the #22 delegation-boundary note. +- [ ] **Adversarial red-team pass** on the three fixes before PR (per AGENTS.md), remediate findings. +- [ ] One PR: `fix(security): receive-path correctness (#22, #23, #24)`, `Closes #22`, `closes #23`, + `closes #24`. + +## Verification standard +- #22: prove the wrong-length-iv path returns `MalformedMessageException` (post-tamper of a real + envelope), not a raw `ArgumentException`. +- #24: prove padded/whitespaced/std-base64 inputs are rejected AND clean inputs still decode; prove an + end-to-end receive path (from_prior) maps the new `FormatException` to a `DidCommException`. +- Diff behavior vs `main` for the unpack exception types. + +## Review section + +All three fixes implemented on `feat/security-receive-correctness`. **615 tests green**, build clean +under warnings-as-errors. + +- **#22** — the audit-cited files are deleted; investigation showed the escape is **already resolved + upstream** (`DataProofsDotnet.Jose.JweParser` now wraps the AEAD's `ArgumentException` and the base64 + `FormatException` as `MalformedJoseException`). Verified empirically (the inner exception is + `MalformedJoseException`, not `ArgumentException`). Shipped a **defensive boundary catch** + (`ArgumentException → MalformedMessageException` at both JWS/JWE delegation points) + a regression + test asserting wrong-length-iv → `MalformedMessageException`. Honest about this in the PR/CHANGELOG. +- **#24** — strict alphabet pre-validation in `Base64Url.Decode`; unit tests for padding/whitespace/ + `+`/`/`, plus an end-to-end from_prior padded-segment → `ProtocolException`. +- **#23** — `AnonymousSender` now derived from the outermost encrypt layer (matches its doc); the #17 + gate already blocked the old contradiction. + +### Adversarial red-team pass (AGENTS.md §2/L36) +Subagents hit the session limit, so I ran the pass inline against the real code. +- **#22 holds** — external parser comprehensively wraps base64/AEAD/AES-KW exceptions; boundary catch + is sound defense-in-depth. +- **#23 holds** — outermost-first derivation correct for every legal shape; gate runs before the + `UnpackResult` return; independent-flags semantics are safe (no dangerous false-positive). +- **#24 — REAL FINDING, fixed.** `ForwardProcessor.ExtractAttachmentBytes` had no `try/catch`, so a + malformed forward attachment threw a raw `FormatException` out of the mediator. The issue's premise + ("call sites map FormatException") was false here. Wrapped → `MalformedMessageException` + regression + test. Lesson L-026 captured. + +**Cross-repo note (for the PR):** #22's underlying defect is already fixed in the dependency +(`DataProofsDotnet.Jose`); this repo's contribution is the boundary guard + regression coverage. diff --git a/tests/DidComm.Core.Tests/Envelopes/Composition/EnvelopeReaderTests.cs b/tests/DidComm.Core.Tests/Envelopes/Composition/EnvelopeReaderTests.cs index 2868f0e..1536bfd 100644 --- a/tests/DidComm.Core.Tests/Envelopes/Composition/EnvelopeReaderTests.cs +++ b/tests/DidComm.Core.Tests/Envelopes/Composition/EnvelopeReaderTests.cs @@ -424,4 +424,70 @@ public async Task Anoncrypt_authcrypt_sign_is_accepted_on_receive() unpacked.AnonymousSender.Should().BeTrue(); unpacked.NonRepudiation.Should().BeTrue(); } + + [Fact] + public async Task Wrong_length_iv_throws_MalformedMessageException_honoring_the_unpack_contract() + { + // Issue #22: a non-canonical iv length must surface as MalformedMessageException, never as a raw + // ArgumentException out of unpack. (A256GCM requires a 12-byte iv; splice a 5-byte one.) Note: + // the delegated DataProofsDotnet.Jose parser already wraps the AEAD's ArgumentException as + // MalformedJoseException, so this is a regression guard on the unpack contract; EnvelopeReader + // also carries a defensive ArgumentException boundary catch for any path the delegate doesn't wrap. + var bob = TestKeyMaterial.Generate(KeyType.X25519, "did:example:bob#x"); + var packed = await EnvelopeWriter.PackEncryptedAsync( + new PackEncryptedParameters(EmptyMessage(), new[] { bob.PublicJwk }, "A256GCM"), _crypto); + + var jwe = System.Text.Json.Nodes.JsonNode.Parse(packed)!.AsObject(); + jwe["iv"] = DidComm.Jose.Base64Url.Encode(new byte[5]); // valid base64url, wrong length + var tampered = jwe.ToJsonString(); + + Action act = () => EnvelopeReader.Unpack(tampered, + new DictionarySecretsLookup(new[] { bob.PrivateJwk }), senderLookup: null, signerLookup: null, _crypto); + + act.Should().Throw(); + } + + [Fact] + public async Task ArgumentException_from_the_delegated_parse_maps_to_MalformedMessageException() + { + // Issue #22 boundary guard — directly exercised. The wrong-length-iv path is wrapped upstream, + // so this drives the new catch via a signer lookup that throws ArgumentException (the lookup is + // invoked inside the delegated parse). Contract: ANY ArgumentException from the delegated parse + // becomes MalformedMessageException, never escapes raw; InnerException is preserved for diagnosis. + var signer = TestKeyMaterial.Generate(KeyType.Ed25519, "did:example:alice#k"); + var packed = await EnvelopeWriter.PackSignedAsync(new PackSignedParameters(EmptyMessage(), new[] { signer.PrivateJwk })); + + Action act = () => EnvelopeReader.Unpack(packed, + new DictionarySecretsLookup(Array.Empty()), + senderLookup: null, + signerLookup: _ => throw new ArgumentException("boom from a buggy lookup"), + _crypto); + + act.Should().Throw().WithInnerException(); + } + + [Fact] + public async Task AnonymousSender_reflects_the_outermost_encrypt_layer_not_accumulation() + { + // Issue #23 — characterization / defense-in-depth (NOT a regression guard: the #17 gate already + // rejects the only shapes where the old OR-accumulation would have differed, so this passes on + // main too). It pins the documented contract: for anoncrypt(authcrypt(...)) the OUTERMOST encrypt + // layer is anoncrypt, so AnonymousSender is true even though the inner authcrypt authenticates + // the sender — the flag now reads from the outermost layer rather than OR-accumulating. + var alice = TestKeyMaterial.Generate(KeyType.X25519, "did:example:alice#x"); + var bob = TestKeyMaterial.Generate(KeyType.X25519, "did:example:bob#x"); + + var packed = await EnvelopeWriter.PackEncryptedAsync( + new PackEncryptedParameters(EmptyMessage(), new[] { bob.PublicJwk }, "A256CBC-HS512", + SenderPrivateJwk: alice.PrivateJwk, Skid: alice.PublicJwk.Kid, ProtectSender: true), _crypto); + + var unpacked = EnvelopeReader.Unpack(packed, + new DictionarySecretsLookup(new[] { bob.PrivateJwk }), + senderLookup: new DictionarySenderKeyLookup(new[] { alice.PublicJwk }), + signerLookup: null, _crypto); + + unpacked.AnonymousSender.Should().BeTrue(); // outermost layer is anoncrypt + unpacked.Authenticated.Should().BeTrue(); // inner authcrypt bound the sender + unpacked.SenderKid.Should().Be(alice.PublicJwk.Kid); + } } diff --git a/tests/DidComm.Core.Tests/Envelopes/Encryption/Base64UrlTests.cs b/tests/DidComm.Core.Tests/Envelopes/Encryption/Base64UrlTests.cs new file mode 100644 index 0000000..7f9e147 --- /dev/null +++ b/tests/DidComm.Core.Tests/Envelopes/Encryption/Base64UrlTests.cs @@ -0,0 +1,51 @@ +using System.Text; +using DidComm.Jose; +using FluentAssertions; +using Xunit; + +namespace DidComm.Tests.Envelopes.Encryption; + +/// +/// Issue #24: must enforce strict JOSE base64url (RFC 7515 §2): +/// no padding, no whitespace, alphabet [A-Za-z0-9-_] — diverging inputs throw +/// , which the receive call sites map to a DidCommException. +/// +public sealed class Base64UrlTests +{ + [Theory] + [InlineData("Mw==")] // trailing '=' padding (RFC 4648 §3.2 forbids it for base64url-no-pad) + [InlineData("SGVsbG8=")] // single '=' pad + [InlineData("M w")] // embedded space + [InlineData(" SGVsbG8")] // leading space + [InlineData("SGVs\tbG8")] // embedded tab + [InlineData("SGVs\r\nbG8")] // embedded CRLF (line break — RFC 4648 §3.3) + [InlineData("a+b/")] // standard-base64 '+' and '/' + [InlineData("SGVsbG8%3D")] // percent-encoded padding (non-alphabet) + public void Decode_rejects_non_strict_base64url(string input) + { + ((Action)(() => Base64Url.Decode(input))).Should().Throw(); + } + + [Fact] + public void Decode_accepts_clean_no_pad_base64url_and_round_trips() + { + // The whole base64url alphabet plus a value that encodes to '-'/'_' must still decode. + foreach (var original in new[] { "Hello, DIDComm v2.1!", "f", "fo", "foo", "ÿþýü" }) + { + var bytes = Encoding.UTF8.GetBytes(original); + var encoded = Base64Url.Encode(bytes); + encoded.Should().NotContain("=").And.NotContain("+").And.NotContain("/"); + + var decoded = Base64Url.Decode(encoded); + Encoding.UTF8.GetString(decoded).Should().Be(original); + } + } + + [Fact] + public void Decode_accepts_values_containing_dash_and_underscore() + { + // 0xFB 0xFF encodes to "-_8" in base64url — proves the '-' and '_' alphabet chars are allowed. + var decoded = Base64Url.Decode("-_8"); + decoded.Should().Equal(0xFB, 0xFF); + } +} diff --git a/tests/DidComm.Core.Tests/Protocols/Routing/ForwardProcessorTests.cs b/tests/DidComm.Core.Tests/Protocols/Routing/ForwardProcessorTests.cs index 59921d9..387e87d 100644 --- a/tests/DidComm.Core.Tests/Protocols/Routing/ForwardProcessorTests.cs +++ b/tests/DidComm.Core.Tests/Protocols/Routing/ForwardProcessorTests.cs @@ -243,6 +243,54 @@ public async Task ProcessAsync_decodes_a_base64url_attachment_payload() Encoding.UTF8.GetString(result.OnwardPacked).Should().Be(innerPayload); } + [Fact] + public async Task ProcessAsync_maps_malformed_base64_attachment_to_MalformedMessageException() + { + // Issue #24 (red-team): a forward attachment whose data.base64 is genuinely invalid base64url + // ('@' is non-alphabet; '+'/'/' are standard-base64, not base64url) must surface as + // MalformedMessageException, not a raw FormatException escaping the mediator's ProcessAsync. + var forward = new MessageBuilder() + .WithType(ForwardConstants.ForwardTypeUri) + .WithBody(new JsonObject { ["next"] = "did:example:n" }) + .WithAttachment(new Attachment { Data = new AttachmentData { Base64 = "@@@@" } }) // not base64url at all + .Build(); + var client = NewClient(); + var packed = await client.PackPlaintextAsync(forward); + + var processor = new ForwardProcessor(client, new EmptyKeyService(), new ForwardProcessorOptions()); + + var act = async () => await processor.ProcessAsync(packed); + + (await act.Should().ThrowAsync()) + .Which.Message.Should().Contain("not valid base64url"); + } + + [Fact] + public async Task ProcessAsync_relays_a_padded_base64_attachment_FR_ATT_02() + { + // PR #38 review: attachment data.base64 has no spec no-pad requirement (FR-ATT-02, cf. Aries + // RFC 0017), and the mediator only relays the bytes for the recipient to re-parse — so a peer's + // '=' padded forward attachment must still be relayed, not refused. (Only the strict JOSE/JWT/ + // OOB paths reject padding.) + var innerPayload = "forward-me!"; // 11 bytes → base64 carries a trailing '=' pad + var bytes = Encoding.UTF8.GetBytes(innerPayload); + var paddedBase64Url = Convert.ToBase64String(bytes).Replace('+', '-').Replace('/', '_'); // padded base64url + paddedBase64Url.Should().EndWith("="); // sanity: this value really is padded + + var forward = new MessageBuilder() + .WithType(ForwardConstants.ForwardTypeUri) + .WithBody(new JsonObject { ["next"] = "did:example:n" }) + .WithAttachment(new Attachment { Data = new AttachmentData { Base64 = paddedBase64Url } }) + .Build(); + var client = NewClient(); + var packed = await client.PackPlaintextAsync(forward); + + var processor = new ForwardProcessor(client, new EmptyKeyService(), new ForwardProcessorOptions()); + var result = await processor.ProcessAsync(packed); + + Encoding.UTF8.GetString(result.OnwardPacked).Should().Be(innerPayload); + } + [Theory] [InlineData(long.MinValue)] [InlineData(-long.MaxValue)] diff --git a/tests/DidComm.InteropTests/Rotation/FromPriorRotationTests.cs b/tests/DidComm.InteropTests/Rotation/FromPriorRotationTests.cs index 9451c5b..2681ba4 100644 --- a/tests/DidComm.InteropTests/Rotation/FromPriorRotationTests.cs +++ b/tests/DidComm.InteropTests/Rotation/FromPriorRotationTests.cs @@ -125,6 +125,7 @@ public async Task Validator_RejectsMalformedJwt() [Theory] [InlineData("!!!not-base64url!!!", 0)] // header segment is not base64url -> FormatException [InlineData("@@@", 2)] // signature segment is not base64url -> FormatException + [InlineData("SGVsbG8=", 1)] // #24: '=' padded segment -> strict base64url FormatException public async Task Validator_RejectsNonBase64UrlSegment_AsProtocolException(string garbage, int segmentIndex) { var jwt = await FromPriorBuilder.BuildAsync(SampleClaims(), SignerPrivateJwk());