Conversation
Summary of ChangesHello @MikeeBuilds, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the existing permissions scanner by adding a critical check for SSH private key file permissions. The new check, CHK-PRM-013, identifies SSH keys with overly permissive settings, which are a significant security vulnerability and can cause SSH clients to refuse their use. The changes include the core scanning logic, comprehensive documentation, and dedicated integration tests to ensure reliability. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an important security check for SSH private key permissions. While the implementation in scan_permissions.sh and the documentation in check-catalog.md are well-done, a critical security issue was identified: the suggested auto-fix command is vulnerable to command injection due to improper escaping of filenames that might contain single quotes. It is recommended to use printf %q for safe handling of special characters. Additionally, consider improving code quality in scripts/helpers/test_integration.sh by reducing duplication.
scripts/scan_permissions.sh
Outdated
| "Run: chmod 600 '$f'" \ | ||
| "chmod 600 '$f'" |
There was a problem hiding this comment.
The auto_fix command and the remediation message are constructed by wrapping the filename $f in single quotes without escaping any single quotes that might be present in the filename itself. If a filename contains a single quote (e.g., id_rsa';touch /tmp/pwned;'), it can break out of the single-quoted string and execute arbitrary commands when the suggested auto_fix is run by a user. This is a command injection vulnerability. While the risk is mitigated by the fact that ~/.ssh is typically only writable by the user, this insecure pattern should be avoided, especially as it is used throughout the script for other directories that might be more accessible.
| "Run: chmod 600 '$f'" \ | |
| "chmod 600 '$f'" | |
| "Run: $(printf 'chmod 600 %q' "$f")" \ | |
| "$(printf 'chmod 600 %q' "$f")" |
| test_prm_013() { | ||
| # Create mock SSH directory | ||
| local ssh_dir="${TEST_DIR}/.ssh" | ||
| mkdir -p "$ssh_dir" | ||
|
|
||
| # Create test SSH private keys with wrong permissions | ||
| local test_key="${ssh_dir}/id_test_rsa" | ||
| local test_pem="${ssh_dir}/test.pem" | ||
|
|
||
| echo "-----BEGIN RSA PRIVATE KEY-----" > "$test_key" | ||
| echo "fake key content" >> "$test_key" | ||
| echo "-----END RSA PRIVATE KEY-----" >> "$test_key" | ||
|
|
||
| echo "-----BEGIN PRIVATE KEY-----" > "$test_pem" | ||
| echo "fake pem content" >> "$test_pem" | ||
| echo "-----END PRIVATE KEY-----" >> "$test_pem" | ||
|
|
||
| # Set insecure permissions | ||
| chmod 644 "$test_key" | ||
| chmod 644 "$test_pem" | ||
|
|
||
| # Run the auto-fix command | ||
| chmod 600 "$test_key" | ||
| chmod 600 "$test_pem" | ||
|
|
||
| # Verify permissions were fixed | ||
| local perms_key perms_pem | ||
| if [[ "$(uname -s)" == "Darwin" ]]; then | ||
| perms_key=$(stat -f "%Lp" "$test_key") | ||
| perms_pem=$(stat -f "%Lp" "$test_pem") | ||
| else | ||
| perms_key=$(stat -c "%a" "$test_key") | ||
| perms_pem=$(stat -c "%a" "$test_pem") | ||
| fi | ||
|
|
||
| [[ "$perms_key" == "600" ]] && [[ "$perms_pem" == "600" ]] | ||
| } |
There was a problem hiding this comment.
The new test function test_prm_013 contains repeated logic for handling the two separate test key files. The file creation, permission changes, and verification steps are duplicated. To improve readability and maintainability, this can be refactored to use an array and a loop. This approach makes the test cleaner and easier to extend if you need to add more test cases in the future.
test_prm_013() {
# Create mock SSH directory
local ssh_dir="${TEST_DIR}/.ssh"
mkdir -p "$ssh_dir"
# Create test SSH private keys
local test_key="${ssh_dir}/id_test_rsa"
local test_pem="${ssh_dir}/test.pem"
cat > "$test_key" <<'EOF'
-----BEGIN RSA PRIVATE KEY-----
fake key content
-----END RSA PRIVATE KEY-----
EOF
cat > "$test_pem" <<'EOF'
-----BEGIN PRIVATE KEY-----
fake pem content
-----END PRIVATE KEY-----
EOF
for key_file in "$test_key" "$test_pem"; do
# Set insecure permissions
chmod 644 "$key_file"
# Run the auto-fix command
chmod 600 "$key_file"
# Verify permissions were fixed
local perms
if [[ "$(uname -s)" == "Darwin" ]]; then
perms=$(stat -f "%Lp" "$key_file")
else
perms=$(stat -c "%a" "$key_file")
fi
if [[ "$perms" != "600" ]]; then
return 1 # Fail test if any key has wrong permissions
fi
done
}
Greptile OverviewGreptile SummaryThis PR extends the existing permissions scanner to detect SSH private key files under No new must-fix issues were identified in the latest head SHA beyond the previously discussed auto-fix quoting/safe-exec mismatch thread (already noted in prior review comments). Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User/Shell
participant SP as scan_permissions.sh
participant FS as Filesystem (~/.ssh)
participant R as Report/Output
U->>SP: Run permissions scan
SP->>FS: Enumerate candidate key files (id_*, *.pem)
FS-->>SP: Return existing file paths
SP->>FS: Read file modes (stat)
FS-->>SP: Return mode bits
alt mode != 600
SP->>R: Emit finding (CHK-PRM-013) + remediation/auto-fix
else mode == 600
SP->>R: No finding for file
end
R-->>U: Display results
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
scripts/scan_permissions.sh
Outdated
| "SSH private keys must be chmod 600 or SSH clients will refuse to use them. Current: $perms" \ | ||
| "$f mode $perms" \ | ||
| "Run: chmod 600 '$f'" \ | ||
| "chmod 600 '$f'" |
There was a problem hiding this comment.
auto-fix command contains single quotes around $f which will be rejected by safe_exec_command() whitelist (pattern: ^chmod [0-7]{3,4} [a-zA-Z0-9/._-]+$ doesn't allow quotes). This is a pre-existing issue affecting all chmod-based auto-fixes in the codebase. Consider either:
- removing quotes:
"chmod 600 $f"(works for paths without spaces) - updating safe_exec whitelist to support quoted paths
| "chmod 600 '$f'" | |
| "chmod 600 $f" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/scan_permissions.sh
Line: 677:677
Comment:
auto-fix command contains single quotes around `$f` which will be rejected by `safe_exec_command()` whitelist (pattern: `^chmod [0-7]{3,4} [a-zA-Z0-9/._-]+$` doesn't allow quotes). This is a pre-existing issue affecting all chmod-based auto-fixes in the codebase. Consider either:
1. removing quotes: `"chmod 600 $f"` (works for paths without spaces)
2. updating safe_exec whitelist to support quoted paths
```suggestion
"chmod 600 $f"
```
How can I resolve this? If you propose a fix, please make it concise.
…issions. Implements CHK-PRM-013 to check SSH private key file permissions. Scans ~/.ssh for id_* files and *.pem files, ensuring they have 600 permissions. Follows existing patterns from check_openclaw_json and check_wallet_files. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Added test_prm_013() function to test_integration.sh - Test creates mock SSH keys with wrong permissions (644) - Verifies auto-fix command (chmod 600) works correctly - All 13 integration tests passing - Manual verification completed: critical → ok severity transition confirmed Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
MikeeBuilds
force-pushed
the
auto-claude/011-add-ssh-key-permissions-check-to-scan-permissions-
branch
from
9d90b61 to
f1c99f7
Compare
…actor test to loop Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All review feedback has been addressed:
@gemini-code-assist @greptile-apps — requesting re-review. Thanks! |
There was a problem hiding this comment.
Code Review
This pull request introduces a new security check for SSH private key permissions, including documentation and an integration test. While the check correctly identifies overly permissive files, a critical command injection vulnerability exists in the auto-fix command generation. This is due to unquoted filenames, which could allow malicious filenames to execute arbitrary commands and also cause the auto-fix to fail for filenames containing spaces.
scripts/scan_permissions.sh
Outdated
| "Run: chmod 600 $f" \ | ||
| "chmod 600 $f" |
There was a problem hiding this comment.
The $f variable in the remediation and auto_fix command strings is not quoted. This creates a critical command injection vulnerability, allowing malicious filenames with shell metacharacters to execute arbitrary commands when the remediation is applied. Furthermore, this omission will cause the auto-fix command to fail for filenames containing spaces. Please ensure $f is properly quoted, consistent with other checks in this file, to prevent both command injection and functional failures.
| "Run: chmod 600 $f" \ | |
| "chmod 600 $f" | |
| "Run: chmod 600 '$f'" \ | |
| "chmod 600 '$f'" |
Instead of quoting $f in the auto_fix string (which would break the safe_exec whitelist pattern [a-zA-Z0-9/._-]+), validate that the filename contains only safe characters before emitting an auto_fix command. Files with unsafe characters (spaces, quotes, metacharacters) still get the critical finding but require manual remediation. Standard SSH key names (id_rsa, id_ed25519, *.pem) always pass this check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Round 3 review fix appliedAddressed: The Approach: Rather than adding single quotes around
This ensures no command injection is possible regardless of filename content, while maintaining compatibility with the existing @gemini-code-assist @greptile-apps — requesting re-review. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new security check, CHK-PRM-013, to ensure SSH private keys have the correct 600 permissions. The scanner implementation is robust, including good security practices like validating filenames before generating auto-fix commands. However, a critical command injection vulnerability exists in the remediation message: if a malicious filename (e.g., containing shell metacharacters) is present in the ~/.ssh directory, the generated remediation command, if copy-pasted, could execute arbitrary code. To address this and other areas, suggestions have been provided to safely handle filenames by only offering copy-pasteable commands for validated, safe paths, improve the test's effectiveness, and make the file detection logic more resilient.
scripts/scan_permissions.sh
Outdated
| local auto_fix_cmd="" remediation_msg="Run: chmod 600 \"$f\" (manual)" | ||
| if [[ "$f" =~ ^[a-zA-Z0-9/._-]+$ ]]; then | ||
| auto_fix_cmd="chmod 600 $f" | ||
| remediation_msg="Run: chmod 600 $f" | ||
| fi |
There was a problem hiding this comment.
The remediation message is constructed using double quotes around the filename, which allows for command substitution if the filename contains characters like $(...) or `...`. While there is a regex check to provide a 'clean' command, the fallback message (line 677) is still vulnerable if a user copy-pastes it into their terminal. An attacker who can create a file with a malicious name in the ~/.ssh directory could achieve arbitrary command execution when the user attempts to fix the permissions.
To remediate this, avoid providing a copy-pasteable command for filenames that do not pass the safety check, or use printf %q to safely escape the filename for the shell.
| local auto_fix_cmd="" remediation_msg="Run: chmod 600 \"$f\" (manual)" | |
| if [[ "$f" =~ ^[a-zA-Z0-9/._-]+$ ]]; then | |
| auto_fix_cmd="chmod 600 $f" | |
| remediation_msg="Run: chmod 600 $f" | |
| fi | |
| local auto_fix_cmd="" remediation_msg="Manual fix required: chmod 600 [file]" | |
| if [[ "$f" =~ ^[a-zA-Z0-9/._-]+$ ]]; then | |
| auto_fix_cmd="chmod 600 $f" | |
| remediation_msg="Run: chmod 600 $f" | |
| fi |
| test_prm_013() { | ||
| # Create mock SSH directory | ||
| local ssh_dir="${TEST_DIR}/.ssh" | ||
| mkdir -p "$ssh_dir" | ||
|
|
||
| # Create test SSH private keys | ||
| local test_key="${ssh_dir}/id_test_rsa" | ||
| local test_pem="${ssh_dir}/test.pem" | ||
|
|
||
| cat > "$test_key" <<'EOF' | ||
| -----BEGIN RSA PRIVATE KEY----- | ||
| fake key content | ||
| -----END RSA PRIVATE KEY----- | ||
| EOF | ||
|
|
||
| cat > "$test_pem" <<'EOF' | ||
| -----BEGIN PRIVATE KEY----- | ||
| fake pem content | ||
| -----END PRIVATE KEY----- | ||
| EOF | ||
|
|
||
| for key_file in "$test_key" "$test_pem"; do | ||
| # Set insecure permissions | ||
| chmod 644 "$key_file" | ||
|
|
||
| # Run the auto-fix command | ||
| chmod 600 "$key_file" | ||
|
|
||
| # Verify permissions were fixed | ||
| local perms | ||
| if [[ "$(uname -s)" == "Darwin" ]]; then | ||
| perms=$(stat -f "%Lp" "$key_file") | ||
| else | ||
| perms=$(stat -c "%a" "$key_file") | ||
| fi | ||
|
|
||
| if [[ "$perms" != "600" ]]; then | ||
| return 1 | ||
| fi | ||
| done | ||
| } |
There was a problem hiding this comment.
This test verifies that chmod 600 sets permissions correctly, but it doesn't actually test the scanner's detection or auto-fix generation logic. It's more of a unit test for the chmod command than an integration test for the check.
A more effective integration test would:
- Create a mock key file with incorrect permissions.
- Execute the
scan_permissions.shscript withHOMEpointing to a test directory. - Capture the JSON output and verify that the correct finding and
auto_fixcommand are generated.
While this test is consistent with the existing style in this file, consider enhancing the test strategy to provide stronger guarantees about the scanner's behavior.
scripts/scan_permissions.sh
Outdated
| -name 'id_*' -o -name '*.pem' \ | ||
| \) -type f ! -name '*.pub' -print0 2>/dev/null) |
There was a problem hiding this comment.
The find command uses the case-sensitive -name predicate. This means it won't detect key files with names like id_rsa.PEM or ID_RSA on case-sensitive filesystems (the default on Linux). Using the case-insensitive -iname predicate for both the key files and the public key exclusion would make the check more robust.
| -name 'id_*' -o -name '*.pem' \ | |
| \) -type f ! -name '*.pub' -print0 2>/dev/null) | |
| -iname 'id_*' -o -iname '*.pem' \ | |
| \) -type f ! -iname '*.pub' -print0 2>/dev/null) |
…tive find - Replace double-quoted filename in fallback remediation message with static placeholder "[file]" to prevent command substitution when the filename fails the safety regex (security-medium). - Switch find -name to -iname for key file matching and .pub exclusion so that ID_RSA, id_rsa.PEM, etc. are all detected (medium). - Add NOTE comment to test_prm_013 acknowledging the test covers auto-fix only, not scanner detection logic (suggestion). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All three review comments have been addressed in commit 4915d71:
cc @greptile-apps @gemini-code-assist — ready for re-review. |
|
Thank you for the update, @MikeeBuilds! I appreciate you addressing the feedback from the previous review rounds, specifically the points regarding I will proceed with a re-review of the pull request to verify these changes and ensure everything is in order. I will provide my findings in a new review comment shortly. |
1 participant
Extend the existing permissions scanner to check SSH private key files (~/.ssh/id_, ~/.ssh/.pem) for proper 600 permissions. SSH keys with overly permissive permissions are a security risk and ssh clients often refuse to use them.