Skip to content

🛡️ Sentinel: [HIGH] Fix TOCTOU vulnerability in SSH key creation #31

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: [HIGH] Fix TOCTOU vulnerability in SSH key creation #31

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 @@
## 2024-05-22 - SSH Key TOCTOU Vulnerability

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: "## 2024-05-22 - SSH Key TOCTOU..."] 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: "## 2024-05-22 - SSH Key TOCTOU Vulnerability"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
**Vulnerability:** SSH private keys were created with default permissions (likely 644/664) and then chmod'ed to 600, creating a Time-of-Check Time-of-Use (TOCTOU) race condition where the key was briefly 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: 219] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Learning:** Shell redirection (`>`) creates files with default umask permissions before any subsequent `chmod` command can run.

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: 129] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Prevention:** Use `(umask 077 && command > file)` in a subshell to ensure the file is created with restrictive permissions (600) from the very beginning.

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: 155] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
Comment on lines +1 to +4
Copy link

@coderabbitai coderabbitai bot Feb 15, 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 violations flagged by CI.

The Lint Documentation check is failing with multiple issues:

  • MD041: First line should be a top-level heading (# instead of ##).
  • MD022: Heading on line 1 needs a blank line below it.
  • MD013: Lines 2–4 exceed the 80-character line length limit.
Proposed fix 
-## 2024-05-22 - SSH Key TOCTOU Vulnerability
-**Vulnerability:** SSH private keys were created with default permissions (likely 644/664) and then chmod'ed to 600, creating a Time-of-Check Time-of-Use (TOCTOU) race condition where the key was briefly world-readable.
-**Learning:** Shell redirection (`>`) creates files with default umask permissions before any subsequent `chmod` command can run.
-**Prevention:** Use `(umask 077 && command > file)` in a subshell to ensure the file is created with restrictive permissions (600) from the very beginning.
+# Sentinel Notes
+
+## 2024-05-22 - SSH Key TOCTOU Vulnerability
+
+**Vulnerability:** SSH private keys were created with default
+permissions (likely 644/664) and then chmod'ed to 600, creating a
+TOCTOU race condition where the key was briefly world-readable.
+
+**Learning:** Shell redirection (`>`) creates files with default
+umask permissions before any subsequent `chmod` command can run.
+
+**Prevention:** Use `(umask 077 && command > file)` in a subshell
+to ensure the file is created with restrictive permissions (600)
+from the very beginning.
📝 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
## 2024-05-22 - SSH Key TOCTOU Vulnerability
**Vulnerability:** SSH private keys were created with default permissions (likely 644/664) and then chmod'ed to 600, creating a Time-of-Check Time-of-Use (TOCTOU) race condition where the key was briefly world-readable.
**Learning:** Shell redirection (`>`) creates files with default umask permissions before any subsequent `chmod` command can run.
**Prevention:** Use `(umask 077 && command > file)` in a subshell to ensure the file is created with restrictive permissions (600) from the very beginning.
# Sentinel Notes
## 2024-05-22 - SSH Key TOCTOU Vulnerability
**Vulnerability:** SSH private keys were created with default
permissions (likely 644/664) and then chmod'ed to 600, creating a
TOCTOU race condition where the key was briefly world-readable.
**Learning:** Shell redirection (`>`) creates files with default
umask permissions before any subsequent `chmod` command can run.
**Prevention:** Use `(umask 077 && command > file)` in a subshell
to ensure the file is created with restrictive permissions (600)
from the very beginning.
🧰 Tools
🪛 GitHub Check: Lint Documentation

[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 155] 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: 129] 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: 219] 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 Key TOCTOU..."] 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 Key TOCTOU Vulnerability"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md

🤖 Prompt for AI Agents 
In @.jules/sentinel.md around lines 1 - 4, Change the heading to a top-level
heading and add the required blank line after it, and wrap the long lines to
<=80 characters to satisfy MD041, MD022 and MD013; specifically update the
heading "2024-05-22 - SSH Key TOCTOU Vulnerability" to use a leading '#' and
insert a blank line below it, then reflow the following three lines
("Vulnerability: ...", "Learning: ...", "Prevention: ...") so each line is
within 80 characters (or split into multiple short lines) while preserving the
original wording (e.g., break long sentences into separate lines or list items).

