From b37e875066f0e6ba945b53f7bd82cd808340eadc Mon Sep 17 00:00:00 2001 From: Jacqueline Garrahan Date: Tue, 14 Jul 2026 14:48:42 -0700 Subject: [PATCH 1/2] asLib: fix privilege escalation via in-place mutation of shared UAG state 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) --- src/p4p/asLib/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/p4p/asLib/__init__.py b/src/p4p/asLib/__init__.py index d5e02174..841ac91f 100644 --- a/src/p4p/asLib/__init__.py +++ b/src/p4p/asLib/__init__.py @@ -240,7 +240,9 @@ def create(self, channel, group, user, host, level, roles=[]): with self._lock: - uags = self._uag.get(user, set()) + # copy the stored set; |= below is in-place and would otherwise + # permanently merge client-asserted roles into the shared UAG state + uags = set(self._uag.get(user, ())) for role in roles: uags |= self._uag.get('role/'+role, set()) hags = self._hag_addr.get(host, set()) From f913e6b25411f8a6ebc081e8cc7ac6877af9194f Mon Sep 17 00:00:00 2001 From: Jacqueline Garrahan Date: Tue, 14 Jul 2026 14:51:41 -0700 Subject: [PATCH 2/2] Security hardening: CI least-privilege, native except+, dynamic_cast 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) --- .github/workflows/build.yml | 5 +++++ .github/workflows/ci-scripts.yml | 4 ++++ src/p4p/_p4p.pyx | 8 ++++---- src/pvxs_source.cpp | 5 ++--- 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 00a07011..e18c986b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -2,6 +2,11 @@ name: Python on: [push, pull_request, workflow_dispatch] +# Least privilege: no job needs to write back to the repo. Publishing to PyPI +# uses TWINE_* secrets, not GITHUB_TOKEN, so read access is sufficient here. +permissions: + contents: read + defaults: run: shell: bash diff --git a/.github/workflows/ci-scripts.yml b/.github/workflows/ci-scripts.yml index a5b656b9..23e0fca2 100644 --- a/.github/workflows/ci-scripts.yml +++ b/.github/workflows/ci-scripts.yml @@ -2,6 +2,10 @@ name: Makefile on: [push, pull_request, workflow_dispatch] +# Least privilege: this workflow only builds/tests, it never writes to the repo. +permissions: + contents: read + defaults: run: shell: bash diff --git a/src/p4p/_p4p.pyx b/src/p4p/_p4p.pyx index e27621a9..3f3da3ba 100644 --- a/src/p4p/_p4p.pyx +++ b/src/p4p/_p4p.pyx @@ -53,10 +53,10 @@ cdef extern from "" namespace "p4p": void disconnectDynamic(const shared_ptr[server.Source]& src) except+ # pvxs_client.cpp - void opHandler[Builder](Builder& builder, object handler) - void opBuilder[Builder](Builder& builder, object handler) - void opEvent(client.MonitorBuilder& builder, object handler) - object monPop(const shared_ptr[client.Subscription]& mon) with gil + void opHandler[Builder](Builder& builder, object handler) except+ + void opBuilder[Builder](Builder& builder, object handler) except+ + void opEvent(client.MonitorBuilder& builder, object handler) except+ + object monPop(const shared_ptr[client.Subscription]& mon) except+ with gil cimport numpy # must cimport after p4p.h is included diff --git a/src/pvxs_source.cpp b/src/pvxs_source.cpp index 6304b487..70803d5b 100644 --- a/src/pvxs_source.cpp +++ b/src/pvxs_source.cpp @@ -136,9 +136,8 @@ void disconnectDynamic(const std::shared_ptr& src) if(!src) return; - auto dynsrc = dynamic_cast(src.get()); - - dynsrc->handler = nullptr; + if(auto dynsrc = dynamic_cast(src.get())) + dynsrc->handler = nullptr; } }