Skip to content

fix: harden JSON parsing and path handling in PAM extend#1891

Open
jlima8900 wants to merge 1 commit intoKeeper-Security:releasefrom
jlima8900:fix/pam-extend-security-hardening
Open

fix: harden JSON parsing and path handling in PAM extend#1891
jlima8900 wants to merge 1 commit intoKeeper-Security:releasefrom
jlima8900:fix/pam-extend-security-hardening

Conversation

@jlima8900
Copy link

Summary

  • Strip NUL bytes from folder paths before placeholder substitution
  • Replace bare except on JSON parse with specific exception handlers
  • Canonicalize import file path with os.path.realpath()

Test plan

  • Tested on Python 3.7 and 3.12

- Strip NUL bytes from folder paths before placeholder substitution
  to prevent path segment manipulation via crafted JSON input
- Replace bare except on JSON parse with specific handlers for
  JSONDecodeError (shows parse error) and PermissionError/OSError
- Canonicalize import file path with os.path.realpath() to resolve
  symlinks before opening
@aaunario-keeper
Copy link
Contributor

aaunario-keeper commented Mar 24, 2026

Suggestions to consider:

If there's a defined "safe directory" for import files (not sure if there is, but that would be a good hardening improvement), add a path confinement check after realpath.
Optionally add data = {} before the try block to satisfy static analysis tools, even though it's not strictly required at runtime.

@jlima8900
Copy link
Author

Thanks for the review! Two thoughts:

Path confinement: Commander is a CLI tool where users explicitly pass file paths as arguments — adding a safe directory restriction would limit usability since users should be able to point at any path on their filesystem. The realpath() call already neutralizes symlink traversal and ../ manipulation, which is the appropriate level of hardening for a CLI context. Path confinement is more suited to web services where input comes from untrusted sources.

data = {} before try: In the refactored version, every path either succeeds (assigns data) or raises CommandError — there's no code path where data is unbound. Adding the pre-assignment would imply a silent fallback that no longer exists, which could be more confusing than helpful.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants