Fix three high-severity audit findings in new 2.0 pub sub/GDS code#3954
Open
marcschier wants to merge 3 commits into
Open
Fix three high-severity audit findings in new 2.0 pub sub/GDS code#3954marcschier wants to merge 3 commits into
marcschier wants to merge 3 commits into
Conversation
Fixes three high-severity, remotely-relevant weaknesses found in a full-codebase security audit: 1. PubSub inbound message-security fail-open for non-UADP frames. The inbound signature/encryption/replay gate lived inside the UADP-only branch, so a reader configured Sign/SignAndEncrypt over a JSON transport silently accepted attacker-forged frames. Non-UADP frames on a secured reader are now dropped fail-closed before decode, mirroring the send-side refusal. 2. Client schema-import SSRF / NetNTLM leak. A malicious server could make the client resolve a server-supplied DataType-dictionary import Location (e.g. a UNC path) against the local file system, triggering an outbound SMB/NTLM handshake or arbitrary local file read. Added a deny-all NullFileSystem used by the in-memory/import-table schema validators, and made the XML loader prefer the in-memory import table before any untrusted location. 3. GDS AuthorizationService JWT impersonation / privilege escalation. FinishRequestToken derived the token subject from the unauthenticated client token and embedded requested roles verbatim, letting any caller on an encrypted channel mint a GDS-signed JWT asserting an arbitrary subject and roles (e.g. SecurityAdmin). The subject is now bound to the identity authenticated on the session, and requested roles are gated by a new fail-closed AuthorizeRoles authorization hook (no roles unless the operator explicitly authorizes them). The legacy RequestAccessToken path is bound to the session identity as well. Adds regression tests for all three findings.
|
agent seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
marcschier
changed the title
marcschier
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Hardens multiple remotely-relevant security paths by failing closed for unauthenticated PubSub frames, preventing schema-import filesystem/UNC resolution, and binding GDS-issued JWT subjects/roles to the server-authenticated session identity with fail-closed role authorization.
Changes:
- PubSub: drop non-UADP (e.g., JSON) inbound frames when inbound security is required, prior to decode.
- Schema validation: introduce a deny-all
NullFileSystemand prefer in-memory import tables over untrusted schema locations to prevent SSRF/local file reads. - GDS JWT issuance: bind JWT subject to session-authenticated identity and gate requested roles behind a fail-closed authorization callback; update legacy access-token path plumbing and tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/Opc.Ua.Types.Tests/Utils/FileSystem/NullFileSystemTests.cs | Adds regression tests ensuring NullFileSystem denies all filesystem operations. |
| Tests/Opc.Ua.PubSub.Tests/Connections/PubSubConnectionSecurityReceiveTests.cs | Adds test asserting secured readers drop non-UADP JSON frames before decode. |
| Tests/Opc.Ua.Gds.Tests/AuthorizationService/StartRequestTokenTests.cs | Adds tests for fail-closed role granting and subject binding to authenticated identity. |
| Tests/Opc.Ua.Gds.Tests/AuthorizationService/InMemoryAccessTokenProviderRefreshTests.cs | Updates refresh tests to configure role-authorization callback for expected role claims. |
| Tests/Opc.Ua.Core.Schema.Tests/BsdSchemaValidationTests.cs | Adds regression test ensuring import validation won’t resolve server-supplied filesystem locations. |
| Stack/Opc.Ua.Types/Utils/FileSystem/NullFileSystem.cs | Introduces deny-all filesystem implementation to block local/UNC access from untrusted inputs. |
| Stack/Opc.Ua.Types/Schema/XmlSchemaValidator.cs | Switches import-table constructor to use NullFileSystem to prevent filesystem resolution. |
| Stack/Opc.Ua.Types/Schema/SchemaValidator.cs | Prefers in-memory imports over untrusted locations and keeps trusted mapping precedence. |
| Stack/Opc.Ua.Types/Schema/BinarySchemaValidator.cs | Switches import-table constructor to use NullFileSystem to prevent filesystem resolution. |
| Libraries/Opc.Ua.PubSub/Connections/PubSubConnection.cs | Implements inbound fail-closed drop for non-UADP frames on secured readers. |
| Libraries/Opc.Ua.Gds.Server.Common/Identity/InMemoryAccessTokenProvider.cs | Binds subject to session identity, adds fail-closed role authorization, and threads caller identity through legacy issuance. |
| Libraries/Opc.Ua.Gds.Server.Common/Hosting/AuthorizationServiceOptions.cs | Adds AuthorizeRoles callback to explicitly authorize role embedding in issued tokens. |
| Libraries/Opc.Ua.Gds.Server.Common/AuthorizationServiceManager.cs | Adds legacy overload to pass session-authenticated identity into token issuance. |
| Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs | Passes session user identity to legacy RequestAccessToken when provider is AuthorizationServiceManager. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3954 +/- ##
=======================================
Coverage 73.51% 73.51%
=======================================
Files 1170 1171 +1
Lines 170175 170206 +31
Branches 29363 29365 +2
=======================================
+ Hits 125097 125128 +31
+ Misses 34072 34067 -5
- Partials 11006 11011 +5
🚀 New features to boost your workflow:
|
- NullFileSystem: add private constructor to enforce the documented singleton, and align the copyright header year to 2026. - PubSub: log the non-UADP secured-frame drop at Debug instead of Warning to avoid log amplification on the per-frame hot path (the security-failure counter remains the rate-safe operational signal). - GDS: simplify the obsolete RequestAccessToken path. It no longer threads a partial session identity; the provider issues an unprivileged anonymous-subject token and never trusts the client-supplied request token. Full identity binding remains available via the Start/Finish flow. This removes the inconsistent per-provider threading and the associated uncovered code. - Tests: cover the obsolete RequestAccessToken path (anonymous subject, no roles) and the AuthorizeRoles edge cases (granted set is intersected with the requested roles; empty authorization yields no roles).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes three high-severity, remotely-relevant security weaknesses identified by a full-codebase security audit. Each fix is minimal and surgical, and ships with regression tests.
1. PubSub inbound message-security fail-open for non-UADP (JSON) frames
PubSubConnectionnested the inbound signature/encryption/replay enforcement inside theUadpDecoder.TryReadOuterPrefix(...)branch. Because that check returnsfalsefor a JSON frame, a reader configured forSign/SignAndEncryptover a JSON transport silently accepted attacker-forged, unauthenticated frames, delivering forged DataSet/MetaData messages to application sinks. The send path already fail-closes the identical case.Fix: drop any non-UADP frame fail-closed on a secured reader before decode (
else if (RequiresInboundSecurity)), mirroring the send-side refusal.2. Client schema-import SSRF / NetNTLM credential leak
When loading complex types, the client falls back to the server's type dictionary and resolves each
Import/SchemaLocationagainst the local file system with no sanitization. A malicious server could supply a UNC path (e.g.\\attacker\share\x), triggering an outbound SMB/NetNTLM handshake (hash theft / SSRF) or arbitrary local file read. This bypassed the existing XXE hardening because imports are followed by manual file-system code, not the XML resolver.Fix: added a deny-all
NullFileSystemand wired it into the in-memory/import-table schema validator constructors, so network-supplied dictionaries never touch the local file system. The XML loader now prefers the in-memory import table before any (untrusted) location, so legitimate in-memory resolution is unchanged.3. GDS AuthorizationService JWT impersonation / privilege escalation
FinishRequestTokenderived the JWT subject from the unauthenticated client-supplieduserIdentityToken(never verifyinguserTokenSignature) and embedded the client'srequestedRolesverbatim. Any caller able to open an encrypted channel — even anonymous — could obtain a GDS-signed token assertingsub=admin,roles=[SecurityAdmin, ...], impersonating any principal to resource servers that trust GDS-issued JWTs.Fix:
AuthorizationServiceOptions.AuthorizeRoleshook. With no callback configured, no roles are granted; the callback can only ever narrow the requested set. This removes privilege escalation via arbitrary role requests.RequestAccessTokenpath no longer trusts the client-supplied token: it issues an unprivileged anonymous-subject token with no roles. UseStartRequestToken/FinishRequestTokenfor authenticated identity binding.Related Issues
No public issue is linked — these findings were identified by an internal security audit and fixed directly. Maintainers may open tracking issues if desired.
Verification
SecuredReaderRejectsNonUadpJsonFrameAsync(+ existing receive-security suite, 6/6).NullFileSystemTests(6/6),ImportTableValidatorDoesNotResolveServerSuppliedFilesystemLocation(BSD, 3/3), source-gen schema validation (6 passed), full ComplexTypes client-load suite (2214/2214).FinishDoesNotGrantRequestedRolesWithoutAuthorization,FinishBindsSubjectToAuthenticatedIdentityNotRequestToken, plusLegacyRequestAccessTokenIssuesAnonymousUnprivilegedTokenandAuthorizeRolesintersection/empty edge cases (AuthorizationService unit 15/15, RefreshToken integration 3/3).Review feedback from the automated reviewer has been addressed (NullFileSystem singleton constructor + header year, PubSub drop logged at Debug to avoid log amplification, and the obsolete
RequestAccessTokenpath simplified to a uniform anonymous subject); all review threads resolved.Checklist
Put an
xin the boxes that apply. You can complete these step by step after opening the PR.