From 5bcbda168525577fb6ceb652d09f06f03c6bb2d5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:20:23 -0400 Subject: [PATCH] fix(driver-historian-wonderware-client): resolve Medium code-review finding (Driver.Historian.Wonderware.Client-007) Introduce DeserializeSampleValue() helper that enforces a 64 KiB per-sample ValueBytes size cap before calling MessagePackSerializer.Deserialize, and documents that the default StandardResolver (primitive-only, no typeless or dynamic-type resolution) is in use. Both ToSnapshots and AlignAtTimeSnapshots route through the new helper. Add inline XML comments to the two NuGetAuditSuppress entries in the csproj recording the advisory title, why each does not apply to this module's primitive-only deserialization, and when to revisit. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../findings.md | 4 +- .../WonderwareHistorianClient.cs | 38 +++++++++++++++++-- ....Driver.Historian.Wonderware.Client.csproj | 10 +++++ 3 files changed, 46 insertions(+), 6 deletions(-) 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 @@ +