Repository navigation
[master378] Bound nested array preallocation, XML depth and inbound PubSub metadata - #4603
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master378 #4603 +/- ##
=============================================
+ Coverage 61.99% 62.10% +0.11%
=============================================
Files 381 382 +1
Lines 79445 79514 +69
Branches 13918 13927 +9
=============================================
+ Hits 49249 49385 +136
+ Misses 25771 25696 -75
- Partials 4425 4433 +8
🚀 New features to boost your workflow:
|
…ubSub metadata Backports minimal fixes for three codec audit findings (B1-1, X1-1, P1-1) reachable from untrusted input on the 1.5.378 line. - B1-1: BinaryDecoder allocated Variant, DataValue, ExtensionObject and encodeable arrays at the length prefix before reading the elements, and every level of a chain of such arrays nested in the first element of its parent saw the same remaining bytes, so ~1 KB of unauthenticated input (RequestHeader.AdditionalHeader) preallocated ~200 MB. These arrays are now allocated at full length only while all of them still being read take at most 8 bytes per remaining message byte; otherwise they start at 16 KB and grow as elements are read (the budget of #4602). Leaf arrays are unchanged and stay bounded by MaxArrayLength. - X1-1: XmlElement values and XML ExtensionObject bodies had unbounded element depth, and the XmlElement.InnerXml setter / XmlNode.CloneNode(true) recurse once per level, so a ~200 KB XML body of a known type inside a binary message overflowed the stack and terminated the process. The binary, XML and JSON decoders now reject XML nested deeper than MaxEncodingNestingLevels with BadEncodingLimitsExceeded, checked with a streaming reader before the XML is loaded into a DOM. - P1-1: received DataSetMetaData (not authenticated) replaced the metadata of every reader with a matching DataSetWriterId, whatever publisher sent it. It now only updates readers whose PublisherId filter accepts the sender, the same filter the data messages use (a reader without PublisherId still accepts any publisher). Allocation of legitimate large arrays is unchanged and decode time is within noise (Variant[100000], DataValue[10000], ReadValueId[20000], ExtensionObject[20000] vs. db39629). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Id filter Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| { | ||
| case Encoding.JsonNetworkMessage jsonNetworkMessage: | ||
| return jsonNetworkMessage.PublisherId != null && | ||
| jsonNetworkMessage.PublisherId.Equals( |
There was a problem hiding this comment.
Use Use Utils.IsEqual(jsonNetworkMessage.PublisherId, readerPublisherId) as in
UA-.NETStandard/Libraries/Opc.Ua.PubSub/UaPubSubConnection.cs
Lines 302 to 304 in d1d15a3
There was a problem hiding this comment.
Updated the JSON PublisherId comparison to use Utils.IsEqual while preserving the existing string normalization; commit 9bbd25f.
| StringComparison.Ordinal); | ||
| case Encoding.UadpNetworkMessage uadpNetworkMessage: | ||
| return uadpNetworkMessage.PublisherId != null && | ||
| uadpNetworkMessage.PublisherId.Equals(readerPublisherId); |
There was a problem hiding this comment.
Updated the UADP PublisherId comparison to use Utils.IsEqual; commit 9bbd25f.
| /// The streaming reader used for the check does not recurse. | ||
| /// </summary> | ||
| /// <exception cref="ServiceResultException"></exception> | ||
| private void CheckXmlElementDepth(string xml) |
There was a problem hiding this comment.
This method is almost identic with the one implemented in XmlDecoder.cs except missing some checks. Why not use that one also in this file?
There was a problem hiding this comment.
The JSON decoder now calls XmlDecoder.CheckXmlElementDepth with document conformance; Opc.Ua.Core was granted internal access so the shared checks stay internal rather than duplicating them or adding public API. Commit 9bbd25f.
|
@copilot adress review feedback |
Co-authored-by: romanett <7413710+romanett@users.noreply.github.com>
|
|
|
@copilot fix failing tests |
Co-authored-by: romanett <7413710+romanett@users.noreply.github.com>
Co-authored-by: romanett <7413710+romanett@users.noreply.github.com>
Fixed the build failure: the friend-assembly declaration exposed the conflicting internal |
| } | ||
|
|
||
| var values = new VariantCollection(length); | ||
| int capacity = ReserveArrayCapacity(length, s_variantElementSize, out long reserved); |
There was a problem hiding this comment.
Why only these Read[Variant]Array reserve space, why not also ReadStringArray and ReadNodeIdArray and other arrays ? can't the length be "spoofed" also and allocate more than necessary ?
There was a problem hiding this comment.
Only Variant Array can Nest which makes a real amplification possible
…5.378.182 (#52) This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [OPCFoundation.NetStandard.Opc.Ua](https://github.com/OPCFoundation/UA-.NETStandard) | `1.5.378.176` → `1.5.378.182` |  |  | --- ### Release Notes <details> <summary>OPCFoundation/UA-.NETStandard (OPCFoundation.NetStandard.Opc.Ua)</summary> ### [`v1.5.378.182`](https://github.com/OPCFoundation/UA-.NETStandard/releases/tag/1.5.378.182): OPC UA 1.05 Maintenance Update [Compare Source](OPCFoundation/UA-.NETStandard@1.5.378.176...1.5.378.182) Maintenance Release for fixing bugs found on the main378 development branch. #### Released packages [OPCFoundation.NetStandard.Opc.Ua](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Core](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Core/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Security.Certificates](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Security.Certificates/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Configuration](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Configuration/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Server](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Server/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Client](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Client/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Client.ComplexTypes](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Client.ComplexTypes/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Bindings.Https](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Bindings.Https/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.PubSub](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.PubSub/1.5.378.182) #### What's Changed - Remove unused fields from ReferenceNodeManager by [@​mrsuciu](https://github.com/mrsuciu) in [#​4472](OPCFoundation/UA-.NETStandard#4472) - Master378 fix incomplete message buffering by [@​mrsuciu](https://github.com/mrsuciu) in [#​4550](OPCFoundation/UA-.NETStandard#4550) - Fix ServerConfiguration URI values for CTT push-management checks by [@​mrsuciu](https://github.com/mrsuciu) in [#​4592](OPCFoundation/UA-.NETStandard#4592) - Bound nested array preallocation, XML depth and inbound PubSub metadata by [@​romanett](https://github.com/romanett) in [#​4603](OPCFoundation/UA-.NETStandard#4603) - Fix queued data delivery after durable subscription transfer by [@​mrsuciu](https://github.com/mrsuciu) in [#​4622](OPCFoundation/UA-.NETStandard#4622) - Fix GDS client deadlock after a failed keep-alive by [@​romanett](https://github.com/romanett) in [#​4623](OPCFoundation/UA-.NETStandard#4623) - merge Master378 to release by [@​romanett](https://github.com/romanett) in [#​4628](OPCFoundation/UA-.NETStandard#4628) **Full Changelog**: <OPCFoundation/UA-.NETStandard@1.5.378.176...1.5.378.182> </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - Between 12:00 AM and 03:59 AM (`* 0-3 * * *`) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC4xMzIuMSIsInVwZGF0ZWRJblZlciI6IjQ0LjEzMi4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Reviewed-on: https://codeberg.org/umati/connect/pulls/52 Signed-off-by: umati-bot <git-bot@umati.org>
…r to 1.5.378.182 (#53) This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [OPCFoundation.NetStandard.Opc.Ua.Server](https://github.com/OPCFoundation/UA-.NETStandard) | `1.5.378.176` → `1.5.378.182` |  |  | --- ### Release Notes <details> <summary>OPCFoundation/UA-.NETStandard (OPCFoundation.NetStandard.Opc.Ua.Server)</summary> ### [`v1.5.378.182`](https://github.com/OPCFoundation/UA-.NETStandard/releases/tag/1.5.378.182): OPC UA 1.05 Maintenance Update [Compare Source](OPCFoundation/UA-.NETStandard@1.5.378.176...1.5.378.182) Maintenance Release for fixing bugs found on the main378 development branch. #### Released packages [OPCFoundation.NetStandard.Opc.Ua](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Core](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Core/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Security.Certificates](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Security.Certificates/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Configuration](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Configuration/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Server](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Server/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Client](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Client/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Client.ComplexTypes](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Client.ComplexTypes/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.Bindings.Https](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.Bindings.Https/1.5.378.182) [OPCFoundation.NetStandard.Opc.Ua.PubSub](https://www.nuget.org/packages/OPCFoundation.NetStandard.Opc.Ua.PubSub/1.5.378.182) #### What's Changed - Remove unused fields from ReferenceNodeManager by [@​mrsuciu](https://github.com/mrsuciu) in [#​4472](OPCFoundation/UA-.NETStandard#4472) - Master378 fix incomplete message buffering by [@​mrsuciu](https://github.com/mrsuciu) in [#​4550](OPCFoundation/UA-.NETStandard#4550) - Fix ServerConfiguration URI values for CTT push-management checks by [@​mrsuciu](https://github.com/mrsuciu) in [#​4592](OPCFoundation/UA-.NETStandard#4592) - Bound nested array preallocation, XML depth and inbound PubSub metadata by [@​romanett](https://github.com/romanett) in [#​4603](OPCFoundation/UA-.NETStandard#4603) - Fix queued data delivery after durable subscription transfer by [@​mrsuciu](https://github.com/mrsuciu) in [#​4622](OPCFoundation/UA-.NETStandard#4622) - Fix GDS client deadlock after a failed keep-alive by [@​romanett](https://github.com/romanett) in [#​4623](OPCFoundation/UA-.NETStandard#4623) - merge Master378 to release by [@​romanett](https://github.com/romanett) in [#​4628](OPCFoundation/UA-.NETStandard#4628) **Full Changelog**: <OPCFoundation/UA-.NETStandard@1.5.378.176...1.5.378.182> </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - Between 12:00 AM and 03:59 AM (`* 0-3 * * *`) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC4xMzIuMSIsInVwZGF0ZWRJblZlciI6IjQ0LjEzMy4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Reviewed-on: https://codeberg.org/umati/connect/pulls/53 Signed-off-by: umati-bot <git-bot@umati.org>
Minimal backports to the 1.5.378 line of three findings from the codec audit behind #4599 / #4602 (B1-1, X1-1, P1-1). All three can be reached from untrusted input. The changes are kept small, and the performance of legitimate messages is unchanged.
Changes
B1-1: nested array preallocation (
BinaryDecoder)Variant, DataValue, ExtensionObject and encodeable arrays were allocated at the full length prefix before any element was read. Arrays nested in the first element of their parent all see the same remaining message, so a chain of them multiplied the allocation: about 1 KB of input in
RequestHeader.AdditionalHeaderpreallocated about 200 MB before authentication.finally.X1-1: XML element depth (
XmlDecoder,BinaryDecoder,JsonDecoder)The depth of XmlElement values and XML ExtensionObject bodies was unbounded, and the
InnerXmlsetter andCloneNode(true)recurse once per level. A known-type XML body of about 200 KB inside a binary message terminated the process with a stack overflow.MaxEncodingNestingLevels(default 200, the same limit master uses) is rejected withBadEncodingLimitsExceeded.P1-1: inbound DataSetMetaData (
UaPubSubConnection)Received metadata is not authenticated, yet it replaced the metadata of every reader with a matching DataSetWriterId, whichever publisher sent it.
Verification (net10.0 unless noted)
BadEncodingLimitsExceededOpc.Ua.Types.Tests(incl. newDecoderNestingLimitTests)Opc.Ua.Encoders.Fuzz.Tests(crash/timeout/slow corpus)Opc.Ua.Core.Tests(Encoders + Types)Opc.Ua.PubSub.Tests(without broker-dependent MQTT/UDP) + newMetaDataPublisherFilterTestsPerformance vs. db39629:
Variant[100000],Variant(Variant[]),DataValue[10000],ReadValueId[20000]andExtensionObject[20000].Notes for review
MaxEncodingNestingLevelswell above the default, the allowed XML depth rises with it. Master behaves the same way.Opc.Ua.Types, andJsonDecoderinOpc.Ua.Corehas a private copy.🤖 Generated with Claude Code