chore(backup): enforce secure permissions and fix arg injection#34
chore(backup): enforce secure permissions and fix arg injection#34
Conversation
- Explicitly set chmod 700 on backup and log directories to prevent unauthorized access.
- Use umask 077 when creating zip archives to ensure they are readable only by the owner (0600).
- Refactor zip exclusion handling to use bash arrays ("${exclude_args[@]}") instead of string splitting, fixing potential argument injection and issues with spaces in filenames.
- Remove obsolete `build_exclude_args` function.
Co-authored-by: kidchenko <5432753+kidchenko@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/backup-projects.sh (2)
427-427: Permission claim in message is asserted, not measured
(permissions: 0600)is hardcoded based on theumask 077assumption, but is not verified. Under normal execution this is always correct; however, if thezipbinary is wrapped or something else modifies the file after creation, the message would silently lie. Consider reading the actual mode withstat:🔍 Optional: verify before asserting
- say "Archive created: $archive_size (permissions: 0600)" + local archive_perms + archive_perms=$(stat -c "%a" "$archive_path" 2>/dev/null || stat -f "%OLp" "$archive_path" 2>/dev/null) + say "Archive created: $archive_size (permissions: ${archive_perms:-0600})"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/backup-projects.sh` at line 427, The message currently hardcodes "(permissions: 0600)" after archive creation; instead read the actual file mode and report it. After the archive is created, call stat to get its mode (e.g., stat -c '%a' "$archive" or fallback to stat -f '%Lp' "$archive" for BSD/macOS) and store it (e.g., archive_mode), then change the say invocation to use "$archive_size" and the queried "$archive_mode" rather than the hardcoded 0600; reference the existing say call and the archive variable used when creating the zip so the message always reflects the real file permissions.
344-347:mkdir -p+chmodcreates a brief TOCTOU window; the proposedinstallfix does not solve the intermediate directory issueThe original concern about TOCTOU is valid:
mkdir -pcreates the directory with the default umask (typically0755), thenchmod 700tightens it. However, the proposed fix usinginstall -d -m 700does not actually address the intermediate directory problem.According to GNU coreutils documentation,
install -d -m MODEdoes not apply the mode to parent directories—they are always created with0755regardless of-m. BSDinstallon macOS does not document this behavior, making it unreliable.For this script, the practical risk is low:
- The TOCTOU window is very brief (milliseconds on a single-user machine).
- Intermediate directories being
0755is acceptable since the finalbackups/directory is0700, which prevents traversal by other users even if parents are readable.If you want to address both concerns, a more portable approach is to create the directory tree with secure permissions atomically:
Alternative: atomic directory creation with umask
- mkdir -p "$BACKUP_TEMP_DIR" - chmod 700 "$BACKUP_TEMP_DIR" - mkdir -p "$LOG_DIR" - chmod 700 "$LOG_DIR" + ( umask 077; mkdir -p "$BACKUP_TEMP_DIR" ) + ( umask 077; mkdir -p "$LOG_DIR" )Alternatively, accept the current approach as sufficient for a single-user backup utility where the parent hierarchy is under
$HOME.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/backup-projects.sh` around lines 344 - 347, The mkdir + chmod sequence for BACKUP_TEMP_DIR and LOG_DIR creates a brief TOCTOU window; to eliminate it portably, save the current umask, set umask to 077 (so newly created directories are 0700), call mkdir -p "$BACKUP_TEMP_DIR" and mkdir -p "$LOG_DIR", then immediately restore the saved umask; keep the chmod 700 as a safe fallback after restore in case the directories already existed with different perms. Ensure you reference and modify the existing mkdir -p/ chmod 700 uses for BACKUP_TEMP_DIR and LOG_DIR and restore umask even on error (use a trap or ensure restore happens).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/backup-projects.sh`:
- Line 427: The message currently hardcodes "(permissions: 0600)" after archive
creation; instead read the actual file mode and report it. After the archive is
created, call stat to get its mode (e.g., stat -c '%a' "$archive" or fallback to
stat -f '%Lp' "$archive" for BSD/macOS) and store it (e.g., archive_mode), then
change the say invocation to use "$archive_size" and the queried "$archive_mode"
rather than the hardcoded 0600; reference the existing say call and the archive
variable used when creating the zip so the message always reflects the real file
permissions.
- Around line 344-347: The mkdir + chmod sequence for BACKUP_TEMP_DIR and
LOG_DIR creates a brief TOCTOU window; to eliminate it portably, save the
current umask, set umask to 077 (so newly created directories are 0700), call
mkdir -p "$BACKUP_TEMP_DIR" and mkdir -p "$LOG_DIR", then immediately restore
the saved umask; keep the chmod 700 as a safe fallback after restore in case the
directories already existed with different perms. Ensure you reference and
modify the existing mkdir -p/ chmod 700 uses for BACKUP_TEMP_DIR and LOG_DIR and
restore umask even on error (use a trap or ensure restore happens).
1 participant
🛡️ Sentinel: [MEDIUM] Fix insecure temporary file creation and argument injection
Vulnerability:
The
tools/backup-projects.shscript created backup directories with default umask permissions, potentially allowing other users on the system to read sensitive source code backups. Additionally, the script constructed zip exclusion arguments as a single string, which could lead to argument injection or failure if patterns contained spaces.Impact:
Fix:
0700) on backup directories immediately after creation.umask 077in a subshell before creating the backup zip file to ensure0600permissions.Verification:
tools/backup-projects.sh --dry-runand custom test script that permissions are correctly set (700 for dir, 600 for file) and spaces in exclusions are handled correctly../build.sh syntax.PR created automatically by Jules for task 12934425055427575936 started by @kidchenko
Summary by CodeRabbit