Skip to content

🛡️ Sentinel: [CRITICAL] Fix TOCTOU vulnerability in ssh key restore #84

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

Closed
kidchenko wants to merge 1 commit into from
Closed

🛡️ Sentinel: [CRITICAL] Fix TOCTOU vulnerability in ssh key restore #84

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-20 - TOCTOU Vulnerability in SSH Key Restoration

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-20 - TOCTOU Vulnera..."] 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-20 - TOCTOU Vulnerability in SSH Key Restoration"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
**Vulnerability:** Time-of-Check to Time-of-Use (TOCTOU) when restoring SSH private keys from 1Password.

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: 104] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Learning:** Writing sensitive data to a file using default permissions and then using `chmod 600` leaves the file briefly readable by other users on the system between the write and the chmod.

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

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

@coderabbitai coderabbitai Bot Apr 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix markdownlint violations to unblock docs checks.

Current lines violate heading-level/spacing and line-length rules (MD041, MD022, MD013). Reflowing this section should resolve the documented CI failures.

📝 Suggested markdown fix 
-## 2025-02-20 - TOCTOU Vulnerability in SSH Key Restoration
-**Vulnerability:** Time-of-Check to Time-of-Use (TOCTOU) when restoring SSH private keys from 1Password.
-**Learning:** Writing sensitive data to a file using default permissions and then using `chmod 600` leaves the file briefly readable by other users on the system between the write and the chmod.
-**Prevention:** Use a subshell with `umask 077` (e.g., `(umask 077 && command > file)`) to ensure the file is created with secure permissions (600) from the start.
+# 2025-02-20 - TOCTOU Vulnerability in SSH Key Restoration
+
+**Vulnerability:** Time-of-Check to Time-of-Use (TOCTOU) when restoring
+SSH private keys from 1Password.
+
+**Learning:** Writing sensitive data to a file using default permissions and
+then using `chmod 600` leaves the file briefly readable by other users on the
+system between the write and the chmod.
+
+**Prevention:** Use a subshell with `umask 077` (for example,
+`(umask 077 && command > file)`) to ensure the file is created with secure
+permissions (`600`) from the start.
📝 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
## 2025-02-20 - TOCTOU Vulnerability in SSH Key Restoration
**Vulnerability:** Time-of-Check to Time-of-Use (TOCTOU) when restoring SSH private keys from 1Password.
**Learning:** Writing sensitive data to a file using default permissions and then using `chmod 600` leaves the file briefly readable by other users on the system between the write and the chmod.
**Prevention:** Use a subshell with `umask 077` (e.g., `(umask 077 && command > file)`) to ensure the file is created with secure permissions (600) from the start.
# 2025-02-20 - TOCTOU Vulnerability in SSH Key Restoration
**Vulnerability:** Time-of-Check to Time-of-Use (TOCTOU) when restoring
SSH private keys from 1Password.
**Learning:** Writing sensitive data to a file using default permissions and
then using `chmod 600` leaves the file briefly readable by other users on the
system between the write and the chmod.
**Prevention:** Use a subshell with `umask 077` (for example,
`(umask 077 && command > file)`) to ensure the file is created with secure
permissions (`600`) from the start.
🧰 Tools
🪛 GitHub Check: Lint Documentation

[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 163] 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: 194] 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: 104] 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-20 - TOCTOU Vulnera..."] 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-20 - TOCTOU Vulnerability in SSH Key Restoration"] 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, The "## 2025-02-20 - TOCTOU
Vulnerability in SSH Key Restoration" heading and its paragraph violate
markdownlint rules (MD041, MD022, MD013); fix by ensuring a blank line after the
heading, breaking the long description lines into wrapped sentences under ~80
chars, and keep consistent heading level (use "##" only if this section matches
other top-level changelog entries). Specifically, edit the heading "##
2025-02-20 - TOCTOU Vulnerability in SSH Key Restoration" to be followed by an
empty line, reflow the three sentences (Vulnerability, Learning, Prevention)
each as its own short paragraph or bullet so no line exceeds the line-length
rule, and use a single-line short heading title if needed to satisfy
MD022/MD041.

