🛡️ Sentinel: [CRITICAL] Fix SSH private key creation race condition #17
Conversation
Identified and fixed a race condition in `tools/setup-ssh-keys.sh` where the private key file was created with default permissions (potentially world-readable) before being restricted with `chmod 600`. Changes: - Wrapped sensitive file creation in `umask 077` block to ensure files are created with 600 permissions immediately. - Added `tests/verify_ssh_permissions.sh` to verify that the script restores keys with correct permissions. - Updated `.jules/sentinel.md` with the security learning. This eliminates the window of exposure for private keys during restoration. Co-authored-by: kidchenko <5432753+kidchenko@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThis change addresses a TOCTOU vulnerability in SSH private key creation where keys were temporarily world-readable due to shell umask inheritance. The fix implements safe umask handling in setup-ssh-keys.sh, adds a test to verify correct permissions, and documents the vulnerability discovered. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.jules/sentinel.md:
- Around line 1-4: Update .jules/sentinel.md to satisfy markdownlint: make the
top heading an H1 (prepend "# " to the first line) and ensure there's a blank
line after it, wrap the following lines (the SSH Private Key Race Condition
paragraph, Vulnerability, Learning, and Prevention lines) to <=80 characters per
line, and change "afterwards" to US spelling "afterward"; keep the same content
and references to tools/setup-ssh-keys.sh, umask, and chmod 600 so the context
remains clear.
🧹 Nitpick comments (2)
tools/setup-ssh-keys.sh (1)
159-171: Restore umask and clean partial key onop readfailure.
op read ... > "$PRIVATE_KEY_FILE"creates/truncates the file before the command runs. Ifop readfails, an empty private key can be left behind and the umask may stay changed until exit. Consider explicit error handling and cleanup.♻️ Suggested fix
local old_umask old_umask=$(umask) umask 077 # Read private key from 1Password and save locally # umask 077 ensures file is created with 600 permissions - op read "op://$VAULT/$KEY_NAME/private_key" > "$PRIVATE_KEY_FILE" + if ! op read "op://$VAULT/$KEY_NAME/private_key" > "$PRIVATE_KEY_FILE"; then + umask "$old_umask" + rm -f "$PRIVATE_KEY_FILE" + error "Failed to read private key from 1Password" + fi # Redundant chmod just to be sure, but file was created securely chmod 600 "$PRIVATE_KEY_FILE" # Restore umask for public key (we want 644) umask "$old_umask"tests/verify_ssh_permissions.sh (1)
10-15: Ensure temp artifacts are cleaned up on failures.With
set -e, any failure exits before the final cleanup, leaving temp dirs behind. Add a trap so cleanup always runs.🧹 Suggested fix
# Clean up previous run rm -rf "$TEMP_HOME" rm -rf "$MOCK_BIN" mkdir -p "$TEMP_HOME" mkdir -p "$MOCK_BIN" + +cleanup() { + rm -rf "$TEMP_HOME" + rm -rf "$MOCK_BIN" +} +trap cleanup EXIT @@ -# Cleanup -rm -rf "$TEMP_HOME" -rm -rf "$MOCK_BIN"Also applies to: 80-82
| ## 2024-05-22 - [SSH Private Key Race Condition] | ||
| **Vulnerability:** Found a TOCTOU (Time-of-Check to Time-of-Use) race condition in `tools/setup-ssh-keys.sh`. The script created the private key file using output redirection (`>`) which uses the default umask (typically 022), resulting in the file being world-readable for a brief window before `chmod 600` was executed. | ||
| **Learning:** Shell redirection and standard file creation tools respect the current `umask`. Relying on a subsequent `chmod` to secure sensitive files leaves a window of exposure. | ||
| **Prevention:** Always set `umask 077` (or similar restrictive mask) *before* creating sensitive files in shell scripts to ensure they are born secure. Restore the original umask afterwards if necessary. |
There was a problem hiding this comment.
Fix markdownlint failures (MD041/MD022/MD013) and US spelling.
Line 1 must be an H1 with blank lines, and Lines 2–4 exceed the 80‑char limit; the linter will block the PR. Also prefer “afterward” for US English.
✅ Suggested fix
-## 2024-05-22 - [SSH Private Key Race Condition]
-**Vulnerability:** Found a TOCTOU (Time-of-Check to Time-of-Use) race condition in `tools/setup-ssh-keys.sh`. The script created the private key file using output redirection (`>`) which uses the default umask (typically 022), resulting in the file being world-readable for a brief window before `chmod 600` was executed.
-**Learning:** Shell redirection and standard file creation tools respect the current `umask`. Relying on a subsequent `chmod` to secure sensitive files leaves a window of exposure.
-**Prevention:** Always set `umask 077` (or similar restrictive mask) *before* creating sensitive files in shell scripts to ensure they are born secure. Restore the original umask afterwards if necessary.
+# 2024-05-22 - [SSH Private Key Race Condition]
+
+**Vulnerability:** Found a TOCTOU (Time-of-Check to Time-of-Use) race condition in
+`tools/setup-ssh-keys.sh`. The script created the private key file using output
+redirection (`>`), which uses the default umask (typically 022), resulting in the
+file being world-readable for a brief window before `chmod 600` was executed.
+
+**Learning:** Shell redirection and standard file creation tools respect the
+current `umask`. Relying on a subsequent `chmod` to secure sensitive files leaves
+a window of exposure.
+
+**Prevention:** Always set `umask 077` (or similar restrictive mask) *before*
+creating sensitive files in shell scripts to ensure they are born secure. Restore
+the original umask afterward if necessary.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 2024-05-22 - [SSH Private Key Race Condition] | |
| **Vulnerability:** Found a TOCTOU (Time-of-Check to Time-of-Use) race condition in `tools/setup-ssh-keys.sh`. The script created the private key file using output redirection (`>`) which uses the default umask (typically 022), resulting in the file being world-readable for a brief window before `chmod 600` was executed. | |
| **Learning:** Shell redirection and standard file creation tools respect the current `umask`. Relying on a subsequent `chmod` to secure sensitive files leaves a window of exposure. | |
| **Prevention:** Always set `umask 077` (or similar restrictive mask) *before* creating sensitive files in shell scripts to ensure they are born secure. Restore the original umask afterwards if necessary. | |
| # 2024-05-22 - [SSH Private Key Race Condition] | |
| **Vulnerability:** Found a TOCTOU (Time-of-Check to Time-of-Use) race condition in | |
| `tools/setup-ssh-keys.sh`. The script created the private key file using output | |
| redirection (`>`), which uses the default umask (typically 022), resulting in the | |
| file being world-readable for a brief window before `chmod 600` was executed. | |
| **Learning:** Shell redirection and standard file creation tools respect the | |
| current `umask`. Relying on a subsequent `chmod` to secure sensitive files leaves | |
| a window of exposure. | |
| **Prevention:** Always set `umask 077` (or similar restrictive mask) *before* | |
| creating sensitive files in shell scripts to ensure they are born secure. Restore | |
| the original umask afterward if necessary. |
🧰 Tools
🪛 GitHub Check: Lint Documentation
[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 203] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
[failure] 3-3: Line length
.jules/sentinel.md:3:81 MD013/line-length Line length [Expected: 80; Actual: 180] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
[failure] 2-2: Line length
.jules/sentinel.md:2:81 MD013/line-length Line length [Expected: 80; Actual: 321] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
[failure] 1-1: First line in a file should be a top-level heading
.jules/sentinel.md:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "## 2024-05-22 - [SSH Private K..."] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md041.md
[failure] 1-1: Headings should be surrounded by blank lines
.jules/sentinel.md:1 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "## 2024-05-22 - [SSH Private Key Race Condition]"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
🪛 LanguageTool
[locale-violation] ~4-~4: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...born secure. Restore the original umask afterwards if necessary.
(AFTERWARDS_US)
🤖 Prompt for AI Agents
In @.jules/sentinel.md around lines 1 - 4, Update .jules/sentinel.md to satisfy
markdownlint: make the top heading an H1 (prepend "# " to the first line) and
ensure there's a blank line after it, wrap the following lines (the SSH Private
Key Race Condition paragraph, Vulnerability, Learning, and Prevention lines) to
<=80 characters per line, and change "afterwards" to US spelling "afterward";
keep the same content and references to tools/setup-ssh-keys.sh, umask, and
chmod 600 so the context remains clear.
1 participant
🛡️ Sentinel: [CRITICAL] Fix SSH private key creation race condition
🚨 Severity: CRITICAL
💡 Vulnerability: TOCTOU race condition in
tools/setup-ssh-keys.shallowed private keys to be world-readable between creation and permission hardening.🎯 Impact: On multi-user systems or if a malicious process is running, the private key could be read during the brief window before
chmod 600was executed.🔧 Fix: Used
umask 077to ensure the file is created with secure permissions (0600) from the start.✅ Verification: Added
tests/verify_ssh_permissions.shwhich mocksopand verifies that the restored keys have the correct permissions. Confirmed manually thatumaskaffects the file creation.PR created automatically by Jules for task 6068129369537875415 started by @kidchenko
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.