Skip to content

🛡️ Sentinel: [HIGH] Fix insecure backup permissions and command injection #49

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 insecure backup permissions and command injection #49

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-03-05 - File permissions and array argument injection in backup 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-03-05 - File permissio..."] 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-03-05 - File permissions and array argument injection in backup scripts"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
**Vulnerability:** A backup script was creating a temporary directory with default permissions and a zip archive containing sensitive files with default `umask`. Furthermore, an array of exclude patterns with strings separated by spaces was being built and executed via command substitution, leading to word-splitting and the risk of command argument injection for patterns with spaces or shell metacharacters.

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: 410] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Learning:** Shell scripts generating sensitive archives (like zip backups) must enforce strict access controls on the parent directories (e.g., `chmod 700`) and the unencrypted archives themselves (e.g., via `umask 077` in the generation subshell). Additionally, injecting arguments constructed as space-separated strings into commands creates word-splitting issues and argument injection vulnerabilities, requiring `# shellcheck disable=SC2086` overrides. Arguments must always be built using proper Bash arrays and mapfile/readarray.

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: 537] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Prevention:** Explicitly set `chmod 700` on backup directories and use `umask 077` subshells when creating archives. Construct arguments dynamically with Bash arrays using `mapfile` to prevent word-splitting and command argument injection vulnerabilities without relying on Shellcheck overrides.

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

@coderabbitai coderabbitai bot Mar 5, 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 markdownlint violations blocking docs checks.

Line 1 should be an H1 with a blank line below it, and Lines 2–4 need wrapping to satisfy MD013.

📝 Proposed markdownlint-compliant rewrite 
-## 2025-03-05 - File permissions and array argument injection in backup scripts
-**Vulnerability:** A backup script was creating a temporary directory with default permissions and a zip archive containing sensitive files with default `umask`. Furthermore, an array of exclude patterns with strings separated by spaces was being built and executed via command substitution, leading to word-splitting and the risk of command argument injection for patterns with spaces or shell metacharacters.
-**Learning:** Shell scripts generating sensitive archives (like zip backups) must enforce strict access controls on the parent directories (e.g., `chmod 700`) and the unencrypted archives themselves (e.g., via `umask 077` in the generation subshell). Additionally, injecting arguments constructed as space-separated strings into commands creates word-splitting issues and argument injection vulnerabilities, requiring `# shellcheck disable=SC2086` overrides. Arguments must always be built using proper Bash arrays and mapfile/readarray.
-**Prevention:** Explicitly set `chmod 700` on backup directories and use `umask 077` subshells when creating archives. Construct arguments dynamically with Bash arrays using `mapfile` to prevent word-splitting and command argument injection vulnerabilities without relying on Shellcheck overrides.
+# 2025-03-05 - File permissions and array argument injection in backup scripts
+
+**Vulnerability:** A backup script created a temporary directory with default
+permissions and a zip archive containing sensitive files with default `umask`.
+It also built exclude patterns as a space-delimited string and executed them
+via command substitution, causing word-splitting and potential argument
+injection when patterns included spaces or shell metacharacters.
+
+**Learning:** Shell scripts generating sensitive archives must enforce strict
+access controls on parent directories (for example, `chmod 700`) and on archive
+creation (for example, `umask 077` inside the generation subshell). Arguments
+must be built as proper Bash arrays (for example, via `mapfile`/`readarray`)
+instead of unsafe space-delimited string expansion.
+
+**Prevention:** Explicitly set `chmod 700` on backup directories and use
+`umask 077` subshells when creating archives. Construct dynamic command
+arguments using Bash arrays to avoid word-splitting and argument injection,
+without relying on broad Shellcheck suppressions.
🧰 Tools
🪛 GitHub Check: Lint Documentation

[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 297] 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: 537] 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: 410] 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-03-05 - File permissio..."] 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-03-05 - File permissions and array argument injection in backup scripts"] 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, Change the first line into an H1 and
add a blank line after it by replacing "## 2025-03-05 - File permissions and
array argument injection in backup scripts" with "# 2025-03-05 - File
permissions and array argument injection in backup scripts" followed by a blank
line, then reflow/wrap the following text (the vulnerability, learning, and
prevention lines) to shorter lines to satisfy MD013 (split long sentences into
multiple lines/paragraphs), preserving the existing content and emphasis but
ensuring each paragraph line length meets markdownlint limits.

