Security: fix ACL privilege escalation + hardening#11
Closed
jacquelinegarrahan wants to merge 2 commits into
Closed
Security: fix ACL privilege escalation + hardening#11jacquelinegarrahan wants to merge 2 commits into
jacquelinegarrahan wants to merge 2 commits into
Conversation
…tate
Engine.create() did `uags = self._uag.get(user, set())` and then
`uags |= self._uag.get('role/'+role, set())`. Because the get() returns the
stored group set for that user, the in-place `|=` union permanently merged the
client-asserted roles into the process-global `self._uag[user]`.
For the common pattern where an account is a static UAG member, a single
connection presenting that account with a forgeable role (roles are
client-asserted under the default `ca` auth) permanently rewrites that
account's group membership. Every subsequent evaluation of the account -
including role-less sessions from other hosts and the periodic _recompute() -
then inherits the escalated permissions, outliving the triggering session and
defeating upstream role revocation.
Copy the set before the in-place union so the shared state is never mutated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…guard
- .github/workflows/{build,ci-scripts}.yml: add top-level
`permissions: contents: read`. Neither workflow writes back to the repo
(PyPI publish uses TWINE_* secrets, not GITHUB_TOKEN), so the default
read/write token was unnecessary ambient authority.
- src/p4p/_p4p.pyx: add `except+` to opHandler/opBuilder/opEvent/monPop.
Without it, a C++ exception other than the three monPop catches
(Finished/Disconnect/RemoteError) - e.g. std::bad_alloc from pop() -
escapes a declaration Cython did not wrap and unwinds into generated C
as std::terminate(), crashing the interpreter. Matches the existing
`except+` idiom used by every sibling declaration in the same block.
- src/pvxs_source.cpp: guard the dynamic_cast in disconnectDynamic() with a
NULL check before dereference (defense-in-depth against future callers
passing a non-DynamicSource).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Superseded by the upstream PR epics-base#224 (opened against epics-base/p4p). Closing this fork PR to avoid duplicates. |
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
1 participant
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.
Security review of p4p. One real vulnerability plus three low-severity hardening items. Reviewed independently of the correctness-bug PRs (#1–#10).
HIGH — Privilege escalation via in-place mutation of shared UAG state
src/p4p/asLib/__init__.py—Engine.create()diduags = self._uag.get(user, set())thenuags |= …role…. Because.get()returns the stored set, the in-place|=permanently merged client-asserted (forgeable, under defaultcaauth) roles into that account's process-global group membership. A single connection could permanently elevate an account; every later session for that account — including role-less ones from other hosts and the periodic_recompute()— then inherited the escalation. Fixed by copying the set before the union.Reproduced against the compiled
Engine: unpatched,_uag['alice']goes['OPS'] → ['ADMINS','OPS']and stays; patched, it stays['OPS'].Low — hardening
permissions: contents: readtobuild.yml/ci-scripts.yml(publish uses TWINE secrets, not the token).except+:monPop/opHandler/opBuilder/opEventin_p4p.pyxlacked the exception spec every sibling has; an unexpected C++ exception (e.g.std::bad_alloc) wouldstd::terminatethe interpreter instead of raising.dynamic_castguard inpvxs_source.cpp::disconnectDynamic(defense-in-depth against a future NULL deref).Verification
Built against
epicscorelibs7.0.10 /pvxslibs1.5.2 (the native changes compile cleanly) and the full test suite passes (157 tests, 2 skipped).