Skip to content

Security: fix ACL privilege escalation + hardening#224

Open
jacquelinegarrahan wants to merge 2 commits into
epics-base:masterfrom
jacquelinegarrahan:security/hardening
Open

Security: fix ACL privilege escalation + hardening#224
jacquelinegarrahan wants to merge 2 commits into
epics-base:masterfrom
jacquelinegarrahan:security/hardening

Conversation

@jacquelinegarrahan

Copy link
Copy Markdown

Security review of p4p. One real vulnerability plus three low-severity hardening items.

HIGH — Privilege escalation via in-place mutation of shared UAG state

src/p4p/asLib/__init__.pyEngine.create() did uags = self._uag.get(user, set()) then uags |= …role…. Because .get() returns the stored set, the in-place |= permanently merged client-asserted (forgeable, under default ca auth) roles into that account's process-global group membership. A single connection could permanently elevate an account; every later evaluation of that account — including role-less sessions 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

  • CI least-privilege: add permissions: contents: read to build.yml/ci-scripts.yml (publish uses TWINE secrets, not the token).
  • Native except+: monPop/opHandler/opBuilder/opEvent in _p4p.pyx lacked the exception spec every sibling has; an unexpected C++ exception (e.g. std::bad_alloc from pop()) would std::terminate the interpreter instead of raising.
  • dynamic_cast guard in pvxs_source.cpp::disconnectDynamic (defense-in-depth against a future NULL deref).

Verification

Built against epicscorelibs 7.0.10 / pvxslibs 1.5.2 — the native changes compile cleanly and the full test suite passes (157 tests, 2 skipped).

jacquelinegarrahan and others added 2 commits July 14, 2026 14:48
…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>
@jacquelinegarrahan

Copy link
Copy Markdown
Author

Reviewer note — this is the priority PR of a small batch.

This is 1 of 11 PRs from a review of p4p (#214#224). The other ten (#214#223) are independent single-commit correctness fixes; this one is the only security-relevant change, so it's worth looking at first.

The load-bearing part is the asLib fix (HIGH): Engine.create() mutated the shared self._uag[user] set in place when merging client-asserted roles, permanently escalating that account's group membership for all later sessions. The other three items in this PR are low-severity hardening (CI token scope, native except+, a dynamic_cast NULL guard) and could be split out if you'd prefer the security fix land on its own — happy to do that.

Each PR is independent and touches disjoint lines, so they can be reviewed/merged in any order. All were built against epicscorelibs 7.0.10 / pvxslibs 1.5.2 and pass the full suite (157 tests, 2 skipped); every fix was also reproduced against the compiled library (buggy on master, fixed on the branch). The only red CI I've seen is an intermittent RefTestCase reference-leak failure on the windows-latest / 3.13 runner that also flakes on unrelated changes — not introduced here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant