From 388f26ad5ee98ae5fd2d39b103305971faea3170 Mon Sep 17 00:00:00 2001 From: Ajin Abraham Date: Sun, 20 Sep 2026 16:30:53 -0700 Subject: [PATCH] Harden path traversal against Windows root-relative escapes (#2673) * Harden path traversal checks for Windows root-relative escapes. Reject POSIX/Windows absolute paths in is_path_traversal and require post-join containment at icon, network-config, and archive extraction sinks. Co-authored-by: Cursor * Restrict network security config names to a basename. CodeQL flagged the manifest-derived join; reduce the resource name first so path construction cannot escape xml_dir. Co-authored-by: Cursor * Use os.path.basename for CodeQL-recognized path sanitization. Path.name did not clear py/path-injection taint; basename plus existing containment checks keep the network-config read inside xml_dir. Co-authored-by: Cursor * Drop CodeQL-appeasement basename sanitization from network config reads. is_safe_path after join already rejects traversal, absolute, and Windows root-relative names; CodeQL py/path-injection is a false positive. Co-authored-by: Cursor --------- Co-authored-by: Cursor --- .github/SECURITY.md | 1 + mobsf/DynamicAnalyzer/views/common/shared.py | 10 ++- mobsf/MobSF/security.py | 84 ++++++++++--------- mobsf/MobSF/tools_download.py | 7 +- .../tools/androguard4/apkinspector/extract.py | 7 +- .../views/android/icon_analysis.py | 32 ++++++- .../views/android/network_security.py | 16 ++-- .../views/common/shared_func.py | 14 +++- 8 files changed, 113 insertions(+), 58 deletions(-) diff --git a/.github/SECURITY.md b/.github/SECURITY.md index 872921f3..a0f7cee1 100644 --- a/.github/SECURITY.md +++ b/.github/SECURITY.md @@ -10,6 +10,7 @@ Please report all security issues [here](https://github.com/MobSF/Mobile-Securit | Vulnerability | Affected Versions | | ------- | ------------------ | +| [Windows-only path traversal via root-relative paths bypasses is_path_traversal()](https://github.com/MobSF/Mobile-Security-Framework-MobSF/security/advisories/GHSA-c8g7-42qj-frj3) | `<=4.5.3` | | [SSRF port restriction bypass in assetlinks_check](https://github.com/MobSF/Mobile-Security-Framework-MobSF/security/advisories/GHSA-95px-34x5-p37h) | `<=4.5.0` | | [Zip Bomb Denial of Service via Per-File Size Limit Bypass in ZIP/APK](https://github.com/MobSF/Mobile-Security-Framework-MobSF/security/advisories/GHSA-x768-8642-mmq9) | `<=4.5.0` | | [Regression: CSRF checks not enforced after Django migration](https://github.com/MobSF/Mobile-Security-Framework-MobSF/security/advisories/GHSA-3p54-567p-2wpr) | `<=4.5.0` | diff --git a/mobsf/DynamicAnalyzer/views/common/shared.py b/mobsf/DynamicAnalyzer/views/common/shared.py index 82362352..42c1194e 100644 --- a/mobsf/DynamicAnalyzer/views/common/shared.py +++ b/mobsf/DynamicAnalyzer/views/common/shared.py @@ -15,7 +15,11 @@ from mobsf.MalwareAnalyzer.views.MalwareDomainCheck import ( MalwareDomainCheck, ) from mobsf.MobSF.exceptions import PathTraversalError -from mobsf.MobSF.security import clean_filename, is_pipe_or_link +from mobsf.MobSF.security import ( + clean_filename, + is_pipe_or_link, + is_safe_path, +) from mobsf.MobSF.utils import ( EMAIL_REGEX, URL_REGEX, @@ -108,8 +112,8 @@ def untar_files(tar_loc, untar_dir): continue member_path = os.path.realpath( os.path.join(safe_root, member.name)) - if not (member_path.startswith(safe_root + os.sep) - or member_path == safe_root): + if not is_safe_path( + safe_root, member_path, member.name): raise PathTraversalError( 'Attempted Path Traversal in Tar File') safe_members.append(member) diff --git a/mobsf/MobSF/security.py b/mobsf/MobSF/security.py index 459d24a8..48308cc9 100644 --- a/mobsf/MobSF/security.py +++ b/mobsf/MobSF/security.py @@ -1,25 +1,29 @@ """Runtime Executable Tampering Detection.""" -import subprocess import functools +import ipaddress import logging +import ntpath +import os +import posixpath import re import socket -import ipaddress -import sys -from contextlib import contextmanager -from shutil import which -from pathlib import Path -from platform import system -import os import stat import string +import subprocess +import sys import unicodedata -from urllib.parse import unquote, urljoin, urlparse, urlunsplit from concurrent.futures import ThreadPoolExecutor +from contextlib import contextmanager +from pathlib import Path +from platform import system +from shutil import which +from urllib.parse import unquote, urljoin, urlparse, urlunsplit import requests from requests.adapters import HTTPAdapter +from django.conf import settings + from mobsf.MobSF.utils import ( find_aapt, find_java_binary, @@ -28,9 +32,6 @@ from mobsf.MobSF.utils import ( sha256, ) -from django.conf import settings - - logger = logging.getLogger(__name__) # Non executable files at host level _SKIP = [ @@ -596,39 +597,41 @@ def sanitize_svg(svg_content): def is_path_traversal(user_input): - """Check for path traversal.""" + """Check for path traversal with POSIX and Windows path semantics.""" if not user_input: return False - # Disallow absolute paths and windows paths and backslashes - if os.path.isabs(user_input) or user_input.startswith(('\\', '//')): - logger.error('Path traversal attack detected with absolute path') + try: + path = os.fspath(user_input) + if isinstance(path, bytes): + path = path.decode('utf-8') + if not isinstance(path, str): + raise TypeError + except (TypeError, UnicodeError): + logger.error('Path traversal attack detected with invalid path') return True - # Normalize and decode URL-encoded characters + paths = [path] try: - # Handle URL decoding (e.g., %2e -> .) - decoded = unquote(user_input) - # Handle double URL decoding (e.g., %252e -> %2e -> .) - double_decoded = unquote(decoded) + # Check single and double URL decoding (for example, %252e -> %2e -> .). + for _ in range(2): + paths.append(unquote(paths[-1])) except Exception: logger.error('Path traversal attack detected with invalid URL encoding') return True - # Check for path traversal in both original and decoded versions - dangerous_patterns = ['..', '../', '..\\', '..\\\\'] - - if any(pattern in user_input for pattern in dangerous_patterns): - logger.error('Path traversal attack detected with invalid path') - return True - - if any(pattern in decoded for pattern in dangerous_patterns): - logger.error('Path traversal attack detected with invalid path') - return True - - if any(pattern in double_decoded for pattern in dangerous_patterns): - logger.error('Path traversal attack detected with invalid path') - return True + for candidate in paths: + drive, _ = ntpath.splitdrive(candidate) + if ('\x00' in candidate + or candidate.startswith(('/', '\\')) + or posixpath.isabs(candidate) + or ntpath.isabs(candidate) + or drive): + logger.error('Path traversal attack detected with absolute path') + return True + if '..' in candidate: + logger.error('Path traversal attack detected with invalid path') + return True return False @@ -636,9 +639,14 @@ def is_safe_path(safe_root, check_path, raw_file): """Detect Path Traversal.""" if is_path_traversal(raw_file): return False - safe_root = os.path.realpath(os.path.normpath(safe_root)) - check_path = os.path.realpath(os.path.normpath(check_path)) - return check_path.startswith(safe_root + os.sep) or check_path == safe_root + try: + safe_root = os.path.normcase( + os.path.realpath(os.path.normpath(safe_root))) + check_path = os.path.normcase( + os.path.realpath(os.path.normpath(check_path))) + return os.path.commonpath((safe_root, check_path)) == safe_root + except (TypeError, ValueError): + return False def clean_filename(filename, replace=' '): diff --git a/mobsf/MobSF/tools_download.py b/mobsf/MobSF/tools_download.py index 7660a1ed..c99daa1b 100644 --- a/mobsf/MobSF/tools_download.py +++ b/mobsf/MobSF/tools_download.py @@ -158,8 +158,11 @@ def install_jadx(mobsf_home, version='1.5.0'): for member in zip_ref.namelist(): member_path = os.path.realpath( os.path.join(abs_extract_dir, member)) - if not (member_path.startswith(abs_extract_dir + os.sep) - or member_path == abs_extract_dir): + common_path = os.path.commonpath(( + os.path.normcase(abs_extract_dir), + os.path.normcase(member_path), + )) + if common_path != os.path.normcase(abs_extract_dir): raise PathTraversalError('Attempted Path Traversal in Zip File') zip_ref.extract(member, extract_dir) diff --git a/mobsf/StaticAnalyzer/tools/androguard4/apkinspector/extract.py b/mobsf/StaticAnalyzer/tools/androguard4/apkinspector/extract.py index 8215cdd5..06473f1a 100644 --- a/mobsf/StaticAnalyzer/tools/androguard4/apkinspector/extract.py +++ b/mobsf/StaticAnalyzer/tools/androguard4/apkinspector/extract.py @@ -98,8 +98,11 @@ def extract_all_files_from_central_directory(apk_file, central_directory_entries # Construct the output file path output_path = os.path.realpath(os.path.join(output_dir, filename)) abs_output_dir = os.path.realpath(output_dir) - if not (output_path.startswith(abs_output_dir + os.sep) - or output_path == abs_output_dir): + common_path = os.path.commonpath(( + os.path.normcase(abs_output_dir), + os.path.normcase(output_path), + )) + if common_path != os.path.normcase(abs_output_dir): raise PathTraversalError('Attempted Path Traversal in APK Zip Entry') # Create directories if necessary os.makedirs(os.path.dirname(output_path), exist_ok=True) diff --git a/mobsf/StaticAnalyzer/views/android/icon_analysis.py b/mobsf/StaticAnalyzer/views/android/icon_analysis.py index c9fda5e2..56c22e4e 100755 --- a/mobsf/StaticAnalyzer/views/android/icon_analysis.py +++ b/mobsf/StaticAnalyzer/views/android/icon_analysis.py @@ -63,6 +63,17 @@ def _search_folder(src, file_pattern): return matches +def _is_safe_icon_path(root, icon_file): + """Verify a resolved icon path remains inside its application root.""" + try: + resolved_root = Path(root).resolve() + resolved_icon = Path(icon_file).resolve() + relative_icon = resolved_icon.relative_to(resolved_root) + except (OSError, ValueError): + return False + return is_safe_path(resolved_root, resolved_icon, relative_icon) + + def guess_icon_path(res_dir): icon_folders = [ 'mipmap-hdpi', @@ -98,7 +109,9 @@ def get_icon_from_src(app_dic, icon_from_mfst): app_dic['md5'], res_path, icon_from_mfst) - if icon_file and Path(icon_file).exists(): + if (icon_file + and Path(icon_file).exists() + and _is_safe_icon_path(res_path, icon_file)): dwd = Path(settings.DWD_DIR) out = dwd / (app_dic['md5'] + '-icon' + Path(icon_file).suffix) copy2(icon_file, out) @@ -188,6 +201,11 @@ def get_icon_apk_res(app_dic): if icon_name and any( icon_name.startswith(x) for x in RESERVED_FILE_NAMES): icon_name = str(Path('_conflict_') / icon_name) + if (icon_name + and not is_safe_path( + app_dir, app_dir / icon_name, icon_name)): + logger.warning('Icon path escapes application directory') + icon_name = None # androguard/aapt2 cannot find icon file, fallback to res or apktool. if not res_path.exists() and apktool_res_path.exists() and not icon_name: @@ -264,9 +282,11 @@ def get_icon_apk(app_dic): # Copy PNG/SVG to Downloads icon = app_dic['md5'] + '-icon' + src.suffix.lower() out = Path(settings.DWD_DIR) / icon - if src and src.exists() and src.is_file(): + if (src.exists() + and src.is_file() + and _is_safe_icon_path(app_dic['app_dir'], src)): copy2(src.as_posix(), out.as_posix()) - app_dic['icon_path'] = out.name + app_dic['icon_path'] = out.name except Exception: logger.exception('Failed to get icon from APK') @@ -298,6 +318,8 @@ def get_icon_svg_from_xml(app_dir, icon_xml_file): """ try: icon_xml = app_dir / 'apktool_out' / icon_xml_file + if not is_safe_path(app_dir, icon_xml, icon_xml_file): + return None parsed = parseString(icon_xml.read_text('utf8', 'ignore')) foreground = parsed.getElementsByTagName('foreground') background = parsed.getElementsByTagName('background') @@ -345,6 +367,8 @@ def convert_axml_to_xml(app_dir, icon_file): try: logger.info('Converting icon axml to xml') icon_bin_xml = app_dir / icon_file + if not is_safe_path(app_dir, icon_bin_xml, icon_file): + return None out_xml = app_dir / icon_file aobj = axml.AXMLPrinter( icon_bin_xml.read_bytes()).get_xml_obj() @@ -370,6 +394,8 @@ def convert_vector_to_svg(app_dir, tools_dir, icon_name, apktool_res): # When xml is android vector values = app_dir / 'res' / 'values' direct = app_dir / icon_name + if not is_safe_path(app_dir, direct, icon_name): + return cwd = direct.parent.as_posix() # When xml is referencing to vector use apktool_res diff --git a/mobsf/StaticAnalyzer/views/android/network_security.py b/mobsf/StaticAnalyzer/views/android/network_security.py index 55cc7de4..1c0283ad 100644 --- a/mobsf/StaticAnalyzer/views/android/network_security.py +++ b/mobsf/StaticAnalyzer/views/android/network_security.py @@ -5,7 +5,7 @@ from pathlib import Path from defusedxml.minidom import parseString -from mobsf.MobSF.security import is_path_traversal +from mobsf.MobSF.security import is_safe_path from mobsf.MobSF.utils import ( append_scan_status, ) @@ -30,13 +30,13 @@ def read_netsec_config(checksum, app_dir, config, src_type): else: # APK xml_dir = base / 'apktool_out' / 'res' / 'xml' - if not is_path_traversal(config): - netsec_file = xml_dir / f'{config}.xml' - if netsec_file.exists(): - desc = f'{msg} from {config}.xml' - logger.info(desc) - append_scan_status(checksum, desc) - return netsec_file.read_text('utf8', 'ignore') + netsec_file = xml_dir / f'{config}.xml' + if (is_safe_path(xml_dir, netsec_file, config) + and netsec_file.exists()): + desc = f'{msg} from {config}.xml' + logger.info(desc) + append_scan_status(checksum, desc) + return netsec_file.read_text('utf8', 'ignore') # Couldn't find the file defined in manifest xmls = Path(xml_dir).glob('*.xml') for xml in xmls: diff --git a/mobsf/StaticAnalyzer/views/common/shared_func.py b/mobsf/StaticAnalyzer/views/common/shared_func.py index 6cbc8c37..5ada86ee 100644 --- a/mobsf/StaticAnalyzer/views/common/shared_func.py +++ b/mobsf/StaticAnalyzer/views/common/shared_func.py @@ -145,7 +145,10 @@ def unzip(checksum, app_path, ext_path): ext_path = str(Path(ext_path) / '_conflict_') # Handle Zip Slip - if is_path_traversal(file_path): + destination = Path(ext_path) / file_path + if (is_path_traversal(file_path) + or not is_safe_path( + ext_path, destination, file_path)): msg = ('Zip slip detected. skipped extracting' f' {sanitize_for_logging(file_path)}') logger.error(msg) @@ -283,7 +286,9 @@ def ar_os(src, dst): [shutil.which('ar'), 't', src], stderr=subprocess.STDOUT) for raw_file in files.decode('utf-8').split('\n'): - if is_path_traversal(raw_file): + out = Path(dst) / raw_file + if (is_path_traversal(raw_file) + or not is_safe_path(dst, out, raw_file)): msg = f'AR slip detected in AR file {src} at {raw_file}' logger.error(msg) raise ValueError(msg) @@ -316,6 +321,11 @@ def ar_extract(checksum, src, dst): append_scan_status(checksum, msg) continue out = Path(dst) / filtered + if not is_safe_path(dst, out, filtered): + msg = f'AR slip detected. skipped extracting {filtered}' + logger.warning(msg) + append_scan_status(checksum, msg) + continue out.write_bytes(val.read()) except Exception: # Possibly dealing with Fat binary, needs Mac host