From eaeaaa93f69a08b8376bb2d69e343d62022460e6 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 15 Oct 2025 14:50:36 -0500 Subject: [PATCH] address PR feedback --- docs/SECURITY.md | 341 +++++++++++++++++++++++++++++ src/core/adb_manager.py | 15 +- src/utils/security_utils.py | 37 ++-- tests/utils/test_security_utils.py | 154 +++++++++++++ 4 files changed, 526 insertions(+), 21 deletions(-) create mode 100644 docs/SECURITY.md diff --git a/docs/SECURITY.md b/docs/SECURITY.md new file mode 100644 index 0000000..0acc1bb --- /dev/null +++ b/docs/SECURITY.md @@ -0,0 +1,341 @@ +# Security Design Document + +## Overview + +This document describes the security design, threat model, and known limitations of the Android File Handler ADB application. The application implements defense-in-depth security controls to protect against command injection, path traversal, and other common attack vectors. + +## Threat Model + +### Assets Protected +1. **Local Filesystem**: User's files and directories on the host system +2. **Android Device Data**: Files and directories on the connected Android device +3. **System Integrity**: Protection against arbitrary command execution +4. **User Privacy**: Prevention of unauthorized access to sensitive files + +### Threat Actors +1. **Malicious Files**: Specially-crafted filenames designed to exploit command injection vulnerabilities +2. **Compromised Android Device**: A device that may attempt to exploit the host system through malicious file metadata +3. **Malicious Input**: User-provided paths or device IDs containing attack payloads +4. **Man-in-the-Middle**: Attacks during ADB platform-tools download (partial mitigation) + +### Attack Vectors + +#### 1. Command Injection +**Description**: Attacker attempts to inject shell commands through user-controlled inputs (paths, device IDs, filenames). + +**Mitigations**: +- Input sanitization with regex-based dangerous character detection +- Subprocess execution without `shell=True` (arguments passed as list, not string) +- Validation of all user-controlled inputs before use +- Specific error messages for rejected inputs + +**Examples Blocked**: +``` +/sdcard/file; rm -rf / +/sdcard/$(whoami) +device123; malicious_command +``` + +#### 2. Path Traversal +**Description**: Attacker attempts to access files outside of intended directories using `..` or symbolic links. + +**Mitigations**: +- Path normalization using `os.path.normpath()` and `os.path.realpath()` +- Symlink resolution to detect symlink-based escape attempts +- Base directory validation for local paths +- Rejection of null bytes in paths + +**Examples Blocked**: +``` +/tmp/safe/../../../etc/passwd +[symlink from /tmp/safe/escape -> /etc/] +/tmp/file\x00.txt +``` + +#### 3. Zip Bomb / Archive Bomb +**Description**: Maliciously crafted compressed files that expand to consume excessive disk space. + +**Mitigations**: +- Size limit checks on downloaded files +- Extraction size validation +- Disk space checks before download + +**Implementation**: See `src/core/platform_tools.py` download validation. + +#### 4. Redirect Attacks +**Description**: Malicious redirects during platform-tools download that could lead to downloading malware. + +**Mitigations**: +- URL validation for redirects +- HTTPS enforcement +- Domain validation for official sources + +**Implementation**: See `src/core/platform_tools.py` download validation. + +## Security Controls + +### Input Sanitization Functions + +#### `sanitize_path_component(component: str)` +**Purpose**: Validates individual path components (filenames, directory names). + +**Checks**: +- Non-empty string +- No null bytes (`\x00`) +- No shell metacharacters: `;`, `|`, `&`, `$`, `` ` ``, `\n`, `\r`, `>`, `<`, `(`, `)`, `{`, `}`, `[`, `]`, `!` +- No command substitution patterns: `$(`, `${` + +**Usage**: Used for validating individual filename components. + +#### `sanitize_android_path(path: str)` +**Purpose**: Validates full paths on Android devices. + +**Checks**: +- Non-empty string +- No null bytes (`\x00`) +- No dangerous patterns: `;`, `|`, `&`, `` ` ``, `\n`, `\r`, `$(`, `${`, `&&`, `||`, `>>` +- **Allows**: Spaces, Unicode characters, forward slashes, dots + +**Usage**: Used for all Android device paths before passing to ADB commands. + +**Rationale**: Android filesystems support Unicode and spaces in filenames. We only block patterns that could enable command injection. + +#### `sanitize_local_path(path: str, base_dir: Optional[str])` +**Purpose**: Validates and normalizes local filesystem paths. + +**Checks**: +- Non-empty string +- No null bytes (`\x00`) +- Path normalization via `os.path.normpath(os.path.realpath())` +- Symlink resolution to detect escapes +- Optional base directory containment validation + +**Usage**: Used for local filesystem paths, especially when restricting operations to specific directories. + +**Rationale**: Using `realpath()` instead of `abspath()` ensures symbolic links are resolved before validation, preventing symlink-based path traversal. + +#### `validate_device_id(device_id: str)` +**Purpose**: Validates Android device IDs. + +**Checks**: +- Non-empty string +- Alphanumeric characters, dots, colons, underscores, hyphens only +- No shell metacharacters +- No spaces + +**Usage**: Validates device IDs before using in ADB commands with `-s` flag. + +**Valid Examples**: +``` +ABC123DEF456 (serial number) +192.168.1.100:5555 (network device) +emulator-5554 (emulator) +``` + +### Subprocess Execution + +**Safe Pattern**: +```python +# SAFE: Arguments as list, no shell=True +subprocess.run([adb_path, "-s", device_id, "shell", "ls", path]) +``` + +**Unsafe Pattern** (NOT USED): +```python +# UNSAFE: Shell=True enables command injection +subprocess.run(f"adb -s {device_id} shell ls {path}", shell=True) +``` + +**Implementation**: All ADB commands use argument lists without `shell=True`, preventing shell interpretation of metacharacters. + +### Error Handling + +**Logging**: Validation failures are logged with specific error messages to help detect attack attempts and debug legitimate issues. + +**User Feedback**: Failed operations return descriptive error messages indicating why paths or device IDs were rejected. + +**Silent Failures**: Removed in favor of explicit logging (see `src/core/adb_manager.py` methods `list_files()` and `get_file_info()`). + +## Known Limitations + +### 1. Android Device Trust +**Limitation**: The application trusts the connected Android device to return valid data. + +**Risk**: A compromised or malicious device could return crafted data through ADB responses. + +**Mitigation**: Input sanitization is applied to user-provided inputs, but responses from `adb shell` commands are parsed but not fully sanitized. The subprocess argument list pattern prevents command injection even with malicious device responses. + +**Residual Risk**: Low. Device responses are parsed but not executed as commands. + +### 2. ADB Binary Trust +**Limitation**: The application trusts the ADB binary downloaded from Google's servers. + +**Risk**: If download is intercepted (MITM) or if Google's servers are compromised, malicious ADB binary could be installed. + +**Mitigation**: +- HTTPS is used for downloads +- URL validation for redirects +- Downloads only from official Google domains + +**Residual Risk**: Low to Medium. Consider adding SHA-256 hash verification in future versions. + +### 3. Local Filesystem Permissions +**Limitation**: The application runs with the same permissions as the user who launched it. + +**Risk**: If user has write access to system directories, the application could be used to overwrite important files (though not through exploitation). + +**Mitigation**: Application uses standard OS permissions. Users should not run the application with elevated privileges unless necessary. + +**Residual Risk**: Low. This is standard behavior for desktop applications. + +### 4. Unicode Normalization +**Limitation**: Unicode characters are allowed but not normalized (e.g., no NFC/NFD conversion). + +**Risk**: Different Unicode representations of the same visual character could bypass filters or cause confusion. + +**Mitigation**: Characters are checked for dangerous patterns regardless of Unicode form. + +**Residual Risk**: Very Low. Path validation is performed before use. + +### 5. Race Conditions +**Limitation**: Time-of-check to time-of-use (TOCTOU) race conditions are possible with filesystem operations. + +**Risk**: A symlink or file could be changed between validation and use. + +**Mitigation**: Paths are validated immediately before use. Symlinks are resolved during validation. + +**Residual Risk**: Very Low. Window for exploitation is extremely small and requires local access. + +### 6. Platform-Specific Behavior +**Limitation**: Path handling differs between Windows, Linux, and macOS. + +**Risk**: Platform-specific path normalization could behave unexpectedly. + +**Mitigation**: +- Use of `os.path` functions for cross-platform compatibility +- Comprehensive tests for different path formats +- Separate handling for Windows root paths in file transfer module + +**Residual Risk**: Low. Extensive testing covers common scenarios. + +## Security Testing + +### Test Coverage +The security validation suite includes tests for: + +1. **Command Injection Prevention** + - Shell metacharacters in paths + - Command substitution patterns + - Backtick substitution + - Newline injection + +2. **Path Traversal Prevention** + - `..` sequences + - Absolute path escapes + - Symlink-based escapes + - Null byte injection + +3. **Unicode Handling** + - Chinese, Russian, Arabic, Emoji characters + - Accented characters + - Mixed Unicode and spaces + +4. **Edge Cases** + - Very long paths (100+ directory levels) + - Very long filenames (255+ characters) + - Paths with multiple dots + - Hidden files (leading dot) + +5. **Cross-Platform** + - Windows-style paths (`C:\Users\...`) + - Unix-style paths (`/tmp/...`) + - Mixed path separators + - Platform-specific normalization + +6. **Device ID Validation** + - Serial numbers + - Network addresses with ports + - Emulator IDs + - Invalid characters + +### Test Location +All security tests are located in: `tests/utils/test_security_utils.py` + +Run tests with: +```bash +poetry run pytest tests/utils/test_security_utils.py -v +``` + +## Security Maintenance + +### Regular Reviews +Security controls should be reviewed: +- When adding new features that accept user input +- When modifying path handling or subprocess execution +- After discovering vulnerabilities in similar applications +- At least annually + +### Dependency Updates +Keep dependencies updated to patch security vulnerabilities: +```bash +poetry update +poetry run pytest # Verify no regressions +``` + +### Vulnerability Reporting +Security issues should be reported via GitHub Issues with the `security` label. + +## Compliance and Best Practices + +### OWASP Guidelines +This implementation follows OWASP recommendations for: +- Input validation (positive security model where possible) +- Output encoding (subprocess argument lists) +- Command injection prevention +- Path traversal prevention + +### Python Security Best Practices +- No use of `eval()`, `exec()`, or `compile()` +- No `shell=True` in subprocess calls +- Type hints for all security-critical functions +- Comprehensive error handling + +### Defense in Depth +Multiple layers of security: +1. Input validation (first line of defense) +2. Subprocess argument lists (prevent shell interpretation) +3. Path normalization (prevent traversal) +4. Symlink resolution (prevent escapes) +5. Logging (detection and debugging) + +## Future Enhancements + +### Recommended Improvements +1. **SHA-256 Hash Verification**: Verify ADB binary downloads against known-good hashes +2. **Code Signing**: Sign application binaries for distribution +3. **Sandboxing**: Consider running ADB operations in a restricted environment +4. **Rate Limiting**: Prevent brute-force attempts on path validation +5. **Audit Logging**: Enhanced logging for security-relevant events +6. **Unicode Normalization**: Normalize Unicode strings to prevent bypass attempts + +### Not Recommended +1. **Filename Whitelisting**: Too restrictive for international users +2. **Path Length Limits**: Android supports long paths; artificial limits harm usability +3. **Blocking All Special Characters**: Many legitimate filenames use special characters + +## References + +- [OWASP Command Injection](https://owasp.org/www-community/attacks/Command_Injection) +- [OWASP Path Traversal](https://owasp.org/www-community/attacks/Path_Traversal) +- [CWE-78: OS Command Injection](https://cwe.mitre.org/data/definitions/78.html) +- [CWE-22: Path Traversal](https://cwe.mitre.org/data/definitions/22.html) +- [Android File System Permissions](https://source.android.com/docs/core/permissions/filesystem) + +## Version History + +- **v1.0** (2025-10-15): Initial security design documentation + - Command injection prevention + - Path traversal prevention + - Symlink resolution + - Comprehensive test coverage + - Error logging for validation failures diff --git a/src/core/adb_manager.py b/src/core/adb_manager.py index 3993dce..be32082 100644 --- a/src/core/adb_manager.py +++ b/src/core/adb_manager.py @@ -7,6 +7,7 @@ import os import sys import shutil import subprocess +import logging from typing import Optional, Tuple, Callable # Import our modular components @@ -34,6 +35,8 @@ except ImportError: OS_TYPE = sys.platform +logger = logging.getLogger(__name__) + class ADBManager: """Main interface for ADB operations, device management, and file transfers.""" @@ -146,7 +149,8 @@ class ADBManager: try: sanitized_path = sanitize_android_path(path) except ValueError as e: - # Return empty list if path is invalid + # Log validation error and return empty list + logger.warning(f"Invalid path rejected in list_files: {str(e)}") return [] device_args = [] @@ -155,7 +159,8 @@ class ADBManager: try: validated_device = validate_device_id(target_device) device_args = ["-s", validated_device] - except ValueError: + except ValueError as e: + logger.warning(f"Invalid device ID rejected in list_files: {str(e)}") return [] args = device_args + ["shell", "ls", "-la", sanitized_path] @@ -403,7 +408,8 @@ class ADBManager: # Sanitize inputs to prevent command injection try: sanitized_path = sanitize_android_path(remote_path) - except ValueError: + except ValueError as e: + logger.warning(f"Invalid path rejected in get_file_info: {str(e)}") return None device_args = [] @@ -412,7 +418,8 @@ class ADBManager: try: validated_device = validate_device_id(target_device) device_args = ["-s", validated_device] - except ValueError: + except ValueError as e: + logger.warning(f"Invalid device ID rejected in get_file_info: {str(e)}") return None args = device_args + ["shell", "ls", "-la", sanitized_path] diff --git a/src/utils/security_utils.py b/src/utils/security_utils.py index 1e70cdb..726f54b 100644 --- a/src/utils/security_utils.py +++ b/src/utils/security_utils.py @@ -23,14 +23,19 @@ def sanitize_path_component(component: str) -> str: if not component: raise ValueError("Path component cannot be empty") - # Check for dangerous characters that could be used for command injection - dangerous_chars = [';', '|', '&', '$', '`', '\n', '\r', '>', '<', '(', ')', '{', '}', '[', ']', '!'] - for char in dangerous_chars: - if char in component: - raise ValueError(f"Path component contains dangerous character: {char}") + # Check for null bytes + if '\x00' in component: + raise ValueError("Path component contains null byte") + + # Check for dangerous characters using optimized regex + # Matches any shell metacharacters that could be used for command injection + dangerous_pattern = r'[;|&$`\n\r><(){}[\]!]' + match = re.search(dangerous_pattern, component) + if match: + raise ValueError(f"Path component contains dangerous character: {match.group()}") # Check for command substitution patterns - if '$(' in component or '${' in component or '`' in component: + if '$(' in component or '${' in component: raise ValueError("Path component contains command substitution pattern") return component @@ -58,17 +63,15 @@ def sanitize_android_path(path: str) -> str: if '\x00' in path: raise ValueError("Path contains null byte") - # Check for command injection patterns + # Check for command injection patterns using optimized regex # Note: We check for shell metacharacters that could be used for command injection # We allow spaces and most characters that are valid in Android paths - dangerous_patterns = [ - ';', '|', '&', '$(', '${', '`', '\n', '\r', - '&&', '||', '>>', - ] - - for pattern in dangerous_patterns: - if pattern in path: - raise ValueError(f"Path contains dangerous pattern: {pattern}") + # Pattern matches: semicolon, pipe, ampersand, dollar-paren, dollar-brace, + # backtick, newline, carriage return, double-ampersand, double-pipe, double-redirect + dangerous_pattern = r'[;|&`\n\r]|\$[({]|&&|\|\||>>' + match = re.search(dangerous_pattern, path) + if match: + raise ValueError(f"Path contains dangerous pattern: {match.group()}") # Note: We allow spaces, Unicode characters, and other characters that are # valid in Android filesystem paths. The dangerous pattern check above is @@ -103,7 +106,7 @@ def sanitize_local_path(path: str, base_dir: Optional[str] = None) -> str: # Normalize the path to resolve .. and symlinks try: - normalized_path = os.path.normpath(os.path.abspath(path)) + normalized_path = os.path.normpath(os.path.realpath(path)) except (ValueError, OSError) as e: raise ValueError(f"Invalid path: {e}") @@ -112,7 +115,7 @@ def sanitize_local_path(path: str, base_dir: Optional[str] = None) -> str: # with base_dir, it's outside the allowed directory tree if base_dir: try: - base_dir_abs = os.path.normpath(os.path.abspath(base_dir)) + base_dir_abs = os.path.normpath(os.path.realpath(base_dir)) # Check if the normalized path starts with the base directory if not normalized_path.startswith(base_dir_abs + os.sep) and normalized_path != base_dir_abs: raise ValueError(f"Path traversal detected: path is outside base directory") diff --git a/tests/utils/test_security_utils.py b/tests/utils/test_security_utils.py index 7de14e4..a03628f 100644 --- a/tests/utils/test_security_utils.py +++ b/tests/utils/test_security_utils.py @@ -40,6 +40,11 @@ class TestSanitizePathComponent: with pytest.raises(ValueError, match="dangerous character"): sanitize_path_component("file${USER}.txt") + def test_null_byte(self): + """Test that null bytes are rejected.""" + with pytest.raises(ValueError, match="null byte"): + sanitize_path_component("file\x00name.txt") + class TestSanitizeAndroidPath: """Tests for sanitize_android_path function.""" @@ -178,3 +183,152 @@ class TestSecurityIntegration: base = "/tmp/restricted" with pytest.raises(ValueError): sanitize_local_path("/etc/passwd", base_dir=base) + + def test_symlink_attack_prevention(self): + """Test that symlink-based path traversal is blocked.""" + import tempfile + with tempfile.TemporaryDirectory() as tmpdir: + # Create a base directory + base_dir = os.path.join(tmpdir, "safe") + os.makedirs(base_dir) + + # Create a directory outside the base + outside_dir = os.path.join(tmpdir, "outside") + os.makedirs(outside_dir) + + # Create a symlink inside the base that points outside + symlink_path = os.path.join(base_dir, "escape") + os.symlink(outside_dir, symlink_path) + + # Attempt to use the symlink should fail base_dir validation + with pytest.raises(ValueError, match="outside base directory"): + sanitize_local_path(symlink_path, base_dir=base_dir) + + +class TestUnicodeAndEdgeCases: + """Tests for Unicode characters, long paths, and cross-platform handling.""" + + def test_unicode_characters_in_android_path(self): + """Test that Unicode characters are accepted in Android paths.""" + # Common Unicode characters in filenames + unicode_paths = [ + "/sdcard/照片/vacation.jpg", # Chinese + "/sdcard/Фото/image.png", # Russian + "/sdcard/صور/photo.jpg", # Arabic + "/sdcard/🎉/emoji.txt", # Emoji + "/sdcard/Ménü/file.txt", # Accented characters + ] + for path in unicode_paths: + result = sanitize_android_path(path) + assert result == path + + def test_unicode_characters_in_path_component(self): + """Test that Unicode characters are accepted in path components.""" + unicode_components = [ + "文件.txt", # Chinese + "файл.doc", # Russian + "ملف.pdf", # Arabic + "archivo_español.txt", # Spanish + ] + for component in unicode_components: + result = sanitize_path_component(component) + assert result == component + + def test_very_long_android_path(self): + """Test that very long paths are handled correctly.""" + # Create a path with many nested directories + long_path = "/sdcard/" + "/".join([f"dir{i}" for i in range(100)]) + "/file.txt" + result = sanitize_android_path(long_path) + assert result == long_path + + def test_very_long_path_component(self): + """Test that very long path components are accepted.""" + # Android typically supports filenames up to 255 characters + long_component = "a" * 255 + result = sanitize_path_component(long_component) + assert result == long_component + + def test_extremely_long_path_component(self): + """Test that extremely long path components are accepted.""" + # Test a 1000 character filename + very_long_component = "x" * 1000 + result = sanitize_path_component(very_long_component) + assert result == very_long_component + + def test_local_path_windows_style(self): + """Test that Windows-style paths are normalized correctly.""" + import platform + if platform.system() == "Windows": + # Windows paths should be normalized + result = sanitize_local_path("C:\\Users\\Test\\Documents") + assert os.path.isabs(result) + assert "\\" in result or "/" in result # May be normalized + + def test_local_path_unix_style(self): + """Test that Unix-style paths are normalized correctly.""" + result = sanitize_local_path("/tmp/test/file.txt") + assert os.path.isabs(result) + + def test_local_path_with_mixed_separators(self): + """Test that paths with mixed separators are normalized.""" + import platform + if platform.system() == "Windows": + # Windows should handle mixed separators + mixed_path = "C:/Users\\Test/Documents" + result = sanitize_local_path(mixed_path) + assert os.path.isabs(result) + + def test_android_path_with_spaces_and_unicode(self): + """Test paths with both spaces and Unicode characters.""" + path = "/sdcard/My Photos 照片/vacation 2023.jpg" + result = sanitize_android_path(path) + assert result == path + + def test_device_id_with_port_number(self): + """Test device IDs with port numbers (emulators and network devices).""" + device_ids = [ + "192.168.1.100:5555", + "10.0.2.15:5037", + "emulator-5554", + "emulator-5556", + ] + for device_id in device_ids: + result = validate_device_id(device_id) + assert result == device_id + + def test_device_id_serial_numbers(self): + """Test various device serial number formats.""" + device_ids = [ + "ABC123DEF456", + "ZX1G427QK9", + "R5CR40CPDXD", + "ce12160c1a2d0b1f01", + ] + for device_id in device_ids: + result = validate_device_id(device_id) + assert result == device_id + + def test_path_component_with_dots(self): + """Test that legitimate dots in filenames are allowed.""" + components = [ + "file.name.with.dots.txt", + "archive.tar.gz", + ".hidden", + "..hidden_but_safe", # Double dot NOT used for traversal + ] + for component in components: + result = sanitize_path_component(component) + assert result == component + + def test_android_path_normalization_preserves_intent(self): + """Test that path normalization preserves the original intent.""" + paths = [ + "/sdcard/DCIM/Camera", + "/data/local/tmp", + "/storage/emulated/0/Download", + "./relative/path/file.txt", + ] + for path in paths: + result = sanitize_android_path(path) + # Should preserve the original path structure + assert result == path