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 1 commit 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
1 commit
Select commit
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
16 changes: 12 additions & 4 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
Copy link
Copy Markdown

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

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.

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
Copy link
Copy Markdown

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

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 Expand Up @@ -1226,9 +1234,9 @@ class PrivleapCommon:
state_dir: Path = Path("/run/privleapd")
control_path: Path = Path(state_dir, "control")
comm_dir: Path = Path(state_dir, "comm")
config_file_regex: re.Pattern[str] = re.compile(r"[-A-Za-z0-9_./]+\.conf\Z")
config_file_regex: re.Pattern[str] = re.compile(r"[-A-Za-z0-9_]+\.conf\Z")
Comment on lines -1229 to +1237
Copy link
Copy Markdown

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Accepted.

user_name_regex: re.Pattern[str] = re.compile(r"[a-z_][-a-z0-9_]*\$?\Z")
uid_regex: re.Pattern[str] = re.compile(r"[0-9]+")
uid_regex: re.Pattern[str] = re.compile(r"[0-9]+\Z")
Comment on lines -1231 to +1239
Copy link
Copy Markdown

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Accepted.

signal_name_regex: re.Pattern[str] = re.compile(r"[-A-Za-z0-9_.]+\Z")
msg_arg_blob_data: dict[str, tuple[int, int, bool]] = {
"CREATE": (1, 1, False),
Expand Down
52 changes: 31 additions & 21 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 @@ -426,27 +426,23 @@ def destroy_comm_socket(
sock: PrivleapSocket = sock_info.listen_socket
if sock.user_name == real_user_name:
socket_path: Path = Path(PrivleapCommon.comm_dir, real_user_name)
if socket_path.exists():
try:
socket_path.unlink()
except Exception as e:
## Probably just a TOCTOU issue, i.e. someone already
## removed the socket. Most likely caused by the user
## fiddling with things, no big deal.
logging.error(
"Destroying comm socket for account '%s', failed to "
"delete UNIX socket at '%s'",
real_user_name,
str(socket_path),
exc_info=e,
)
else:
try:
socket_path.unlink()
except FileNotFoundError:
logging.warning(
"Destroying comm socket for account '%s', no UNIX socket "
"to delete at '%s'",
real_user_name,
str(socket_path),
)
except Exception as e:
logging.error(
"Destroying comm socket for account '%s', failed to "
"delete UNIX socket at '%s'",
real_user_name,
str(socket_path),
exc_info=e,
)
Comment on lines -429 to +445
Copy link
Copy Markdown

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Accepted.

remove_sock_idx = sock_idx
break

Expand Down Expand Up @@ -1398,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 -1401 to +1417
Copy link
Copy Markdown

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

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 @@ -1439,7 +1449,7 @@ def parse_config_files() -> bool:
continue

if not PrivleapCommon.validate_id(
str(config_file), PrivleapValidateType.CONFIG_FILE
config_file.name, PrivleapValidateType.CONFIG_FILE
Comment on lines -1442 to +1452
Copy link
Copy Markdown

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

Choose a reason for hiding this comment

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

Accepted.

):
logging.warning(
"Config file '%s' has an illegal name, skipping.",
Expand Down Expand Up @@ -1486,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 -1489 to +1499
Copy link
Copy Markdown

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

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 @@ -1504,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 -1507 to +1517
Copy link
Copy Markdown

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

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
Copy link
Copy Markdown

@ArrayBolt3 ArrayBolt3 Apr 8, 2026

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