Skip to content

🛡️ Sentinel: Fix TOCTOU vulnerability in SSH key creation #30

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 vulnerability in SSH key creation #30

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-14 - Shell Script 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: "## 2025-02-14 - Shell Script T..."] 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-14 - Shell Script TOCTOU Vulnerability"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
**Vulnerability:** SSH keys were created with default permissions (often world-readable) before being restricted with `chmod`, creating a race condition (TOCTOU).

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: 162] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Learning:** Shell scripts using `>` redirection to create sensitive files inherit the current umask, leading to insecure default permissions.

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: 143] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Prevention:** Always use `umask 077` in a subshell `(umask 077; command > file)` when creating sensitive files in shell scripts.

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

@coderabbitai coderabbitai bot Feb 14, 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 "Lint Documentation" check is failing with multiple violations:

  • Line 1: missing top-level heading (#) and no blank line after the heading.
  • Lines 2–4: line length exceeds 80 characters.

Also, the date reads 2025-02-14 — likely should be 2026-02-14 given the PR timestamp.

Proposed fix 
-## 2025-02-14 - Shell Script TOCTOU Vulnerability
-**Vulnerability:** SSH keys were created with default permissions (often world-readable) before being restricted with `chmod`, creating a race condition (TOCTOU).
-**Learning:** Shell scripts using `>` redirection to create sensitive files inherit the current umask, leading to insecure default permissions.
-**Prevention:** Always use `umask 077` in a subshell `(umask 077; command > file)` when creating sensitive files in shell scripts.
+# Sentinel Notes
+
+## 2026-02-14 - Shell Script TOCTOU Vulnerability
+
+**Vulnerability:** SSH keys were created with default permissions
+(often world-readable) before being restricted with `chmod`,
+creating a race condition (TOCTOU).
+
+**Learning:** Shell scripts using `>` redirection to create
+sensitive files inherit the current umask, leading to insecure
+default permissions.
+
+**Prevention:** Always use `umask 077` in a subshell
+`(umask 077; command > file)` when creating sensitive files
+in shell scripts.
📝 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-14 - Shell Script TOCTOU Vulnerability
**Vulnerability:** SSH keys were created with default permissions (often world-readable) before being restricted with `chmod`, creating a race condition (TOCTOU).
**Learning:** Shell scripts using `>` redirection to create sensitive files inherit the current umask, leading to insecure default permissions.
**Prevention:** Always use `umask 077` in a subshell `(umask 077; command > file)` when creating sensitive files in shell scripts.
# Sentinel Notes
## 2026-02-14 - Shell Script TOCTOU Vulnerability
**Vulnerability:** SSH keys were created with default permissions
(often world-readable) before being restricted with `chmod`,
creating a race condition (TOCTOU).
**Learning:** Shell scripts using `>` redirection to create
sensitive files inherit the current umask, leading to insecure
default permissions.
**Prevention:** Always use `umask 077` in a subshell
`(umask 077; command > file)` when creating sensitive files
in shell scripts.
🧰 Tools
🪛 GitHub Check: Lint Documentation

[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 130] 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: 143] 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: 162] 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-14 - Shell Script T..."] 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-14 - Shell Script 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, Add a top-level heading and blank
line, update the date from 2025-02-14 to 2026-02-14, and reflow long lines to
<=80 chars; specifically, change the first line to a proper H1 (e.g. "# Shell
Script TOCTOU Vulnerability — 2026-02-14") followed by a blank line, then split
the bullets that exceed 80 chars (the lines describing the TOCTOU issue and the
prevention using "(umask 077; command > file)") into shorter lines or multiple
sentences so they pass markdown lints while preserving the original content.

64 changes: 64 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,64 @@
#!/bin/bash
set -e

# Setup mock environment
TEST_DIR="$(mktemp -d)"
MOCK_BIN="$TEST_DIR/bin"
mkdir -p "$MOCK_BIN"
export PATH="$MOCK_BIN:$PATH"

# Mock 'op'
cat << 'EOF' > "$MOCK_BIN/op"
#!/bin/bash
if [[ "$1" == "item" && "$2" == "get" ]]; then
# Pretend key exists in 1Password
exit 0
elif [[ "$1" == "read" ]]; then
echo "dummy-key-content"
exit 0
else
# Allow other commands (like account list)
exit 0
fi
EOF
chmod +x "$MOCK_BIN/op"

# Mock 'chmod' to inspect permissions
cat << 'EOF' > "$MOCK_BIN/chmod"
#!/bin/bash
target="${@: -1}"
if [[ -f "$target" ]]; then
perms=$(ls -l "$target" | awk '{print $1}')
echo "MOCK CHMOD: Inspecting $target before chmod: $perms"
fi
# Do nothing (mocked) or actually change permissions?
# If we do nothing, the file remains with creation permissions.
# This helps us verify the umask effect.
EOF
chmod +x "$MOCK_BIN/chmod"

# Set up test environment
export HOME="$TEST_DIR"
export XDG_CONFIG_HOME="$TEST_DIR/.config"
export XDG_DATA_HOME="$TEST_DIR/.local/share"
export XDG_STATE_HOME="$TEST_DIR/.local/state"

# Create config file
mkdir -p "$XDG_CONFIG_HOME/dotfiles"
echo "ssh:
vault: development
item_name: SSH Key
key_type: ed25519" > "$XDG_CONFIG_HOME/dotfiles/config.yaml"

# Run the script
# We expect it to restore the key
echo "Running tools/setup-ssh-keys.sh restore..."
# We pipe 'y' to confirm overwrite if prompted (though initial restore shouldn't need it)
echo "y" | ./tools/setup-ssh-keys.sh restore

# Verify output
# The output should contain "MOCK CHMOD: Inspecting ... before chmod: -rw-r--r--" for private key (if vulnerable)
# or "-rw-------" (if secure)

# Cleanup
rm -rf "$TEST_DIR"
Comment on lines +59 to +64
Copy link

@coderabbitai coderabbitai bot Feb 14, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Test has no assertions — it will always pass regardless of actual permissions.

Lines 59–61 describe the expected output in comments but never actually check it. The test exits 0 whether the private key was created with 0600 or 0644, making this a no-op regression test.

Additionally, with set -e, if the script fails before line 64 the rm -rf cleanup is skipped — use a trap instead.

Proposed fix 
+# Cleanup on exit (including failures)
+cleanup() { rm -rf "$TEST_DIR"; }
+trap cleanup EXIT
+
 # Run the script
 echo "Running tools/setup-ssh-keys.sh restore..."
-echo "y" | ./tools/setup-ssh-keys.sh restore
+OUTPUT=$(echo "y" | ./tools/setup-ssh-keys.sh restore 2>&1)
 
-# Verify output
-# The output should contain "MOCK CHMOD: Inspecting ... before chmod: -rw-r--r--" for private key (if vulnerable)
-# or "-rw-------" (if secure)
+# Assert private key was created with secure permissions (umask 077 → -rw-------)
+PRIVATE_KEY="$TEST_DIR/.ssh/id_ed25519"
+if [[ -f "$PRIVATE_KEY" ]]; then
+    PERMS=$(stat -c '%a' "$PRIVATE_KEY" 2>/dev/null || stat -f '%Lp' "$PRIVATE_KEY")
+    if [[ "$PERMS" != "600" ]]; then
+        echo "FAIL: Private key has permissions $PERMS, expected 600"
+        exit 1
+    fi
+    echo "PASS: Private key created with permissions $PERMS"
+else
+    echo "FAIL: Private key file not found"
+    exit 1
+fi
 
-# Cleanup
-rm -rf "$TEST_DIR"
+echo "All security permission tests passed."

Note: stat syntax differs between Linux (-c '%a') and macOS (-f '%Lp'); the snippet above handles both. Also move the trap cleanup EXIT near the top (after TEST_DIR is set) so it always fires.

🤖 Prompt for AI Agents 
In `@tests/security_ssh_permissions.sh` around lines 59 - 64, The test currently
only documents expected output and never asserts, and cleanup can be skipped
because trap is missing; add a cleanup function and register trap cleanup EXIT
immediately after TEST_DIR is set, then after the SSH key is created use a
portable mode check (branch on stat availability) to read the private key mode
(e.g., via stat -c '%a' or stat -f '%Lp') and assert it equals the secure mode
(0600) or fail the test (exit non-zero) if it is 0644; update the test harness
logic around tests/security_ssh_permissions.sh to perform this explicit check
and return a non-zero status on mismatch so the test actually fails when
permissions are insecure.

11 changes: 9 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,11 +153,18 @@ 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 subshell with umask to ensure secure file permissions at creation time
(
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"

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