diff --git a/AGENTS.md b/AGENTS.md index 3deef49..395f367 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -33,6 +33,7 @@ Maintenance rules: - Use subagents liberally to keep main context window clean - Offload research, exploration, and parallel analysis to subagents +- Always use adversarial agents to attempt to exploit the code that is being generated. The adversarial agents must report in detail about any findings - For complex problems, throw more compute at it via subagents - One task per subagent for focused execution diff --git a/CHANGELOG.md b/CHANGELOG.md index 6900303..d0cec54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,58 @@ All notable changes to didcomm-dotnet are documented here. Format follows ## [Unreleased] +### Security — Medium-severity audit remediation (`feat/security-medium-cluster`) + +Remediates the five Medium-severity findings from the multi-agent security & compliance audit +(GitHub issues #17–#21), plus two lower-severity defects on the same code path (#28, #33). Each fix +ships with negative and regression tests, and each was then put through an adversarial red-team pass +which surfaced two further hardening items (folded in below) and two deeper residuals (filed as +follow-ups). The full suite (600 tests) is green. + +- **Illegal envelope compositions rejected (#17, FR-ENV-04a).** `EnvelopeReader.Unpack` now tracks + each unwrapped layer with its authenticated/anonymous flag and enforces the legal DIDComm v2.1 + receive grammar `anoncrypt? authcrypt? sign? plaintext` before any content/consistency processing. + Illegal nestings — sign-outside-encrypt (FR-ENV-05), `anoncrypt(anoncrypt)`, `authcrypt(authcrypt)`, + `authcrypt(anoncrypt)`, double-sign — are rejected with `MalformedMessageException`. The grammar + admits every spec Appendix C unpack vector, including `authcrypt(sign)` (FR-ENV-03) and + `anoncrypt(authcrypt(sign))` (Appendix C.3). The auth/anon flag is what distinguishes the legal + `anoncrypt(authcrypt)` from the illegal `anoncrypt(anoncrypt)` (indistinguishable at envelope-kind + level). PRD gains **FR-ENV-04a** documenting the receive-acceptance set. +- **Controller-aware resolver authorization (#18, FR-CONSIST-06 / DD-01).** `NetDidKeyService` + `IsKeyAuthorizedAsync` now walks the raw resolved verification methods (not the curve-projected + JWKs, which dropped `controller`) and authorizes a `kid` only when its id-subject **and** its + `controller` resolve to the asserted DID — rejecting a key listed under the DID's relationship but + controlled by a different DID, and cross-DID embedded ids. did:key / did:peer keep authorizing + (controller == subject by construction). +- **`from_prior` JWT parse errors normalized (#19).** `FromPriorValidator` wraps base64url decode and + all JSON reads in one boundary that surfaces malformed input (`FormatException`, + `InvalidOperationException`, `ArgumentException` on an empty segment, `JsonException`, + `KeyNotFoundException`) as the typed `ProtocolException` with a generic message — closing an + unhandled-exception path out of `UnpackAsync` and removing the header-vs-claims info leak. +- **Receive endpoint response oracle closed (#20, #28, #33).** Both ASP.NET Core HTTP receive overloads + collapse every input- or handler-triggered failure to a single opaque `400` with an empty body, + logging the reason server-side only. This closes the verbatim-`CryptoException` decryption / + recipient-`kid` enumeration oracle (#20), the `Consistency`/`Resolution`/`UnsupportedDidMethod` + exceptions that previously escaped to `500` + dev stack-trace (#28), and the echoed handler-bug + detail (#33). The 202 success, 415, and 413 paths are unchanged; cancellation still propagates. + Red-team hardening: the inline-callback overload now also wraps `onReceive` so a consumer callback + that throws after a successful unpack returns 202 (logged) instead of escaping as a distinguishable + `500` — removing an unpack-success oracle the uniform-400 path would otherwise leave. (A residual + *timing* side-channel in the underlying decrypt path — held-vs-unheld recipient kid — is **not** + closed by this change and is tracked as a follow-up.) A downstream DID-resolution timeout + (`TaskCanceledException` with the request token not cancelled — e.g. a hung `did:webvh` host) also + collapses to the uniform 400 rather than escaping as a `500`; only a genuine client abort propagates. + **Operator note:** a receiver-side handler bug (FR-THR-04 `InvalidOperationException`) now returns + `400` instead of `500` for response uniformity — it is logged at `Error` level, so alert on the + `DidComm.AspNetCore` error log rather than on a 5xx rate for handler faults. +- **Thread-state store bounded (#21).** `InMemoryThreadStateStore` is now capped + (`DefaultMaxEntries = 10_000`, overridable via ctor) with approximate-LRU eviction, converting an + unbounded-growth memory-exhaustion DoS (attacker-chosen, unauthenticated `thid`/`pthid`) into a + few-MB bound. Secure-by-default — no DI wiring change. Red-team hardening: eviction is **single-flight** + (an `Interlocked` guard) so N concurrent inserters over the cap can't each run a full O(n log n) + snapshot-sort — closing a CPU-amplification DoS. (A residual cascade-guard-reset-via-eviction vector, + costing the attacker ~`cap` messages per suppressed report, is tracked as a follow-up alongside #29.) + ### Changed — delegate the envelope layer to DataProofsDotnet.Jose (`feat/delegate-jose-dataproofs`) didcomm-dotnet no longer carries its own cryptography or JWE/JWS assembly. The entire `Crypto/` diff --git a/docs/didcomm-dotnet_PRD.md b/docs/didcomm-dotnet_PRD.md index b32ec52..4fab53e 100644 --- a/docs/didcomm-dotnet_PRD.md +++ b/docs/didcomm-dotnet_PRD.md @@ -265,6 +265,7 @@ public sealed class DidComm { | FR-ENV-02 | MUST | Support exactly these legal envelope compositions for a single hop: `plaintext`, `signed(plaintext)`, `anoncrypt(plaintext)`, `authcrypt(plaintext)`, `anoncrypt(sign(plaintext))`, `anoncrypt(authcrypt(plaintext))`. | Each composition packs and unpacks. | | FR-ENV-03 | SHOULD NOT | Do not emit `authcrypt(sign(plaintext))`; MAY accept it on receive (subject to FR-CONSIST-05). | Library never produces this; can ingest it. | | FR-ENV-04 | MUST NOT | Do not emit any single-hop composition other than those in FR-ENV-02 (e.g. never `anoncrypt(authcrypt(sign))`). | No disallowed nesting produced. | +| FR-ENV-04a | MUST | On **receive**, accept exactly the compositions matching the grammar `anoncrypt? authcrypt? sign? plaintext` (at most one anoncrypt outermost, then at most one authcrypt, then at most one signature innermost, then the plaintext). This is a superset of the FR-ENV-02 emit set that additionally admits the receive-only `authcrypt(sign(plaintext))` (FR-ENV-03) and the protect-sender-plus-sign `anoncrypt(authcrypt(sign(plaintext)))` (spec Appendix C.3). Reject any other layering — sign-outside-encrypt (FR-ENV-05), `anoncrypt(anoncrypt)`, `authcrypt(authcrypt)`, `authcrypt(anoncrypt)`, more than one signature — as malformed. | Every spec Appendix C unpack vector succeeds; illegal layer orderings are rejected with `MalformedMessageException` before any content/consistency processing. | | FR-ENV-05 | MUST | When signing+encrypting, sign first, then encrypt (nested JWM). | Sign-then-encrypt order enforced. | | FR-ENV-06 | MUST | Encrypted form is a JWE in **General JSON Serialization** (multi-recipient). | Output contains `protected`/`recipients`/`iv`/`ciphertext`/`tag`. | | FR-ENV-07 | SHOULD | Set envelope `typ` in the JWE/JWS protected header. | `typ` present in protected header. | diff --git a/src/DidComm.AspNetCore/DidCommEndpointRouteBuilderExtensions.cs b/src/DidComm.AspNetCore/DidCommEndpointRouteBuilderExtensions.cs index d439cb0..daf9fea 100644 --- a/src/DidComm.AspNetCore/DidCommEndpointRouteBuilderExtensions.cs +++ b/src/DidComm.AspNetCore/DidCommEndpointRouteBuilderExtensions.cs @@ -27,9 +27,12 @@ public static class DidCommEndpointRouteBuilderExtensions /// /// Map an HTTP POST endpoint that accepts DIDComm envelopes, unpacks them via /// , and dispatches to - /// (FR-TRN-07). Returns 202 Accepted on success. Returns 415 for an unknown - /// content type, 413 when the body exceeds - /// (FR-API-06), 400 for malformed envelopes, and 500 for unexpected errors. + /// (FR-TRN-07). Returns 202 Accepted on success, 415 for an unknown content type, + /// and 413 when the body exceeds (FR-API-06). + /// ANY envelope/processing failure — crypto, malformed structure, consistency, or DID resolution — + /// returns a uniform 400 Bad Request with an empty body and the reason logged server-side + /// only, so the response cannot be used as a decryption / recipient-kid / resolution oracle + /// (FR-API-07, issues #20/#28/#33). /// /// The ASP.NET Core endpoint route builder. /// URL pattern (e.g. "/didcomm"). @@ -78,21 +81,46 @@ public static IEndpointConventionBuilder MapDidCommEndpoint( return Results.StatusCode(StatusCodes.Status413PayloadTooLarge); } + var logger = httpContext.RequestServices.GetService()?.CreateLogger("DidComm.AspNetCore.Receive"); + UnpackResult unpacked; try { unpacked = await client.UnpackAsync(body, httpContext.RequestAborted).ConfigureAwait(false); } - catch (MalformedMessageException ex) + catch (OperationCanceledException) when (httpContext.RequestAborted.IsCancellationRequested) { - return Results.Problem(detail: ex.Message, statusCode: StatusCodes.Status400BadRequest); + throw; // genuine client abort — propagate, do not mask as a rejection } - catch (CryptoException ex) + catch (Exception ex) { - return Results.Problem(detail: ex.Message, statusCode: StatusCodes.Status400BadRequest); + // Every unpack failure collapses to one opaque 400 so the peer can't distinguish them: + // crypto / malformed / consistency / resolution / did:web / missing-secret, AND a + // downstream DID-resolution timeout (TaskCanceledException : OperationCanceledException + // with the request token NOT cancelled, e.g. an attacker-controlled did:webvh host that + // hangs). Only a real client abort is rethrown above, so there is no 400-vs-500 oracle. + return NormalizedReceiveRejection(logger, ex); + } + + // The message was received and unpacked; HTTP receive is one-way / fire-and-forget + // (FR-TRN-10). A fault in the consumer callback is the receiver's own concern — log it but + // still answer 202, so a throwing callback can't turn a successfully-unpacked message into a + // distinguishable 500. Otherwise an attacker could probe "did this envelope decrypt for us?" + // by inducing a callback throw — re-exposing the unpack-success oracle the uniform-400 + // rejection closes (#20 red-team). + try + { + await onReceive(unpacked, httpContext.RequestAborted).ConfigureAwait(false); + } + catch (OperationCanceledException) when (httpContext.RequestAborted.IsCancellationRequested) + { + throw; // genuine client abort + } + catch (Exception ex) + { + logger?.LogError(ex, "DidComm receive: onReceive callback threw after a successful unpack; acknowledging anyway (one-way receive)."); } - await onReceive(unpacked, httpContext.RequestAborted).ConfigureAwait(false); return Results.StatusCode(StatusCodes.Status202Accepted); }); } @@ -142,42 +170,26 @@ public static IEndpointConventionBuilder MapDidCommEndpoint( return Results.StatusCode(StatusCodes.Status413PayloadTooLarge); } - UnpackResult unpacked; - try - { - unpacked = await client.UnpackAsync(body, httpContext.RequestAborted).ConfigureAwait(false); - } - catch (MalformedMessageException ex) - { - return Results.Problem(detail: ex.Message, statusCode: StatusCodes.Status400BadRequest); - } - catch (CryptoException ex) - { - return Results.Problem(detail: ex.Message, statusCode: StatusCodes.Status400BadRequest); - } - - DispatchOutcome outcome; + // Unpack AND dispatch share one opaque exit: a handler bug (FR-THR-04 InvalidOperationException), + // an unpack rejection (crypto/malformed/consistency/resolution), and a downstream + // DID-resolution timeout (TaskCanceledException with the request token NOT cancelled) all + // return the same 400 with no body, so the status code itself is not an oracle (no + // 400-vs-500 partition). Only a genuine client abort is rethrown. try { - outcome = await dispatcher.DispatchAsync(unpacked, client, coreOptions, httpContext.RequestAborted).ConfigureAwait(false); + var unpacked = await client.UnpackAsync(body, httpContext.RequestAborted).ConfigureAwait(false); + var outcome = await dispatcher.DispatchAsync(unpacked, client, coreOptions, httpContext.RequestAborted).ConfigureAwait(false); + LogOutcome(logger, outcome, sameSocketDelivered: false); + return Results.StatusCode(StatusCodes.Status202Accepted); } - catch (InvalidOperationException ex) + catch (OperationCanceledException) when (httpContext.RequestAborted.IsCancellationRequested) { - // FR-THR-04 rule 2 violation by a handler (a bug): surface as 500 with a clear - // detail rather than letting it bubble out as an opaque unhandled exception. - logger?.LogWarning(ex, "MapDidCommEndpoint: handler bug (FR-THR-04 rule 2); responding 500."); - return Results.Problem(detail: ex.Message, statusCode: StatusCodes.Status500InternalServerError); + throw; // genuine client abort — propagate } - catch (Exception ex) when (ex is not OperationCanceledException) + catch (Exception ex) { - // Handlers are application code; an unexpected throw is a server error, not a - // protocol error. Map to 500 so the inbound peer learns we couldn't process it - // without leaking the inner exception type. - logger?.LogError(ex, "MapDidCommEndpoint: handler threw an unhandled exception; responding 500."); - return Results.Problem(detail: "Handler failed; see server logs.", statusCode: StatusCodes.Status500InternalServerError); + return NormalizedReceiveRejection(logger, ex); } - LogOutcome(logger, outcome, sameSocketDelivered: false); - return Results.StatusCode(StatusCodes.Status202Accepted); }); } @@ -494,6 +506,30 @@ private static void LogOutcome(ILogger? logger, DispatchOutcome outcome, bool sa } } + /// + /// FR-API-07 / FR-TRN-10: collapse every input- or handler-triggered failure on the HTTP receive + /// path to one opaque rejection — a bare 400 with no body — so a remote peer cannot use the + /// status code or response text as a decryption / recipient-kid / DID-resolution oracle (issues + /// #20/#28/#33). The detailed reason is logged server-side only. The WebSocket paths already take + /// this posture (log-and-discard, echo nothing); this brings the HTTP paths to parity. + /// + private static IResult NormalizedReceiveRejection(ILogger? logger, Exception ex) + { + if (ex is DidCommException) + { + // Expected, peer-triggered category (malformed / crypto / consistency / resolution / + // unsupported-method / secret-missing) — it's the peer's envelope, not our bug. + logger?.LogWarning(ex, "DidComm receive: rejecting inbound envelope ({Reason}).", ex.GetType().Name); + } + else + { + // A handler bug (FR-THR-04 InvalidOperationException) or any other unexpected throw. + logger?.LogError(ex, "DidComm receive: inbound processing failed ({Reason}).", ex.GetType().Name); + } + + return Results.StatusCode(StatusCodes.Status400BadRequest); + } + private static async Task ReceiveLoopAsync( System.Net.WebSockets.WebSocket socket, DidCommClient client, diff --git a/src/DidComm.Core/Composition/EnvelopeReader.cs b/src/DidComm.Core/Composition/EnvelopeReader.cs index 654e080..594e920 100644 --- a/src/DidComm.Core/Composition/EnvelopeReader.cs +++ b/src/DidComm.Core/Composition/EnvelopeReader.cs @@ -56,6 +56,11 @@ public static UnpackResult Unpack( ArgumentNullException.ThrowIfNull(cryptoProvider); var stack = new List(); + // Per-layer composition trace (outer→inner) carrying the authenticated/anonymous distinction + // the EnvelopeKind stack can't (anoncrypt and authcrypt are both EnvelopeKind.Encrypted). Used + // by AssertLegalComposition to reject illegal layer orderings — e.g. the illegal + // anoncrypt(anoncrypt) vs the legal anoncrypt(authcrypt) (FR-ENV-02/04, issue #17). + var shape = new List(); bool authenticated = false, nonRepudiation = false, anonymous = false, encrypted = false; string? contentEnc = null, keyWrap = null, sigAlg = null, signerKid = null, senderKid = null, recipientKid = null; IReadOnlyList allRecipientKids = Array.Empty(); @@ -70,6 +75,13 @@ public static UnpackResult Unpack( { case EnvelopeKind.Plaintext: { + // Reject illegal envelope compositions before any content/consistency work: the legal + // receive grammar is anoncrypt? authcrypt? sign? plaintext (DIDComm v2.1 Appendix C). + // The auth/anon flag on each encrypt layer is what separates the legal anoncrypt(authcrypt) + // from the illegal anoncrypt(anoncrypt). (Issue #17.) + shape.Add(LayerShape.Plaintext); + AssertLegalComposition(shape); + Message? deserialized; try { @@ -151,6 +163,7 @@ public static UnpackResult Unpack( case EnvelopeKind.Signed: { + shape.Add(LayerShape.Sign); if (signerLookup is null) throw new CryptoException("Signed envelope encountered but no signer-key lookup was supplied."); @@ -222,10 +235,12 @@ public static UnpackResult Unpack( { authenticated = true; senderKid = jweResult.SenderKid; + shape.Add(LayerShape.AuthEncrypt); } else { anonymous = true; + shape.Add(LayerShape.AnonEncrypt); } current = Encoding.UTF8.GetString(jweResult.Plaintext); @@ -239,4 +254,40 @@ public static UnpackResult Unpack( throw new MalformedMessageException("Envelope nesting exceeded the legal depth of 4."); } + + /// A single envelope layer, outer→inner, with the auth/anon distinction the composition gate needs. + private enum LayerShape + { + Sign, + AnonEncrypt, + AuthEncrypt, + Plaintext, + } + + /// + /// Assert the unwrapped layer sequence (outer→inner) is a legal DIDComm v2.1 composition. The + /// receive grammar is anoncrypt? authcrypt? sign? plaintext: at most one anoncrypt + /// (outermost), then at most one authcrypt, then at most one signature (innermost), then the + /// plaintext. This admits every shape in spec Appendix C — including the FR-ENV-02 emit set, the + /// receive-only authcrypt(sign) (FR-ENV-03), and the protect-sender-plus-sign + /// anoncrypt(authcrypt(sign)) (Appendix C.3) — and rejects sign-outside-encrypt + /// (FR-ENV-05 ordering), authcrypt-outside-anoncrypt, double anoncrypt/authcrypt, and more than + /// one signature. (Issue #17.) + /// + private static void AssertLegalComposition(IReadOnlyList shape) + { + var i = 0; + if (i < shape.Count && shape[i] == LayerShape.AnonEncrypt) i++; // at most one anoncrypt, outermost + if (i < shape.Count && shape[i] == LayerShape.AuthEncrypt) i++; // then at most one authcrypt + if (i < shape.Count && shape[i] == LayerShape.Sign) i++; // then at most one signature, innermost + + // Whatever remains must be exactly the terminal plaintext; anything else is an illegal nesting. + var legal = i == shape.Count - 1 && shape[i] == LayerShape.Plaintext; + if (!legal) + { + throw new MalformedMessageException( + $"Illegal DIDComm envelope composition [{string.Join(", ", shape)}]; the legal receive grammar " + + "is anoncrypt? authcrypt? sign? plaintext (DIDComm v2.1, Appendix C)."); + } + } } diff --git a/src/DidComm.Core/Protocols/Rotation/FromPriorValidator.cs b/src/DidComm.Core/Protocols/Rotation/FromPriorValidator.cs index 3ee4ba7..5be26ba 100644 --- a/src/DidComm.Core/Protocols/Rotation/FromPriorValidator.cs +++ b/src/DidComm.Core/Protocols/Rotation/FromPriorValidator.cs @@ -53,27 +53,27 @@ internal static async Task ValidateAsync( if (parts.Length != 3) throw new ProtocolException("from_prior JWT must have three dot-separated segments (compact JWS)."); - var headerJson = Encoding.UTF8.GetString(Base64Url.Decode(parts[0])); - var claimsJson = Encoding.UTF8.GetString(Base64Url.Decode(parts[1])); - var signature = Base64Url.Decode(parts[2]); - + byte[] signature; string? alg, kid; + FromPriorClaims claims; try { + // Everything in this block is a pure function of the attacker-controlled JWT string: + // base64url-decode (FormatException on bad chars, ArgumentException on an empty segment), + // JSON parse (JsonException), member access (KeyNotFoundException), and typed reads + // (InvalidOperationException on a wrong JSON value kind, FormatException on an out-of-range + // number). All of these mean "the JWT is malformed" and MUST surface as the typed + // ProtocolException rather than escaping UnpackAsync as a raw runtime exception (issue #19, + // FR-API-07). The resolver/crypto calls below stay OUTSIDE this block so a genuine + // ConsistencyException/CryptoException is never masked. + var headerJson = Encoding.UTF8.GetString(Base64Url.Decode(parts[0])); + var claimsJson = Encoding.UTF8.GetString(Base64Url.Decode(parts[1])); + signature = Base64Url.Decode(parts[2]); + using var headerDoc = JsonDocument.Parse(headerJson, DidCommJson.StrictDocument); alg = headerDoc.RootElement.GetProperty("alg").GetString(); kid = headerDoc.RootElement.GetProperty("kid").GetString(); - } - catch (Exception ex) when (ex is JsonException or KeyNotFoundException) - { - throw new ProtocolException("from_prior JWT header is malformed.", ex); - } - if (string.IsNullOrEmpty(alg) || string.IsNullOrEmpty(kid)) - throw new ProtocolException("from_prior JWT header is missing 'alg' or 'kid'."); - FromPriorClaims claims; - try - { using var claimsDoc = JsonDocument.Parse(claimsJson, DidCommJson.StrictDocument); var iss = claimsDoc.RootElement.GetProperty("iss").GetString() ?? throw new ProtocolException("from_prior JWT 'iss' is missing or null."); @@ -86,11 +86,16 @@ internal static async Task ValidateAsync( ? nbfEl.GetInt64() : null; claims = new FromPriorClaims(Sub: sub, Iss: iss, Iat: iat, Exp: exp, Nbf: nbf); } - catch (Exception ex) when (ex is JsonException or KeyNotFoundException) + catch (Exception ex) + when (ex is JsonException or KeyNotFoundException or FormatException or InvalidOperationException or ArgumentException) { - throw new ProtocolException("from_prior JWT claims are malformed.", ex); + // Generic message: do not echo the offending alg/kid/segment (no parse-failure oracle). + throw new ProtocolException("from_prior JWT is malformed.", ex); } + if (string.IsNullOrEmpty(alg) || string.IsNullOrEmpty(kid)) + throw new ProtocolException("from_prior JWT header is missing 'alg' or 'kid'."); + // FR-ROT-02 — sub MUST equal the message 'from' DID. if (!string.Equals(claims.Sub, currentSenderDid, StringComparison.Ordinal)) { diff --git a/src/DidComm.Core/Resolution/NetDidKeyService.cs b/src/DidComm.Core/Resolution/NetDidKeyService.cs index bb74781..06eaf3b 100644 --- a/src/DidComm.Core/Resolution/NetDidKeyService.cs +++ b/src/DidComm.Core/Resolution/NetDidKeyService.cs @@ -1,3 +1,4 @@ +using DidComm.Consistency; using DidComm.Crypto.KeyAgreement; using DidComm.Exceptions; using NetDid.Core; @@ -108,16 +109,68 @@ public async Task IsKeyAuthorizedAsync( VerificationRelationship relationship, CancellationToken ct = default) { + ArgumentException.ThrowIfNullOrEmpty(did); ArgumentException.ThrowIfNullOrEmpty(kid); - var keys = await GetVerificationMethodsAsync(did, relationship, ct).ConfigureAwait(false); - foreach (var key in keys) + RejectUnsupportedMethod(did); + + // FR-CONSIST-06 / DD-01 require more than string-matching the kid into the relationship: + // the verification method must be genuinely *controlled by* the asserted DID. So we walk the + // raw resolved verification methods (not the curve-projected JWKs, which drop `controller`) + // and authorize the matching kid only when its id-subject AND its controller are the asserted + // DID — rejecting a key that sits under the relationship but is controlled by a different DID, + // or an embedded VM whose id belongs to another DID. (Issue #18.) + var result = await _resolver.ResolveAsync(did, ct: ct).ConfigureAwait(false); + if (result.DidDocument is null) + { + var error = result.ResolutionMetadata?.Error ?? "resolver returned no document"; + throw new DidResolutionException(did, error); + } + + var entries = relationship switch + { + VerificationRelationship.KeyAgreement => result.DidDocument.KeyAgreement, + VerificationRelationship.Authentication => result.DidDocument.Authentication, + _ => throw new ArgumentOutOfRangeException(nameof(relationship), relationship, "Unknown VerificationRelationship."), + }; + if (entries is null || entries.Count == 0) + return false; + + foreach (var entry in entries) { - if (string.Equals(key.Kid, kid, StringComparison.Ordinal)) - return true; + var method = ResolveVerificationMethod(result.DidDocument, entry, did); + + // VerificationMethod.id MAY be a relative DID URL ("#key-1"); normalize to absolute so it + // matches the absolute kid the envelope layer uses and so DidSubjectOf can extract a DID. + var methodId = method.Id is { } id && id.StartsWith('#') ? did + id : method.Id; + if (!string.Equals(methodId, kid, StringComparison.Ordinal)) + continue; + + // Found the kid under the right relationship; authorize only if it is controlled by `did` + // and is usable for the relationship's curve (preserving the prior curve-support filter). + return IsControlledBy(did, methodId, method) + && TryMaterialise(method, relationship) is not null; } + return false; } + /// + /// FR-CONSIST-06 controller rule: a verification method is authorized for + /// only when its (absolute) id resolves to the asserted DID subject AND its controller + /// (when present) is the asserted DID. An absent controller falls back to the id-subject rule. + /// + private static bool IsControlledBy(string did, string absoluteMethodId, VerificationMethod method) + { + if (!string.Equals(DidSubject.DidSubjectOf(absoluteMethodId), did, StringComparison.Ordinal)) + return false; // cross-DID verification-method id + + var controller = method.Controller.Value; + if (string.IsNullOrEmpty(controller)) + return true; // controller omitted — the id-subject rule above already bound it to `did` + + return string.Equals(DidSubject.DidSubjectOf(controller), did, StringComparison.Ordinal); + } + /// /// Dereference a verification-relationship entry: when it's a fragment reference, look the /// id up in the document's top-level verificationMethod array; when it's embedded, diff --git a/src/DidComm.Core/Threading/InMemoryThreadStateStore.cs b/src/DidComm.Core/Threading/InMemoryThreadStateStore.cs index 8df51b3..f47a4ae 100644 --- a/src/DidComm.Core/Threading/InMemoryThreadStateStore.cs +++ b/src/DidComm.Core/Threading/InMemoryThreadStateStore.cs @@ -7,22 +7,105 @@ namespace DidComm.Threading; /// agents; replace with a distributed store (Redis, Cosmos, etc.) for horizontally-scaled /// mediators where the same thread can be served by different processes. /// +/// +/// The store is bounded (issue #21). Thread states are keyed on the inbound message's +/// thid/id/pthid, which on a plaintext or anoncrypt envelope are +/// attacker-controlled and not cryptographically authenticated. An unbounded dictionary +/// would let a peer flood fresh ids and grow the store until the process OOMs. To prevent that, +/// retained entries are capped at (overridable via the ctor) and +/// the oldest-touched entries are evicted (approximate-LRU) once the cap is reached. A thread is +/// touched on every access, so one that is actively receiving is not evicted; note this holds only +/// within the eviction window — a thread left idle while a peer floods more than (cap − low-water) +/// fresh ids can still age out (the approximate-LRU tradeoff). +/// public sealed class InMemoryThreadStateStore : IThreadStateStore { + /// + /// Default cap on retained thread states. Large enough that no realistic legitimate + /// concurrent-thread workload on a single-instance agent is ever evicted, small enough that + /// worst-case memory stays bounded to a few MB instead of growing without limit (issue #21). + /// + public const int DefaultMaxEntries = 10_000; + private readonly ConcurrentDictionary _states = new(StringComparer.Ordinal); + private readonly int _maxEntries; + private readonly int _lowWaterMark; + private long _tick; // Interlocked monotonic clock for approximate-LRU stamping. + private int _evicting; // single-flight guard: 1 while an eviction pass is running, else 0. + private long _evictionPasses; // diagnostics: number of EvictDownToLowWater passes that ran. + + /// Create a store bounded at . + public InMemoryThreadStateStore() : this(DefaultMaxEntries) + { + } + + /// Create a store bounded at retained thread states. + /// Hard cap on retained thread states. Must be greater than zero. + /// is not positive. + public InMemoryThreadStateStore(int maxEntries) + { + if (maxEntries <= 0) + { + throw new ArgumentOutOfRangeException( + nameof(maxEntries), + maxEntries, + "Thread-state store capacity must be > 0; the cap bounds memory to prevent a flood of " + + "unauthenticated thread ids from exhausting the process (issue #21)."); + } + + _maxEntries = maxEntries; + // Trim to ~90% so eviction is amortized over ~10% of inserts rather than firing on every over-cap insert. + _lowWaterMark = Math.Max(1, maxEntries - Math.Max(1, maxEntries / 10)); + } + + /// Number of currently-retained thread states. Exposed for tests. + internal int Count => _states.Count; + + /// Number of eviction passes run so far. Exposed for tests (the single-flight guard). + internal long EvictionPasses => Interlocked.Read(ref _evictionPasses); /// public ThreadState GetOrCreate(string thid) { ArgumentException.ThrowIfNullOrEmpty(thid); - return _states.GetOrAdd(thid, static id => new ThreadState(id)); + var state = _states.GetOrAdd( + thid, + static (id, self) => new ThreadState(id) { LastTouchedTick = Interlocked.Increment(ref self._tick) }, + this); + state.LastTouchedTick = Interlocked.Increment(ref _tick); // touch on access (approximate-LRU) + + // Single-flight eviction: only ONE thread sorts+trims at a time; concurrent inserters that + // also see the store over capacity skip the pass instead of each running their own + // O(n log n) snapshot-sort. Without this guard, N concurrent inserts over the cap trigger up + // to N redundant full sorts per burst — a CPU-amplification DoS (issue #21 red-team). The + // store may transiently exceed the cap by the number of in-flight inserts during a pass; the + // next insert after the flag clears trims it back. + if (_states.Count > _maxEntries && Interlocked.CompareExchange(ref _evicting, 1, 0) == 0) + { + try + { + EvictDownToLowWater(); + } + finally + { + Volatile.Write(ref _evicting, 0); + } + } + + return state; } /// public ThreadState? Get(string thid) { ArgumentException.ThrowIfNullOrEmpty(thid); - return _states.TryGetValue(thid, out var state) ? state : null; + if (!_states.TryGetValue(thid, out var state)) + { + return null; + } + + state.LastTouchedTick = Interlocked.Increment(ref _tick); // a read counts as activity for LRU + return state; } /// @@ -31,4 +114,22 @@ public bool Remove(string thid) ArgumentException.ThrowIfNullOrEmpty(thid); return _states.TryRemove(thid, out _); } + + /// + /// Evict the oldest-touched entries until the store is back at the low-water mark. Runs only + /// when an insert pushes the store over capacity, so steady-state access to existing threads + /// pays nothing. is + /// atomic and idempotent, so concurrent eviction passes are harmless (at worst a slight over-trim). + /// + private void EvictDownToLowWater() + { + Interlocked.Increment(ref _evictionPasses); + var snapshot = _states.ToArray(); + Array.Sort(snapshot, static (a, b) => a.Value.LastTouchedTick.CompareTo(b.Value.LastTouchedTick)); + var target = snapshot.Length - _lowWaterMark; + for (var i = 0; i < snapshot.Length && i < target; i++) + { + _states.TryRemove(snapshot[i].Key, out _); + } + } } diff --git a/src/DidComm.Core/Threading/ThreadState.cs b/src/DidComm.Core/Threading/ThreadState.cs index d0bcc5b..82223c9 100644 --- a/src/DidComm.Core/Threading/ThreadState.cs +++ b/src/DidComm.Core/Threading/ThreadState.cs @@ -38,6 +38,13 @@ public sealed class ThreadState /// public bool MaxErrorsNoticeSent { get; set; } + /// + /// Monotonic last-touched stamp used by 's + /// approximate-LRU eviction to bound the store under a flood of fresh, unauthenticated + /// thids (issue #21). Not part of the public thread-state contract; other stores ignore it. + /// + internal long LastTouchedTick; + /// Construct empty state for . /// The thread id. Must be non-empty. public ThreadState(string thid) diff --git a/tasks/lessons.md b/tasks/lessons.md index 05c3819..422a260 100644 --- a/tasks/lessons.md +++ b/tasks/lessons.md @@ -447,3 +447,38 @@ Format per entry: - **How to apply:** When delegating, name the hard constraints explicitly ("the dataproofs ref MUST stay a local ProjectReference") and re-diff csproj/props after a subagent runs. Treat any reference-kind change as needing review, not auto-accept. + +## L-024 — A receive-side validation gate must match the spec's *receive-acceptance* set (Appendix C vectors), not the PRD's *emit* set. + +- **Lesson:** When building an inbound validation gate (e.g. legal envelope compositions), enumerate + the accepted set from the spec's interop test vectors, not from the PRD's "what we emit" rules. The + receive set is a superset of the emit set. +- **Why:** Issue #17's first cut enforced the FR-ENV-02 *emit* compositions (6) plus the FR-ENV-03 + receive-only `authcrypt(sign)` (7 total) and rejected everything else. That broke the real spec + fixture `c3-authcrypt-p521` — DIDComm v2.1 Appendix C.3 `anoncrypt(authcrypt(sign))`, an inbound + vector with `outcome: success`. FR-ENV-04 ("never emit `anoncrypt(authcrypt(sign))`") is an *emit* + MUST-NOT, not a receive prohibition; the PRD was simply silent on the full receive set. Fixed to the + grammar `anoncrypt? authcrypt? sign? plaintext` (8 shapes) and added PRD **FR-ENV-04a**. Relates to + L-005 (self-round-trip ≠ interop) and L-020 (verify against spec before trusting a test outcome). +- **How to apply:** For any inbound gate, list the spec/Appendix vectors it must accept and run the + interop fixtures *before* declaring the allow-set complete. If the PRD only describes emit behavior, + the receive-acceptance set is incomplete — fill it in (and update the PRD). + +## L-025 — Closing a body/status oracle leaves a timing oracle; and amortized-eviction analysis is single-threaded. + +- **Lesson:** Two red-team findings on my own #20/#21 fixes. (a) Normalizing error *responses* (uniform + 400, empty body) closes the content/status oracle but NOT the timing oracle — a held-vs-unheld + recipient kid still separates by ~180 µs because the decrypt path fast-fails before ECDH. (b) An + eviction whose cost is "amortized O(log n) per insert" is only amortized *single-threaded*; with N + concurrent inserters over the cap, each independently runs the full O(n log n) snapshot-sort → + up-to-Nx CPU stampede. Fixed with a single-flight `Interlocked.CompareExchange` gate. +- **Why:** Adversarial agents tasked with *breaking* the just-written fix (per the repo's "use + adversarial agents to attempt to exploit the code" rule) found both empirically — a body/status test + (`Should().BeEmpty()`) passes while the timing channel is wide open, and a serial eviction-cost + argument hides a concurrency stampede. +- **How to apply:** (1) When closing an oracle, enumerate ALL observables — body, status, headers, + **timing**, and connection behavior — not just the obvious one; if you can't make it constant-time + cheaply, file it and say so rather than claiming "no longer an oracle." (2) Any "amortized" cost + 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. diff --git a/tasks/todo20260616T214747Z.md b/tasks/todo20260616T214747Z.md new file mode 100644 index 0000000..8a32dc3 --- /dev/null +++ b/tasks/todo20260616T214747Z.md @@ -0,0 +1,175 @@ +# Plan — Fix the Medium-severity security cluster (issues #17–#21) in a single PR + +**Branch:** `feat/security-medium-cluster` (off `main` — never commit to main) +**Single PR** closing: **#17, #18, #19, #20, #21** — and, because the #20 fix is one shared error-handling +path, it also cleanly closes **#28** and **#33** (same receive/dispatch catch ladder). Flagged below for +explicit go/no-go. + +Source of each design: 5 parallel research agents that read the real code; designs cite exact file:line. +No source/test edits until this plan is approved. + +--- + +## Scope & approach per issue + +### [ ] #17 — EnvelopeReader accepts illegal envelope compositions (spec-noncompliance, Medium) +- **File:** `src/DidComm.Core/Composition/EnvelopeReader.cs` (the `Unpack` loop). +- **Fix:** add a per-layer composition trace that records each layer with the **authenticated/anonymous** + distinction (reusing the already-computed `JweParseResult.IsAuthenticated`), then assert the accumulated + outer→inner sequence is one of the 7 legal shapes at the terminal `Plaintext` branch (before + `message.Validate()`): + `[Plain]`, `[Sign,Plain]`, `[AnonEnc,Plain]`, `[AuthEnc,Plain]`, `[AnonEnc,Sign,Plain]`, + `[AnonEnc,AuthEnc,Plain]`, and (receive-only per FR-ENV-03) `[AuthEnc,Sign,Plain]`. +- **Reject** (→ `MalformedMessageException`): `sign(encrypt())`, `anoncrypt(anoncrypt())`, + `authcrypt(authcrypt())`, `authcrypt(anoncrypt())`, `signed(authcrypt())`, `signed(signed())`, + `anoncrypt(authcrypt(sign()))`, etc. The auth-flag is what distinguishes the **illegal** + `anoncrypt(anoncrypt)` from the **legal** `anoncrypt(authcrypt)` (identical at EnvelopeKind level). +- **Why MalformedMessageException:** it's the codebase's canonical structural-rejection type (used by + `EnvelopeDetector`, the depth-cap, JOSE normalization) and is in the FR-API-07 hierarchy. +- **Keeps green:** all existing round-trips (the 5 FR-ENV-02 shapes + the `authcrypt(sign)` consistency test). +- **New tests:** 6 illegal-shape negatives (hand-composed by re-wrapping a packed envelope) → `MalformedMessageException`; + 1 positive `authcrypt(sign)` round-trip. In `tests/DidComm.Core.Tests/Envelopes/Composition/EnvelopeReaderTests.cs`. +- **Grounding:** PRD FR-ENV-02/03/04/05, FR-API-03, FR-SIG-06. + +### [ ] #18 — Resolver authorization ignores VM `controller` (spec-noncompliance, Medium) +- **File:** `src/DidComm.Core/Resolution/NetDidKeyService.cs` (`IsKeyAuthorizedAsync`, ~105-119). +- **Root cause:** authorizes on `string.Equals(key.Kid, kid)` only; `TryMaterialise`/`ProjectMethod` discard + the VM's `controller` and `id`. Zero `controller` refs in `src/DidComm.Core`. +- **Fix:** re-implement `IsKeyAuthorizedAsync` to walk the **raw resolved `VerificationMethod`s** (not the + projected `Jwk`s) so `Id`/`Controller` are visible, and authorize a kid only when **(a)** the VM id's + DID-subject equals the queried DID (reject cross-DID embedded ids) **and (b)** the VM `controller` + (read as `vm.Controller.Value`, guarding `default(Did)`/null) equals the queried DID. **Reject** on a + foreign controller — do **not** recurse into it (the message asserts the queried DID as `from`/`to`, so a + foreign-controlled key can never satisfy the binding; simpler and strictly correct). +- **Confirmed available:** `NetDid.Core 2.0.1` `VerificationMethod.Controller` (a `Did` value-struct) is + populated on resolved docs; did:key/did:peer set controller = subject, so deterministic keys keep + authorizing. Apply the existing `#`-fragment normalization (lines 95-96) to the VM id before compare. +- **Exception:** unchanged path → `ConsistencyException (FR-CONSIST-06)` from `AddressingConsistency`. +- **New tests** (`tests/DidComm.Core.Tests/Resolution/NetDidKeyServiceTests.cs`, reuse `StubResolver`): + foreign-controller key under `from`'s relationship ⇒ `false`; cross-DID embedded id ⇒ `false`; + did:peer:2 relative-id ⇒ `true` (normalization regression guard); plus an interop unpack → + `ConsistencyException`. +- **Grounding:** PRD FR-CONSIST-06, DD-01. + +### [ ] #19 — from_prior JWT validator leaks FormatException/InvalidOperationException (bug/dos, Medium) +- **File:** `src/DidComm.Core/Protocols/Rotation/FromPriorValidator.cs` (decodes ~56-58 outside try; + catch filters at ~67/89 match only `JsonException`/`KeyNotFoundException`). +- **Fix:** wrap the base64url-decode + both JSON-read blocks in **one** try and broaden the catch filter to + `JsonException or KeyNotFoundException or FormatException or InvalidOperationException`, re-throwing a + single generic `ProtocolException("from_prior JWT is malformed.", ex)`. Keep `ProtocolException` (the + file's existing malformed-JWT type, matches the documented `` and the existing test). Resolver/ + crypto calls stay **outside** the try so genuine `ConsistencyException`/`CryptoException` aren't masked. +- **No oracle:** collapse the header-vs-claims messages into one generic string (strictly less info than today). +- **New tests** (`tests/DidComm.InteropTests/Rotation/FromPriorRotationTests.cs`): non-base64url segment, + `alg`/`iat`/`sub` wrong JSON kind, `iat` out-of-Int64-range, truncated JWT → all `ProtocolException` + (not raw runtime exceptions). +- **Grounding:** PRD FR-API-07, FR-ROT-01..06. + +### [ ] #20 — Verbatim CryptoException echoed to peer = decryption/kid oracle (security-vuln, Medium) +### (+ #28 escaping Consistency/Resolution/UnsupportedDidMethod → 500 oracle; + #33 handler-bug detail echoed) +- **File:** `src/DidComm.AspNetCore/DidCommEndpointRouteBuilderExtensions.cs` (both `MapPost` overloads). +- **Root cause:** `Results.Problem(detail: ex.Message, 400)` echoes JWE-layer reasons (kid, "AES-KW unwrap + failed", "epk not on curve", "apu…skid"); three `DidCommException` subtypes escape entirely → 500 (+ dev + stack trace); dispatch path echoes handler `InvalidOperationException` detail. +- **Fix:** one private helper `NormalizedReceiveRejection(logger, ex)` that **logs the real reason + server-side only** and returns a **bare `Results.StatusCode(400)` with no body** — identical status + empty + body for **every** input/handler-triggered failure (malformed, crypto, consistency, resolution, did:web, + secret-missing, handler bug). Replace both catch ladders with + `catch (OperationCanceledException) { throw; } catch (Exception ex) { return NormalizedReceiveRejection(...); }`; + in the registry overload, put unpack **and** dispatch under the same try so 400-vs-500 is gone. Add a logger + to the callback overload (it has none today). +- **Unchanged:** 202 success, 415 (Content-Type), 413 (MaxReceiveBytes) — all sit outside the unpack try; + cancellation still propagates. +- **New tests** (`tests/DidComm.InteropTests/Transports/AspNetCoreReceiveRoundTripTests.cs`): malformed, + crypto-reject, consistency-fail, did:web all return **identical** status+empty body (oracle closed); body + contains no kid/layer/DID/stack-trace text even under `Development`; handler-bug → 400 not 500 and no detail; + 202/415/413 unchanged. +- **DECISION FLAG:** including #28 + #33 is free (same code path) and is the elegant single-error-path. Default: + **include them** and close all three in this PR. Say so if you'd rather scope strictly to #20. +- **Grounding:** PRD FR-API-07, FR-TRN-10. + +### [ ] #21 — Unbounded per-thread state growth → memory-exhaustion DoS (security-vuln, Medium) +- **Files:** `src/DidComm.Core/Threading/InMemoryThreadStateStore.cs` (primary) + + `src/DidComm.Core/Threading/ThreadState.cs` (one `internal long LastTouchedTick`). +- **Fix:** bound the store. Add a `maxEntries` ctor arg defaulting to a new + `public const int DefaultMaxEntries = 10_000`; stamp each `ThreadState` with an `Interlocked` monotonic tick + on create/access; in `GetOrCreate`, when an insert pushes `Count` over the cap, evict the oldest-touched + entries down to a ~90% low-water mark (snapshot + sort by tick + `TryRemove`, batched/amortized). + Approximate-LRU, no new lock, no interaction with the `lock (failingThread)` cascade seam. +- **Why ctor arg, not DidCommOptions:** the cap is specific to this replaceable in-memory store; matches the + `ProblemReportOptions` pattern. Parameterless ctor → default, so secure-by-default; **no `DidCommBuilder` + wiring change** (the `TryAddSingleton` keeps working). +- **Out of scope (follow-up):** terminal-state eviction (no reliable "thread ended" signal today) and a TTL + sweep. The cap alone converts "unbounded → OOM" into "bounded ≈ a few MB", which closes the DoS. +- **New tests** (`tests/DidComm.Core.Tests/Threading/InMemoryThreadStateStoreTests.cs`): flood 1000 distinct + thids with cap 100 ⇒ `Count ≤ 100`; oldest evicted / newest retained; a continuously-touched live thread + survives a flood with state intact; ctor validation (`0`/negative ⇒ throw); concurrency smoke. + +--- + +## Cross-cutting + +- [ ] Create branch `feat/security-medium-cluster` off `main`. +- [ ] Implement the 5 fixes (each minimal, no public-API breakage; only additive surface: #21's ctor overload + const). +- [ ] Add the negative/regression tests listed per issue. +- [ ] `dotnet build` (warnings-as-errors is on) + `dotnet test` — full suite green, including the new tests. +- [ ] Update `CHANGELOG.md` (Security section) with the cluster + issue refs. +- [ ] Update `tasks/lessons.md` only if a correction surfaces during implementation. +- [ ] Open ONE PR: title `fix(security): remediate Medium-severity audit cluster (#17–#21)`, body summarizing + each fix with exploit-before/after, `Closes #17 #18 #19 #20 #21` (+ `Closes #28 #33` if the flag is accepted). + +## Verification standard (per AGENTS.md §4) +- For #17/#18/#20: test BOTH post-tamper and self-consistent-forged inputs where applicable (e.g. #18 forged + doc whose signed identity fields disagree with controller; #17 hand-composed illegal shapes). +- Prove oracle-closed for #20 by asserting byte-identical responses across distinct failure classes. +- Diff behavior vs `main` for the receive endpoint and the composition gate. + +## Review section + +All five Medium fixes (+ #28/#33) implemented on `feat/security-medium-cluster`. Full suite **598 +tests green** (491 Core + 107 Interop), `dotnet build` clean under warnings-as-errors. +770/−58 across +8 source/doc files and 5 test files. + +- **#17** — implemented as the receive grammar `anoncrypt? authcrypt? sign? plaintext`. **Correction + mid-implementation:** the first cut enumerated only 7 shapes and broke the real spec interop fixture + `c3-authcrypt-p521` (DIDComm v2.1 Appendix C.3 `anoncrypt(authcrypt(sign))`, `outcome: success`). + Root cause: I conflated the PRD **emit** set (FR-ENV-02/04) with the **receive-acceptance** set, + which is broader. Fixed the gate to the 8-shape ordered grammar and added **PRD FR-ENV-04a** + documenting the receive set. Lesson captured as L-022. +- **#18** — controller + id-subject check; preserved did:key/did:peer authorization and the did:peer:2 + relative-id normalization (explicit regression test). No public API change. +- **#19** — single normalize-to-`ProtocolException` boundary; also caught `ArgumentException` + (empty JWT segment) which the design had missed. +- **#20/#28/#33** — one `NormalizedReceiveRejection` helper; oracle-closed proven by a test asserting + malformed / crypto-reject / consistency-forgery all return byte-identical `400` + empty body. +- **#21** — bounded store + approximate-LRU; concurrency-smoke test; no lock added, cascade-guard + `lock` seam untouched. + +**Excluded from the commit:** an unrelated `AGENTS.md` edit (user's own contributor-guide addition) +and the untracked `.claude/` dir. + +**Follow-ups (not in this PR):** thread-state terminal-eviction + TTL sweep (#21 noted as follow-up); +the remaining Low/Info issues #22–#27, #29–#32, #34. + +### Adversarial red-team pass (per AGENTS.md "use adversarial agents to attempt to exploit the code") + +Ran 5 break-it agents (one per fix) with a remote-attacker threat model; each built PoCs. + +- **#17, #18, #19 — held.** No bypass found. #17 PoC hand-wrapped illegal onions through the real + crypto (all rejected; `IsAuthenticated` is verified-crypto-derived, not attacker-header-derived; + gate is on the sole unpack path). #18 confirmed id-subject-then-controller ordering, ordinal + non-canonicalizing DID compare (no case/unicode/percent collisions), fail-closed. #19 verified every + reachable exception type is in the catch filter (incl. `GetInt64` overflow → `FormatException`, not + `OverflowException`; UTF8 uses replacement fallback) and depth/size are bounded. +- **#20 — TWO findings, both addressed.** (a) MED-HIGH callback-overload 500 oracle: `onReceive` ran + outside the try → a throwing callback returned 500, distinguishing unpack-success. **Fixed** — + callback wrapped, returns 202 (logged). (b) HIGH residual *timing* oracle (held-vs-unheld kid, + ~180 µs): pre-existing in the decrypt fast-fail; proper fix is a response-time floor (throughput + tradeoff). **Filed as #35**; CHANGELOG wording softened to "response-content oracle closed". +- **#21 — TWO findings.** (a) HIGH concurrent eviction-storm (up-to-Nx O(n log n) sorts): **fixed** with + single-flight `Interlocked` gate + regression test. (b) MED cascade-guard reset via forced eviction + (~`cap` msgs/report): **filed as #36** (alongside #29); not "fixed" by exempting entries (that + re-opens unbounded growth). + +Net: 600 tests green. Lesson L-025 captured. Two in-scope hardening fixes folded into the PR; two +deeper residuals filed (#35, #36). diff --git a/tests/DidComm.Core.Tests/Envelopes/Composition/EnvelopeReaderTests.cs b/tests/DidComm.Core.Tests/Envelopes/Composition/EnvelopeReaderTests.cs index f0dc6fe..2868f0e 100644 --- a/tests/DidComm.Core.Tests/Envelopes/Composition/EnvelopeReaderTests.cs +++ b/tests/DidComm.Core.Tests/Envelopes/Composition/EnvelopeReaderTests.cs @@ -1,10 +1,14 @@ +using System.Text; using DidComm.Composition; using DidComm.Exceptions; using DidComm.Jose; +using DidComm.Jose.Signing; using DidComm.Messages; using FluentAssertions; using NetCrypto; using Xunit; +using DpEnc = DataProofsDotnet.Jose.Encryption; +using DpSig = DataProofsDotnet.Jose.Signing; using JoseCryptoProvider = DataProofsDotnet.Jose.JoseCryptoProvider; namespace DidComm.Tests.Envelopes.Composition; @@ -267,4 +271,157 @@ public async Task Consistency_check_blocks_authcrypt_sign_inner_signer_not_match act.Should().Throw().WithMessage("*FR-CONSIST-05*"); } + + // ---- Issue #17: only the legal FR-ENV-02 compositions (+ authcrypt(sign) on receive, FR-ENV-03) + // may unpack. Illegal layer orderings, built by hand-wrapping a packed envelope, MUST be + // rejected as malformed structure BEFORE any consistency/content processing. ---- + + private static string AnoncryptWrap(string inner, Jwk recipientPublicJwk) => + DpEnc.JweBuilder.BuildEcdhEsA256Kw( + Encoding.UTF8.GetBytes(inner), new[] { recipientPublicJwk }, "A256GCM", _crypto, MediaTypes.Encrypted); + + private static string AuthcryptWrap(string inner, Jwk recipientPublicJwk, Jwk senderPrivateJwk, string skid) => + DpEnc.JweBuilder.BuildEcdh1PuA256Kw( + Encoding.UTF8.GetBytes(inner), new[] { recipientPublicJwk }, senderPrivateJwk, skid, "A256CBC-HS512", _crypto, MediaTypes.Encrypted); + + private static Task SignWrapAsync(string inner, Jwk signerPrivateJwk) + { + var signers = new List { JwsSignerFactory.FromPrivateJwk(signerPrivateJwk) }; + return DpSig.JwsBuilder.BuildJsonAsync(Encoding.UTF8.GetBytes(inner), signers, MediaTypes.Signed, detachedPayload: false); + } + + private static Message EmptyMessage() => new MessageBuilder() + .WithType("https://didcomm.org/empty/1.0/empty") + .WithFrom("did:example:alice") + .WithTo("did:example:bob") + .Build(); + + [Fact] + public async Task Anoncrypt_of_anoncrypt_is_rejected_as_illegal_composition() + { + // [AnonEncrypt, AnonEncrypt, Plaintext] — only anoncrypt(authcrypt) is legal; the inner + // encrypt MUST be authenticated. This is the case the EnvelopeKind stack alone can't tell + // apart from the legal anoncrypt(authcrypt) — the auth flag is what distinguishes them. + var bob = TestKeyMaterial.Generate(KeyType.X25519, "did:example:bob#x"); + var inner = await EnvelopeWriter.PackEncryptedAsync( + new PackEncryptedParameters(EmptyMessage(), new[] { bob.PublicJwk }, "A256GCM"), _crypto); + var outer = AnoncryptWrap(inner, bob.PublicJwk); + + Action act = () => EnvelopeReader.Unpack(outer, + new DictionarySecretsLookup(new[] { bob.PrivateJwk }), senderLookup: null, signerLookup: null, _crypto); + + act.Should().Throw().WithMessage("*Illegal*composition*"); + } + + [Fact] + public async Task Authcrypt_of_authcrypt_is_rejected_as_illegal_composition() + { + // [AuthEncrypt, AuthEncrypt, Plaintext] — double authcrypt is not a legal shape. + var alice = TestKeyMaterial.Generate(KeyType.X25519, "did:example:alice#x"); + var bob = TestKeyMaterial.Generate(KeyType.X25519, "did:example:bob#x"); + var inner = await EnvelopeWriter.PackEncryptedAsync( + new PackEncryptedParameters(EmptyMessage(), new[] { bob.PublicJwk }, "A256CBC-HS512", + SenderPrivateJwk: alice.PrivateJwk, Skid: alice.PublicJwk.Kid), _crypto); + var outer = AuthcryptWrap(inner, bob.PublicJwk, alice.PrivateJwk, alice.PublicJwk.Kid!); + + Action act = () => EnvelopeReader.Unpack(outer, + new DictionarySecretsLookup(new[] { bob.PrivateJwk }), + senderLookup: new DictionarySenderKeyLookup(new[] { alice.PublicJwk }), + signerLookup: null, _crypto); + + act.Should().Throw().WithMessage("*Illegal*composition*"); + } + + [Fact] + public async Task Sign_outside_encrypt_is_rejected_as_illegal_composition() + { + // [Sign, AnonEncrypt, Plaintext] — DIDComm signs INSIDE encrypt (FR-ENV-05). A JWS wrapping a + // JWE signs opaque ciphertext, defeating FR-SIG-06's anti-surreptitious-forwarding intent. + var signer = TestKeyMaterial.Generate(KeyType.Ed25519, "did:example:alice#k"); + var bob = TestKeyMaterial.Generate(KeyType.X25519, "did:example:bob#x"); + var inner = await EnvelopeWriter.PackEncryptedAsync( + new PackEncryptedParameters(EmptyMessage(), new[] { bob.PublicJwk }, "A256GCM"), _crypto); + var outer = await SignWrapAsync(inner, signer.PrivateJwk); + + Action act = () => EnvelopeReader.Unpack(outer, + new DictionarySecretsLookup(new[] { bob.PrivateJwk }), + senderLookup: null, + signerLookup: _ => signer.PublicJwk, _crypto); + + act.Should().Throw().WithMessage("*Illegal*composition*"); + } + + [Fact] + public async Task Signed_of_signed_is_rejected_as_illegal_composition() + { + // [Sign, Sign, Plaintext] — double signature is not a legal shape. + var s1 = TestKeyMaterial.Generate(KeyType.Ed25519, "did:example:alice#k1"); + var s2 = TestKeyMaterial.Generate(KeyType.Ed25519, "did:example:alice#k2"); + var inner = await EnvelopeWriter.PackSignedAsync(new PackSignedParameters(EmptyMessage(), new[] { s1.PrivateJwk })); + var outer = await SignWrapAsync(inner, s2.PrivateJwk); + + Action act = () => EnvelopeReader.Unpack(outer, + new DictionarySecretsLookup(Array.Empty()), + senderLookup: null, + signerLookup: kid => kid == s1.PublicJwk.Kid ? s1.PublicJwk : kid == s2.PublicJwk.Kid ? s2.PublicJwk : null, + _crypto); + + act.Should().Throw().WithMessage("*Illegal*composition*"); + } + + [Fact] + public async Task Authcrypt_of_sign_is_accepted_on_receive_FrEnv03() + { + // [AuthEncrypt, Sign, Plaintext] — we never EMIT authcrypt(sign), but the spec lets us ACCEPT + // it on receive (FR-ENV-03). With the signer and skid both under did:example:alice, the + // consistency checks pass and the message unpacks; the gate must NOT reject the shape. + var aliceKa = TestKeyMaterial.Generate(KeyType.X25519, "did:example:alice#x"); + var aliceSign = TestKeyMaterial.Generate(KeyType.Ed25519, "did:example:alice#k"); + var bob = TestKeyMaterial.Generate(KeyType.X25519, "did:example:bob#x"); + + var packed = await EnvelopeWriter.PackEncryptedAsync( + new PackEncryptedParameters(EmptyMessage(), new[] { bob.PublicJwk }, "A256CBC-HS512", + SenderPrivateJwk: aliceKa.PrivateJwk, + Skid: aliceKa.PublicJwk.Kid, + SignerPrivateJwks: new[] { aliceSign.PrivateJwk }), _crypto); + + var unpacked = EnvelopeReader.Unpack(packed, + new DictionarySecretsLookup(new[] { bob.PrivateJwk }), + senderLookup: new DictionarySenderKeyLookup(new[] { aliceKa.PublicJwk }), + signerLookup: kid => kid == aliceSign.PublicJwk.Kid ? aliceSign.PublicJwk : null, + _crypto); + + unpacked.Stack.Should().Equal(EnvelopeKind.Encrypted, EnvelopeKind.Signed, EnvelopeKind.Plaintext); + unpacked.Authenticated.Should().BeTrue(); + unpacked.NonRepudiation.Should().BeTrue(); + } + + [Fact] + public async Task Anoncrypt_authcrypt_sign_is_accepted_on_receive() + { + // [AnonEncrypt, AuthEncrypt, Sign, Plaintext] — protect-sender + sign. The library emits this + // via ProtectSender + signers, and the spec's Appendix C.3 vector requires accepting it. The + // legal receive grammar (anoncrypt? authcrypt? sign? plaintext) admits it. + var aliceKa = TestKeyMaterial.Generate(KeyType.X25519, "did:example:alice#x"); + var aliceSign = TestKeyMaterial.Generate(KeyType.Ed25519, "did:example:alice#k"); + var bob = TestKeyMaterial.Generate(KeyType.X25519, "did:example:bob#x"); + + var packed = await EnvelopeWriter.PackEncryptedAsync( + new PackEncryptedParameters(EmptyMessage(), new[] { bob.PublicJwk }, "A256CBC-HS512", + SenderPrivateJwk: aliceKa.PrivateJwk, + Skid: aliceKa.PublicJwk.Kid, + SignerPrivateJwks: new[] { aliceSign.PrivateJwk }, + ProtectSender: true), _crypto); + + var unpacked = EnvelopeReader.Unpack(packed, + new DictionarySecretsLookup(new[] { bob.PrivateJwk }), + senderLookup: new DictionarySenderKeyLookup(new[] { aliceKa.PublicJwk }), + signerLookup: kid => kid == aliceSign.PublicJwk.Kid ? aliceSign.PublicJwk : null, + _crypto); + + unpacked.Stack.Should().Equal(EnvelopeKind.Encrypted, EnvelopeKind.Encrypted, EnvelopeKind.Signed, EnvelopeKind.Plaintext); + unpacked.Authenticated.Should().BeTrue(); + unpacked.AnonymousSender.Should().BeTrue(); + unpacked.NonRepudiation.Should().BeTrue(); + } } diff --git a/tests/DidComm.Core.Tests/Resolution/NetDidKeyServiceTests.cs b/tests/DidComm.Core.Tests/Resolution/NetDidKeyServiceTests.cs index 7f929e5..6c09b48 100644 --- a/tests/DidComm.Core.Tests/Resolution/NetDidKeyServiceTests.cs +++ b/tests/DidComm.Core.Tests/Resolution/NetDidKeyServiceTests.cs @@ -245,6 +245,104 @@ public async Task IsKeyAuthorizedAsync_TrueWhenKidPresent() .Should().BeFalse(); } + private const string X25519Point = "avH0O2Y4tqLAq8y9zpianr8ajii5m4F_mICrzNlatXs"; + + [Fact] + public async Task IsKeyAuthorizedAsync_FalseWhenControllerIsADifferentDid() + { + // Issue #18 / FR-CONSIST-06 negative test: the key sits under alice's keyAgreement and its id + // subject is alice, but it is CONTROLLED by eve. A key controlled by a different DID MUST be + // rejected even though DidSubjectOf(kid) == alice. + var vm = new VerificationMethod + { + Id = "did:example:alice#k", + Type = "JsonWebKey2020", + Controller = new Did("did:example:eve"), + PublicKeyJwk = new JsonWebKey { Kty = "OKP", Crv = "X25519", X = X25519Point }, + }; + var doc = new DidDocument + { + Id = new Did("did:example:alice"), + VerificationMethod = new[] { vm }, + KeyAgreement = new[] { VerificationRelationshipEntry.FromEmbedded(vm) }, + }; + var sut = new NetDidKeyService(new StubResolver((doc.Id.Value!, doc))); + + (await sut.IsKeyAuthorizedAsync("did:example:alice", "did:example:alice#k", VerificationRelationship.KeyAgreement)) + .Should().BeFalse(); + } + + [Fact] + public async Task IsKeyAuthorizedAsync_FalseForCrossDidEmbeddedId() + { + // Issue #18: an embedded VM whose id belongs to a different DID must not be authorized for the + // queried DID, even if it is listed under the queried DID's relationship. + var vm = new VerificationMethod + { + Id = "did:example:eve#k", + Type = "JsonWebKey2020", + Controller = new Did("did:example:eve"), + PublicKeyJwk = new JsonWebKey { Kty = "OKP", Crv = "X25519", X = X25519Point }, + }; + var doc = new DidDocument + { + Id = new Did("did:example:alice"), + VerificationMethod = new[] { vm }, + KeyAgreement = new[] { VerificationRelationshipEntry.FromEmbedded(vm) }, + }; + var sut = new NetDidKeyService(new StubResolver((doc.Id.Value!, doc))); + + (await sut.IsKeyAuthorizedAsync("did:example:alice", "did:example:eve#k", VerificationRelationship.KeyAgreement)) + .Should().BeFalse(); + } + + [Fact] + public async Task IsKeyAuthorizedAsync_TrueForRelativeIdWithSelfController() + { + // Regression guard: a did:peer:2-style relative VM id ("#key-1") controlled by the same DID + // must still authorize. Proves the id is normalized to absolute before both the kid compare + // and the controller/subject check (the highest-risk spot for the #18 change). + var vm = new VerificationMethod + { + Id = "#key-1", + Type = "JsonWebKey2020", + Controller = new Did("did:example:alice"), + PublicKeyJwk = new JsonWebKey { Kty = "OKP", Crv = "X25519", X = X25519Point }, + }; + var doc = new DidDocument + { + Id = new Did("did:example:alice"), + VerificationMethod = new[] { vm }, + KeyAgreement = new[] { VerificationRelationshipEntry.FromEmbedded(vm) }, + }; + var sut = new NetDidKeyService(new StubResolver((doc.Id.Value!, doc))); + + (await sut.IsKeyAuthorizedAsync("did:example:alice", "did:example:alice#key-1", VerificationRelationship.KeyAgreement)) + .Should().BeTrue(); + } + + [Fact] + public async Task IsKeyAuthorizedAsync_TrueWhenControllerOmittedAndIdSubjectMatches() + { + // Absent controller falls back to the id-subject rule, which already binds the key to alice. + var vm = new VerificationMethod + { + Id = "did:example:alice#k", + Type = "JsonWebKey2020", + PublicKeyJwk = new JsonWebKey { Kty = "OKP", Crv = "X25519", X = X25519Point }, + }; + var doc = new DidDocument + { + Id = new Did("did:example:alice"), + VerificationMethod = new[] { vm }, + KeyAgreement = new[] { VerificationRelationshipEntry.FromEmbedded(vm) }, + }; + var sut = new NetDidKeyService(new StubResolver((doc.Id.Value!, doc))); + + (await sut.IsKeyAuthorizedAsync("did:example:alice", "did:example:alice#k", VerificationRelationship.KeyAgreement)) + .Should().BeTrue(); + } + /// Hand-rolled resolver that returns pre-canned documents and counts invocations. private sealed class StubResolver : IDidResolver { diff --git a/tests/DidComm.Core.Tests/Threading/InMemoryThreadStateStoreTests.cs b/tests/DidComm.Core.Tests/Threading/InMemoryThreadStateStoreTests.cs index 60aa502..ee5c8c0 100644 --- a/tests/DidComm.Core.Tests/Threading/InMemoryThreadStateStoreTests.cs +++ b/tests/DidComm.Core.Tests/Threading/InMemoryThreadStateStoreTests.cs @@ -62,4 +62,102 @@ public void Empty_thid_throws() ((Action)(() => store.Get(string.Empty))).Should().Throw(); ((Action)(() => store.Remove(string.Empty))).Should().Throw(); } + + [Fact] + public void Store_is_bounded_under_a_flood_of_fresh_thids() + { + // Issue #21: a peer flooding fresh, unauthenticated thids must not grow the store without + // limit. With a small cap, inserting far more distinct ids keeps the count at/under the cap. + var store = new InMemoryThreadStateStore(maxEntries: 100); + for (var i = 0; i < 1_000; i++) + { + store.GetOrCreate($"thid-{i}"); + } + + store.Count.Should().BeLessThanOrEqualTo(100); + } + + [Fact] + public void Oldest_untouched_threads_are_evicted_newest_retained() + { + var store = new InMemoryThreadStateStore(maxEntries: 100); + for (var i = 0; i < 1_000; i++) + { + store.GetOrCreate($"thid-{i}"); + } + + // The most recently created id survives; an early one has aged out. + store.Get("thid-999").Should().NotBeNull(); + store.Get("thid-0").Should().BeNull(); + } + + [Fact] + public void A_live_thread_within_the_cap_survives_a_flood_with_state_intact() + { + // A legitimate multi-message thread is touched on every access, so approximate-LRU never + // evicts it even while an attacker floods junk ids around it. + var store = new InMemoryThreadStateStore(maxEntries: 100); + var live = store.GetOrCreate("live"); + live.AcceptLang = new[] { "fr" }; + + for (var i = 0; i < 1_000; i++) + { + store.GetOrCreate($"junk-{i}"); + store.GetOrCreate("live"); // keep the live thread warm, as real traffic would + } + + var stillLive = store.Get("live"); + stillLive.Should().NotBeNull(); + stillLive!.AcceptLang.Should().Equal("fr"); + } + + [Fact] + public void Non_positive_capacity_throws() + { + ((Action)(() => _ = new InMemoryThreadStateStore(0))).Should().Throw(); + ((Action)(() => _ = new InMemoryThreadStateStore(-5))).Should().Throw(); + } + + [Fact] + public async Task Concurrent_flood_stays_bounded_and_does_not_throw() + { + // NFR-03: shared store under concurrent access. Eviction races must not throw and the + // final count must respect the cap. + var store = new InMemoryThreadStateStore(maxEntries: 200); + var tasks = Enumerable.Range(0, 8).Select(worker => Task.Run(() => + { + for (var i = 0; i < 1_000; i++) + { + store.GetOrCreate($"w{worker}-thid-{i}"); + } + })); + + await Task.WhenAll(tasks); + store.Count.Should().BeLessThanOrEqualTo(200); + } + + [Fact] + public async Task Concurrent_flood_does_not_trigger_an_eviction_storm() + { + // Issue #21 red-team: without single-flight eviction, N concurrent inserters over the cap each + // run their own O(n log n) snapshot-sort, so eviction passes scale with concurrency (a CPU DoS). + // With the guard, passes track the insert count (~one per cap/10 inserts), not the thread count. + const int cap = 500; + const int perWorker = 50_000; + const int workers = 8; + var store = new InMemoryThreadStateStore(maxEntries: cap); + + var tasks = Enumerable.Range(0, workers).Select(w => Task.Run(() => + { + for (var i = 0; i < perWorker; i++) + store.GetOrCreate($"w{w}-{i}"); + })); + await Task.WhenAll(tasks); + + // Serial expectation ≈ totalInserts / (cap - lowWater) = 400_000 / 50 ≈ 8_000 passes. A storm + // would multiply that by up to `workers`. Assert we stay within a small multiple of serial. + const long serialExpectation = (long)workers * perWorker / (cap / 10); + store.EvictionPasses.Should().BeLessThan(serialExpectation * 3); + store.Count.Should().BeLessThanOrEqualTo(cap); + } } diff --git a/tests/DidComm.InteropTests/Rotation/FromPriorRotationTests.cs b/tests/DidComm.InteropTests/Rotation/FromPriorRotationTests.cs index 5ecd69a..9451c5b 100644 --- a/tests/DidComm.InteropTests/Rotation/FromPriorRotationTests.cs +++ b/tests/DidComm.InteropTests/Rotation/FromPriorRotationTests.cs @@ -117,6 +117,99 @@ public async Task Validator_RejectsMalformedJwt() await act.Should().ThrowAsync(); } + // Issue #19: malformed from_prior JWT bytes (bad base64url, empty segment, or a claim/header of + // the wrong JSON value-kind) must surface as the typed ProtocolException, NOT escape UnpackAsync + // as a raw FormatException / InvalidOperationException / ArgumentException. Each case corrupts a + // segment of an otherwise-valid JWT and fails at the parse stage, before signature verification. + + [Theory] + [InlineData("!!!not-base64url!!!", 0)] // header segment is not base64url -> FormatException + [InlineData("@@@", 2)] // signature segment is not base64url -> FormatException + public async Task Validator_RejectsNonBase64UrlSegment_AsProtocolException(string garbage, int segmentIndex) + { + var jwt = await FromPriorBuilder.BuildAsync(SampleClaims(), SignerPrivateJwk()); + var parts = jwt.Split('.'); + parts[segmentIndex] = garbage; + var malformed = string.Join('.', parts); + + var act = async () => await FromPriorValidator.ValidateAsync(malformed, NewSenderDid, NewKeyService()); + + await act.Should().ThrowAsync(); + } + + [Fact] + public async Task Validator_RejectsEmptySegment_AsProtocolException() + { + var jwt = await FromPriorBuilder.BuildAsync(SampleClaims(), SignerPrivateJwk()); + var parts = jwt.Split('.'); + var malformed = $"{parts[0]}..{parts[2]}"; // empty claims segment -> ArgumentException in Base64Url.Decode + + var act = async () => await FromPriorValidator.ValidateAsync(malformed, NewSenderDid, NewKeyService()); + + await act.Should().ThrowAsync(); + } + + [Fact] + public async Task Validator_RejectsWrongKindAlgHeader_AsProtocolException() + { + // "alg" is a number instead of a string -> GetString() throws InvalidOperationException. + var jwt = await FromPriorBuilder.BuildAsync(SampleClaims(), SignerPrivateJwk()); + var malformed = MutateHeader(jwt, h => h["alg"] = 123); + + var act = async () => await FromPriorValidator.ValidateAsync(malformed, NewSenderDid, NewKeyService()); + + await act.Should().ThrowAsync(); + } + + [Fact] + public async Task Validator_RejectsWrongKindSubClaim_AsProtocolException() + { + // "sub" is an object instead of a string -> GetString() throws InvalidOperationException. + var jwt = await FromPriorBuilder.BuildAsync(SampleClaims(), SignerPrivateJwk()); + var malformed = MutateClaims(jwt, c => c["sub"] = new JsonObject()); + + var act = async () => await FromPriorValidator.ValidateAsync(malformed, NewSenderDid, NewKeyService()); + + await act.Should().ThrowAsync(); + } + + [Fact] + public async Task Validator_RejectsWrongKindIatClaim_AsProtocolException() + { + // "iat" is a string instead of a number -> GetInt64() throws InvalidOperationException. + var jwt = await FromPriorBuilder.BuildAsync(SampleClaims(), SignerPrivateJwk()); + var malformed = MutateClaims(jwt, c => c["iat"] = "not-a-number"); + + var act = async () => await FromPriorValidator.ValidateAsync(malformed, NewSenderDid, NewKeyService()); + + await act.Should().ThrowAsync(); + } + + [Fact] + public async Task Validator_RejectsOutOfRangeIatClaim_AsProtocolException() + { + // "iat" is a JSON number outside Int64 range -> GetInt64() throws FormatException. + var jwt = await FromPriorBuilder.BuildAsync(SampleClaims(), SignerPrivateJwk()); + var malformed = MutateClaims(jwt, c => c["iat"] = JsonNode.Parse("99999999999999999999999")); + + var act = async () => await FromPriorValidator.ValidateAsync(malformed, NewSenderDid, NewKeyService()); + + await act.Should().ThrowAsync(); + } + + private static string MutateHeader(string jwt, Action mutate) => MutateSegment(jwt, 0, mutate); + + private static string MutateClaims(string jwt, Action mutate) => MutateSegment(jwt, 1, mutate); + + private static string MutateSegment(string jwt, int segmentIndex, Action mutate) + { + var parts = jwt.Split('.'); + var obj = JsonNode.Parse(Encoding.UTF8.GetString(Base64Url.Decode(parts[segmentIndex])))!.AsObject(); + mutate(obj); + parts[segmentIndex] = Base64Url.Encode(Encoding.UTF8.GetBytes(obj.ToJsonString())); + return string.Join('.', parts); + } + [Fact] public async Task DidCommClient_PopulatesFromPriorOnAuthcryptedUnpack() { diff --git a/tests/DidComm.InteropTests/Transports/AspNetCoreReceiveRoundTripTests.cs b/tests/DidComm.InteropTests/Transports/AspNetCoreReceiveRoundTripTests.cs index 5050106..320c558 100644 --- a/tests/DidComm.InteropTests/Transports/AspNetCoreReceiveRoundTripTests.cs +++ b/tests/DidComm.InteropTests/Transports/AspNetCoreReceiveRoundTripTests.cs @@ -17,6 +17,8 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Options; +using NetDid.Core; +using NetDid.Core.Model; using Xunit; namespace DidComm.InteropTests.Transports; @@ -111,15 +113,145 @@ public async Task Returns_400_when_body_is_not_a_valid_didcomm_envelope() var response = await client.SendAsync(request); response.StatusCode.Should().Be(HttpStatusCode.BadRequest); + // Issue #20: the body carries no detail (no Problem JSON) that could form an oracle. + (await response.Content.ReadAsStringAsync()).Should().BeEmpty(); } - private static async Task<(TestServer Server, List Received)> BuildServerAsync(Action? configureOptions = null) + [Fact] + public async Task Receive_rejections_are_indistinguishable_so_there_is_no_oracle() + { + // Issues #20/#28: a malformed envelope (MalformedMessageException), an undecryptable one + // (CryptoException), and a self-consistent forgery that fails an addressing rule + // (ConsistencyException) MUST all yield the SAME response — same 400 status, same empty body. + // Previously crypto failures leaked the failing recipient kid / layer, and a ConsistencyException + // escaped to a 500 (+ dev stack trace), so a peer could distinguish the failure class. + var (server, _) = await BuildServerAsync(); + + var malformed = "not even json"; + var undecryptable = CorruptField(await PackAnoncryptForBobAsync(), "ciphertext"); + var forged = await PackMismatchedFromAuthcryptAsync(); + + var results = new[] + { + await PostEnvelopeAsync(server, malformed), + await PostEnvelopeAsync(server, undecryptable), + await PostEnvelopeAsync(server, forged), + }; + + results.Should().AllSatisfy(r => + { + r.Status.Should().Be(HttpStatusCode.BadRequest); + r.Body.Should().BeEmpty(); // no kid / layer / DID / stack-trace text distinguishes the classes + }); + } + + private static DidCommClient NewPackerClient() { - var received = new List(); var actors = SpecActorRegistry.LoadDefault(); var resolver = LoadResolver(); var keyService = new NetDidKeyService(resolver); var serviceResolver = new NetDidServiceEndpointResolver(resolver, keyService, new DidCommOptions()); + return new DidCommClient(actors.AsSecretsResolver(), keyService, serviceResolver, new DidCommOptions()); + } + + private static async Task PackAnoncryptForBobAsync() + { + var packed = await NewPackerClient().PackEncryptedAsync( + NewProposal(), new PackEncryptedOptions(Recipients: new[] { "did:example:bob" })); + return packed.Message; + } + + private static async Task PackMismatchedFromAuthcryptAsync() + { + // authcrypt skid = alice, but the inner plaintext claims from = carol → FR-CONSIST-01 fails + // on unpack (a self-consistent forgery the server can fully decrypt before rejecting). + var message = new MessageBuilder() + .WithType("http://example.com/protocols/lets_do_lunch/1.0/proposal") + .WithFrom("did:example:carol") + .WithTo("did:example:bob") + .WithBody(System.Text.Json.Nodes.JsonNode.Parse("""{"a":"b"}""")!.AsObject()) + .Build(); + var packed = await NewPackerClient().PackEncryptedAsync( + message, new PackEncryptedOptions(Recipients: new[] { "did:example:bob" }, From: "did:example:alice")); + return packed.Message; + } + + private static string CorruptField(string jweJson, string field) + { + var node = System.Text.Json.Nodes.JsonNode.Parse(jweJson)!.AsObject(); + var original = node[field]!.GetValue(); + var flipped = original[..^1] + (original[^1] == 'A' ? 'B' : 'A'); // still valid base64url; AEAD tag fails + node[field] = flipped; + return node.ToJsonString(); + } + + private static async Task<(HttpStatusCode Status, string Body)> PostEnvelopeAsync(TestServer server, string body) + { + using var client = server.CreateClient(); + using var request = new HttpRequestMessage(HttpMethod.Post, "/didcomm") + { + Content = new ByteArrayContent(Encoding.UTF8.GetBytes(body)), + }; + request.Content.Headers.ContentType = new MediaTypeHeaderValue("application/didcomm-encrypted+json"); + using var response = await client.SendAsync(request); + return (response.StatusCode, await response.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task Throwing_onReceive_callback_still_returns_202_not_a_500_oracle() + { + // Issue #20 red-team: a consumer callback that throws after a successful unpack must NOT escape + // as a 500 — otherwise an attacker could probe "did this envelope decrypt for us?" by inducing + // the throw, re-exposing the unpack-success oracle. The receive is one-way: log + ack 202. + var (server, _) = await BuildServerAsync( + onReceive: (_, _) => throw new InvalidOperationException("downstream handler blew up")); + + var valid = await PackAnoncryptForBobAsync(); + var result = await PostEnvelopeAsync(server, valid); + + result.Status.Should().Be(HttpStatusCode.Accepted); // 202, not 500 + } + + [Fact] + public async Task Downstream_resolution_timeout_returns_400_not_a_500_oracle() + { + // PR #37 review: a downstream DID-resolution timeout surfaces as TaskCanceledException : + // OperationCanceledException with the request token NOT cancelled. It must collapse to the + // uniform 400, not be mistaken for a client abort and rethrown as a distinguishable 500 — which + // would re-open the 400-vs-500 oracle via an attacker-controlled did:webvh host that hangs until + // the resolver's HttpClient.Timeout fires. + var (server, _) = await BuildServerAsync(resolverOverride: new TimeoutResolver()); + var valid = await PackAnoncryptForBobAsync(); // decrypts (Bob's secret), then resolution throws + + var result = await PostEnvelopeAsync(server, valid); + + result.Status.Should().Be(HttpStatusCode.BadRequest); // 400, not 500; RequestAborted is not cancelled + result.Body.Should().BeEmpty(); + } + + /// + /// Simulates a downstream resolver whose HttpClient.Timeout fires: throws + /// (a ) with the + /// caller's token NOT cancelled — exactly what net-did's webvh client surfaces on a hung host. + /// + private sealed class TimeoutResolver : IDidResolver + { + public bool CanResolve(string did) => true; + + public Task ResolveAsync(string did, DidResolutionOptions? options = null, CancellationToken ct = default) + => throw new TaskCanceledException("simulated downstream HttpClient.Timeout (caller token not cancelled)"); + } + + private static async Task<(TestServer Server, List Received)> BuildServerAsync( + Action? configureOptions = null, + Func? onReceive = null, + IDidResolver? resolverOverride = null) + { + var received = new List(); + var actors = SpecActorRegistry.LoadDefault(); + IDidResolver resolver = resolverOverride ?? LoadResolver(); + var keyService = new NetDidKeyService(resolver); + var serviceResolver = new NetDidServiceEndpointResolver(resolver, keyService, new DidCommOptions()); var builder = WebApplication.CreateBuilder(); builder.WebHost.UseTestServer(); @@ -141,11 +273,11 @@ public async Task Returns_400_when_body_is_not_a_valid_didcomm_envelope() var app = builder.Build(); app.UseRouting(); - app.MapDidCommEndpoint("/didcomm", async (result, ct) => + app.MapDidCommEndpoint("/didcomm", onReceive ?? (async (result, ct) => { received.Add(result); await Task.CompletedTask; - }); + })); await app.StartAsync(); return (app.GetTestServer(), received);