51 changes: 51 additions & 0 deletions tests/reproduce_issue.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#!/bin/bash
set -e

# Create a temporary directory for the test environment
TEST_HOME=$(mktemp -d)
trap 'rm -rf "$TEST_HOME"' EXIT

# Export HOME to point to the temporary directory
export HOME="$TEST_HOME"

# Also set XDG vars to use temp dir (for safety)
export XDG_CONFIG_HOME="$TEST_HOME/.config"
export XDG_DATA_HOME="$TEST_HOME/.local/share"
export XDG_STATE_HOME="$TEST_HOME/.local/state"

# Setup mock op
mkdir -p "$TEST_HOME/bin"
cat > "$TEST_HOME/bin/op" <<'EOF'
#!/bin/bash
if [[ "$1" == "account" && "$2" == "list" ]]; then
echo "fake-account"
exit 0
fi
if [[ "$1" == "item" && "$2" == "get" ]]; then
exit 0
fi
if [[ "$1" == "read" ]]; then
echo "fake-key-content"
exit 0
fi
EOF
chmod +x "$TEST_HOME/bin/op"
export PATH="$TEST_HOME/bin:$PATH"

# Setup config
mkdir -p "$XDG_CONFIG_HOME/dotfiles"
echo "ssh:" > "$XDG_CONFIG_HOME/dotfiles/config.yaml"
echo " vault: test-vault" >> "$XDG_CONFIG_HOME/dotfiles/config.yaml"
echo " item_name: test-key" >> "$XDG_CONFIG_HOME/dotfiles/config.yaml"

# Run restore
# We run the script from the repo root
./tools/setup-ssh-keys.sh restore

# Verify file exists in the fake home
if [[ -f "$HOME/.ssh/id_ed25519" ]]; then
echo "Key restored successfully to $HOME/.ssh/id_ed25519"
else
echo "Key restore failed"
exit 1
fi
Comment on lines +46 to +51
Copy link

@coderabbitai coderabbitai bot Feb 15, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Test doesn't verify file permissions — the core assertion for this fix is missing.

The entire PR is about ensuring the private key is created with 0600 permissions, but the test only checks file existence. Add a permission check to make this test meaningful for the TOCTOU fix.

Proposed fix 
 # Verify file exists in the fake home
 if [[ -f "$HOME/.ssh/id_ed25519" ]]; then
     echo "Key restored successfully to $HOME/.ssh/id_ed25519"
+    # Verify permissions are 600 (the whole point of the TOCTOU fix)
+    PERMS=$(stat -c '%a' "$HOME/.ssh/id_ed25519" 2>/dev/null || stat -f '%Lp' "$HOME/.ssh/id_ed25519")
+    if [[ "$PERMS" != "600" ]]; then
+        echo "FAIL: Expected permissions 600, got $PERMS"
+        exit 1
+    fi
+    echo "Permissions verified: $PERMS"
 else
     echo "Key restore failed"
     exit 1
 fi
🤖 Prompt for AI Agents 
In `@tests/reproduce_issue.sh` around lines 46 - 51, The test currently only
verifies existence of "$HOME/.ssh/id_ed25519"; add a permission assertion
immediately after that check to confirm the private key's mode is 0600. Use a
portable permission check (for example, call stat -c %a "$HOME/.ssh/id_ed25519"
and compare the result to "600" or use another reliable method on the platform)
and if the mode is not 600, print a clear error like "Incorrect permissions on
id_ed25519" and exit 1; update the success message to reflect both existence and
correct permissions.

3 changes: 2 additions & 1 deletion tools/setup-ssh-keys.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ 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"
# Use umask in subshell to ensure file is created with 600 permissions (preventing TOCTOU race)
(umask 077 && op read "op://$VAULT/$KEY_NAME/private_key" > "$PRIVATE_KEY_FILE")
chmod 600 "$PRIVATE_KEY_FILE"

# Read public key from 1Password and save locally
Expand Down
Loading