From 168e536faee4b8fffc721cdb89e266e391f78fae Mon Sep 17 00:00:00 2001 From: Glenn Jocher Date: Sat, 4 Nov 2023 22:25:49 +0100 Subject: [PATCH] Improve path traversal security vulnerability (#6138) Signed-off-by: Glenn Jocher Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- docs/hub/app/android.md | 2 +- docs/reference/utils/checks.md | 8 ++++++++ ultralytics/engine/exporter.py | 23 ++++++++++------------- ultralytics/utils/checks.py | 17 +++++++++++++++++ ultralytics/utils/downloads.py | 6 +++++- 5 files changed, 41 insertions(+), 15 deletions(-) diff --git a/docs/hub/app/android.md b/docs/hub/app/android.md index cdd7b365..e1f4f75f 100644 --- a/docs/hub/app/android.md +++ b/docs/hub/app/android.md @@ -39,7 +39,7 @@ Here's a table showing the primary vendors, their product lines, popular devices |-----------------------------------------|--------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------------------| | [Qualcomm](https://www.qualcomm.com/) | [Snapdragon (e.g., 800 series)](https://www.qualcomm.com/snapdragon) | [Samsung Galaxy S21](https://www.samsung.com/global/galaxy/galaxy-s21-5g/), [OnePlus 9](https://www.oneplus.com/9), [Google Pixel 6](https://store.google.com/product/pixel_6) | CPU, GPU, Hexagon, NNAPI | | [Samsung](https://www.samsung.com/) | [Exynos (e.g., Exynos 2100)](https://www.samsung.com/semiconductor/minisite/exynos/) | [Samsung Galaxy S21 (Global version)](https://www.samsung.com/global/galaxy/galaxy-s21-5g/) | CPU, GPU, NNAPI | -| [MediaTek](https://www.mediatek.com/) | [Dimensity (e.g., Dimensity 1200)](https://www.mediatek.com/products/smartphones) | [Realme GT](https://www.realme.com/global/realme-gt), [Xiaomi Redmi Note](https://www.mi.com/en/phone/redmi/note-list) | CPU, GPU, NNAPI | +| [MediaTek](https://i.mediatek.com/) | [Dimensity (e.g., Dimensity 1200)](https://i.mediatek.com/dimensity-1200) | [Realme GT](https://www.realme.com/global/realme-gt), [Xiaomi Redmi Note](https://www.mi.com/en/phone/redmi/note-list) | CPU, GPU, NNAPI | | [HiSilicon](https://www.hisilicon.com/) | [Kirin (e.g., Kirin 990)](https://www.hisilicon.com/en/products/Kirin) | [Huawei P40 Pro](https://consumer.huawei.com/en/phones/p40-pro/), [Huawei Mate 30 Pro](https://consumer.huawei.com/en/phones/mate30-pro/) | CPU, GPU, NNAPI | | [NVIDIA](https://www.nvidia.com/) | [Tegra (e.g., Tegra X1)](https://developer.nvidia.com/content/tegra-x1) | [NVIDIA Shield TV](https://www.nvidia.com/en-us/shield/shield-tv/), [Nintendo Switch](https://www.nintendo.com/switch/) | CPU, GPU, NNAPI | diff --git a/docs/reference/utils/checks.md b/docs/reference/utils/checks.md index 42db921c..a45f9987 100644 --- a/docs/reference/utils/checks.md +++ b/docs/reference/utils/checks.md @@ -61,6 +61,10 @@ keywords: Ultralytics, utility checks, ASCII, check_version, pip_update, check_p ## ::: ultralytics.utils.checks.check_yolov5u_filename

+--- +## ::: ultralytics.utils.checks.check_model_file_from_stem +

+ --- ## ::: ultralytics.utils.checks.check_file

@@ -69,6 +73,10 @@ keywords: Ultralytics, utility checks, ASCII, check_version, pip_update, check_p ## ::: ultralytics.utils.checks.check_yaml

+--- +## ::: ultralytics.utils.checks.check_is_path_safe +

+ --- ## ::: ultralytics.utils.checks.check_imshow

diff --git a/ultralytics/engine/exporter.py b/ultralytics/engine/exporter.py index 522c049b..2fb6f720 100644 --- a/ultralytics/engine/exporter.py +++ b/ultralytics/engine/exporter.py @@ -69,7 +69,7 @@ from ultralytics.nn.modules import C2f, Detect, RTDETRDecoder from ultralytics.nn.tasks import DetectionModel, SegmentationModel from ultralytics.utils import (ARM64, DEFAULT_CFG, LINUX, LOGGER, MACOS, ROOT, WINDOWS, __version__, callbacks, colorstr, get_default_args, yaml_save) -from ultralytics.utils.checks import check_imgsz, check_requirements, check_version +from ultralytics.utils.checks import check_imgsz, check_is_path_safe, check_requirements, check_version from ultralytics.utils.downloads import attempt_download_asset, get_github_assets from ultralytics.utils.files import file_size, spaces_in_path from ultralytics.utils.ops import Profile @@ -450,12 +450,9 @@ class Exporter: f = Path(str(self.file).replace(self.file.suffix, f'_ncnn_model{os.sep}')) f_ts = self.file.with_suffix('.torchscript') - pnnx_filename = 'pnnx.exe' if WINDOWS else 'pnnx' - if Path(pnnx_filename).is_file(): - pnnx = pnnx_filename - elif (ROOT / pnnx_filename).is_file(): - pnnx = ROOT / pnnx_filename - else: + name = Path('pnnx.exe' if WINDOWS else 'pnnx') # PNNX filename + pnnx = name if name.is_file() else ROOT / name + if not pnnx.is_file(): LOGGER.warning( f'{prefix} WARNING ⚠️ PNNX not found. Attempting to download binary file from ' 'https://github.com/pnnx/pnnx/.\nNote PNNX Binary file must be placed in current working directory ' @@ -465,12 +462,12 @@ class Exporter: asset = [x for x in assets if system in x][0] if assets else \ f'https://github.com/pnnx/pnnx/releases/download/20230816/pnnx-20230816-{system}.zip' # fallback asset = attempt_download_asset(asset, repo='pnnx/pnnx', release='latest') - unzip_dir = Path(asset).with_suffix('') - pnnx = ROOT / pnnx_filename # new location - (unzip_dir / pnnx_filename).rename(pnnx) # move binary to ROOT - shutil.rmtree(unzip_dir) # delete unzip dir - Path(asset).unlink() # delete zip - pnnx.chmod(0o777) # set read, write, and execute permissions for everyone + if check_is_path_safe(Path.cwd(), asset): # avoid path traversal security vulnerability + unzip_dir = Path(asset).with_suffix('') + (unzip_dir / name).rename(pnnx) # move binary to ROOT + shutil.rmtree(unzip_dir) # delete unzip dir + Path(asset).unlink() # delete zip + pnnx.chmod(0o777) # set read, write, and execute permissions for everyone ncnn_args = [ f'ncnnparam={f / "model.ncnn.param"}', diff --git a/ultralytics/utils/checks.py b/ultralytics/utils/checks.py index 04c9f49e..8cf14ccb 100644 --- a/ultralytics/utils/checks.py +++ b/ultralytics/utils/checks.py @@ -463,6 +463,23 @@ def check_yaml(file, suffix=('.yaml', '.yml'), hard=True): return check_file(file, suffix, hard=hard) +def check_is_path_safe(basedir, path): + """ + Check if the resolved path is under the intended directory to prevent path traversal. + + Args: + basedir (Path | str): The intended directory. + path (Path | str): The path to check. + + Returns: + (bool): True if the path is safe, False otherwise. + """ + base_dir_resolved = Path(basedir).resolve() + path_resolved = Path(path).resolve() + + return path_resolved.is_file() and path_resolved.parts[:len(base_dir_resolved.parts)] == base_dir_resolved.parts + + def check_imshow(warn=False): """Check if environment supports image displays.""" try: diff --git a/ultralytics/utils/downloads.py b/ultralytics/utils/downloads.py index 843764e8..e46ff1f6 100644 --- a/ultralytics/utils/downloads.py +++ b/ultralytics/utils/downloads.py @@ -159,7 +159,11 @@ def unzip_file(file, path=None, exclude=('.DS_Store', '__MACOSX'), exist_ok=Fals return path for f in TQDM(files, desc=f'Unzipping {file} to {Path(path).resolve()}...', unit='file', disable=not progress): - zipObj.extract(f, path=extract_path) + # Ensure the file is within the extract_path to avoid path traversal security vulnerability + if '..' in Path(f).parts: + LOGGER.warning(f'Potentially insecure file path: {f}, skipping extraction.') + continue + zipObj.extract(f, extract_path) return path # return unzip dir