# 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](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:**
```python
for line in os.popen("udevadm info -q env -n %s" % name):
    # Process output
```

**After:**
```python
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](tasks.py), [libtisbackup/backup_xva.py](libtisbackup/backup_xva.py)

**Changes:**
- [tasks.py:37](tasks.py#L37): Changed `os.system("/bin/umount %s")` to `subprocess.run(["/bin/umount", mount_point])`
- [backup_xva.py:199](libtisbackup/backup_xva.py#L199): 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](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:**
```python
# 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](tisbackup_gui.py)

**Before:**
```python
line = open(elem).readline()
```

**After:**
```python
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](tisbackup_gui.py#L415)

**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](tisbackup_gui.py)

**Before:**
```python
from shutil import *
```

**After:**
```python
import shutil
import subprocess
```

**Impact:** Cleaner namespace, easier to track dependencies

### 7. Fixed Hardcoded Secret Key
**Files Modified:** [tisbackup_gui.py:67-79](tisbackup_gui.py#L67), [README.md](README.md)

**Before:**
```python
app.secret_key = "fsiqefiuqsefARZ4Zfesfe34234dfzefzfe"
```

**After:**
```python
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:**
```bash
# 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](libtisbackup/common.py#L140), all backup drivers, [README.md](README.md)

**Before:**
```python
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:**
```python
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](libtisbackup/common.py): `do_preexec`, `do_postexec`, `run_remote_command`
  - [backup_mysql.py](libtisbackup/backup_mysql.py)
  - [backup_pgsql.py](libtisbackup/backup_pgsql.py)
  - [backup_sqlserver.py](libtisbackup/backup_sqlserver.py)
  - [backup_oracle.py](libtisbackup/backup_oracle.py)
  - [backup_samba4.py](libtisbackup/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:**
```bash
# 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](libtisbackup/common.py#L649))
```python
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#L128)
- [libtisbackup/common.py:883](libtisbackup/common.py#L883)
- [libtisbackup/common.py:986](libtisbackup/common.py#L986)
- [libtisbackup/backup_rsync.py:176](libtisbackup/backup_rsync.py#L176)
- [libtisbackup/backup_rsync_btrfs.py](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)