TISbackup/SECURITY_IMPROVEMENTS.md

Security and Code Quality Improvements

This document summarizes the security and code quality improvements made to TISBackup.

Completed Improvements (High Priority)

1. Replaced os.popen() with subprocess.run()

Files Modified: tisbackup_gui.py

Changes:

• Replaced deprecated os.popen() calls with modern subprocess.run()
• All subprocess calls now use list arguments instead of shell strings
• Added timeout protection (5-30 seconds depending on operation)
• Proper error handling with try/except blocks

Before:

for line in os.popen("udevadm info -q env -n %s" % name):
    # Process output

After:

result = subprocess.run(
    ["udevadm", "info", "-q", "env", "-n", name],
    capture_output=True,
    text=True,
    check=True,
    timeout=5
)
for line in result.stdout.splitlines():
    # Process output

Security Impact: Prevents command injection vulnerabilities

2. Replaced os.system() with subprocess.run()

Files Modified: tasks.py, libtisbackup/backup_xva.py

Changes:

• tasks.py:37: Changed os.system("/bin/umount %s") to subprocess.run(["/bin/umount", mount_point])
• backup_xva.py:199: Changed os.system('tar tf "%s"') to subprocess.run(["tar", "tf", filename_temp])
• Added proper error handling and logging

Security Impact: Eliminates command injection risk from potentially user-controlled mount points and filenames

3. Added Input Validation

Files Modified: tisbackup_gui.py

Changes:

• Added regex validation for device/partition names: ^/dev/sd[a-z]1?$
• Validates partition names before using in mount/unmount operations
• Prevents path traversal and command injection attacks

Example:

# Validate partition name to prevent command injection
if not re.match(r"^/dev/sd[a-z]1$", partition):
    continue

Security Impact: Prevents malicious input from reaching system commands

4. Fixed File Operations with Context Managers

Files Modified: tisbackup_gui.py

Before:

line = open(elem).readline()

After:

with open(elem) as f:
    line = f.readline()

Impact: Ensures files are properly closed, prevents resource leaks

5. Improved run_command() Function

Files Modified: tisbackup_gui.py:415-453

Changes:

• Now accepts list arguments for safe command execution
• Backward compatible with string commands (marked as legacy)
• Added timeout protection (30 seconds)
• Better error handling and reporting

Security Impact: Provides safe command execution interface while maintaining backward compatibility

6. Removed Wildcard Import

Files Modified: tisbackup_gui.py

Before:

from shutil import *

After:

import shutil
import subprocess

Impact: Cleaner namespace, easier to track dependencies

7. Fixed Hardcoded Secret Key

Files Modified: tisbackup_gui.py:67-79, README.md

Before:

app.secret_key = "fsiqefiuqsefARZ4Zfesfe34234dfzefzfe"

After:

SECRET_KEY = os.environ.get("TISBACKUP_SECRET_KEY")
if not SECRET_KEY:
    import secrets
    SECRET_KEY = secrets.token_hex(32)
    logging.warning(
        "TISBACKUP_SECRET_KEY environment variable not set. "
        "Using a randomly generated secret key. "
        "Sessions will not persist across application restarts. "
        "Set TISBACKUP_SECRET_KEY environment variable for production use."
    )
app.secret_key = SECRET_KEY

Changes:

• Reads secret key from TISBACKUP_SECRET_KEY environment variable
• Falls back to cryptographically secure random key if not set
• Logs warning when using random key (sessions won't persist across restarts)
• Uses Python's secrets module for cryptographically strong random generation
• Updated README.md with setup instructions

Setup Instructions:

# Generate a secure secret key
python3 -c "import secrets; print(secrets.token_hex(32))"

# Set in Docker Compose (compose.yml)
environment:
  - TISBACKUP_SECRET_KEY=your-generated-key-here

# Or export in shell
export TISBACKUP_SECRET_KEY=your-generated-key-here

Security Impact: Eliminates hardcoded secret in source code, prevents session hijacking and CSRF attacks

Remaining Security Issues (Critical - Not Fixed)

1. No Authentication on Flask Routes

All routes are publicly accessible without authentication.

Recommendation: Implement Flask-Login or similar authentication

2. Insecure SSH Host Key Policy (libtisbackup/common.py:649)

ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())

Recommendation: Use proper host key verification with known_hosts

3. Command Injection in Legacy Code

Multiple files still use subprocess.call(shell_string, shell=True) and subprocess.Popen(..., shell=True):

• libtisbackup/common.py:128
• libtisbackup/common.py:883
• libtisbackup/common.py:986
• libtisbackup/backup_rsync.py:176
• libtisbackup/backup_rsync_btrfs.py (multiple locations)

Recommendation: Refactor to use list arguments without shell=True

Code Quality Issues Remaining

1. Global State Management - Use Flask application context instead
2. Wildcard imports from common - from libtisbackup.common import *
3. Configuration loaded at module level - Should use application factory pattern
4. Duplicated code - read_config() and read_all_configs() share significant logic

Testing Recommendations

Before deploying these changes:

1. Test USB disk detection and mounting functionality
2. Test backup export operations
3. Verify XVA backup tar validation
4. Test error handling for invalid device names
5. Verify backward compatibility with existing configurations

Migration Notes

All changes are backward compatible. The run_command() function accepts both:

• New format: run_command(["/bin/command", "arg1", "arg2"])
• Legacy format: run_command("/bin/command arg1 arg2") (less secure, marked for deprecation)