🛡️ Sentinel: [CRITICAL] Fix insecure SSH key creation permissions#28
🛡️ Sentinel: [CRITICAL] Fix insecure SSH key creation permissions#28
Conversation
- Fixed a TOCTOU race condition in `tools/setup-ssh-keys.sh` where private keys were created with default permissions (0664/0644) before being restricted to 0600. - Wrapped the file creation in a subshell with `umask 077` to ensure atomic secure creation. - Added `tests/reproduce_issue.sh` as a regression test to verify secure file creation permissions. - Added security journal entry 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 addresses an SSH key file permissions vulnerability where keys were briefly created with insecure permissions due to default umask. The fix wraps private key file creation in a subshell with strict umask (077) to ensure atomic, secure creation. Documentation and a test script verify the vulnerability and its resolution. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 2
🤖 Fix all issues with AI agents
In @.jules/sentinel.md:
- Around line 1-4: Update the Sentinel markdown: change the date string
"2024-02-12" to "2026-02-12" in the entry header, add a top-level H1 (e.g., "#
Sentinel") above the existing "## 2026-02-12 - Insecure SSH Key Creation
(TOCTOU)" and insert a blank line after that H1 to satisfy MD041 and MD022, and
reflow or convert the long lines in the vulnerability/learning/prevention
paragraphs into wrapped lines or a short bullet list so no line (the text under
the "Vulnerability:", "Learning:", and "Prevention:" sections) exceeds 80
characters to satisfy MD013. Ensure the content (TOCTOU description, umask
example "(umask 077; cmd > file)", and chmod note) remains intact while
wrapping.
In `@tests/reproduce_issue.sh`:
- Line 53: The PERMS assignment uses GNU-only stat syntax (PERMS=$(stat -c "%a"
"$KEY_FILE")), which breaks on macOS; add a small portable helper (e.g.,
get_file_perms) that detects the platform via uname -s (or checks which stat
variant works) and calls stat -c "%a" for Linux/GNU or stat -f "%Lp" for Darwin,
then replace the direct PERMS=... call with PERMS=$(get_file_perms "$KEY_FILE")
referencing KEY_FILE so the script works on both Linux and macOS.
🧹 Nitpick comments (2)
tests/reproduce_issue.sh (2)
5-8: No cleanup of temp directory on failure.If the test fails or is interrupted,
tests/tmpis left behind. Atrapensures cleanup in all exit paths.Suggested improvement
TEST_DIR="$PWD/tests/tmp" rm -rf "$TEST_DIR" mkdir -p "$TEST_DIR/home" mkdir -p "$TEST_DIR/bin" +trap 'rm -rf "$TEST_DIR"' EXIT
23-28: Mockingchmodis clever but has a subtle gap.The mock
chmodintercepts allchmodcalls during script execution, includingchmod 700 "$SSH_DIR"andchmod 644 "$PUBLIC_KEY_FILE". This means the.sshdirectory won't have700permissions either, which is fine for this specific test's purpose but worth a comment in the script for future maintainers.Also,
$@should be quoted as"$@"on line 27 to avoid word-splitting issues (though it's cosmetic here since it's just an echo).-echo "MOCK CHMOD: $@" +echo "MOCK CHMOD: $*"
| ## 2024-02-12 - Insecure SSH Key Creation (TOCTOU) | ||
| **Vulnerability:** `tools/setup-ssh-keys.sh` created sensitive SSH private keys with default permissions (usually 664/644) before restricting them with `chmod 600`. This created a race condition (TOCTOU) where the key was briefly readable by other users. | ||
| **Learning:** Standard shell redirection (`> file`) uses the default umask (typically 022 or 002), resulting in world-readable files for a split second. Relying on a subsequent `chmod` is insecure for sensitive data. | ||
| **Prevention:** Always wrap sensitive file creation commands in a subshell with `umask 077` (e.g., `(umask 077; cmd > file)`). This ensures the file is created with 600 permissions atomically. |
There was a problem hiding this comment.
Fix markdown lint failures and incorrect date.
-
Wrong year: Line 1 says
2024-02-12but this PR is from 2026. Should be2026-02-12. -
Lint failures (from the
Lint Documentationcheck):- MD041: First line should be a top-level heading (
#). Add a# Sentinel(or similar) H1 heading before the##. - MD022: Add a blank line after the heading.
- MD013: Lines 2–4 exceed the 80-character limit. Wrap them or restructure as bullet lists with shorter lines.
- MD041: First line should be a top-level heading (
Proposed fix
-## 2024-02-12 - Insecure SSH Key Creation (TOCTOU)
-**Vulnerability:** `tools/setup-ssh-keys.sh` created sensitive SSH private keys with default permissions (usually 664/644) before restricting them with `chmod 600`. This created a race condition (TOCTOU) where the key was briefly readable by other users.
-**Learning:** Standard shell redirection (`> file`) uses the default umask (typically 022 or 002), resulting in world-readable files for a split second. Relying on a subsequent `chmod` is insecure for sensitive data.
-**Prevention:** Always wrap sensitive file creation commands in a subshell with `umask 077` (e.g., `(umask 077; cmd > file)`). This ensures the file is created with 600 permissions atomically.
+# Sentinel
+
+## 2026-02-12 - Insecure SSH Key Creation (TOCTOU)
+
+**Vulnerability:** `tools/setup-ssh-keys.sh` created SSH private
+keys with default permissions (664/644) before restricting them
+with `chmod 600`, creating a TOCTOU race where the key was
+briefly readable by other users.
+
+**Learning:** Shell redirection (`> file`) uses the default umask
+(typically 022 or 002), producing world-readable files for a
+split second. A subsequent `chmod` is insufficient for sensitive
+data.
+
+**Prevention:** Wrap sensitive file creation in a subshell with
+`umask 077` (e.g., `(umask 077; cmd > file)`) so the file is
+created with 600 permissions atomically.🧰 Tools
🪛 GitHub Check: Lint Documentation
[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 192] 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: 216] 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: 254] 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-02-12 - Insecure SSH 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-02-12 - Insecure SSH Key Creation (TOCTOU)"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
🤖 Prompt for AI Agents
In @.jules/sentinel.md around lines 1 - 4, Update the Sentinel markdown: change
the date string "2024-02-12" to "2026-02-12" in the entry header, add a
top-level H1 (e.g., "# Sentinel") above the existing "## 2026-02-12 - Insecure
SSH Key Creation (TOCTOU)" and insert a blank line after that H1 to satisfy
MD041 and MD022, and reflow or convert the long lines in the
vulnerability/learning/prevention paragraphs into wrapped lines or a short
bullet list so no line (the text under the "Vulnerability:", "Learning:", and
"Prevention:" sections) exceeds 80 characters to satisfy MD013. Ensure the
content (TOCTOU description, umask example "(umask 077; cmd > file)", and chmod
note) remains intact while wrapping.
| fi | ||
|
|
||
| # Get permissions (Linux stat) | ||
| PERMS=$(stat -c "%a" "$KEY_FILE") |
There was a problem hiding this comment.
stat -c is Linux-only — test will fail on macOS.
stat -c "%a" is a GNU coreutils flag. On macOS (BSD stat), the equivalent is stat -f "%Lp". Since this is a dotfiles repo likely used on macOS too, consider a portable helper:
Proposed fix
-# Get permissions (Linux stat)
-PERMS=$(stat -c "%a" "$KEY_FILE")
+# Get permissions (portable across Linux and macOS)
+if stat -c "%a" /dev/null >/dev/null 2>&1; then
+ PERMS=$(stat -c "%a" "$KEY_FILE")
+else
+ PERMS=$(stat -f "%Lp" "$KEY_FILE")
+fi📝 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.
| PERMS=$(stat -c "%a" "$KEY_FILE") | |
| # Get permissions (portable across Linux and macOS) | |
| if stat -c "%a" /dev/null >/dev/null 2>&1; then | |
| PERMS=$(stat -c "%a" "$KEY_FILE") | |
| else | |
| PERMS=$(stat -f "%Lp" "$KEY_FILE") | |
| fi |
🤖 Prompt for AI Agents
In `@tests/reproduce_issue.sh` at line 53, The PERMS assignment uses GNU-only stat
syntax (PERMS=$(stat -c "%a" "$KEY_FILE")), which breaks on macOS; add a small
portable helper (e.g., get_file_perms) that detects the platform via uname -s
(or checks which stat variant works) and calls stat -c "%a" for Linux/GNU or
stat -f "%Lp" for Darwin, then replace the direct PERMS=... call with
PERMS=$(get_file_perms "$KEY_FILE") referencing KEY_FILE so the script works on
both Linux and macOS.
1 participant
This PR fixes a critical security vulnerability where SSH private keys were temporarily world-readable during creation.
Vulnerability:
The
tools/setup-ssh-keys.shscript used shell redirection (>) to write the private key, which uses the default umask (typically 022 or 002), resulting in file permissions of 0644 or 0664. Achmod 600command was run immediately after, but this left a small window (race condition) where other users on the system could read the private key.Fix:
The file creation is now wrapped in a subshell with
umask 077, ensuring the file is created with 0600 permissions from the start.Verification:
Added
tests/reproduce_issue.shwhich mocksopandchmodto verify that the file is created with 0600 permissions even without the subsequentchmodcommand.PR created automatically by Jules for task 18157296879826234291 started by @kidchenko
Summary by CodeRabbit
Bug Fixes
Tests
Documentation