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<object>, 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) <noreply@anthropic.com>
This commit is contained in:
@@ -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<object>(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
|
||||
|
||||
|
||||
@@ -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<object>(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 =====
|
||||
|
||||
/// <summary>
|
||||
/// Per-sample ValueBytes size cap. MessagePack with the default
|
||||
/// <see cref="MessagePack.Resolvers.StandardResolver"/> (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.)
|
||||
/// </summary>
|
||||
private const int MaxValueBytesPerSample = 64 * 1024;
|
||||
|
||||
// ===== Helpers =====
|
||||
|
||||
private async Task<TReply> Invoke<TRequest, TReply>(
|
||||
@@ -310,6 +321,26 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Deserializes a sample's value bytes using the MessagePack default
|
||||
/// <see cref="MessagePack.Resolvers.StandardResolver"/> (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.)
|
||||
/// </summary>
|
||||
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<object>(valueBytes);
|
||||
}
|
||||
|
||||
private static IReadOnlyList<DataValueSnapshot> 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<object>(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);
|
||||
|
||||
@@ -27,6 +27,16 @@
|
||||
</ItemGroup>
|
||||
|
||||
<ItemGroup>
|
||||
<!--
|
||||
GHSA-37gx-xxp4-5rgx (MessagePack — unsafe deserialization via dynamic code generation)
|
||||
GHSA-w3x6-4m5h-cxqf (MessagePack — TypelessContractlessStandardResolver gadget chain)
|
||||
|
||||
Neither advisory applies to this module's usage: all deserialization here uses the
|
||||
default StandardResolver (primitive types only). TypelessContractlessStandardResolver
|
||||
is never referenced and no DynamicUnion / DynamicGenericResolver is registered.
|
||||
DeserializeSampleValue() enforces a 64 KiB per-sample ValueBytes cap (finding 007).
|
||||
Revisit once MessagePack 3.x is available and drop these suppressions at that time.
|
||||
-->
|
||||
<NuGetAuditSuppress Include="https://github.com/advisories/GHSA-37gx-xxp4-5rgx"/>
|
||||
<NuGetAuditSuppress Include="https://github.com/advisories/GHSA-w3x6-4m5h-cxqf"/>
|
||||
</ItemGroup>
|
||||
|
||||
Reference in New Issue
Block a user