Skip to content

Security hardening: improve file handling and socket permissions #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
assisted-by-ai wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Security hardening: improve file handling and socket permissions #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ca72ac
Fix 4 security vulnerabilities found during audit
claude Apr 3, 2026
38f8354
Add AGENTS.md with security design decisions and audit history
claude Apr 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
12 changes: 10 additions & 2 deletions usr/lib/python3/dist-packages/privleap/privleap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines -1051 to +1055

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this as being a problem. If the socket setup fails, maybe it will leave a socket with bad permissions on disk, but the application won't use it because socket setup failed. If the socket setup succeeds, the ownership and permissions are set properly, so the umask isn't needed. privleapd already sets a restrictive umask anyway.

@ArrayBolt3 ArrayBolt3 Apr 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this here for the sake of context; there actually could have been an issue here if privleapd's umask wasn't restrictive by default. If a file system object is created with insecure permissions, and is then chown'd/chmod'd to safety, it leaves a race window where something else could open that object with its original insecure permissions. The file descriptor won't be invalidated when the permissions change.

This isn't a problem in practice however, because privleapd already does os.umask(0o077) at the very beginning of its main() function. At most, we might want to add a comment or some documentation explaining that a server should generally do this before using the library.

os.chown(PrivleapCommon.control_path, 0, 0)
os.chmod(PrivleapCommon.control_path, stat.S_IRUSR | stat.S_IWUSR)
self.backend_socket.listen(10)
Expand All @@ -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)
Comment on lines -1078 to +1086

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

os.chown(socket_path, target_uid, target_gid)
os.chmod(socket_path, stat.S_IRUSR | stat.S_IWUSR)
self.backend_socket.listen(10)
Expand Down
24 changes: 19 additions & 5 deletions usr/lib/python3/dist-packages/privleap/privleapd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines -1397 to +1417

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the reasoning here, but disagree with it. The original code is easier to read, and while it is arguably susceptible to a TOCTOU, in practice a TOCTOU doesn't matter here. If an attacker can change the permissions from secure to insecure, swap out the directory, etc. between the check and the open, they already have the ability to write anything they want into the configuration directory and compromise the system, or set up the permissions to be correct before the swap. This patch does plug the TOCTOU, but in practice that does nothing for security.


for config_file in config_dir.iterdir():
if not config_file.is_file() or not config_file.name.endswith(
Expand Down Expand Up @@ -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)
Comment on lines -1485 to +1499

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break the ability to list the directory contents. I don't think being able to list the contents is a risk.

except Exception as e:
logging.critical(
"Cannot create '%s'!",
Expand All @@ -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)
Comment on lines -1503 to +1517

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. (This would have the advantage of hiding the list of active users from others, but most of that info can be gotten using loginctl, so I don't think that's particularly valuable.)

except Exception as e:
logging.critical(
"Cannot create '%s'!",
Expand Down
14 changes: 9 additions & 5 deletions usr/libexec/privleap/shim.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines -80 to +95

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this is useful. The PAM documentation doesn't seem to hint at why this might be desirable, and sudo doesn't seem to make separate PAM sessions for this scenario, so rejecting for now.

pam_obj.setcred(PAM.PAM_REINITIALIZE_CRED)
try:
pam_obj.open_session()
Expand Down