🛡️ Sentinel: [CRITICAL] Fix TOCTOU vulnerability during SSH key creation#75
🛡️ Sentinel: [CRITICAL] Fix TOCTOU vulnerability during SSH key creation#75
Conversation
- Wrapped private key file and directory creation in a subshell using `umask 077` in `tools/setup-ssh-keys.sh`. - Removed explicitly applied `chmod` commands which caused a window where keys were readable by other users. - Logged this security finding in `.jules/sentinel.md`. 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 pull request documents and fixes a Time-of-Check-Time-of-Use (TOCTOU) vulnerability in SSH private key creation. The vulnerability existed because keys were created with the system default umask, then explicitly chmod'd afterward, creating a brief window where permissions could be too open. The fix uses Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.jules/sentinel.md:
- Around line 1-4: Add a top-level H1 heading to .jules/sentinel.md, ensure a
blank line follows the heading, and wrap the body prose lines (the note starting
"2024-05-24 - [CRITICAL] Prevent TOCTOU...") to satisfy line-length rules
(MD013) and duplicate-heading rules (MD041/MD022); keep the existing content but
reflow paragraphs to ~80 chars per line and place the date/title text under the
new H1 as a paragraph separated by a blank line.
In `@tools/setup-ssh-keys.sh`:
- Around line 151-156: The current block uses umask 077 then directly writes op
read output to "$PRIVATE_KEY_FILE", which truncates existing files and doesn't
change pre-existing directory/file permissions; instead, create "$SSH_DIR" with
chmod 700, write the secret to a temporary file inside that directory with umask
077 (or explicitly chmod 600 the temp file), verify op read succeeded, then
atomically replace "$PRIVATE_KEY_FILE" with mv (rename) from the temp file to
preserve safe permissions and avoid TOCTOU; ensure any temp file is removed on
failure and reference the symbols SSH_DIR, PRIVATE_KEY_FILE, op read, umask, mv,
and chmod in the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f8ed795-fafc-4f1c-bc96-d97800423f8a
📒 Files selected for processing (2)
.jules/sentinel.mdtools/setup-ssh-keys.sh
| ## 2024-05-24 - [CRITICAL] Prevent TOCTOU Vulnerabilities During SSH Key Creation | ||
| **Vulnerability:** Private SSH keys were written to disk with the system default umask before `chmod 600` was explicitly applied, creating a brief window where the file might be readable by other users. | ||
| **Learning:** Calling `chmod` after file creation is insufficient to prevent Time-of-Check to Time-of-Use (TOCTOU) vulnerabilities for highly sensitive files like SSH keys. | ||
| **Prevention:** Always enforce strict permissions during file creation by using `umask 077` within a subshell (`(...)`) to securely create the directory (`700`) and the file (`600`) without affecting the parent shell's umask. |
There was a problem hiding this comment.
This new note currently fails markdownlint.
CI is already flagging Line 1 for MD041/MD022 and Lines 2-4 for MD013, so this file needs an H1, a blank line after the heading, and wrapped prose before merge.
📝 Proposed fix
-## 2024-05-24 - [CRITICAL] Prevent TOCTOU Vulnerabilities During SSH Key Creation
-**Vulnerability:** Private SSH keys were written to disk with the system default umask before `chmod 600` was explicitly applied, creating a brief window where the file might be readable by other users.
-**Learning:** Calling `chmod` after file creation is insufficient to prevent Time-of-Check to Time-of-Use (TOCTOU) vulnerabilities for highly sensitive files like SSH keys.
-**Prevention:** Always enforce strict permissions during file creation by using `umask 077` within a subshell (`(...)`) to securely create the directory (`700`) and the file (`600`) without affecting the parent shell's umask.
+# Sentinel findings
+
+## 2024-05-24 - [CRITICAL] Prevent TOCTOU Vulnerabilities During SSH Key Creation
+
+**Vulnerability:** Private SSH keys were written to disk with the system
+default umask before `chmod 600` was explicitly applied, creating a brief
+window where the file might be readable by other users.
+
+**Learning:** Calling `chmod` after file creation is insufficient to prevent
+Time-of-Check to Time-of-Use (TOCTOU) vulnerabilities for highly sensitive
+files like SSH keys.
+
+**Prevention:** Always enforce strict permissions during file creation by using
+`umask 077` within a subshell (`(...)`) to securely create the directory
+(`700`) and the file (`600`) without affecting the parent shell's umask.📝 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-24 - [CRITICAL] Prevent TOCTOU Vulnerabilities During SSH Key Creation | |
| **Vulnerability:** Private SSH keys were written to disk with the system default umask before `chmod 600` was explicitly applied, creating a brief window where the file might be readable by other users. | |
| **Learning:** Calling `chmod` after file creation is insufficient to prevent Time-of-Check to Time-of-Use (TOCTOU) vulnerabilities for highly sensitive files like SSH keys. | |
| **Prevention:** Always enforce strict permissions during file creation by using `umask 077` within a subshell (`(...)`) to securely create the directory (`700`) and the file (`600`) without affecting the parent shell's umask. | |
| # Sentinel findings | |
| ## 2024-05-24 - [CRITICAL] Prevent TOCTOU Vulnerabilities During SSH Key Creation | |
| **Vulnerability:** Private SSH keys were written to disk with the system | |
| default umask before `chmod 600` was explicitly applied, creating a brief | |
| window where the file might be readable by other users. | |
| **Learning:** Calling `chmod` after file creation is insufficient to prevent | |
| Time-of-Check to Time-of-Use (TOCTOU) vulnerabilities for highly sensitive | |
| files like SSH keys. | |
| **Prevention:** Always enforce strict permissions during file creation by using | |
| `umask 077` within a subshell (`(...)`) to securely create the directory | |
| (`700`) and the file (`600`) without affecting the parent shell's umask. |
🧰 Tools
🪛 GitHub Check: Lint Documentation
[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 225] 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: 172] 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: 202] 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-24 - [CRITICAL] Pre..."] 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-24 - [CRITICAL] Prevent TOCTOU Vulnerabilities During SSH Key Creation"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.jules/sentinel.md around lines 1 - 4, Add a top-level H1 heading to
.jules/sentinel.md, ensure a blank line follows the heading, and wrap the body
prose lines (the note starting "2024-05-24 - [CRITICAL] Prevent TOCTOU...") to
satisfy line-length rules (MD013) and duplicate-heading rules (MD041/MD022);
keep the existing content but reflow paragraphs to ~80 chars per line and place
the date/title text under the new H1 as a paragraph separated by a blank line.
| # Create SSH directory and save keys securely to prevent TOCTOU | ||
| ( | ||
| umask 077 | ||
| mkdir -p "$SSH_DIR" | ||
| op read "op://$VAULT/$KEY_NAME/private_key" > "$PRIVATE_KEY_FILE" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In bash/POSIX shells, does umaskonly apply when a file or directory is newly created, and does> redirection preserve the existing mode bits when the target file already exists?
💡 Result:
Yes, umask only applies when a file or directory is newly created. It modifies the permission bits provided to system calls like open(2) with O_CREAT or mkdir(2) by clearing bits set in the umask from the requested mode (typically 0666 for files, 0777 for directories). It does not affect existing files or operations that truncate them. No, > redirection does not preserve the existing mode bits when the target file already exists. The > operator opens the file with O_WRONLY|O_CREAT|O_TRUNC and mode 0666 (masked by umask if creating new), but for existing regular files, O_TRUNC truncates the length to 0 while leaving the mode and owner unchanged. Thus, permissions remain as they were.
Citations:
- 1: https://oneuptime.com/blog/post/2026-03-04-set-default-umask-values-rhel-9/view
- 2: https://en.wikipedia.org/wiki/Umask
- 3: https://www.unix.com/man_page/linux/3posix/umask/
- 4: https://superuser.com/questions/704910/how-to-set-specific-file-permissions-when-redirecting-output
- 5: https://www.gnu.org/software/bash/manual/html_node/Redirections.html
- 6: https://man7.org/linux/man-pages/man2/umask.2.html
- 7: http://pubs.opengroup.org/onlinepubs/9699919799/functions/umask.html
- 8: https://unix.stackexchange.com/questions/202797/does-bash-open-files-in-o-append-when-using-on-linux
- 9: https://pubs.opengroup.org/onlinepubs/9699959299/functions/open.html
umask 077 only protects first-time restores; existing keys and directories remain vulnerable on overwrite.
When the overwrite path (Lines 140–147) is taken, Line 155 truncates the existing private key file in place. Since umask does not affect existing files, an existing key with permissive permissions retains those permissions after truncation. Additionally, if op read fails after the file is already truncated, the working key is destroyed. Stage the private key into a temp file with restrictive umask, then use atomic mv for replacement; also apply chmod 700 to $SSH_DIR to harden pre-existing directories.
🔐 Proposed fix
- (
- umask 077
- mkdir -p "$SSH_DIR"
- op read "op://$VAULT/$KEY_NAME/private_key" > "$PRIVATE_KEY_FILE"
- )
+ (
+ umask 077
+ mkdir -p "$SSH_DIR"
+ chmod 700 "$SSH_DIR"
+ tmp_private_key=$(mktemp "$SSH_DIR/.tmp-private-key.XXXXXX")
+ trap 'rm -f "$tmp_private_key"' EXIT
+ op read "op://$VAULT/$KEY_NAME/private_key" > "$tmp_private_key"
+ mv -f "$tmp_private_key" "$PRIVATE_KEY_FILE"
+ trap - EXIT
+ )📝 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.
| # Create SSH directory and save keys securely to prevent TOCTOU | |
| ( | |
| umask 077 | |
| mkdir -p "$SSH_DIR" | |
| op read "op://$VAULT/$KEY_NAME/private_key" > "$PRIVATE_KEY_FILE" | |
| ) | |
| ( | |
| umask 077 | |
| mkdir -p "$SSH_DIR" | |
| chmod 700 "$SSH_DIR" | |
| tmp_private_key=$(mktemp "$SSH_DIR/.tmp-private-key.XXXXXX") | |
| trap 'rm -f "$tmp_private_key"' EXIT | |
| op read "op://$VAULT/$KEY_NAME/private_key" > "$tmp_private_key" | |
| mv -f "$tmp_private_key" "$PRIVATE_KEY_FILE" | |
| trap - EXIT | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/setup-ssh-keys.sh` around lines 151 - 156, The current block uses umask
077 then directly writes op read output to "$PRIVATE_KEY_FILE", which truncates
existing files and doesn't change pre-existing directory/file permissions;
instead, create "$SSH_DIR" with chmod 700, write the secret to a temporary file
inside that directory with umask 077 (or explicitly chmod 600 the temp file),
verify op read succeeded, then atomically replace "$PRIVATE_KEY_FILE" with mv
(rename) from the temp file to preserve safe permissions and avoid TOCTOU;
ensure any temp file is removed on failure and reference the symbols SSH_DIR,
PRIVATE_KEY_FILE, op read, umask, mv, and chmod in the change.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Private SSH keys in
tools/setup-ssh-keys.shwere written to disk with the system's default umask, meaning the file was created with potentially readable permissions for a short time beforechmod 600was executed. This causes a Time-of-Check to Time-of-Use (TOCTOU) vulnerability where an attacker on the same machine could briefly read the secret file.🎯 Impact: Local privilege escalation or unauthorized access via leaked SSH keys if an attacker continuously monitors file creation in the user's
~/.sshdirectory.🔧 Fix: Wrapped the directory and file creation logic in a subshell
(...)and explicitly enforcedumask 077. This ensures the directory is natively created with700permissions and the SSH key is natively created with600permissions, entirely eliminating the readable window. Removed the now-redundantchmodcommands.✅ Verification: Verified fix manually by reviewing bash syntax to ensure the
umaskonly applied securely to the subshell without affecting subsequent script execution (like the public key which requires644). Ran./build.sh lintand./build.sh syntaxto confirm no regressions. Added finding to.jules/sentinel.md.PR created automatically by Jules for task 10007967055272675910 started by @kidchenko
Summary by CodeRabbit
Bug Fixes
Documentation