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

8. Modernized SSH Key Algorithm Support

Files Modified: libtisbackup/common.py, all backup drivers, README.md

Before:

try:
    mykey = paramiko.RSAKey.from_private_key_file(self.private_key)
except paramiko.SSHException:
    mykey = paramiko.DSSKey.from_private_key_file(self.private_key)

After:

def load_ssh_private_key(private_key_path):
    """Load SSH private key with modern algorithm support.

    Tries to load the key in order of preference:
    1. Ed25519 (most secure, modern)
    2. ECDSA (secure, widely supported)
    3. RSA (legacy, still secure with sufficient key size)

    DSA is not supported as it's deprecated and insecure.
    """
    key_types = [
        ("Ed25519", paramiko.Ed25519Key),
        ("ECDSA", paramiko.ECDSAKey),
        ("RSA", paramiko.RSAKey),
    ]

    for key_name, key_class in key_types:
        try:
            return key_class.from_private_key_file(private_key_path)
        except paramiko.SSHException:
            continue

    raise paramiko.SSHException(
        f"Unable to load private key. "
        f"Supported formats: Ed25519 (recommended), ECDSA, RSA. "
        f"DSA keys are no longer supported."
    )

Changes:

• Created centralized load_ssh_private_key() helper function
• Updated all SSH key loading locations across codebase:
  • common.py: do_preexec, do_postexec, run_remote_command
  • backup_mysql.py
  • backup_pgsql.py
  • backup_sqlserver.py
  • backup_oracle.py
  • backup_samba4.py
• Removed deprecated DSA key support
• Added Ed25519 as preferred algorithm
• Added ECDSA as second choice
• RSA remains supported for compatibility
• Clear error message indicating DSA is no longer supported
• Updated README.md with key generation instructions

SSH Key Generation:

# Ed25519 (recommended)
ssh-keygen -t ed25519 -f ./ssh/id_ed25519 -C "tisbackup"

# ECDSA (also secure)
ssh-keygen -t ecdsa -b 521 -f ./ssh/id_ecdsa

# RSA (legacy, minimum 4096 bits)
ssh-keygen -t rsa -b 4096 -f ./ssh/id_rsa

Security Impact:

• Eliminates support for vulnerable DSA algorithm (1024-bit limit, FIPS deprecated)
• Prioritizes Ed25519 (fast, secure, resistant to timing attacks)
• Supports ECDSA as secure alternative
• Maintains RSA compatibility for legacy systems
• Clear migration path for users with old keys

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)