From 5d2386cc9d28bebb3787793bb877bf319856ebf8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 28 May 2026 04:14:07 -0400 Subject: [PATCH] fix(transport): close bundle security + plaintext-retention gaps (4 findings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T-003: move the unlock lockout server-side. The 3-strike counter used to live in the Razor page only — a second tab / CLI caller could re-upload the same bytes and grind PBKDF2 indefinitely. The counter now lives in IBundleSessionStore, keyed by ContentHash, so retries against identical bundle bytes are throttled regardless of client. BundleLockedException surfaces the new typed error path. T-005: bind the manifest's non-derivative fields into AES-GCM AAD. A SHA-256 of the manifest (with ContentHash + Encryption normalised to sentinels) is now passed to AesGcm.Encrypt / .Decrypt, so a tampered SourceEnvironment / ExportedBy / CreatedAtUtc on a stolen bundle yields an authentication-tag mismatch instead of slipping past the Step-4 typo-resistant confirmation gate. T-006: cap zip entry count, decompressed length, and compression ratio in LoadAsync's envelope validator BEFORE any payload is decompressed, using ZipArchiveEntry.Length / .CompressedLength. New TransportOptions fields default to 4 entries / 200 MB / 50x ratio. T-007: clear decrypted plaintext on the ApplyAsync failure path and zero the buffer on success before removing the session, so a 100 MB DecryptedContent doesn't sit in memory for the 30-min TTL after a failed apply. A BundleSessionEvictionService BackgroundService now also drives EvictExpired periodically so abandoned sessions clear without needing a fresh Get() call to trigger lazy eviction. Also resolves NO-010 — the misleading "writer never throws" XML doc was the same code+comment my prior NO-004 await-the-writer fix already rewrote. --- code-reviews/NotificationOutbox/findings.md | 2 +- code-reviews/README.md | 19 +- code-reviews/Transport/findings.md | 8 +- .../Pages/Design/TransportImport.razor.cs | 84 ++++--- .../Transport/IBundleSessionStore.cs | 25 ++ .../Types/Transport/BundleSession.cs | 14 +- .../Encryption/BundleManifestAad.cs | 56 +++++ .../Encryption/BundleSecretEncryptor.cs | 27 ++- .../Import/BundleImporter.cs | 142 +++++++++++- .../Import/BundleLockedException.cs | 31 +++ .../Import/BundleSessionEvictionService.cs | 55 +++++ .../Import/BundleSessionStore.cs | 91 +++++++- .../ScadaLink.Transport.csproj | 2 + .../Serialization/BundleSerializer.cs | 17 +- .../ServiceCollectionExtensions.cs | 3 + src/ScadaLink.Transport/TransportOptions.cs | 22 ++ .../Import/BundleImporterApplyTests.cs | 34 +++ .../Encryption/BundleSecretEncryptorTests.cs | 31 +++ .../Import/BundleImporterLoadTests.cs | 219 ++++++++++++++++++ .../Import/BundleSessionStoreTests.cs | 63 ++++- 20 files changed, 879 insertions(+), 66 deletions(-) create mode 100644 src/ScadaLink.Transport/Encryption/BundleManifestAad.cs create mode 100644 src/ScadaLink.Transport/Import/BundleLockedException.cs create mode 100644 src/ScadaLink.Transport/Import/BundleSessionEvictionService.cs diff --git a/code-reviews/NotificationOutbox/findings.md b/code-reviews/NotificationOutbox/findings.md index 75e2951a..f2edff0a 100644 --- a/code-reviews/NotificationOutbox/findings.md +++ b/code-reviews/NotificationOutbox/findings.md @@ -439,7 +439,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Documentation & comments | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationOutbox/NotificationOutboxActor.cs:469-477` | **Description** diff --git a/code-reviews/README.md b/code-reviews/README.md index b8a3989c..8b56cf28 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,10 +40,10 @@ module file and counted in **Total**. | Severity | Open findings | |----------|---------------| | Critical | 0 | -| High | 13 | -| Medium | 56 | +| High | 12 | +| Medium | 52 | | Low | 90 | -| **Total** | **159** | +| **Total** | **154** | ## Module Status @@ -63,7 +63,7 @@ module file and counted in **Total**. | [Host](Host/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/5 | 7 | 22 | | [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/3/4 | 8 | 25 | | [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/2 | 4 | 23 | -| [NotificationOutbox](NotificationOutbox/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/3 | 6 | 10 | +| [NotificationOutbox](NotificationOutbox/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/3 | 5 | 10 | | [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/2/3 | 6 | 25 | | [Security](Security/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 21 | | [SiteCallAudit](SiteCallAudit/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/4 | 6 | 6 | @@ -71,7 +71,7 @@ module file and counted in **Total**. | [SiteRuntime](SiteRuntime/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/4/3 | 7 | 26 | | [StoreAndForward](StoreAndForward/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/3/3 | 7 | 24 | | [TemplateEngine](TemplateEngine/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/4/1 | 6 | 22 | -| [Transport](Transport/findings.md) | 2026-05-28 | `1eb6e97` | 0/3/5/4 | 12 | 12 | +| [Transport](Transport/findings.md) | 2026-05-28 | `1eb6e97` | 0/2/2/4 | 8 | 12 | ## Pending Findings @@ -84,7 +84,7 @@ description, location, recommendation — lives in the module's `findings.md`. _None open._ -### High (13) +### High (12) | ID | Module | Title | |----|--------|-------| @@ -100,9 +100,8 @@ _None open._ | TemplateEngine-017 | [TemplateEngine](TemplateEngine/findings.md) | Revision hash and diff both ignore `Description` and `Connections`, defeating staleness detection for real deployment changes | | Transport-001 | [Transport](Transport/findings.md) | Template Overwrite never syncs attributes / alarms / scripts | | Transport-002 | [Transport](Transport/findings.md) | ExternalSystem Overwrite never syncs methods | -| Transport-003 | [Transport](Transport/findings.md) | Unlock lockout is enforced only client-side; server session is never marked Locked | -### Medium (56) +### Medium (52) | ID | Module | Title | |----|--------|-------| @@ -139,7 +138,6 @@ _None open._ | ManagementService-021 | [ManagementService](ManagementService/findings.md) | Transport bundle handlers have zero test coverage | | NotificationOutbox-005 | [NotificationOutbox](NotificationOutbox/findings.md) | Ingest persistence inherits the CD-015 check-then-act race; under contention the second writer throws and the site retries | | NotificationOutbox-007 | [NotificationOutbox](NotificationOutbox/findings.md) | `NotificationOutboxOptions.DispatchBatchSize`, `DeliveredKpiWindow`, and `PurgeInterval` are not in the design document | -| NotificationOutbox-010 | [NotificationOutbox](NotificationOutbox/findings.md) | Comment claims `PipeTo` is not used "because the writer never throws"; the surrounding try/catch is dead-letter for the documented failure mode | | NotificationService-020 | [NotificationService](NotificationService/findings.md) | NS-001 fix superseded; `AkkaHostedService` would register two competing `Notification` S&F handlers if both code paths ran | | NotificationService-024 | [NotificationService](NotificationService/findings.md) | No test affirms the central-only invariant; the orphaned-path tests give a false coverage signal | | SiteCallAudit-001 | [SiteCallAudit](SiteCallAudit/findings.md) | SupervisorStrategy override is dead code; XML claims Resume that is not enforced | @@ -158,9 +156,6 @@ _None open._ | TemplateEngine-020 | [TemplateEngine](TemplateEngine/findings.md) | `Create*` audit entries are written with `EntityId = "0"` before `SaveChangesAsync` populates the real key | | TemplateEngine-021 | [TemplateEngine](TemplateEngine/findings.md) | `MoveTemplateAsync` skips folder cycle and sibling-name-collision validation | | Transport-004 | [Transport](Transport/findings.md) | `MaxUnlockAttemptsPerIpPerHour` option is declared but never enforced | -| Transport-005 | [Transport](Transport/findings.md) | Manifest fields outside `ContentHash` are not bound to the encrypted payload | -| Transport-006 | [Transport](Transport/findings.md) | Bundle ZIP read has no per-entry size cap or entry-count cap (zip-bomb / decompression-bomb) | -| Transport-007 | [Transport](Transport/findings.md) | Failed import sessions retain decrypted plaintext for the full 30-minute TTL | | Transport-010 | [Transport](Transport/findings.md) | Critical Overwrite + cross-cutting paths uncovered by tests | ### Low (90) diff --git a/code-reviews/Transport/findings.md b/code-reviews/Transport/findings.md index 8d4bf376..22751c65 100644 --- a/code-reviews/Transport/findings.md +++ b/code-reviews/Transport/findings.md @@ -121,7 +121,7 @@ _Unresolved._ |--|--| | Severity | High | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Transport/Import/BundleImporter.cs:184-203`, `src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor.cs:267-309`, `src/ScadaLink.Commons/Types/Transport/BundleSession.cs:14-16` | **Description** @@ -190,7 +190,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Transport/Encryption/BundleSecretEncryptor.cs:31-49`, `src/ScadaLink.Transport/Serialization/ManifestValidator.cs:29-53` | **Description** @@ -226,7 +226,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Transport/Serialization/BundleSerializer.cs:121-156`, `src/ScadaLink.Transport/Import/BundleImporter.cs:132-143` | **Description** @@ -259,7 +259,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Transport/Import/BundleImporter.cs:614-696`, `src/ScadaLink.Transport/Import/BundleSessionStore.cs:67-93` | **Description** diff --git a/src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor.cs b/src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor.cs index f18394c6..81492135 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor.cs +++ b/src/ScadaLink.CentralUI/Components/Pages/Design/TransportImport.razor.cs @@ -243,9 +243,15 @@ public partial class TransportImport : ComponentBase // ============================================================ /// - /// Submits the entered passphrase. On - /// increments the per-session counter; once the configured threshold is - /// reached the wizard resets to Step 1 with an explanatory error. + /// Submits the entered passphrase. + /// + /// T-003: lockout enforcement is now server-side and keyed by the bundle's + /// content hash. means "wrong passphrase, + /// try again"; a means the importer has + /// observed enough failures against this bundle to lock it (the count is + /// shared across tabs / CLI / circuits). The Razor counter is kept ONLY for + /// display ("3 of N attempts used") — it is no longer the source of truth. + /// /// private async Task SubmitPassphraseAsync() { @@ -264,37 +270,28 @@ public partial class TransportImport : ComponentBase await LoadPreviewAndAdvanceAsync(); } } + catch (BundleLockedException ex) + { + // T-003: server-side lockout reached. Emit a final audit row so the + // lockout is visible in the audit log, reset the wizard, and surface + // the typed message verbatim. + _passphrase = string.Empty; + _failedUnlockAttempts = ex.FailedAttempts; + await EmitUnlockFailedAuditRowAsync(ex.BundleContentHash, ex.FailedAttempts, ex.Message); + _errorMessage = ex.Message; + ResetSessionState(); + _step = ImportWizardStep.Upload; + } catch (CryptographicException ex) { _failedUnlockAttempts++; _passphrase = string.Empty; - // Emit audit row for every wrong-passphrase attempt (BundleImportUnlockFailed). - // Best-effort — audit failure must never abort the user-facing action. - try - { - var user = await Auth.GetCurrentUsernameAsync(); - var entityId = _session?.Manifest.ContentHash ?? ""; - var entityName = _session?.Manifest.SourceEnvironment ?? ""; - await AuditService.LogAsync( - user: user, - action: "BundleImportUnlockFailed", - entityType: "Bundle", - entityId: entityId, - entityName: entityName, - afterState: new - { - AttemptNumber = _failedUnlockAttempts, - Reason = ex.Message, - }, - cancellationToken: CancellationToken.None); - await DbContext.SaveChangesAsync(); - } - catch - { - // Audit failure is non-fatal — swallow and continue. - } + var entityId = _session?.Manifest.ContentHash ?? ""; + await EmitUnlockFailedAuditRowAsync(entityId, _failedUnlockAttempts, ex.Message); + // The server tracks the authoritative counter; the local count is + // kept in sync for the Razor display only. if (_failedUnlockAttempts >= Options.Value.MaxUnlockAttemptsPerSession) { _errorMessage = @@ -318,6 +315,37 @@ public partial class TransportImport : ComponentBase } } + /// + /// T-003: best-effort audit row for a wrong-passphrase attempt. Audit failure + /// must never abort the user-facing action — same defensive pattern as the + /// original page used. + /// + private async Task EmitUnlockFailedAuditRowAsync(string entityId, int attemptNumber, string reason) + { + try + { + var user = await Auth.GetCurrentUsernameAsync(); + var entityName = _session?.Manifest.SourceEnvironment ?? ""; + await AuditService.LogAsync( + user: user, + action: "BundleImportUnlockFailed", + entityType: "Bundle", + entityId: entityId, + entityName: entityName, + afterState: new + { + AttemptNumber = attemptNumber, + Reason = reason, + }, + cancellationToken: CancellationToken.None); + await DbContext.SaveChangesAsync(); + } + catch + { + // Audit failure is non-fatal — swallow and continue. + } + } + private void BackToUpload() { _step = ImportWizardStep.Upload; diff --git a/src/ScadaLink.Commons/Interfaces/Transport/IBundleSessionStore.cs b/src/ScadaLink.Commons/Interfaces/Transport/IBundleSessionStore.cs index cea04d63..666bcbc4 100644 --- a/src/ScadaLink.Commons/Interfaces/Transport/IBundleSessionStore.cs +++ b/src/ScadaLink.Commons/Interfaces/Transport/IBundleSessionStore.cs @@ -15,4 +15,29 @@ public interface IBundleSessionStore void Remove(Guid sessionId); /// Removes all sessions whose expiry has passed. void EvictExpired(); + + /// + /// T-003: returns the current unlock-failure count for a bundle keyed by its + /// content hash. The counter is server-owned so a second tab / CLI caller + /// cannot side-step the lockout by re-uploading the same bytes. + /// + /// SHA-256 hex from BundleManifest.ContentHash. + /// Number of recorded failures for this bundle (0 if none, or if any record has expired). + int GetUnlockFailureCount(string bundleContentHash); + + /// + /// T-003: atomically increments the unlock-failure counter for a bundle and + /// returns the new count. Tracking is scoped by content hash so retries + /// against identical bundle bytes are throttled regardless of client. + /// + /// SHA-256 hex from BundleManifest.ContentHash. + int IncrementUnlockFailureCount(string bundleContentHash); + + /// + /// T-003: clears the unlock-failure counter for a bundle (called on a + /// successful unlock so a legitimate operator who eventually types the + /// right passphrase is not penalised for earlier typos). + /// + /// SHA-256 hex from BundleManifest.ContentHash. + void ClearUnlockFailures(string bundleContentHash); } diff --git a/src/ScadaLink.Commons/Types/Transport/BundleSession.cs b/src/ScadaLink.Commons/Types/Transport/BundleSession.cs index defed650..82942684 100644 --- a/src/ScadaLink.Commons/Types/Transport/BundleSession.cs +++ b/src/ScadaLink.Commons/Types/Transport/BundleSession.cs @@ -10,8 +10,18 @@ public sealed class BundleSession public byte[] DecryptedContent { get; init; } = Array.Empty(); /// UTC timestamp after which this session is considered expired and must be re-uploaded. public DateTimeOffset ExpiresAt { get; init; } - /// Number of failed passphrase unlock attempts for this session. + /// + /// T-003: legacy per-session unlock-attempt counter. The unlock lockout is now + /// owned by IBundleSessionStore and keyed by BundleManifest.ContentHash + /// so retries from a second tab / CLI caller share the counter. A successful + /// LoadAsync never increments this — it stays 0 on every opened session, + /// and is unreachable on a session returned by the store. + /// Retained as a compatibility shim for callers/tests that still set it directly. + /// public int FailedUnlockAttempts { get; set; } - /// True when three or more unlock attempts have failed, locking further attempts. + /// + /// T-003 legacy: always false on a session returned by LoadAsync + /// because lockout enforcement moved server-side; see . + /// public bool Locked => FailedUnlockAttempts >= 3; } diff --git a/src/ScadaLink.Transport/Encryption/BundleManifestAad.cs b/src/ScadaLink.Transport/Encryption/BundleManifestAad.cs new file mode 100644 index 00000000..2b84a215 --- /dev/null +++ b/src/ScadaLink.Transport/Encryption/BundleManifestAad.cs @@ -0,0 +1,56 @@ +using System.Security.Cryptography; +using System.Text.Json; +using System.Text.Json.Serialization; +using ScadaLink.Commons.Types.Transport; + +namespace ScadaLink.Transport.Encryption; + +/// +/// T-005: computes the AES-GCM Associated Authenticated Data (AAD) for a bundle's +/// encrypted payload. AAD is the SHA-256 of the manifest after normalising the two +/// derivative fields (ContentHash, Encryption) to known sentinel +/// values — those depend on the ciphertext and the IV, so they cannot themselves +/// be authenticated, but every OTHER manifest field (SourceEnvironment, +/// ExportedBy, ScadaLinkVersion, Summary, Contents, +/// CreatedAtUtc, …) participates in the GCM tag. +/// +/// Threading this byte array through AesGcm.Encrypt / AesGcm.Decrypt +/// makes the Step-4 "type the source environment name to confirm" gate +/// tamper-evident: a flipped SourceEnvironment on a stolen bundle yields +/// an AuthenticationTagMismatchException on decrypt instead of producing +/// a valid plaintext with a forged origin label. +/// +/// +public static class BundleManifestAad +{ + /// + /// JSON options matching BundleSerializer.JsonOptions so the AAD bytes + /// are stable across the encrypt + decrypt side. + /// + private static readonly JsonSerializerOptions JsonOptions = new() + { + WriteIndented = false, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + Converters = { new JsonStringEnumConverter() }, + }; + + /// + /// Computes the AAD bytes for the supplied manifest. The two derivative + /// fields are normalised to fixed sentinels so the AAD is independent of the + /// ciphertext / IV that will eventually populate them in the on-disk + /// manifest. + /// + /// The manifest whose non-derivative fields should be authenticated. + /// SHA-256 digest of the canonicalised manifest bytes. + public static byte[] Compute(BundleManifest manifest) + { + ArgumentNullException.ThrowIfNull(manifest); + var canonical = manifest with + { + ContentHash = string.Empty, + Encryption = null, + }; + var canonicalBytes = JsonSerializer.SerializeToUtf8Bytes(canonical, JsonOptions); + return SHA256.HashData(canonicalBytes); + } +} diff --git a/src/ScadaLink.Transport/Encryption/BundleSecretEncryptor.cs b/src/ScadaLink.Transport/Encryption/BundleSecretEncryptor.cs index 73b36680..5a728422 100644 --- a/src/ScadaLink.Transport/Encryption/BundleSecretEncryptor.cs +++ b/src/ScadaLink.Transport/Encryption/BundleSecretEncryptor.cs @@ -22,11 +22,20 @@ public sealed class BundleSecretEncryptor /// The data to encrypt. /// The passphrase used to derive the encryption key. /// PBKDF2 iteration count for key derivation. + /// + /// T-005: optional AES-GCM Associated Authenticated Data — typically + /// (manifest). Binds the manifest's + /// non-derivative fields (source environment, exporter, summary, …) to the + /// ciphertext so any tampering of those fields yields an authentication-tag + /// mismatch on decrypt. Pass for the + /// legacy no-AAD format; the decrypter must mirror the choice. + /// /// The ciphertext (with appended GCM tag) and the encryption metadata needed to decrypt. public (byte[] Ciphertext, EncryptionMetadata Metadata) Encrypt( ReadOnlySpan plaintext, string passphrase, - int iterations) + int iterations, + ReadOnlySpan associatedData = default) { var salt = RandomNumberGenerator.GetBytes(SaltBytes); var nonce = RandomNumberGenerator.GetBytes(NonceBytes); @@ -35,7 +44,7 @@ public sealed class BundleSecretEncryptor var ciphertext = new byte[plaintext.Length]; var tag = new byte[TagBytes]; using var aes = new AesGcm(key, TagBytes); - aes.Encrypt(nonce, plaintext, ciphertext, tag); + aes.Encrypt(nonce, plaintext, ciphertext, tag, associatedData); // Format: ciphertext || tag. var output = new byte[ciphertext.Length + TagBytes]; @@ -54,8 +63,18 @@ public sealed class BundleSecretEncryptor /// The ciphertext with appended GCM tag. /// Encryption metadata carrying the algorithm, KDF, salt, nonce, and iterations. /// The passphrase used to derive the decryption key. + /// + /// T-005: AAD that was passed to . Must match exactly — + /// any tampering of the manifest used to derive AAD yields an authentication- + /// tag mismatch (surfaces as , specifically + /// AuthenticationTagMismatchException on .NET 10). + /// /// The decrypted plaintext bytes. - public byte[] Decrypt(ReadOnlySpan payload, EncryptionMetadata metadata, string passphrase) + public byte[] Decrypt( + ReadOnlySpan payload, + EncryptionMetadata metadata, + string passphrase, + ReadOnlySpan associatedData = default) { ArgumentNullException.ThrowIfNull(metadata); @@ -79,7 +98,7 @@ public sealed class BundleSecretEncryptor var plaintext = new byte[ctLen]; using var aes = new AesGcm(key, TagBytes); - aes.Decrypt(nonce, ciphertext, tag, plaintext); + aes.Decrypt(nonce, ciphertext, tag, plaintext, associatedData); return plaintext; } diff --git a/src/ScadaLink.Transport/Import/BundleImporter.cs b/src/ScadaLink.Transport/Import/BundleImporter.cs index 28366730..4e9d3cb4 100644 --- a/src/ScadaLink.Transport/Import/BundleImporter.cs +++ b/src/ScadaLink.Transport/Import/BundleImporter.cs @@ -1,3 +1,4 @@ +using System.IO.Compression; using System.Security.Cryptography; using System.Text.Json; using System.Text.Json.Serialization; @@ -142,6 +143,18 @@ public sealed class BundleImporter : IBundleImporter $"Bundle exceeds maximum allowed size of {_options.Value.MaxBundleSizeMb} MB."); } + // T-006: zip-bomb / decompression-bomb defences. We enforce three caps + // BEFORE any entry is decompressed by ReadManifest / ReadContentBytes: + // 1) total entry count + // 2) per-entry decompressed length + // 3) per-entry compression ratio (Length / CompressedLength) + // Each cap is configurable via TransportOptions so an operator can tune + // for an environment with legitimately large or unusually compressible + // bundles. Using ZipArchiveEntry.Length / .CompressedLength avoids + // reading the entry payload at all. + ms.Position = 0; + ValidateArchiveEnvelope(ms); + BundleManifest manifest; try { @@ -181,12 +194,16 @@ public sealed class BundleImporter : IBundleImporter throw new InvalidDataException($"Unrecognised manifest validation result: {validation}."); } - // Decrypt when the manifest carries EncryptionMetadata. AES-GCM tag - // mismatch surfaces as a CryptographicException (or its - // AuthenticationTagMismatchException subclass on .NET 10+) — bubble it - // unchanged so the caller can detect wrong-passphrase via type check - // and increment the lockout counter on the (about-to-be-rejected) - // session reference. The session is not opened on the failure path. + // Decrypt when the manifest carries EncryptionMetadata. + // + // T-003: lockout enforcement is server-side and keyed by ContentHash so a + // second tab / CLI caller re-uploading the same bundle bytes cannot + // side-step the limit by skipping the Razor page. The counter is + // consulted BEFORE attempting decrypt (rejects further attempts on an + // already-locked bundle) and incremented on a CryptographicException + // from _encryptor.Decrypt; a successful decrypt clears the counter so a + // legitimate operator who eventually types the right passphrase is not + // penalised for earlier typos. byte[] decryptedContent; if (manifest.Encryption is not null) { @@ -195,7 +212,39 @@ public sealed class BundleImporter : IBundleImporter throw new ArgumentException( "Passphrase required for encrypted bundle.", nameof(passphrase)); } - decryptedContent = _encryptor.Decrypt(contentBytes, manifest.Encryption, passphrase); + + var maxAttempts = _options.Value.MaxUnlockAttemptsPerSession; + var priorFailures = _sessionStore.GetUnlockFailureCount(manifest.ContentHash); + if (priorFailures >= maxAttempts) + { + throw new BundleLockedException(manifest.ContentHash, priorFailures); + } + + // T-005: bind the manifest's non-derivative fields into AES-GCM AAD so + // a tampered SourceEnvironment / ExportedBy / etc. yields an + // authentication-tag mismatch (surfaced as CryptographicException) on + // decrypt — preventing a forged origin label from slipping past the + // Step-4 typo-resistant confirmation gate. + var aad = Encryption.BundleManifestAad.Compute(manifest); + try + { + decryptedContent = _encryptor.Decrypt(contentBytes, manifest.Encryption, passphrase, aad); + } + catch (CryptographicException) + { + var newCount = _sessionStore.IncrementUnlockFailureCount(manifest.ContentHash); + if (newCount >= maxAttempts) + { + // Surface the lockout as the typed exception so the caller can + // distinguish "wrong passphrase, try again" from "no more attempts". + throw new BundleLockedException(manifest.ContentHash, newCount); + } + // Otherwise rebubble the CryptographicException so the UI's + // wrong-passphrase audit + retry path continues to work unchanged. + throw; + } + + _sessionStore.ClearUnlockFailures(manifest.ContentHash); } else { @@ -213,6 +262,57 @@ public sealed class BundleImporter : IBundleImporter return _sessionStore.Open(session); } + /// + /// T-006: validates the zip envelope against the configured caps BEFORE any + /// entry payload is decompressed. Reads only the central-directory headers + /// ( / ) + /// so a hostile bundle can't OOM the central node through this method itself. + /// + /// Buffered bundle bytes. Position is preserved. + private void ValidateArchiveEnvelope(MemoryStream bundleBytes) + { + var opts = _options.Value; + var maxEntries = opts.MaxBundleEntryCount; + var maxEntryDecompressed = opts.MaxBundleEntryDecompressedMb * 1024L * 1024L; + var maxRatio = opts.MaxBundleEntryCompressionRatio; + + var savedPosition = bundleBytes.Position; + try + { + bundleBytes.Position = 0; + using var archive = new ZipArchive(bundleBytes, ZipArchiveMode.Read, leaveOpen: true); + + if (archive.Entries.Count > maxEntries) + { + throw new InvalidDataException( + $"Bundle contains {archive.Entries.Count} zip entries; the configured maximum is {maxEntries}."); + } + + foreach (var entry in archive.Entries) + { + if (entry.Length > maxEntryDecompressed) + { + throw new InvalidDataException( + $"Bundle entry '{entry.FullName}' declares a decompressed size of {entry.Length} bytes; " + + $"the configured maximum is {maxEntryDecompressed} bytes " + + $"({opts.MaxBundleEntryDecompressedMb} MB)."); + } + + // CompressedLength of 0 means store-only or empty — skip ratio check. + if (entry.CompressedLength > 0 && entry.Length / entry.CompressedLength > maxRatio) + { + throw new InvalidDataException( + $"Bundle entry '{entry.FullName}' has compression ratio " + + $"{entry.Length / entry.CompressedLength}x; the configured maximum is {maxRatio}x."); + } + } + } + finally + { + bundleBytes.Position = savedPosition; + } + } + /// public async Task PreviewAsync(Guid sessionId, CancellationToken ct = default) { @@ -611,6 +711,12 @@ public sealed class BundleImporter : IBundleImporter await _dbContext.SaveChangesAsync(ct).ConfigureAwait(false); await tx.CommitAsync(ct).ConfigureAwait(false); + // T-007: zero out the decrypted plaintext BEFORE remove so any + // caller-held reference (e.g. the Razor page that built the + // ImportPreview) sees the cleared buffer too. Remove drops the + // dictionary entry; together they release the secrets immediately + // instead of leaving them in process memory for the full TTL. + ZeroDecryptedContent(session); _sessionStore.Remove(sessionId); return new ImportResult( @@ -692,6 +798,14 @@ public sealed class BundleImporter : IBundleImporter // any failure here so the original exception below propagates // unchanged rather than being masked by an audit-layer fault. } + + // T-007: a failed apply used to leave the BundleSession (with its + // decrypted secrets) in the in-memory store for the full 30-minute + // TTL — 10 failed 100 MB imports = 1 GB of plaintext still rooted. + // Drop the session here too so the secrets are released as soon as + // the failure surfaces, not when the next Get() happens to evict. + ZeroDecryptedContent(session); + _sessionStore.Remove(sessionId); throw; } finally @@ -704,6 +818,20 @@ public sealed class BundleImporter : IBundleImporter } } + /// + /// T-007: zeros the session's + /// buffer in place so any caller still holding a reference observes the + /// cleared bytes. Best-effort — a null/empty buffer is a no-op. + /// + private static void ZeroDecryptedContent(BundleSession session) + { + var buf = session.DecryptedContent; + if (buf is { Length: > 0 }) + { + Array.Clear(buf, 0, buf.Length); + } + } + /// Mutable per-apply counter struct, accumulated through every helper. private sealed class ImportSummary { diff --git a/src/ScadaLink.Transport/Import/BundleLockedException.cs b/src/ScadaLink.Transport/Import/BundleLockedException.cs new file mode 100644 index 00000000..4e40137c --- /dev/null +++ b/src/ScadaLink.Transport/Import/BundleLockedException.cs @@ -0,0 +1,31 @@ +namespace ScadaLink.Transport.Import; + +/// +/// T-003: thrown by when an encrypted bundle has +/// exceeded the configured failed-unlock attempt limit +/// (). The lockout is tracked +/// server-side keyed by BundleManifest.ContentHash, so a second tab / CLI caller +/// re-uploading the same bytes hits the same counter and cannot side-step the limit. +/// +public sealed class BundleLockedException : Exception +{ + /// Number of recorded unlock failures for this bundle. + public int FailedAttempts { get; } + + /// SHA-256 (hex) of the bundle's content bytes, the lockout's tracking key. + public string BundleContentHash { get; } + + /// + /// Initializes a new . + /// + /// SHA-256 hex from BundleManifest.ContentHash. + /// Number of failures recorded against this bundle. + public BundleLockedException(string bundleContentHash, int failedAttempts) + : base( + $"Bundle is locked after {failedAttempts} failed unlock attempts. " + + "Wait for the lockout window to expire or re-export the bundle to obtain a new content hash.") + { + BundleContentHash = bundleContentHash ?? throw new ArgumentNullException(nameof(bundleContentHash)); + FailedAttempts = failedAttempts; + } +} diff --git a/src/ScadaLink.Transport/Import/BundleSessionEvictionService.cs b/src/ScadaLink.Transport/Import/BundleSessionEvictionService.cs new file mode 100644 index 00000000..5715c7a3 --- /dev/null +++ b/src/ScadaLink.Transport/Import/BundleSessionEvictionService.cs @@ -0,0 +1,55 @@ +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; +using ScadaLink.Commons.Interfaces.Transport; + +namespace ScadaLink.Transport.Import; + +/// +/// T-007: periodic background sweep that drives +/// so abandoned import sessions clear from memory on their own, without needing a +/// new to trigger lazy eviction. Each session +/// owns the decrypted bundle content (potentially up to ~100 MB of secrets — DB +/// connection strings, SMTP credentials, external-system auth configs), and the +/// design contract is "bundles are not retained server-side after ApplyAsync +/// commits". This service keeps abandoned / failed sessions from pinning that +/// plaintext for the full 30-minute TTL when no other traffic flows. +/// +internal sealed class BundleSessionEvictionService : BackgroundService +{ + private static readonly TimeSpan SweepInterval = TimeSpan.FromMinutes(1); + + private readonly IBundleSessionStore _sessionStore; + private readonly ILogger _logger; + + public BundleSessionEvictionService( + IBundleSessionStore sessionStore, + ILogger logger) + { + _sessionStore = sessionStore ?? throw new ArgumentNullException(nameof(sessionStore)); + _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + } + + protected override async Task ExecuteAsync(CancellationToken stoppingToken) + { + while (!stoppingToken.IsCancellationRequested) + { + try + { + await Task.Delay(SweepInterval, stoppingToken).ConfigureAwait(false); + } + catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) + { + return; + } + + try + { + _sessionStore.EvictExpired(); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Bundle session sweep failed; will retry on next interval."); + } + } + } +} diff --git a/src/ScadaLink.Transport/Import/BundleSessionStore.cs b/src/ScadaLink.Transport/Import/BundleSessionStore.cs index ab644b70..a14fc585 100644 --- a/src/ScadaLink.Transport/Import/BundleSessionStore.cs +++ b/src/ScadaLink.Transport/Import/BundleSessionStore.cs @@ -36,19 +36,35 @@ namespace ScadaLink.Transport.Import; public sealed class BundleSessionStore : IBundleSessionStore { private readonly ConcurrentDictionary _sessions = new(); - private readonly TimeProvider _timeProvider; - // Options are accepted to honor the documented constructor contract and to - // be ready for future per-store knobs (e.g. max in-flight sessions). The - // current store does not read any field — TTL is on the session itself. + /// + /// T-003: per-bundle unlock-failure counters, keyed by + /// (SHA-256 hex of the bundle's content bytes). Failures are tracked here — not on + /// — so retries against the same bundle bytes from a + /// second tab / CLI caller share the counter and cannot side-step the lockout. Entries + /// expire on the same TTL as a session. + /// + private readonly ConcurrentDictionary _unlockFailures = new(StringComparer.OrdinalIgnoreCase); + + private readonly TimeProvider _timeProvider; + private readonly IOptions _options; + + /// T-003: per-bundle unlock-failure entry with expiry. + private sealed class UnlockFailureRecord + { + public int Count; + public DateTimeOffset ExpiresAt; + } + /// /// Initializes a new . /// - /// Transport options (reserved for future per-store configuration). + /// Transport options. is also used as the TTL for the T-003 per-bundle unlock-failure tracker. /// Time provider used to evaluate session expiry. public BundleSessionStore(IOptions options, TimeProvider timeProvider) { ArgumentNullException.ThrowIfNull(options); + _options = options; _ = options.Value; _timeProvider = timeProvider ?? throw new ArgumentNullException(nameof(timeProvider)); } @@ -90,5 +106,70 @@ public sealed class BundleSessionStore : IBundleSessionStore _sessions.TryRemove(kv.Key, out _); } } + + // T-003: also expire stale per-bundle unlock-failure entries so a bundle + // that was previously locked clears once the lockout window passes. + foreach (var kv in _unlockFailures) + { + if (kv.Value.ExpiresAt <= now) + { + _unlockFailures.TryRemove(kv.Key, out _); + } + } + } + + /// + public int GetUnlockFailureCount(string bundleContentHash) + { + ArgumentException.ThrowIfNullOrEmpty(bundleContentHash); + if (!_unlockFailures.TryGetValue(bundleContentHash, out var record)) + { + return 0; + } + + // Lazy expiry — if the entry has aged past its window treat it as cleared. + if (record.ExpiresAt <= _timeProvider.GetUtcNow()) + { + _unlockFailures.TryRemove(bundleContentHash, out _); + return 0; + } + + return record.Count; + } + + /// + public int IncrementUnlockFailureCount(string bundleContentHash) + { + ArgumentException.ThrowIfNullOrEmpty(bundleContentHash); + var ttl = TimeSpan.FromMinutes(_options.Value.BundleSessionTtlMinutes); + var now = _timeProvider.GetUtcNow(); + var record = _unlockFailures.AddOrUpdate( + bundleContentHash, + _ => new UnlockFailureRecord { Count = 1, ExpiresAt = now + ttl }, + (_, existing) => + { + // Treat an expired record as a fresh start so a legitimate operator + // returning hours later does not face a stale lockout. + if (existing.ExpiresAt <= now) + { + existing.Count = 1; + } + else + { + existing.Count++; + } + + existing.ExpiresAt = now + ttl; + return existing; + }); + + return record.Count; + } + + /// + public void ClearUnlockFailures(string bundleContentHash) + { + ArgumentException.ThrowIfNullOrEmpty(bundleContentHash); + _unlockFailures.TryRemove(bundleContentHash, out _); } } diff --git a/src/ScadaLink.Transport/ScadaLink.Transport.csproj b/src/ScadaLink.Transport/ScadaLink.Transport.csproj index a1ac6db9..a2e1e03a 100644 --- a/src/ScadaLink.Transport/ScadaLink.Transport.csproj +++ b/src/ScadaLink.Transport/ScadaLink.Transport.csproj @@ -8,6 +8,8 @@ + + diff --git a/src/ScadaLink.Transport/Serialization/BundleSerializer.cs b/src/ScadaLink.Transport/Serialization/BundleSerializer.cs index d0ba97e3..cee0b78d 100644 --- a/src/ScadaLink.Transport/Serialization/BundleSerializer.cs +++ b/src/ScadaLink.Transport/Serialization/BundleSerializer.cs @@ -82,7 +82,17 @@ public sealed class BundleSerializer // count but rebuild ContentHash + Encryption fields against the bytes we // actually write. The non-encryption manifest fields (source env, exported // by, summary, contents, version) are preserved verbatim. - var (cipher, freshMeta) = encryptor.Encrypt(contentBytes, passphrase, manifest.Encryption.Iterations); + // + // T-005: bind the manifest's non-derivative fields into the AES-GCM AAD + // so a tampered SourceEnvironment / ExportedBy / etc. on a stolen bundle + // yields an authentication-tag mismatch on decrypt instead of a forged + // origin label slipping past the Step-4 confirmation gate. AAD is + // computed over a manifest normalised to empty ContentHash + null + // Encryption (those fields are derivative of the ciphertext / IV and + // cannot themselves be authenticated). + var aad = BundleManifestAad.Compute(manifest); + var (cipher, freshMeta) = encryptor.Encrypt( + contentBytes, passphrase, manifest.Encryption.Iterations, aad); payload = cipher; payloadEntryName = ContentEncEntryName; finalManifest = manifest with @@ -178,7 +188,10 @@ public sealed class BundleSerializer { throw new ArgumentException("Encrypted bundle requires both passphrase and encryptor."); } - plaintext = encryptor.Decrypt(contentBytes, manifest.Encryption, passphrase); + // T-005: mirror the encrypt side — AAD is derived from the manifest's + // non-derivative fields. Tampering yields an authentication-tag mismatch. + var aad = BundleManifestAad.Compute(manifest); + plaintext = encryptor.Decrypt(contentBytes, manifest.Encryption, passphrase, aad); } else { diff --git a/src/ScadaLink.Transport/ServiceCollectionExtensions.cs b/src/ScadaLink.Transport/ServiceCollectionExtensions.cs index 74a43ad8..a0a436ec 100644 --- a/src/ScadaLink.Transport/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.Transport/ServiceCollectionExtensions.cs @@ -35,6 +35,9 @@ public static class ServiceCollectionExtensions services.AddScoped(); services.AddScoped(); services.AddSingleton(); + // T-007: periodic eviction sweep so abandoned sessions clear without + // needing a fresh Get() to trigger lazy eviction. + services.AddHostedService(); // SemanticValidator is a stateless utility used by ApplyAsync; use // TryAdd so a host that already calls AddTemplateEngine() (which // registers the same type as Transient) wins. Either registration diff --git a/src/ScadaLink.Transport/TransportOptions.cs b/src/ScadaLink.Transport/TransportOptions.cs index fb7e9ed9..be11c896 100644 --- a/src/ScadaLink.Transport/TransportOptions.cs +++ b/src/ScadaLink.Transport/TransportOptions.cs @@ -6,6 +6,28 @@ public sealed class TransportOptions public int BundleSessionTtlMinutes { get; set; } = 30; /// Gets or sets the maximum allowed bundle size in megabytes. public int MaxBundleSizeMb { get; set; } = 100; + /// + /// T-006: maximum allowed decompressed size of any single zip entry in megabytes. + /// A 100 MB DEFLATE-compressed bundle can decompress to gigabytes; this cap + /// stops a malicious bundle from OOM-ing the central node before its entries + /// are decompressed. + /// + public int MaxBundleEntryDecompressedMb { get; set; } = 200; + /// + /// T-006: maximum permitted number of entries inside a bundle zip. A well-formed + /// bundle has exactly two (manifest.json plus content.json or + /// content.enc); a small upper bound limits the surface a zip-bomb can + /// exploit without rejecting future schema additions out of hand. + /// + public int MaxBundleEntryCount { get; set; } = 4; + /// + /// T-006: maximum permitted compression ratio (uncompressed length / compressed + /// length) per zip entry. Defence-in-depth against decompression bombs whose + /// declared is + /// trustworthy on read; legitimate JSON compresses around 5–10x, so 50x has + /// generous headroom for unusually compressible bundles. + /// + public int MaxBundleEntryCompressionRatio { get; set; } = 50; /// Gets or sets the maximum number of failed passphrase unlock attempts before a session is locked. public int MaxUnlockAttemptsPerSession { get; set; } = 3; /// Gets or sets the maximum number of unlock attempts allowed per IP address per hour. diff --git a/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs b/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs index e6c54ea6..2aab5a40 100644 --- a/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs +++ b/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs @@ -310,6 +310,40 @@ public sealed class BundleImporterApplyTests : IDisposable Assert.Equal(0, await ctx.Templates.CountAsync()); Assert.True(await ctx.AuditLogEntries.AnyAsync(a => a.Action == "BundleImportFailed")); } + + // T-007: a failed apply used to keep the BundleSession (and its decrypted + // secrets) in the in-memory store for the full 30-minute TTL. The session + // must now be removed immediately so the plaintext is released. + var sessionStore = _provider.GetRequiredService(); + Assert.Null(sessionStore.Get(sessionId)); + } + + [Fact] + public async Task ApplyAsync_removes_session_on_success_path_too() + { + // T-007: companion to the failed-apply test — the success path must also + // remove the session (it was already doing so before T-007, but the new + // test asserts the contract explicitly so a future refactor cannot + // accidentally leave plaintext in the store). + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + ctx.Templates.Add(new Template("PumpForT007") { Description = "fresh" }); + await ctx.SaveChangesAsync(); + } + var sessionId = await ExportAndLoadAsync(); + await WipeContentAsync(); + + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + await importer.ApplyAsync(sessionId, + new List { new("Template", "PumpForT007", ResolutionAction.Add, null) }, + user: "alice"); + } + + var sessionStore = _provider.GetRequiredService(); + Assert.Null(sessionStore.Get(sessionId)); } [Fact] diff --git a/tests/ScadaLink.Transport.Tests/Encryption/BundleSecretEncryptorTests.cs b/tests/ScadaLink.Transport.Tests/Encryption/BundleSecretEncryptorTests.cs index e01ff96d..579367df 100644 --- a/tests/ScadaLink.Transport.Tests/Encryption/BundleSecretEncryptorTests.cs +++ b/tests/ScadaLink.Transport.Tests/Encryption/BundleSecretEncryptorTests.cs @@ -57,6 +57,37 @@ public sealed class BundleSecretEncryptorTests Assert.NotEqual(meta1.SaltB64, meta2.SaltB64); } + [Fact] + public void Decrypt_with_mismatched_AAD_throws_CryptographicException() + { + // T-005: AES-GCM AAD binds extra context (e.g. manifest non-derivative fields) + // into the auth tag. Decrypting with different AAD yields a tag mismatch even + // when the passphrase and ciphertext are correct. + var sut = new BundleSecretEncryptor(); + var plaintext = Encoding.UTF8.GetBytes("secret payload"); + var aadOnEncrypt = Encoding.UTF8.GetBytes("manifest-canonical-hash-A"); + var aadOnDecrypt = Encoding.UTF8.GetBytes("manifest-canonical-hash-B"); + + var (ciphertext, metadata) = sut.Encrypt(plaintext, "pass", TestIterations, aadOnEncrypt); + + Assert.ThrowsAny( + () => sut.Decrypt(ciphertext, metadata, "pass", aadOnDecrypt)); + } + + [Fact] + public void Decrypt_with_matching_AAD_recovers_plaintext() + { + // T-005: round-trip with non-empty AAD must succeed when both sides match. + var sut = new BundleSecretEncryptor(); + var plaintext = Encoding.UTF8.GetBytes("secret payload"); + var aad = Encoding.UTF8.GetBytes("manifest-canonical-hash"); + + var (ciphertext, metadata) = sut.Encrypt(plaintext, "pass", TestIterations, aad); + var recovered = sut.Decrypt(ciphertext, metadata, "pass", aad); + + Assert.Equal(plaintext, recovered); + } + [Fact] public void Encrypt_emits_metadata_matching_decryption_inputs() { diff --git a/tests/ScadaLink.Transport.Tests/Import/BundleImporterLoadTests.cs b/tests/ScadaLink.Transport.Tests/Import/BundleImporterLoadTests.cs index ca34c252..e39456b8 100644 --- a/tests/ScadaLink.Transport.Tests/Import/BundleImporterLoadTests.cs +++ b/tests/ScadaLink.Transport.Tests/Import/BundleImporterLoadTests.cs @@ -207,6 +207,225 @@ public sealed class BundleImporterLoadTests () => rig.Importer.LoadAsync(stream, passphrase: "wrong")); } + [Fact] + public async Task LoadAsync_locks_bundle_after_three_wrong_passphrases_even_across_callers() + { + // T-003: the lockout is server-side and keyed by ContentHash, so replaying + // the SAME bundle bytes from a second caller (different stream, different + // session) must hit the same counter. After MaxUnlockAttemptsPerSession (3) + // failures the importer throws BundleLockedException, not another + // CryptographicException — and the lock survives a fresh LoadAsync from a + // pristine caller that has no idea about the prior attempts. + var rig = BuildRig(); + using var packed = PackEncryptedBundle( + rig.Serializer, rig.ManifestBuilder, rig.Encryptor, EmptyContent(), "correct"); + var bundleBytes = ((MemoryStream)packed).ToArray(); + + // First two wrong-passphrase attempts surface as CryptographicException. + for (var attempt = 1; attempt <= 2; attempt++) + { + await Assert.ThrowsAnyAsync( + () => rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: "wrong")); + } + + // Third wrong-passphrase attempt crosses the threshold and surfaces as + // BundleLockedException — even though this is a fresh stream / a caller + // that never saw the earlier failures. + var locked = await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: "wrong")); + Assert.Equal(3, locked.FailedAttempts); + + // Fourth attempt — even with the CORRECT passphrase — is still locked, + // because the lockout sticks until the TTL expires or the bundle is + // re-exported with a new content hash. + await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: "correct")); + } + + [Fact] + public async Task LoadAsync_rejects_bundle_with_too_many_entries() + { + // T-006: a malicious bundle could pad the archive with arbitrary entries to + // exhaust per-entry handles or to slip an unexpected payload past the + // serializer. The envelope check rejects any archive whose entry count + // exceeds MaxBundleEntryCount BEFORE any decompression. + var rig = BuildRig(opts => opts.MaxBundleEntryCount = 2); + using var packed = PackPlainBundle(rig.Serializer, rig.ManifestBuilder, EmptyContent()); + var bytes = ((MemoryStream)packed).ToArray(); + var paddedBytes = AppendExtraZipEntry(bytes, "extra.bin", new byte[8]); + + await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync(new MemoryStream(paddedBytes), passphrase: null)); + } + + [Fact] + public async Task LoadAsync_rejects_bundle_with_oversized_entry() + { + // T-006: caller-declared decompressed Length above the configured cap is + // a hostile bundle; reject without decompressing. + var rig = BuildRig(opts => opts.MaxBundleEntryDecompressedMb = 1); + using var packed = PackPlainBundle(rig.Serializer, rig.ManifestBuilder, EmptyContent()); + var bytes = ((MemoryStream)packed).ToArray(); + // Replace content.json with a 2 MB entry of compressible zeros — uncompressed Length > cap. + var bigPayload = new byte[2 * 1024 * 1024]; + var oversizedBytes = ReplaceZipEntry(bytes, "content.json", bigPayload); + + await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync(new MemoryStream(oversizedBytes), passphrase: null)); + } + + [Fact] + public async Task LoadAsync_rejects_bundle_whose_entry_exceeds_compression_ratio() + { + // T-006: defence-in-depth — even if Length is within the per-entry cap, an + // extreme compression ratio is a hallmark of a decompression bomb and is + // rejected outright. + var rig = BuildRig(opts => + { + opts.MaxBundleEntryDecompressedMb = 100; + opts.MaxBundleEntryCompressionRatio = 10; + }); + using var packed = PackPlainBundle(rig.Serializer, rig.ManifestBuilder, EmptyContent()); + var bytes = ((MemoryStream)packed).ToArray(); + // 1 MB of zeros compresses extremely well (>100x ratio) — well over the + // configured 10x cap. + var compressible = new byte[1024 * 1024]; + var bombBytes = ReplaceZipEntry(bytes, "content.json", compressible); + + await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync(new MemoryStream(bombBytes), passphrase: null)); + } + + /// + /// T-006 helper: rewrites an existing zip to add a fresh entry alongside the + /// originals. Used by the "too many entries" test. + /// + private static byte[] AppendExtraZipEntry(byte[] zipBytes, string newEntryName, byte[] payload) + { + var output = new MemoryStream(); + using (var src = new MemoryStream(zipBytes)) + using (var srcZip = new ZipArchive(src, ZipArchiveMode.Read)) + using (var dstZip = new ZipArchive(output, ZipArchiveMode.Create, leaveOpen: true)) + { + foreach (var entry in srcZip.Entries) + { + var dst = dstZip.CreateEntry(entry.FullName); + using var inStream = entry.Open(); + using var outStream = dst.Open(); + inStream.CopyTo(outStream); + } + var extra = dstZip.CreateEntry(newEntryName); + using (var s = extra.Open()) { s.Write(payload); } + } + return output.ToArray(); + } + + /// + /// T-006 helper: rewrites an existing zip, replacing one entry's bytes with + /// the supplied payload while preserving every other entry verbatim. + /// + private static byte[] ReplaceZipEntry(byte[] zipBytes, string entryToReplace, byte[] newPayload) + { + var output = new MemoryStream(); + using (var src = new MemoryStream(zipBytes)) + using (var srcZip = new ZipArchive(src, ZipArchiveMode.Read)) + using (var dstZip = new ZipArchive(output, ZipArchiveMode.Create, leaveOpen: true)) + { + foreach (var entry in srcZip.Entries) + { + var dst = dstZip.CreateEntry(entry.FullName); + using var outStream = dst.Open(); + if (entry.FullName == entryToReplace) + { + outStream.Write(newPayload); + } + else + { + using var inStream = entry.Open(); + inStream.CopyTo(outStream); + } + } + } + return output.ToArray(); + } + + [Fact] + public async Task LoadAsync_rejects_bundle_with_tampered_manifest_field_even_with_correct_passphrase() + { + // T-005: a stolen bundle whose plaintext manifest fields (SourceEnvironment, + // ExportedBy, …) have been edited must fail decryption with a tag mismatch. + // Without AAD an attacker could rewrite the SourceEnvironment label and slip + // past the Step-4 typo-resistant confirmation gate. We tamper the field by + // re-packing the manifest (with everything else, including the original + // ciphertext, unchanged) into a fresh ZIP and verify decrypt fails. + var rig = BuildRig(); + using var packed = PackEncryptedBundle( + rig.Serializer, rig.ManifestBuilder, rig.Encryptor, EmptyContent(), "correct"); + var bytes = ((MemoryStream)packed).ToArray(); + + // Read manifest + ciphertext from the legit bundle, mutate SourceEnvironment, + // and re-pack with the SAME ciphertext bytes — the cipher is fine, only the + // plaintext manifest is changed. + BundleManifest originalManifest; + byte[] cipherBytes; + using (var src = new MemoryStream(bytes)) + using (var srcZip = new ZipArchive(src, ZipArchiveMode.Read)) + { + using var ms = new MemoryStream(); + srcZip.GetEntry("manifest.json")!.Open().CopyTo(ms); + originalManifest = JsonSerializer.Deserialize(ms.ToArray(), BundleJsonOptions)!; + + using var ctMs = new MemoryStream(); + srcZip.GetEntry("content.enc")!.Open().CopyTo(ctMs); + cipherBytes = ctMs.ToArray(); + } + + var tampered = originalManifest with { SourceEnvironment = "prod-spoofed" }; + var tamperedManifestBytes = JsonSerializer.SerializeToUtf8Bytes(tampered, BundleJsonOptions); + + var tamperedZipBytes = new MemoryStream(); + using (var outZip = new ZipArchive(tamperedZipBytes, ZipArchiveMode.Create, leaveOpen: true)) + { + var mEntry = outZip.CreateEntry("manifest.json"); + using (var s = mEntry.Open()) { s.Write(tamperedManifestBytes); } + var cEntry = outZip.CreateEntry("content.enc"); + using (var s = cEntry.Open()) { s.Write(cipherBytes); } + } + tamperedZipBytes.Position = 0; + + // ContentHash check is the FIRST thing that catches the tamper here because + // ContentHash is sealed against the cipher bytes and the cipher is unchanged + // BUT the manifest's plaintext fields are mutated — wait, ContentHash is + // against the cipher which we kept, so it still matches. The defence is + // the AES-GCM AAD: the tag check fails because AAD differs. + await Assert.ThrowsAnyAsync( + () => rig.Importer.LoadAsync(tamperedZipBytes, passphrase: "correct")); + } + + [Fact] + public async Task LoadAsync_resets_unlock_counter_on_correct_passphrase() + { + // T-003: a legitimate operator who typos once or twice before getting it + // right must not be penalised on the next bundle. A successful LoadAsync + // clears the per-bundle counter. + var rig = BuildRig(); + using var packed = PackEncryptedBundle( + rig.Serializer, rig.ManifestBuilder, rig.Encryptor, EmptyContent(), "correct"); + var bundleBytes = ((MemoryStream)packed).ToArray(); + + await Assert.ThrowsAnyAsync( + () => rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: "wrong")); + + var session = await rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: "correct"); + Assert.NotNull(session); + + // Counter cleared — a subsequent wrong-passphrase attempt starts from 0 + // and surfaces as CryptographicException, not BundleLockedException. + await Assert.ThrowsAnyAsync( + () => rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: "wrong")); + Assert.Equal(1, rig.SessionStore.GetUnlockFailureCount(session.Manifest.ContentHash)); + } + [Fact] public async Task LoadAsync_throws_NotSupportedException_when_bundleFormatVersion_unsupported() { diff --git a/tests/ScadaLink.Transport.Tests/Import/BundleSessionStoreTests.cs b/tests/ScadaLink.Transport.Tests/Import/BundleSessionStoreTests.cs index 271de055..3954c27b 100644 --- a/tests/ScadaLink.Transport.Tests/Import/BundleSessionStoreTests.cs +++ b/tests/ScadaLink.Transport.Tests/Import/BundleSessionStoreTests.cs @@ -96,8 +96,13 @@ public sealed class BundleSessionStoreTests } [Fact] - public void Three_failed_unlock_attempts_locks_session() + public void Legacy_FailedUnlockAttempts_field_still_round_trips_via_shared_reference() { + // T-003 legacy guard: the per-session FailedUnlockAttempts / Locked fields on + // BundleSession are no longer the source of truth (lockout moved to the store + // keyed by ContentHash), but the fields remain as a compatibility shim for + // callers/tests that still set them directly. The store hands out the shared + // reference so mutations made on one Get() are observable from another. var clock = new TestTimeProvider(DateTimeOffset.UtcNow); var sut = new BundleSessionStore(Options(), clock); var session = SessionExpiringAt(clock.GetUtcNow().AddMinutes(30)); @@ -120,6 +125,62 @@ public sealed class BundleSessionStoreTests Assert.True(refetch!.Locked); } + [Fact] + public void UnlockFailureCount_TracksPerBundleAndResets() + { + // T-003: the per-bundle unlock-failure counter is keyed by ContentHash and + // shared across sessions, so a second tab / CLI caller cannot side-step the + // lockout by re-uploading the same bytes. Increment must be atomic and + // ClearUnlockFailures must drop the entry so a legitimate operator who + // eventually types the right passphrase is not stuck on a stale count. + var clock = new TestTimeProvider(DateTimeOffset.UtcNow); + var sut = new BundleSessionStore(Options(), clock); + const string contentHashA = "sha-A"; + const string contentHashB = "sha-B"; + + Assert.Equal(0, sut.GetUnlockFailureCount(contentHashA)); + Assert.Equal(1, sut.IncrementUnlockFailureCount(contentHashA)); + Assert.Equal(2, sut.IncrementUnlockFailureCount(contentHashA)); + + // Per-bundle isolation: another bundle's counter is unaffected. + Assert.Equal(0, sut.GetUnlockFailureCount(contentHashB)); + + sut.ClearUnlockFailures(contentHashA); + Assert.Equal(0, sut.GetUnlockFailureCount(contentHashA)); + } + + [Fact] + public void UnlockFailures_ExpireOnTtlAndGetReturnsZero() + { + // T-003: failure records share the session TTL so a bundle that was locked + // hours ago clears on its own. Get must lazily expire stale entries. + var clock = new TestTimeProvider(DateTimeOffset.UtcNow); + var sut = new BundleSessionStore(Options(), clock); + const string contentHash = "sha-expiring"; + + sut.IncrementUnlockFailureCount(contentHash); + sut.IncrementUnlockFailureCount(contentHash); + Assert.Equal(2, sut.GetUnlockFailureCount(contentHash)); + + // Advance beyond the configured TTL (default 30 min). + clock.Advance(TimeSpan.FromMinutes(31)); + Assert.Equal(0, sut.GetUnlockFailureCount(contentHash)); + } + + [Fact] + public void UnlockFailures_EvictExpired_ClearsStaleEntries() + { + // T-003: EvictExpired sweep cleans the per-bundle counters too, not just sessions. + var clock = new TestTimeProvider(DateTimeOffset.UtcNow); + var sut = new BundleSessionStore(Options(), clock); + sut.IncrementUnlockFailureCount("sha-old"); + + clock.Advance(TimeSpan.FromMinutes(31)); + sut.EvictExpired(); + + Assert.Equal(0, sut.GetUnlockFailureCount("sha-old")); + } + /// /// Minimal in-test with a mutable clock. Used in /// place of Microsoft.Extensions.TimeProvider.Testing (not in the