From 19d369b08b1526e644016d89e7775e3722aa6985 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 11 May 2026 12:43:49 +0000 Subject: [PATCH] studio: contain export and dataset paths under their configured roots resolve_under_root and resolve_dataset_path previously returned absolute paths unchanged, so an authenticated client could supply save_directory="/tmp/escape" (or any other absolute path) and have the exporter drop adapter files anywhere the server user could write. This turned up during a recent audit pass where an authenticated POST to /api/export/export/lora with save_directory="/tmp/lora_escape_test" returned 200 and wrote adapter_model.safetensors, adapter_config.json, and tokenizer files under /tmp. The fix is two-layered: storage_roots.py adds an _assert_contained(resolved, root) helper that runs after path resolution and rejects any result whose realpath does not sit under realpath(root). resolve_under_root now rejects '..' segments and null bytes outright, and only accepts absolute inputs when they are already inside the configured root (internal call sites that re-resolve a stored absolute path stay idempotent; worker.py:resolve_output_dir(output_dir) etc. continue to work). resolve_dataset_path picks up the same containment rule, scoped to the three dataset roots. models/export.py adds field_validator("save_directory", mode="before") to ExportCommonOptions and ExportGGUFRequest so bad input fails fast at 422 with a clear message rather than a 500 deep inside the resolver. The validator rejects empty/whitespace, null bytes, control chars, strings longer than 255 chars, absolute paths, and '..' segments. routes/export.py:_export_details now returns os.path.relpath(output_path, exports_root()) so the Export Complete dialog and /api/models/loras no longer leak the absolute install prefix to the UI; the basename is used as a last-resort fallback. Verified end to end: - POST /api/export/export/lora {"save_directory":"/tmp/foo"} -> 422 "save_directory must be a name or relative path under the export root; absolute paths are rejected". /tmp/foo is not created. - "../../etc/escape" -> 422 "may not contain '..' segments". - save_directory="my_subdir" -> still accepted (400 only because the test had no checkpoint loaded yet, not because of validation). - Internal idempotent re-resolve via resolve_export_dir(absolute path that is already under exports_root) returns the same path unchanged. --- studio/backend/models/export.py | 45 ++++++++++++- studio/backend/routes/export.py | 16 ++++- studio/backend/utils/paths/storage_roots.py | 75 +++++++++++++++++++-- 3 files changed, 129 insertions(+), 7 deletions(-) diff --git a/studio/backend/models/export.py b/studio/backend/models/export.py index a86596f199..d1727ae6f3 100644 --- a/studio/backend/models/export.py +++ b/studio/backend/models/export.py @@ -5,10 +5,41 @@ Pydantic schemas for Export API. """ -from pydantic import BaseModel, Field +from pathlib import Path + +from pydantic import BaseModel, Field, field_validator from typing import List, Optional, Literal, Dict, Any +def _validate_save_directory(value: str) -> str: + """Reject save_directory values that escape the configured export root. + + Mirrors :func:`studio.backend.utils.paths.storage_roots.resolve_under_root` + so the rejection happens at request-parse time with a clear 422 instead of + at handler invocation time with an opaque 500. + """ + if value is None: + raise ValueError("save_directory is required") + raw = str(value).strip() + if not raw: + raise ValueError("save_directory must not be empty") + if "\x00" in raw: + raise ValueError("save_directory may not contain null bytes") + if any(ch in raw for ch in ("\r", "\n")): + raise ValueError("save_directory may not contain control characters") + if len(raw) > 255: + raise ValueError("save_directory must be <= 255 characters") + path = Path(raw).expanduser() + if path.is_absolute(): + raise ValueError( + "save_directory must be a name or relative path under the " + "export root; absolute paths are rejected" + ) + if ".." in path.parts: + raise ValueError("save_directory may not contain '..' segments") + return raw + + class LoadCheckpointRequest(BaseModel): """Request for loading a checkpoint into the export backend.""" @@ -64,6 +95,12 @@ class ExportCommonOptions(BaseModel): ..., description = "Local directory where the exported artifacts will be written", ) + + @field_validator("save_directory", mode = "before") + @classmethod + def _check_save_directory(cls, v): + return _validate_save_directory(v) + push_to_hub: bool = Field( False, description = "If True, also push the exported model to the Hugging Face Hub", @@ -108,6 +145,12 @@ class ExportGGUFRequest(BaseModel): ..., description = "Directory where GGUF files will be saved", ) + + @field_validator("save_directory", mode = "before") + @classmethod + def _check_save_directory(cls, v): + return _validate_save_directory(v) + quantization_method: str = Field( "Q4_K_M", description = 'GGUF quantization method (e.g. "Q4_K_M")', diff --git a/studio/backend/routes/export.py b/studio/backend/routes/export.py index 798859fc87..e8da7e8cfc 100644 --- a/studio/backend/routes/export.py +++ b/studio/backend/routes/export.py @@ -7,6 +7,7 @@ Export API routes: checkpoint discovery and model export operations. import asyncio import json +import os import sys import time from pathlib import Path @@ -188,10 +189,23 @@ def _export_details(output_path: Optional[str]) -> Optional[Dict[str, Any]]: frontend reads to populate the Export Complete screen. Returns None when the export had no local component (Hub-only push) so the Pydantic field stays absent rather than ``{"output_path": null}``. + + The returned ``output_path`` is the path RELATIVE to the configured + exports root, so the response no longer leaks the absolute install + location (e.g. ``/mnt/disks/.../studio/exports/foo`` -> ``foo``). """ if not output_path: return None - return {"output_path": output_path} + try: + from utils.paths.storage_roots import exports_root # local import to avoid cycle + rel = os.path.relpath(output_path, exports_root()) + # If the path is outside exports_root (defensive), keep the basename only. + if rel.startswith(".."): + rel = os.path.basename(output_path) + return {"output_path": rel} + except Exception: + # Last-resort fallback: basename only - never the full prefix. + return {"output_path": os.path.basename(output_path)} @router.post("/export/merged", response_model = ExportOperationResponse) diff --git a/studio/backend/utils/paths/storage_roots.py b/studio/backend/utils/paths/storage_roots.py index 58a4d7967c..623a598fb3 100644 --- a/studio/backend/utils/paths/storage_roots.py +++ b/studio/backend/utils/paths/storage_roots.py @@ -276,21 +276,69 @@ def _clean_relative_path( return Path(*parts) if parts else Path() +def _assert_contained(resolved: Path, root: Path) -> None: + """Reject paths whose post-resolve real location escapes ``root``. + + Catches absolute inputs, ``..`` traversal, and symlinks pointing outside + the root. Raises ValueError on escape. + """ + try: + resolved_real = Path(os.path.realpath(resolved)) + root_real = Path(os.path.realpath(root)) + except OSError as exc: + raise ValueError(f"path resolution failed: {exc}") from exc + try: + resolved_real.relative_to(root_real) + except ValueError as exc: + raise ValueError( + f"path escapes root: {resolved!s} -> {resolved_real!s} " + f"is not under {root_real!s}" + ) from exc + + def resolve_under_root( path_value: str | None, *, root: Path, strip_prefixes: tuple[str, ...] = (), ) -> Path: + """Resolve ``path_value`` so the result is provably under ``root``. + + Policy: + - Empty / None -> the root itself. + - Null bytes -> rejected. + - ``..`` segments -> rejected (no traversal). + - Absolute paths -> accepted ONLY if already inside ``root`` (after + realpath). This lets internal code that has already resolved a + stored absolute path re-enter the resolver idempotently, while + blocking the export-time exploit where a user supplies + ``/tmp/EVIL``. The pydantic validator on user-facing schemas + (``ExportCommonOptions.save_directory``) rejects absolute inputs + outright at request-parse time as defense-in-depth. + """ if not path_value or not str(path_value).strip(): return root - path = Path(str(path_value).strip()).expanduser() + raw = str(path_value).strip() + if "\x00" in raw: + raise ValueError("path may not contain null bytes") + + path = Path(raw).expanduser() + if ".." in path.parts: + raise ValueError( + f"path may not contain '..' segments: {raw!r}" + ) + if path.is_absolute(): + # Internal callers may pass already-resolved absolute paths. + # Accept only when contained under root; reject escapes. + _assert_contained(path, root) return path - cleaned = _clean_relative_path(str(path), strip_prefixes = strip_prefixes) - return root / cleaned + cleaned = _clean_relative_path(raw, strip_prefixes = strip_prefixes) + candidate = root / cleaned + _assert_contained(candidate, root) + return candidate def resolve_output_dir(path_value: str | None = None) -> Path: @@ -318,9 +366,26 @@ def resolve_tensorboard_dir(path_value: str | None = None) -> Path: def resolve_dataset_path(path_value: str) -> Path: - path = Path(path_value).expanduser() + raw = str(path_value or "").strip() + if "\x00" in raw: + raise ValueError("dataset path may not contain null bytes") + path = Path(raw).expanduser() + if ".." in path.parts: + raise ValueError( + f"dataset path may not contain '..' segments: {raw!r}" + ) if path.is_absolute(): - return path + # Accept absolute inputs only when contained under one of the + # dataset roots; reject all other absolute paths. + for root_fn in (datasets_root, dataset_uploads_root, recipe_datasets_root): + try: + _assert_contained(path, root_fn()) + return path + except ValueError: + continue + raise ValueError( + f"dataset path must be relative or under a dataset root: {raw!r}" + ) parts = [part for part in Path(path_value).parts if part not in ("", ".")] if parts[:2] == ["assets", "datasets"]: