Skip to content

🛡️ Sentinel: Fix TOCTOU race condition in SSH key creation #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kidchenko wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🛡️ Sentinel: Fix TOCTOU race condition in SSH key creation #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## 2025-02-16 - TOCTOU Race Condition in Shell Scripts

Check failure on line 1 in .jules/sentinel.md

View workflow job for this annotation

GitHub Actions / Lint Documentation

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: "## 2025-02-16 - TOCTOU Race Co..."] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md041.md

Check failure on line 1 in .jules/sentinel.md

View workflow job for this annotation

GitHub Actions / Lint Documentation

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: "## 2025-02-16 - TOCTOU Race Condition in Shell Scripts"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
**Vulnerability:** Private SSH keys were created using redirection (`> file`) before restricting permissions (`chmod 600`), leaving a race window where keys were world-readable.

Check failure on line 2 in .jules/sentinel.md

View workflow job for this annotation

GitHub Actions / Lint Documentation

Line length 

.jules/sentinel.md:2:81 MD013/line-length Line length [Expected: 80; Actual: 177] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Learning:** Shell redirection creates files with default umask permissions (often 644) *before* any subsequent `chmod` command runs.

Check failure on line 3 in .jules/sentinel.md

View workflow job for this annotation

GitHub Actions / Lint Documentation

Line length 

.jules/sentinel.md:3:81 MD013/line-length Line length [Expected: 80; Actual: 134] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Prevention:** Use `(umask 077; command > file)` in a subshell to ensure files are created with restrictive permissions atomically. Also ensure existing files are removed before recreation.

Check failure on line 4 in .jules/sentinel.md

View workflow job for this annotation

GitHub Actions / Lint Documentation

Line length 

.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 190] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
Comment on lines +1 to +4
Copy link

@coderabbitai coderabbitai bot Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix markdown lint failures flagged by CI.

The documentation lint check is failing with multiple issues:

  1. MD041: First line should be a top-level heading (#), not ##.
  2. MD022: A blank line is needed after the heading.
  3. MD013: Lines 2–4 exceed the 80-character line length limit.

Also, the date reads 2025-02-16 — should this be 2026-02-16?

Proposed fix 
-## 2025-02-16 - TOCTOU Race Condition in Shell Scripts
-**Vulnerability:** Private SSH keys were created using redirection (`> file`) before restricting permissions (`chmod 600`), leaving a race window where keys were world-readable.
-**Learning:** Shell redirection creates files with default umask permissions (often 644) *before* any subsequent `chmod` command runs.
-**Prevention:** Use `(umask 077; command > file)` in a subshell to ensure files are created with restrictive permissions atomically. Also ensure existing files are removed before recreation.
+# Sentinel Journal
+
+## 2026-02-16 - TOCTOU Race Condition in Shell Scripts
+
+**Vulnerability:** Private SSH keys were created using
+redirection (`> file`) before restricting permissions
+(`chmod 600`), leaving a race window where keys were
+world-readable.
+
+**Learning:** Shell redirection creates files with default
+umask permissions (often 644) *before* any subsequent
+`chmod` command runs.
+
+**Prevention:** Use `(umask 077; command > file)` in a
+subshell to ensure files are created with restrictive
+permissions atomically. Also ensure existing files are
+removed before recreation.
🧰 Tools
🪛 GitHub Check: Lint Documentation

[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 190] 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: 134] 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: 177] 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: "## 2025-02-16 - TOCTOU Race Co..."] 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: "## 2025-02-16 - TOCTOU Race Condition in Shell Scripts"] 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 markdown so the first line
is a top-level heading and the date is corrected: replace the existing "##
2025-02-16 - TOCTOU Race Condition in Shell Scripts" with "# 2026-02-16 - TOCTOU
Race Condition in Shell Scripts"; add a single blank line immediately after that
heading; then wrap or reflow the following paragraph lines (the vulnerability,
learning, and prevention sentences) to be <=80 characters per line (preserving
content such as `(umask 077; command > file)` and `chmod 600`) and ensure any
suggested code snippets remain inline or fenced appropriately so MD013 and
MD022/MD041 lint rules pass.

62 changes: 62 additions & 0 deletions tests/security_ssh_permissions.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
#!/bin/bash
# Test for secure file creation logic
set -e

TEST_FILE="tests/ssh_key_test"

# Function to check permissions
check_permissions() {
local file="$1"
local expected="$2"
local perms
if [[ "$(uname -s)" == "Darwin" ]]; then
perms=$(stat -f "%Lp" "$file")
else
perms=$(stat -c "%a" "$file")
fi

if [[ "$perms" != "$expected" ]]; then
echo "FAIL: Permissions for $file are $perms, expected $expected"
return 1
else
echo "PASS: Permissions for $file are $perms"
return 0
fi
}

echo "=== Test 1: Insecure Creation (Baseline) ==="
rm -f "$TEST_FILE"
# Standard creation (vulnerable pattern)
echo "secret" > "$TEST_FILE"
# Check if permissions are 644 (or 664 depending on umask)
# Assuming default umask 022 -> 644
# We check if it is NOT 600
perms=$(stat -c "%a" "$TEST_FILE" 2>/dev/null || stat -f "%Lp" "$TEST_FILE")
if [[ "$perms" != "600" ]]; then
echo "PASS: Default creation is insecure ($perms)"
else
echo "WARN: Default umask is already strict (077)? This test assumes default umask allows group/other read."
fi

echo "=== Test 2: Secure Creation (Fix Verification) ==="
rm -f "$TEST_FILE"
# Secure creation
(umask 077; echo "secret" > "$TEST_FILE")
check_permissions "$TEST_FILE" "600"

echo "=== Test 3: Existing File (Regression Check) ==="
rm -f "$TEST_FILE"
# Create with 644
echo "old content" > "$TEST_FILE"
chmod 644 "$TEST_FILE"

# Try to overwrite with umask 077 (simulating the fix applied blindly)
(umask 077; echo "new content" > "$TEST_FILE")
# Should still be 644 because file existed
if check_permissions "$TEST_FILE" "644"; then
echo "Confirmed: Overwriting existing file preserves insecure permissions (Must delete file first!)"
else
echo "Unexpected: Permissions changed to $perms?"
fi
Comment on lines +53 to +60
Copy link

@coderabbitai coderabbitai bot Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Bug: $perms on line 59 is stale from Test 1.

The $perms variable was set on line 34 during Test 1 and is never updated in Test 3. The else branch on line 59 will print the old value, not the file's current permissions.

Proposed fix 
 # Try to overwrite with umask 077 (simulating the fix applied blindly)
 (umask 077; echo "new content" > "$TEST_FILE")
 # Should still be 644 because file existed
 if check_permissions "$TEST_FILE" "644"; then
     echo "Confirmed: Overwriting existing file preserves insecure permissions (Must delete file first!)"
 else
-    echo "Unexpected: Permissions changed to $perms?"
+    perms_actual=$(if [[ "$(uname -s)" == "Darwin" ]]; then stat -f "%Lp" "$TEST_FILE"; else stat -c "%a" "$TEST_FILE"; fi)
+    echo "Unexpected: Permissions changed to $perms_actual?"
 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.

Suggested change
# Try to overwrite with umask 077 (simulating the fix applied blindly)
(umask 077; echo "new content" > "$TEST_FILE")
# Should still be 644 because file existed
if check_permissions "$TEST_FILE" "644"; then
echo "Confirmed: Overwriting existing file preserves insecure permissions (Must delete file first!)"
else
echo "Unexpected: Permissions changed to $perms?"
fi
# Try to overwrite with umask 077 (simulating the fix applied blindly)
(umask 077; echo "new content" > "$TEST_FILE")
# Should still be 644 because file existed
if check_permissions "$TEST_FILE" "644"; then
echo "Confirmed: Overwriting existing file preserves insecure permissions (Must delete file first!)"
else
perms_actual=$(if [[ "$(uname -s)" == "Darwin" ]]; then stat -f "%Lp" "$TEST_FILE"; else stat -c "%a" "$TEST_FILE"; fi)
echo "Unexpected: Permissions changed to $perms_actual?"
fi
🤖 Prompt for AI Agents 
In `@tests/security_ssh_permissions.sh` around lines 53 - 60, The else branch
prints a stale $perms from Test 1; recompute the current permission before using
it: after the failed check_permissions "$TEST_FILE" "644" case, refresh perms
for "$TEST_FILE" (e.g., via stat) and then use that updated $perms in the echo,
or change the echo to report the result of a permission-query helper instead of
the old $perms; update references to the variable name perms and the TEST_FILE
usage so the message reflects current permissions.


rm -f "$TEST_FILE"
10 changes: 8 additions & 2 deletions tools/setup-ssh-keys.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,14 @@ cmd_restore() {
chmod 700 "$SSH_DIR"

# Read private key from 1Password and save locally
op read "op://$VAULT/$KEY_NAME/private_key" > "$PRIVATE_KEY_FILE"
chmod 600 "$PRIVATE_KEY_FILE"
# Note: Use umask in subshell to ensure file is created with 600 permissions
# to avoid race condition where file is briefly world-readable.
# Remove existing file first to ensure permissions are reset.
[[ -f "$PRIVATE_KEY_FILE" ]] && rm -f "$PRIVATE_KEY_FILE"
(
umask 077
op read "op://$VAULT/$KEY_NAME/private_key" > "$PRIVATE_KEY_FILE"
)

# Read public key from 1Password and save locally
op read "op://$VAULT/$KEY_NAME/public_key" > "$PUBLIC_KEY_FILE"
Expand Down
Loading