fix: set file permissions on AWS credentials, remove sensitive data from SSH logs

keepercommander/plugins/awskey/aws_accesskey.py

@@ -12,6 +12,7 @@
 import logging
 import shutil
 from configparser import RawConfigParser
+import os
 from os.path import expandvars, expanduser, isfile
 
 from botocore.exceptions import ClientError
@@ -134,8 +135,14 @@ def sync_with_creds_file(self):
             cp.set(sync_profile, AWS_CREDS_FILE_KEY_ID_OPTION, self.new_key_id)
             cp.set(sync_profile, AWS_CREDS_FILE_KEY_SECRET_OPTION, self.new_secret)
             backup_file = f'{creds_filename}{AWS_CREDS_FILE_BACKUP_EXTENSION}'
-            shutil.copy2(creds_filename, backup_file)
-            with open(creds_filename, 'w') as f:
+            old_umask = os.umask(0o077)
+            try:
+                shutil.copy2(creds_filename, backup_file)
+            finally:
+                os.umask(old_umask)
+            fd = os.open(creds_filename, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
+            os.fchmod(fd, 0o600)
+            with os.fdopen(fd, 'w') as f:
                 cp.write(f)
             logging.info(
                 f'Synced AWS key rotation with AWS credential file "{creds_filename}"'

keepercommander/plugins/ssh/ssh.py

@@ -115,27 +115,27 @@ def rotate_ssh(host, port, user, old_password, new_password, timeout=5, revert=F
                     ia.send('passwd')
                     answer = ia.expect([r'(?i).*current.*password.*', r'(?i).*old.*password.*', r'(?i).*new.*password.*'])
                     result = ia.current_output
-                    logging.debug('Output from passwd command: \"%s\"', result)
+                    logging.debug('Rotation command responded (%d bytes)', len(result))
                     if answer < 0:
                         logging.debug('Unexpected response from the passwd command. Old password is assumed.')
                     if answer < 2:
                         ia.send(old_password)
                         logging.debug('Old Password sent')
                         ia.expect(r'(?i).*new.*password.*')
                         result = ia.current_output
-                        logging.debug('Output from Old Password: \"%s\"', result)
+                        logging.debug('Old credential prompt responded (%d bytes)', len(result))
                     ia.send(new_password)
                     logging.debug('New Password sent')
                     ia.expect(r'(?i).*new.*password.*')
                     result = ia.current_output
-                    logging.debug('Output from New Password: \"%s\"', result)
+                    logging.debug('New credential prompt responded (%d bytes)', len(result))
                     try:
                         ia.send(new_password)
                         logging.debug('New Password Again sent')
                         time.sleep(0.2)
                         ia.expect('.+')
                         result = ia.current_output
-                        logging.debug('Output from New Password Again: \"%s\"', result)
+                        logging.debug('Credential confirmation responded (%d bytes)', len(result))
                         results = []
                         lines = [x for x in result.splitlines() if x]
                         has_prompt = False

keepercommander/service/decorators/logging.py

@@ -115,13 +115,13 @@ def wrapper(*args: Any, **kwargs: Any) -> Any:
             args_repr = [repr(a) for a in args]
             kwargs_repr = [f"{k}={v!r}" for k, v in kwargs.items()]
             signature = ", ".join(args_repr + kwargs_repr)
-            logger.debug(f"Call: {fn.__name__}({signature})")
-        
+            logger.debug(f"Call: {fn.__name__}({sanitize_debug_data(signature)})")
+
         value = fn(*args, **kwargs)
-        
+
         if logger._logger.isEnabledFor(logging.INFO):
-            logger.debug(f"Return: {fn.__name__} → {value!r}")
-        
+            logger.debug(f"Return: {fn.__name__} → {sanitize_debug_data(repr(value))}")
+
         return value
     return wrapper
 
@@ -144,13 +144,19 @@ def sanitize_debug_data(data: str) -> str:
 
     sanitized = data
 
-    # Sanitize common password patterns
+    # Sanitize sensitive data patterns
     patterns = [
         (r'"password"\s*:\s*"[^"]*"', '"password": "***"'),
-        (r'"login"\s*:\s*"[^"]*"', '"login": "***"'),  
+        (r'"login"\s*:\s*"[^"]*"', '"login": "***"'),
         (r'"secret"\s*:\s*"[^"]*"', '"secret": "***"'),
         (r'"token"\s*:\s*"[^"]*"', '"token": "***"'),
         (r'"key"\s*:\s*"[^"]*"', '"key": "***"'),
+        (r'"private_pem_key"\s*:\s*"[^"]*"', '"private_pem_key": "***"'),
+        (r'"device_token"\s*:\s*"[^"]*"', '"device_token": "***"'),
+        (r'"access_token"\s*:\s*"[^"]*"', '"access_token": "***"'),
+        (r'"gateway_token"\s*:\s*"[^"]*"', '"gateway_token": "***"'),
+        (r'"session_token"\s*:\s*"[^"]*"', '"session_token": "***"'),
+        (r'"data_key"\s*:\s*"[^"]*"', '"data_key": "***"'),
         (r'password=[^\s]*', 'password=***'),
         (r'login=[^\s]*', 'login=***'),
         # Sanitize email addresses in logs to protect PII

keepercommander/utils.py

@@ -91,63 +91,70 @@ def generate_aes_key():         # type: () -> bytes
 def set_file_permissions(file_path):     # type: (str) -> None
     """
     Set secure file permissions (600) for configuration files containing sensitive data.
-    This ensures only the owner can read and write the file.
+    Uses fd-based operations on Unix to prevent TOCTOU symlink race conditions.
     """
     file_path = os.path.abspath(file_path)
-    
+
     try:
-        if os.path.islink(file_path):
-            logging.warning(f'Skipping permission setting on symbolic link: {file_path}')
-            return
-
         if platform.system() != 'Windows':
-            file_stat = os.stat(file_path)
-            if file_stat.st_uid != os.getuid():
-                logging.warning(f'Skipping permission setting on file not owned by current user: {file_path}')
+            # Open with O_NOFOLLOW to atomically reject symlinks
+            try:
+                fd = os.open(file_path, os.O_RDONLY | os.O_NOFOLLOW)
+            except OSError:
+                logging.warning(f'Skipping permission setting (symlink or inaccessible): {file_path}')
                 return
-
-            os.chmod(file_path, stat.S_IRUSR | stat.S_IWUSR)
-            logging.debug(f'Set secure permissions (600) for file: {file_path}')
+            try:
+                file_stat = os.fstat(fd)
+                if file_stat.st_uid != os.getuid():
+                    logging.warning(f'Skipping permission setting on file not owned by current user: {file_path}')
+                    return
+                os.fchmod(fd, stat.S_IRUSR | stat.S_IWUSR)
+                logging.debug(f'Set secure permissions (600) for file: {file_path}')
+            finally:
+                os.close(fd)
         else:
             username = os.getlogin()
             subprocess.run(["icacls", file_path, "/inheritance:r"], check=True, capture_output=True)
             subprocess.run(["icacls", file_path, "/remove", "NT AUTHORITY\\SYSTEM", "BUILTIN\\Administrators"], check=False, capture_output=True)
             subprocess.run(["icacls", file_path, "/grant", f"{username}:RW"], check=True, capture_output=True)
             logging.debug(f'Set secure permissions (owner RW only) for Windows file: {file_path}')
-    except Exception:
+    except (OSError, subprocess.SubprocessError):
         logging.warning(f'Failed to set file permissions for {file_path}')
 
 
 def ensure_config_permissions(file_path):     # type: (str) -> None
     """
     Check and fix file permissions for existing configuration files.
-    If the file has overly permissive permissions, log a warning and fix them.
+    Uses fd-based operations on Unix to prevent TOCTOU symlink race conditions.
     """
     file_path = os.path.abspath(file_path)
-
-    if not os.path.exists(file_path):
-        return
-
+
     try:
-        if os.path.islink(file_path):
-            logging.warning(f'Skipping permission check on symbolic link: {file_path}')
-            return
-
         if platform.system() != 'Windows':
-            file_stat = os.stat(file_path)
-            if file_stat.st_uid != os.getuid():
-                logging.warning(f'Skipping permission check on file not owned by current user: {file_path} (owner: {file_stat.st_uid}, current: {os.getuid()})')
+            # Open with O_NOFOLLOW to atomically reject symlinks
+            try:
+                fd = os.open(file_path, os.O_RDONLY | os.O_NOFOLLOW)
+            except OSError:
+                # File doesn't exist or is a symlink — either way, skip
                 return
-
-            current_permissions = file_stat.st_mode & 0o777
-            if current_permissions != 0o600:
-                logging.warning(f'Configuration file {file_path} has insecure permissions '
-                              f'{oct(current_permissions)}. Setting to secure permissions (600).')
-                set_file_permissions(file_path)
+            try:
+                file_stat = os.fstat(fd)
+                if file_stat.st_uid != os.getuid():
+                    logging.warning(f'Skipping permission check on file not owned by current user: {file_path}')
+                    return
+                current_permissions = file_stat.st_mode & 0o777
+                if current_permissions & 0o177:  # any bits beyond owner rw
+                    logging.warning(f'Configuration file {file_path} has insecure permissions '
+                                  f'{oct(current_permissions)}. Setting to secure permissions (600).')
+                    os.fchmod(fd, stat.S_IRUSR | stat.S_IWUSR)
+            finally:
+                os.close(fd)
         else:
+            if not os.path.exists(file_path):
+                return
             logging.debug(f'Checking Windows file permissions for: {file_path}')
             set_file_permissions(file_path)
-            
+
     except OSError:
         logging.warning(f'Failed to check file permissions for {file_path}')