From 3ca72ac577cf7efe61f9683e8c4bc01a936e425b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Apr 2026 09:44:24 +0000 Subject: [PATCH 1/2] Fix 4 security vulnerabilities found during audit - Fix TOCTOU race in socket creation by setting umask 0177 before bind() - Fix config directory permission check to use file descriptor instead of path - Tighten state_dir and comm_dir permissions from 0755 to 0711 - Use separate PAM handles for account check vs session to prevent policy leak https://claude.ai/code/session_01W6kXMiJ5ULwNV9cega66Dx --- .../dist-packages/privleap/privleap.py | 12 ++++++++-- .../dist-packages/privleap/privleapd.py | 24 +++++++++++++++---- usr/libexec/privleap/shim.py | 14 +++++++---- 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/usr/lib/python3/dist-packages/privleap/privleap.py b/usr/lib/python3/dist-packages/privleap/privleap.py index 36b9153..5504671 100644 --- a/usr/lib/python3/dist-packages/privleap/privleap.py +++ b/usr/lib/python3/dist-packages/privleap/privleap.py @@ -1048,7 +1048,11 @@ def __init__( "PrivleapSocketType.COMMUNICATION" ) self.backend_socket = socket.socket(family=socket.AF_UNIX) - self.backend_socket.bind(str(PrivleapCommon.control_path)) + old_umask = os.umask(0o177) + try: + self.backend_socket.bind(str(PrivleapCommon.control_path)) + finally: + os.umask(old_umask) os.chown(PrivleapCommon.control_path, 0, 0) os.chmod(PrivleapCommon.control_path, stat.S_IRUSR | stat.S_IWUSR) self.backend_socket.listen(10) @@ -1075,7 +1079,11 @@ def __init__( self.backend_socket = socket.socket(family=socket.AF_UNIX) socket_path = Path(PrivleapCommon.comm_dir, user_name) - self.backend_socket.bind(str(socket_path)) + old_umask = os.umask(0o177) + try: + self.backend_socket.bind(str(socket_path)) + finally: + os.umask(old_umask) os.chown(socket_path, target_uid, target_gid) os.chmod(socket_path, stat.S_IRUSR | stat.S_IWUSR) self.backend_socket.listen(10) diff --git a/usr/lib/python3/dist-packages/privleap/privleapd.py b/usr/lib/python3/dist-packages/privleap/privleapd.py index e5efa30..9eb4bee 100644 --- a/usr/lib/python3/dist-packages/privleap/privleapd.py +++ b/usr/lib/python3/dist-packages/privleap/privleapd.py @@ -1394,13 +1394,27 @@ def parse_config_files() -> bool: str(config_dir), ) continue - if not PrivleapCommon.check_secure_file_permissions(str(config_dir)): + try: + config_dir_fd: int = os.open( + str(config_dir), os.O_RDONLY | os.O_DIRECTORY + ) + except OSError as e: logging.warning( - "Config directory '%s' exists but has insecure permissions, " - "ignoring all files in this directory.", + "Config directory '%s' could not be opened, skipping.", str(config_dir), + exc_info=e, ) continue + try: + if not PrivleapCommon.check_secure_file_permissions(config_dir_fd): + logging.warning( + "Config directory '%s' exists but has insecure " + "permissions, ignoring all files in this directory.", + str(config_dir), + ) + continue + finally: + os.close(config_dir_fd) for config_file in config_dir.iterdir(): if not config_file.is_file() or not config_file.name.endswith( @@ -1482,7 +1496,7 @@ def populate_state_dir() -> None: if not PrivleapCommon.state_dir.exists(): try: PrivleapCommon.state_dir.mkdir(parents=True) - PrivleapCommon.state_dir.chmod(0o755) + PrivleapCommon.state_dir.chmod(0o711) except Exception as e: logging.critical( "Cannot create '%s'!", @@ -1500,7 +1514,7 @@ def populate_state_dir() -> None: if not PrivleapCommon.comm_dir.exists(): try: PrivleapCommon.comm_dir.mkdir(parents=True) - PrivleapCommon.comm_dir.chmod(0o755) + PrivleapCommon.comm_dir.chmod(0o711) except Exception as e: logging.critical( "Cannot create '%s'!", diff --git a/usr/libexec/privleap/shim.py b/usr/libexec/privleap/shim.py index 590af2a..31ba958 100755 --- a/usr/libexec/privleap/shim.py +++ b/usr/libexec/privleap/shim.py @@ -77,18 +77,22 @@ def signal_handler(sig: int, frame: FrameType | None) -> None: sys.exit(255) os.umask(init_umask_int) -pam_obj: Any = PAM.pam() -pam_obj.start("privleapd") -pam_obj.set_item(PAM.PAM_USER, calling_user) -pam_obj.set_item(PAM.PAM_RUSER, calling_user) +pam_acct_obj: Any = PAM.pam() +pam_acct_obj.start("privleapd") +pam_acct_obj.set_item(PAM.PAM_USER, calling_user) +pam_acct_obj.set_item(PAM.PAM_RUSER, calling_user) try: - pam_obj.acct_mgmt() + pam_acct_obj.acct_mgmt() except PAM.error as e: if e.args[1] == PAM.PAM_NEW_AUTHTOK_REQD: pass else: sys.exit(255) + +pam_obj: Any = PAM.pam() +pam_obj.start("privleapd") pam_obj.set_item(PAM.PAM_USER, target_user) +pam_obj.set_item(PAM.PAM_RUSER, calling_user) pam_obj.setcred(PAM.PAM_REINITIALIZE_CRED) try: pam_obj.open_session() From 38f8354093687d454950e7abefffe999cdbd9ab1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 21:10:25 +0000 Subject: [PATCH 2/2] Add AGENTS.md with security design decisions and audit history Captures reviewer conclusions from PR #1, architecture invariants, and resolved vulnerabilities so future contributors do not re-propose changes that have already been evaluated. https://claude.ai/code/session_01W6kXMiJ5ULwNV9cega66Dx --- AGENTS.md | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..05fe89c --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,75 @@ +# Agent Guidelines for privleap + +This document captures security design decisions, reviewer conclusions, and +past audit findings so that future contributors (human or AI) do not re-propose +changes that have already been evaluated and resolved. + +## Architecture invariants + +- **No SUID binaries.** privleap runs as a background daemon (`privleapd`). + Privilege separation is achieved through Unix domain sockets, not SUID. +- **File permissions are the sole authentication mechanism** for socket access. + Each comm socket is owned by its user with mode `0600`. Do not add UID peer + credential checks (`SO_PEERCRED`) — they are redundant and can break when a + process's UID and EUID differ. (Reviewed and rejected in PR #1.) +- **One-way communication only.** stdin is never forwarded to actions. This is + intentional to prevent interactive privilege escalation. +- **PAM environment variables are trusted.** Do not add env-var whitelisting in + `shim.py`. If `pam_env.so` passes something through, that is the system + administrator's decision. (Reviewed and rejected in PR #1.) +- **Supplementary groups are always cleared.** `extra_groups=[]` in `shim.py` + is the correct fix. Do not set the target user's supplementary groups — + privleap actions are not expected to inherit them. A future + `EnableSupplementaryGroups` config key could change this if needed. + (Reviewed in PR #1.) +- **No per-connection DoS rate limiting.** Each comm session runs in its own + thread; rate limiting adds complexity without meaningful protection. + (Reviewed and rejected in PR #1.) + +## Code style + +- **Formatter:** Black. Do not reformat code in ways Black disagrees with. +- **Python version:** Targets Python 3.12+ (uses nested f-strings). +- **Comments:** Minimal. The code should be self-explanatory. + +## Security fixes already applied + +The following vulnerabilities have been identified and fixed. Do not re-report +them. + +### On master + +- `config_file_regex` allowed `/` and `.` (path traversal characters). Fixed: + regex is now `r"[-A-Za-z0-9_]+\.conf\Z"`. +- `uid_regex` was missing `\Z` end anchor, accepting trailing garbage like + `"123abc"`. Fixed: regex is now `r"[0-9]+\Z"`. +- Socket cleanup in `destroy_comm_socket()` had a TOCTOU race + (`exists()` then `unlink()`). Fixed: call `unlink()` directly and catch + `FileNotFoundError`. +- Config file name validation used the full path (`str(config_file)`) against + the filename regex. Fixed: validates `config_file.name` only. + +### On branch `claude/security-audit-5BmJL` + +- Socket creation TOCTOU: `bind()` created the socket file with the process + umask, leaving a window before `chmod()`/`chown()`. Fixed: set + `umask(0o177)` before `bind()`, restore after. +- Config directory permission check used a path string (follows symlinks, + TOCTOU-vulnerable). Fixed: open the directory with `os.open()` and check + permissions on the file descriptor. +- `state_dir` and `comm_dir` were world-readable (`0o755`), leaking which + users have active sockets. Fixed: `0o711` (traverse-only for others). +- PAM handle reuse in `shim.py`: the same handle was used for + `calling_user` account check and `target_user` session open, risking + cached state leaking between users. Fixed: separate PAM handles. + +## Things that look like bugs but are not + +- **Nonexistent users in `AuthorizedUsers`** do not cause errors. This is + intentional — a config may reference a user that does not yet exist (e.g. + `sysmaint`). Crashing would break package installation workflows. +- **`action_command` passed to `bash -c`** is not shell-escaped. This is by + design — commands come from root-owned, permission-checked config files and + are meant to be shell expressions. +- **`pwd.getpwall()`** is a valid (if unusual) Python stdlib function that + returns all password database entries. It is not a typo.