6 changes: 3 additions & 3 deletions tools/setup-ssh-keys.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,15 +149,15 @@ cmd_restore() {
say "Restoring SSH key from 1Password..."

# Create SSH directory
mkdir -p "$SSH_DIR"
(umask 077 && mkdir -p "$SSH_DIR")
chmod 700 "$SSH_DIR"

# Read private key from 1Password and save locally
op read "op://$VAULT/$KEY_NAME/private_key" > "$PRIVATE_KEY_FILE"
(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
op read "op://$VAULT/$KEY_NAME/public_key" > "$PUBLIC_KEY_FILE"
(umask 022 && op read "op://$VAULT/$KEY_NAME/public_key" > "$PUBLIC_KEY_FILE")
chmod 644 "$PUBLIC_KEY_FILE"
Comment on lines +156 to 161

@coderabbitai coderabbitai Bot Apr 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

f="$(mktemp)"
printf 'EXISTING_KEY\n' > "$f"

# Simulate a failing command with direct redirection.
(false > "$f") || true

# Expected: file size becomes 0, demonstrating truncation-before-success.
wc -c "$f"

Repository: kidchenko/dotfiles

Length of output: 83

🏁 Script executed:

# First, locate the setup-ssh-keys.sh file
find . -name "setup-ssh-keys.sh" -type f

Repository: kidchenko/dotfiles

Length of output: 87

🏁 Script executed:

# Once found, let's read the relevant section
sed -n '140,175p' tools/setup-ssh-keys.sh

Repository: kidchenko/dotfiles

Length of output: 1085

🏁 Script executed:

# Also check for the cmd_restore function definition and context
sed -n '1,160p' tools/setup-ssh-keys.sh | tail -100

Repository: kidchenko/dotfiles

Length of output: 3333

🏁 Script executed:

# Check for variable definitions: SSH_DIR, KEY_TYPE, PRIVATE_KEY_FILE, PUBLIC_KEY_FILE
grep -n "SSH_DIR\|KEY_TYPE\|PRIVATE_KEY_FILE\|PUBLIC_KEY_FILE" tools/setup-ssh-keys.sh | head -30

Repository: kidchenko/dotfiles

Length of output: 1426

🏁 Script executed:

# Check the full structure of the script to understand variable scope
head -60 tools/setup-ssh-keys.sh

Repository: kidchenko/dotfiles

Length of output: 1848

🏁 Script executed:

# Look for any existing error handling patterns around file operations
grep -B2 -A2 "mv\|atomic\|tmp" tools/setup-ssh-keys.sh

Repository: kidchenko/dotfiles

Length of output: 44

Add error handling and atomic writes for SSH key restore operations.

Lines 156–161 truncate the destination files before op read completes. If the command fails or is interrupted, a previously valid key can be replaced with an empty or partial file. Wrap each read in a temp file + atomic move pattern with explicit error handling.

Proposed atomic-write fix 
     # Read private key from 1Password and save locally
-    (umask 077 && op read "op://$VAULT/$KEY_NAME/private_key" > "$PRIVATE_KEY_FILE")
-    chmod 600 "$PRIVATE_KEY_FILE"
+    local tmp_private
+    tmp_private="$(mktemp "$SSH_DIR/.id_${KEY_TYPE}.tmp.XXXXXX")"
+    if (umask 077 && op read "op://$VAULT/$KEY_NAME/private_key" > "$tmp_private"); then
+        mv -f "$tmp_private" "$PRIVATE_KEY_FILE"
+        chmod 600 "$PRIVATE_KEY_FILE"
+    else
+        rm -f "$tmp_private"
+        error "Failed to read private key from 1Password"
+    fi
 
     # Read public key from 1Password and save locally
-    (umask 022 && op read "op://$VAULT/$KEY_NAME/public_key" > "$PUBLIC_KEY_FILE")
-    chmod 644 "$PUBLIC_KEY_FILE"
+    local tmp_public
+    tmp_public="$(mktemp "$SSH_DIR/.id_${KEY_TYPE}.pub.tmp.XXXXXX")"
+    if (umask 022 && op read "op://$VAULT/$KEY_NAME/public_key" > "$tmp_public"); then
+        mv -f "$tmp_public" "$PUBLIC_KEY_FILE"
+        chmod 644 "$PUBLIC_KEY_FILE"
+    else
+        rm -f "$tmp_public"
+        error "Failed to read public key from 1Password"
+    fi
🤖 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 156 - 161, The SSH key restore
currently writes directly to "$PRIVATE_KEY_FILE" and "$PUBLIC_KEY_FILE" which
can leave truncated keys on failure; change the op read steps (the two lines
using op read "op://$VAULT/$KEY_NAME/private_key" and op read
"op://$VAULT/$KEY_NAME/public_key") to write into a uniquely named temp file in
the same directory (respecting umask as before), verify the op read succeeds,
fsync the temp file, set the correct permissions (chmod 600 for private, chmod
644 for public) on the temp file, and then atomically move (mv) the temp file
over the target file; if any step fails, remove the temp file, log/exit with
non-zero status, and do not touch the existing target file.


say "SSH key restored to $SSH_DIR"
Expand Down
Loading