diff --git a/code-reviews/Driver.Historian.Wonderware.Client/findings.md b/code-reviews/Driver.Historian.Wonderware.Client/findings.md index 5b44b95..a5c8d88 100644 --- a/code-reviews/Driver.Historian.Wonderware.Client/findings.md +++ b/code-reviews/Driver.Historian.Wonderware.Client/findings.md @@ -191,7 +191,7 @@ retry/backoff is owned by the caller (the alarm drain worker / history router). | Severity | Medium | | Category | Security | | Location | `WonderwareHistorianClient.cs:276` | -| Status | Open | +| Status | Resolved | **Description:** `ToSnapshots` deserializes peer-supplied bytes with `MessagePackSerializer.Deserialize(dto.ValueBytes)`, typeless MessagePack @@ -209,7 +209,7 @@ that. Prefer round-tripping the value as a constrained set of known primitive ty than `object`, and validate `ValueBytes.Length` against a sane per-sample cap before deserializing. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added `DeserializeSampleValue()` helper that enforces a 64 KiB per-sample `ValueBytes` cap before deserialization and documents that the default `StandardResolver` (primitive-only, no `TypelessContractlessStandardResolver`) is in use; both `ToSnapshots` and `AlignAtTimeSnapshots` now route through the helper; added inline XML comments to the two `NuGetAuditSuppress` entries in the csproj stating the advisory title, why it does not apply to this usage, and the revisit trigger. ### Driver.Historian.Wonderware.Client-008 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/WonderwareHistorianClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/WonderwareHistorianClient.cs index 49dc4a2..3c8dd9b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/WonderwareHistorianClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/WonderwareHistorianClient.cs @@ -135,9 +135,8 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist var requested = DateTime.SpecifyKind(timestampsUtc[i], DateTimeKind.Utc); if (byTicks.TryGetValue(requested.Ticks, out var dto)) { - var value = dto.ValueBytes is null ? null : MessagePackSerializer.Deserialize(dto.ValueBytes); result[i] = new DataValueSnapshot( - Value: value, + Value: DeserializeSampleValue(dto.ValueBytes), StatusCode: QualityMapper.Map(dto.Quality), SourceTimestampUtc: requested, ServerTimestampUtc: DateTime.UtcNow); @@ -242,6 +241,18 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist } } + // ===== Constants ===== + + /// + /// Per-sample ValueBytes size cap. MessagePack with the default + /// (primitive-only — no typeless + /// or dynamic-type resolution) is not susceptible to type-confusion gadget chains, but + /// we still cap the per-sample byte budget to guard against a buggy or unexpectedly + /// large peer payload. 64 KiB is well above any primitive historian value. + /// (Finding 007 — NuGetAuditSuppress GHSA-37gx-xxp4-5rgx / GHSA-w3x6-4m5h-cxqf.) + /// + private const int MaxValueBytesPerSample = 64 * 1024; + // ===== Helpers ===== private async Task Invoke( @@ -310,6 +321,26 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist } } + /// + /// Deserializes a sample's value bytes using the MessagePack default + /// (primitive types only — no + /// typeless or dynamic-type resolution). A per-sample size cap guards against a + /// hostile or buggy peer sending an unexpectedly large payload before deserialization + /// allocates memory for it. (Finding 007.) + /// + private static object? DeserializeSampleValue(byte[]? valueBytes) + { + if (valueBytes is null) return null; + if (valueBytes.Length > MaxValueBytesPerSample) + throw new InvalidDataException( + $"Sidecar sample ValueBytes length {valueBytes.Length} exceeds the {MaxValueBytesPerSample}-byte cap."); + // Deserializes using the default resolver which only handles primitive types + // (bool, int, long, float, double, string, byte[], DateTime, etc.). The resolver + // does NOT support TypelessContractlessStandardResolver so no type-confusion gadget + // chains are reachable from this call site. + return MessagePackSerializer.Deserialize(valueBytes); + } + private static IReadOnlyList ToSnapshots(HistorianSampleDto[] dtos) { if (dtos.Length == 0) return []; @@ -317,9 +348,8 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist for (var i = 0; i < dtos.Length; i++) { var dto = dtos[i]; - var value = dto.ValueBytes is null ? null : MessagePackSerializer.Deserialize(dto.ValueBytes); snapshots[i] = new DataValueSnapshot( - Value: value, + Value: DeserializeSampleValue(dto.ValueBytes), StatusCode: QualityMapper.Map(dto.Quality), SourceTimestampUtc: new DateTime(dto.TimestampUtcTicks, DateTimeKind.Utc), ServerTimestampUtc: DateTime.UtcNow); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.csproj b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.csproj index 8651126..ae5532d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.csproj +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.csproj @@ -27,6 +27,16 @@ +