fix(security): receive-path correctness (#22, #23, #24)

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

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);
                     }
 

src/DidComm.Core/Jose/Base64Url.cs

@@ -27,8 +27,44 @@ public static string EncodeUtf8(string utf8String)
     }
 
     /// <summary>Decode <paramref name="value"/> from base64url without padding to bytes.</summary>
-    /// <param name="value">Base64url string (no padding).</param>
+    /// <param name="value">Strict base64url string: no padding, no whitespace, alphabet <c>[A-Za-z0-9-_]</c>.</param>
+    /// <exception cref="FormatException">When <paramref name="value"/> contains a non-base64url character or non-canonical trailing bits.</exception>
     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 == '_';
+
+    /// <summary>
+    /// Decode <paramref name="value"/> tolerating <c>'='</c> 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 <c>data.base64</c>
+    /// (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), <c>from_prior</c> JWTs, and OOB URLs
+    /// have an explicit no-pad requirement and MUST use the strict <see cref="Decode"/>.
+    /// </summary>
+    /// <param name="value">Base64url string, possibly padded.</param>
+    /// <exception cref="FormatException">When the input is not valid base64url even allowing padding/whitespace.</exception>
+    public static byte[] DecodeRelaxed(string value)
     {
         ArgumentException.ThrowIfNullOrEmpty(value);
         return SystemBase64Url.DecodeFromChars(value.AsSpan());

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.");

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.