Security hardening: improve file handling and socket permissions by assisted-by-ai · Pull Request #1 · Kicksecure/privleap

assisted-by-ai · 2026-04-03T09:46:34Z

Summary

This PR improves security and robustness across the privleap codebase by hardening file permission handling, refining socket creation with proper umask management, and fixing several edge cases in configuration parsing and PAM authentication.

Key Changes

File and Socket Permission Hardening

Set restrictive umask (0o177) when binding UNIX sockets to prevent world-readable socket creation
Changed state and communication directory permissions from 0o755 to 0o711 to restrict directory listing while maintaining execute access
Improved socket cleanup logic to distinguish between missing files (warning) and actual errors (error level)

Configuration Parsing Improvements

Refactored config directory permission checking to use file descriptor-based validation instead of path-based checks
Added proper error handling for directory opening failures
Fixed config file name validation to use only the filename instead of full path
Ensured file descriptors are properly closed with try/finally blocks

PAM Authentication Fix

Separated account management PAM session from credential initialization PAM session
Account management now uses a dedicated pam_acct_obj instance
Credential initialization uses a fresh pam_obj instance with proper user context
This prevents credential state from being mixed between different PAM operations

Regex Pattern Refinements

Removed path separators (. and /) from config file regex pattern to prevent directory traversal
Added anchor (\Z) to uid_regex for stricter matching
These changes prevent malformed or malicious filenames from being processed

Notable Implementation Details

Socket binding now properly restores the original umask after creation
Configuration directory validation uses os.open() with O_RDONLY | O_DIRECTORY flags for safer file descriptor operations
Exception handling is more granular, distinguishing between FileNotFoundError and other exceptions for appropriate logging levels

https://claude.ai/code/session_01W6kXMiJ5ULwNV9cega66Dx

ArrayBolt3

Partially accepted in ArrayBolt3@763ba07. Notes below.

ArrayBolt3 · 2026-04-08T04:22:22Z

-            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)


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.

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.

ArrayBolt3 · 2026-04-08T04:22:30Z

-            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)


ArrayBolt3 · 2026-04-08T04:27:18Z

-    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")


ArrayBolt3 · 2026-04-08T04:28:11Z

-    uid_regex: re.Pattern[str] = re.compile(r"[0-9]+")
+    uid_regex: re.Pattern[str] = re.compile(r"[0-9]+\Z")


ArrayBolt3 · 2026-04-08T04:31:52Z

-            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,
+                )


ArrayBolt3 · 2026-04-08T04:36:34Z

-        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)


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.

ArrayBolt3 · 2026-04-08T04:39:40Z

-            str(config_file), PrivleapValidateType.CONFIG_FILE
+            config_file.name, PrivleapValidateType.CONFIG_FILE


ArrayBolt3 · 2026-04-08T04:41:27Z

-            PrivleapCommon.state_dir.chmod(0o755)
+            PrivleapCommon.state_dir.chmod(0o711)


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

ArrayBolt3 · 2026-04-08T04:42:48Z

-            PrivleapCommon.comm_dir.chmod(0o755)
+            PrivleapCommon.comm_dir.chmod(0o711)


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.)

ArrayBolt3 · 2026-04-08T04:55:45Z

-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)


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.

- 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

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

ArrayBolt3 · 2026-05-01T02:19:41Z

Accepted some of the AGENTS.md file, modifying it quite a bit and refactoring it to work with our agents/ directory structure. See ArrayBolt3@e012eca.

All actual code changes are still undesirable and therefore rejected.

ArrayBolt3 reviewed Apr 8, 2026

View reviewed changes

assisted-by-ai force-pushed the claude/security-audit-5BmJL branch from b4c7365 to 3ca72ac Compare April 27, 2026 18:25

		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")

		uid_regex: re.Pattern[str] = re.compile(r"[0-9]+")
		uid_regex: re.Pattern[str] = re.compile(r"[0-9]+\Z")

		str(config_file), PrivleapValidateType.CONFIG_FILE
		config_file.name, PrivleapValidateType.CONFIG_FILE

		PrivleapCommon.state_dir.chmod(0o755)
		PrivleapCommon.state_dir.chmod(0o711)

		PrivleapCommon.comm_dir.chmod(0o755)
		PrivleapCommon.comm_dir.chmod(0o711)

Conversation

assisted-by-ai commented Apr 3, 2026

Summary

Key Changes

File and Socket Permission Hardening

Configuration Parsing Improvements

PAM Authentication Fix

Regex Pattern Refinements

Notable Implementation Details

Uh oh!

ArrayBolt3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArrayBolt3 commented May 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants