Skip to content

Commit

Permalink
Improve path traversal security vulnerability (ultralytics#6138)
Browse files Browse the repository at this point in the history
Signed-off-by: Glenn Jocher <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
  • Loading branch information
glenn-jocher and pre-commit-ci[bot] authored Nov 4, 2023
1 parent c0e707a commit 168e536
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 15 deletions.
2 changes: 1 addition & 1 deletion docs/hub/app/android.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |

Expand Down
8 changes: 8 additions & 0 deletions docs/reference/utils/checks.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ keywords: Ultralytics, utility checks, ASCII, check_version, pip_update, check_p
## ::: ultralytics.utils.checks.check_yolov5u_filename
<br><br>

---
## ::: ultralytics.utils.checks.check_model_file_from_stem
<br><br>

---
## ::: ultralytics.utils.checks.check_file
<br><br>
Expand All @@ -69,6 +73,10 @@ keywords: Ultralytics, utility checks, ASCII, check_version, pip_update, check_p
## ::: ultralytics.utils.checks.check_yaml
<br><br>

---
## ::: ultralytics.utils.checks.check_is_path_safe
<br><br>

---
## ::: ultralytics.utils.checks.check_imshow
<br><br>
Expand Down
23 changes: 10 additions & 13 deletions ultralytics/engine/exporter.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
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
Expand Down Expand Up @@ -450,12 +450,9 @@ def export_ncnn(self, prefix=colorstr('ncnn:')):
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 '
Expand All @@ -465,12 +462,12 @@ def export_ncnn(self, prefix=colorstr('ncnn:')):
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"}',
Expand Down
17 changes: 17 additions & 0 deletions ultralytics/utils/checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
6 changes: 5 additions & 1 deletion ultralytics/utils/downloads.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit 168e536

Please sign in to comment.