Skip to content

🛡️ Sentinel: [CRITICAL] Fix insecure SSH key creation permissions #28

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: [CRITICAL] Fix insecure SSH key creation permissions #28

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-02-12 - Insecure SSH Key Creation (TOCTOU)

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-02-12 - Insecure SSH K..."] 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-02-12 - Insecure SSH Key Creation (TOCTOU)"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
**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.

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: 254] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**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.

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: 216] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**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.

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

@coderabbitai coderabbitai bot Feb 12, 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 and incorrect date.

  1. Wrong year: Line 1 says 2024-02-12 but this PR is from 2026. Should be 2026-02-12.

  2. Lint failures (from the Lint Documentation check):

    • 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.
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.

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

# Setup mock environment
TEST_DIR="$PWD/tests/tmp"
rm -rf "$TEST_DIR"
mkdir -p "$TEST_DIR/home"
mkdir -p "$TEST_DIR/bin"

# Mock op
cat << 'EOF' > "$TEST_DIR/bin/op"
#!/bin/bash
if [[ "$1" == "read" ]]; then
echo "MOCK PRIVATE KEY CONTENT"
elif [[ "$1" == "account" ]]; then
exit 0
elif [[ "$1" == "item" && "$2" == "get" ]]; then
exit 0
fi
EOF
chmod +x "$TEST_DIR/bin/op"

# Mock chmod to do nothing, so we can see the permissions at creation
cat << 'EOF' > "$TEST_DIR/bin/chmod"
#!/bin/bash
# no-op
echo "MOCK CHMOD: $@"
EOF
chmod +x "$TEST_DIR/bin/chmod"

export PATH="$TEST_DIR/bin:$PATH"
export HOME="$TEST_DIR/home"
export XDG_CONFIG_HOME="$HOME/.config"

# Create config
mkdir -p "$XDG_CONFIG_HOME/dotfiles"
echo "ssh: { vault: 'test', item_name: 'testkey' }" > "$XDG_CONFIG_HOME/dotfiles/config.yaml"

echo "Running setup-ssh-keys.sh with mocked chmod..."

# Run the script
./tools/setup-ssh-keys.sh restore > "$TEST_DIR/output.log" 2>&1 || true

# Check permissions of the private key
KEY_FILE="$TEST_DIR/home/.ssh/id_ed25519"

if [[ ! -f "$KEY_FILE" ]]; then
echo "FAIL: Key file not found at $KEY_FILE"
exit 1
fi

# Get permissions (Linux stat)
PERMS=$(stat -c "%a" "$KEY_FILE")
Copy link

@coderabbitai coderabbitai bot Feb 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

echo "File permissions detected: $PERMS"

if [[ "$PERMS" == "600" ]]; then
echo "SECURE: File created with secure permissions (600)"
exit 0
else
echo "VULNERABILITY CONFIRMED: File created with insecure permissions ($PERMS)"
exit 1
fi
5 changes: 4 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,10 @@ 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"
(
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