22 changes: 14 additions & 8 deletions tools/backup-projects.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,9 @@ build_exclude_args() {
for pattern in "${EXCLUDE_PATTERNS[@]}"; do
args+=("-x" "*/${pattern}/*" "-x" "*/${pattern}")
done
echo "${args[@]}"
if [[ ${#args[@]} -gt 0 ]]; then
printf '%s\n' "${args[@]}"
fi
}

# --- Git Sync ---
Expand Down Expand Up @@ -267,6 +269,7 @@ sync_git_repos() {
local repo_dir
repo_dir=$(dirname "$git_dir")
local repo_name
# shellcheck disable=SC2034
repo_name=$(basename "$repo_dir")
local relative_path="${repo_dir#$HOME/}"

Expand Down Expand Up @@ -303,7 +306,8 @@ sync_git_repos() {
git -C "$repo_dir" add -A 2>/dev/null

# Commit with auto-generated message
local commit_msg="chore: auto-backup commit $(date '+%Y-%m-%d %H:%M')"
local commit_msg
commit_msg="chore: auto-backup commit $(date '+%Y-%m-%d %H:%M')"
if git -C "$repo_dir" commit -m "$commit_msg" 2>/dev/null; then
echo -e " ${GREEN}✓${NC} Committed changes"
else
Expand Down Expand Up @@ -352,6 +356,7 @@ cmd_backup() {
if [[ "$DRY_RUN" != true ]]; then
mkdir -p "$BACKUP_TEMP_DIR"
mkdir -p "$LOG_DIR"
chmod 700 "$BACKUP_TEMP_DIR" "$LOG_DIR"
else
Comment on lines 356 to 360
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fail closed if secure directory permissions cannot be enforced.

Line 359 applies chmod 700, but failure is not handled. If permission hardening fails, backup should abort to avoid producing sensitive artifacts in an insecure location.

🔒 Proposed fail-closed handling for secure directory setup 
     if [[ "$DRY_RUN" != true ]]; then
-        mkdir -p "$BACKUP_TEMP_DIR"
-        mkdir -p "$LOG_DIR"
-        chmod 700 "$BACKUP_TEMP_DIR" "$LOG_DIR"
+        if ! (umask 077 && mkdir -p "$BACKUP_TEMP_DIR" "$LOG_DIR"); then
+            error "Failed to create secure backup/log directories"
+            return 1
+        fi
+        if ! chmod 700 "$BACKUP_TEMP_DIR" "$LOG_DIR"; then
+            error "Failed to enforce 0700 permissions on backup/log directories"
+            return 1
+        fi
     else
📝 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
if [[ "$DRY_RUN" != true ]]; then
mkdir -p "$BACKUP_TEMP_DIR"
mkdir -p "$LOG_DIR"
chmod 700 "$BACKUP_TEMP_DIR" "$LOG_DIR"
else
if [[ "$DRY_RUN" != true ]]; then
if ! (umask 077 && mkdir -p "$BACKUP_TEMP_DIR" "$LOG_DIR"); then
error "Failed to create secure backup/log directories"
return 1
fi
if ! chmod 700 "$BACKUP_TEMP_DIR" "$LOG_DIR"; then
error "Failed to enforce 0700 permissions on backup/log directories"
return 1
fi
else
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tools/backup-projects.sh` around lines 356 - 360, The mkdir/chmod block that
sets up BACKUP_TEMP_DIR and LOG_DIR when DRY_RUN is not true must fail closed:
after creating the dirs (mkdir -p "$BACKUP_TEMP_DIR" "$LOG_DIR") and after chmod
700 "$BACKUP_TEMP_DIR" "$LOG_DIR", check each command's exit status and if any
command fails log an error and exit non‑zero (e.g., exit 1) to abort the backup;
update the DRY_RUN conditional around these ops and reference the
BACKUP_TEMP_DIR and LOG_DIR variables so the script does not continue if secure
permissions cannot be enforced.

debug "Would create: $BACKUP_TEMP_DIR"
debug "Would create: $LOG_DIR"
Expand Down Expand Up @@ -406,17 +411,18 @@ cmd_backup() {
done
fi
else
local exclude_args
exclude_args=$(build_exclude_args)
local exclude_args=()
if [[ ${#EXCLUDE_PATTERNS[@]} -gt 0 ]]; then
mapfile -t exclude_args < <(build_exclude_args)
fi
Comment on lines +414 to +417
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

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

echo "=== Shebang in script ==="
head -n1 tools/backup-projects.sh

echo -e "\n=== System /bin/bash version ==="
/bin/bash --version | head -n1

echo -e "\n=== Does /bin/bash support mapfile? ==="
if /bin/bash -c 'type mapfile >/dev/null 2>&1'; then
  echo "mapfile supported"
else
  echo "mapfile NOT supported"
fi

echo -e "\n=== Actual code at lines 414-417 ==="
sed -n '414,417p' tools/backup-projects.sh

echo -e "\n=== Current mapfile usage in script ==="
rg -nP '\bmapfile\b' tools/backup-projects.sh

Repository: kidchenko/dotfiles

Length of output: 547

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== All mapfile usages in tools/backup-projects.sh ==="
rg -n '\bmapfile\b' tools/backup-projects.sh

echo -e "\n=== Total mapfile occurrences ==="
rg -c '\bmapfile\b' tools/backup-projects.sh || echo "0"

echo -e "\n=== Check for Bash version requirements in README or docs ==="
find . -maxdepth 2 -type f \( -name "README*" -o -name "readme*" -o -name "*.md" \) | head -5 | while read f; do
  if rg -l "bash.*version|version.*bash|mapfile" "$f" 2>/dev/null; then
    echo "Found in: $f"
  fi
done

echo -e "\n=== Check shebang compatibility across other scripts ==="
find tools -name "*.sh" -type f -exec head -n1 {} + | sort | uniq -c

echo -e "\n=== Check if script has version checks ==="
rg -n "bash.*version|version.*bash|BASH_VERSINFO" tools/backup-projects.sh || echo "No version checks found"

Repository: kidchenko/dotfiles

Length of output: 1128

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== README or setup documentation ==="
if [ -f README.md ]; then
  echo "=== README.md content (first 100 lines) ==="
  head -100 README.md
else
  echo "No README.md found"
fi

echo -e "\n=== Check for any documentation mentioning Bash ==="
find . -maxdepth 2 -name "*.md" -type f -exec grep -l -i "bash" {} \;

echo -e "\n=== Check bootstrap or installation scripts ==="
head -30 tools/bootstrap.sh 2>/dev/null || echo "bootstrap.sh not found"

Repository: kidchenko/dotfiles

Length of output: 4671

mapfile is not available in Bash 3.2 (default on older macOS).

Line 416 uses mapfile, which was introduced in Bash 4.0. With the current shebang (#!/bin/bash), this will fail on macOS systems with the default Bash 3.2 (though modern macOS now includes Bash 5.x). Since the project explicitly supports macOS, this should be made compatible.

♻️ Bash-3.2-compatible alternative 
         local exclude_args=()
         if [[ ${`#EXCLUDE_PATTERNS`[@]} -gt 0 ]]; then
-            mapfile -t exclude_args < <(build_exclude_args)
+            while IFS= read -r arg; do
+                exclude_args+=("$arg")
+            done < <(build_exclude_args)
         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
local exclude_args=()
if [[ ${#EXCLUDE_PATTERNS[@]} -gt 0 ]]; then
mapfile -t exclude_args < <(build_exclude_args)
fi
local exclude_args=()
if [[ ${`#EXCLUDE_PATTERNS`[@]} -gt 0 ]]; then
while IFS= read -r arg; do
exclude_args+=("$arg")
done < <(build_exclude_args)
fi
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tools/backup-projects.sh` around lines 414 - 417, The script uses Bash-only
`mapfile` to populate local array `exclude_args` from `build_exclude_args`,
which breaks on Bash 3.2; replace the `mapfile` call with a
POSIX/Bash-3.2-compatible loop that reads each line from the
`build_exclude_args` output and appends it to `exclude_args` (preserve the
`local exclude_args=()` declaration and the conditional check on
`EXCLUDE_PATTERNS[@]`), e.g., run `build_exclude_args` via process substitution
or a pipe and use a `while read -r line; do exclude_args+=("$line"); done`
pattern so `exclude_args` receives the same entries without relying on
`mapfile`.


(
umask 077
cd "$HOME" || exit 1
if [[ "$VERBOSE" == true ]]; then
# shellcheck disable=SC2086
zip -r "$archive_path" "${relative_paths[@]}" $exclude_args
zip -r "$archive_path" "${relative_paths[@]}" "${exclude_args[@]}"
else
# shellcheck disable=SC2086
zip -r -q "$archive_path" "${relative_paths[@]}" $exclude_args
zip -r -q "$archive_path" "${relative_paths[@]}" "${exclude_args[@]}"
fi
)

Expand Down
Loading