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

Open

jlima8900 wants to merge 5 commits intoKeeper-Security:masterfrom

jlima8900:fix/security-audit-findings-pr1873

jlima8900 commented Mar 28, 2026

Summary

Two validated findings from a security audit of the Commander codebase (445 Python files, 10 categories, 3-pass validation).

KC-2026-001: AWS credentials file permissions

The awskey rotation command writes credentials to ~/.aws/credentials without restricting file permissions, leaving them world-readable by default.

KC-2026-002: SSH connection logging

SSH connection handler logs contain sensitive session data that should be redacted.

Test plan

Verified file permissions set to 600 on AWS credentials
Verified sensitive data removed from SSH logs

jlima8900 added 5 commits

March 23, 2026 15:59


          fix: set secure file permissions and sanitize SSH log output

02e8131

- AWS credentials file: use os.open() with 0o600 at creation time
  to eliminate TOCTOU race window (was open()+chmod())
- SSH plugin: remove sensitive data from debug log messages


          fix: eliminate TOCTOU race in backup file creation

bc24c66

Address PR Keeper-Security#1873 review from @aaunario-keeper: use restrictive umask
around shutil.copy2 for the backup file so it's never world-readable,
even briefly. Removes the post-hoc os.chmod call.


          fix: eliminate TOCTOU race in AWS credentials file write

e367b0a

Address review feedback:
- Use os.open() with O_CREAT | 0o600 to set permissions at file creation
- Removes race window where file existed with default umask permissions
- Remove now-redundant os.chmod() call


          fix: narrow exception catches and add structured JSON error details

b60ee7a

Address review feedback:
- Narrow json.loads catches to JSONDecodeError only (drop unreachable ValueError/TypeError/KeyError)
- Add e.msg, e.lineno, e.colno to log messages for better debuggability
- Split import_schedule_field handler into JSONDecodeError vs (ValueError, KeyError)


          fix: use os.fdopen + fchmod to eliminate double-close bug and harden …

82d99f3

…existing files

- Replace open(fd, 'w') with os.fdopen(fd, 'w') to prevent double-close
  on write failure (with-block closes fd, then bare except also called
  os.close(fd))
- Add os.fchmod(fd, 0o600) so existing files also get restrictive
  permissions (os.open mode only applies on creation)
- Remove bare except clause
- Add 19 tests covering file permissions, log sanitization, and
  exception narrowing

github-advanced-security bot found potential problems

View reviewed changes

keepercommander/commands/pam_import/base.py

    
                      obj = cls()

                      try: data = json.loads(data) if isinstance(data, str) else data

                      except: logging.error(f"Pam Scripts failed to load from: {str(data)[:80]}...")

                      except json.JSONDecodeError as e: logging.error(f"Pam Scripts: invalid JSON at line {e.lineno}, col {e.colno} — {e.msg}. Input (truncated): {str(data)[:80]}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs

as clear text.

keepercommander/commands/pam_import/base.py

@@ -463,7 +463,7 @@ def load(cls, data: Union[str, dict]) -> PamRotationScheduleObject:

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs

as clear text.

keepercommander/commands/pam_import/base.py

@@ -478,7 +478,11 @@ def load(cls, data: Union[str, dict]) -> PamRotationScheduleObject:

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs

as clear text.

keepercommander/commands/pam_import/base.py

@@ -517,7 +521,7 @@ def load(cls, data: Optional[Union[str, dict]], rotation_params: Optional[PamRot

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs

as clear text.
This expression logs

as clear text.

keepercommander/commands/pam_import/base.py

		@@ -590,7 +594,7 @@ def __init__(self):
		def load(cls, data: Union[str, dict]):

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs

as clear text.

keepercommander/commands/pam_import/base.py

@@ @@ -1665,7 +1669,7 @@ def __init__(self): @@
                   def load(cls, data: Union[str, dict], rotation_params: Optional[PamRotationParams] = None):
                       obj = cls()
                       try: data = json.loads(data) if isinstance(data, str) else data

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs

as clear text.

keepercommander/commands/pam_import/base.py

                   def load(cls, data: Union[str, dict]):
                       obj = cls()
                       try: data = json.loads(data) if isinstance(data, str) else data

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs

as clear text.

keepercommander/commands/pam_import/base.py

                   def load(cls, data: Union[str, dict]):
                       obj = cls()
                       try: data = json.loads(data) if isinstance(data, str) else data

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs

as clear text.

keepercommander/commands/pam_import/base.py

@@ @@ -1896,7 +1900,7 @@ def __init__( @@
                   def load(cls, data: Union[str, dict]):
                       obj = cls()
                       try: data = json.loads(data) if isinstance(data, str) else data

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs

as clear text.

keepercommander/commands/pam_import/base.py Outdated

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs

as clear text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet