fix(security): confine save_ad_image_locally to an allowed base directory (GHSA-2h5x-4qc8-3x27) (#166)

ad_id and output_dir are caller-supplied tool arguments that were joined into a destination path unchecked. os.path.join returns an absolute second argument unchanged and does not neutralize '..', and os.makedirs built whatever tree was needed, so output_dir=/etc/cron.d or ad_id=../../../../tmp/evil wrote the downloaded image outside the intended directory with the server process's privileges.

resolve_ad_image_save_path now decides the destination before anything touches disk: ad_id must be digits, the image hash must be filename-safe, and output_dir is resolved with realpath and must land on or under the base directory (the server's working directory, or META_ADS_IMAGE_OUTPUT_DIR). Absolute paths, traversal and symlinks out of the base are rejected.

The tool only registers when META_ADS_ENABLE_SAVE_AD_IMAGE_LOCALLY is set; the hosted deployment does not set it.

Bumps to 1.0.126 and documents the advisory in SECURITY.md.

Author: nictuku

SECURITY.md

@@ -338,3 +338,45 @@ IPv6 addresses are still unwrapped first, so `::ffff:100.64.1.1` is rejected too
   review outbound request logs for fetches into that range.
 - Credited to [@Gal3m](https://github.com/Gal3m) and
   [@mohammad228](https://github.com/mohammad228).
+
+### GHSA-2h5x-4qc8-3x27 — `save_ad_image_locally` wrote to a caller-chosen path
+
+- **Severity:** Medium
+- **Affected versions:** `<= 1.0.125`, and only when the tool is enabled.
+- **Fixed in:** `1.0.126`
+- **Affected configurations:** Installs that set
+  `META_ADS_ENABLE_SAVE_AD_IMAGE_LOCALLY` — the tool is not registered without
+  it. The hosted MCP at `*.mcp.pipeboard.co` does not set it, so the tool has
+  never been exposed there.
+
+**What went wrong.** `save_ad_image_locally` built its destination from two
+caller-supplied tool arguments with no validation:
+
+```python
+filename = f"{ad_id}_{image_hashes[0]}.jpg"
+filepath = os.path.join(output_dir, filename)
+os.makedirs(output_dir)          # built whatever tree was needed
+```
+
+`os.path.join` returns an absolute second argument unchanged and does not
+neutralize `..`, so `output_dir="/etc/cron.d"`, `ad_id="/tmp/PWNED"` or
+`ad_id="../../../../tmp/evil"` all wrote the downloaded image outside the
+intended `ad_images` directory, with the server process's privileges, creating
+directories along the way. The bytes are an ad image, which a caller who
+controls the creative can influence.
+
+**Fix.** A new `resolve_ad_image_save_path()` decides the destination before
+anything touches the disk: `ad_id` must be a Meta object id (digits), the image
+hash must be filename-safe, and `output_dir` is resolved with `os.path.realpath`
+and must land on or under an allowed base directory — so absolute paths, `..`
+and symlinks pointing out of the base are all rejected. The base is the working
+directory the server was started in, which is where the documented default
+(`ad_images`) has always resolved; operators who want images elsewhere set
+`META_ADS_IMAGE_OUTPUT_DIR`. Directories are only created after the path passes.
+
+**Action for operators.**
+- Upgrade to `1.0.126` or later.
+- If you enabled the tool on an earlier version, check for unexpected files
+  matching `*_<hash>.jpg` outside your image directory.
+- Credited to [@Gal3m](https://github.com/Gal3m) and
+  [@mohammad228](https://github.com/mohammad228).

meta_ads_mcp/__init__.py

@@ -6,7 +6,7 @@
 
 from meta_ads_mcp.core.server import main
 
-__version__ = "1.0.125"
+__version__ = "1.0.126"
 
 __all__ = [
     'get_ad_accounts',

meta_ads_mcp/core/ads.py

@@ -8,6 +8,7 @@
 from PIL import Image as PILImage
 from mcp.server.fastmcp import Image
 import os
+import re
 import time
 
 logger = logging.getLogger(__name__)
@@ -441,6 +442,58 @@ def _translate_video_customization_rules_for_existing_post(
 ENABLE_SAVE_AD_IMAGE_LOCALLY = bool(os.environ.get("META_ADS_ENABLE_SAVE_AD_IMAGE_LOCALLY", ""))
 
 
+class UnsafeSavePathError(Exception):
+    """Raised when a requested save path escapes the allowed base directory."""
+
+
+# `ad_id` and `output_dir` are caller-supplied tool arguments that used to be
+# joined into a destination path unchecked. os.path.join() returns an absolute
+# second argument unchanged and does not neutralize "..", so a caller could write
+# the downloaded image anywhere the process can reach — and os.makedirs() would
+# build the directory tree to get there. See GHSA-2h5x-4qc8-3x27.
+_AD_ID_PATTERN = re.compile(r"^[0-9]+$")
+_IMAGE_HASH_PATTERN = re.compile(r"^[A-Za-z0-9_-]+$")
+
+
+def _save_image_base_dir() -> str:
+    """Directory that saved ad images may never escape.
+
+    Defaults to the working directory the server was started in, which is where
+    the documented default (`ad_images`) has always resolved. Operators who want
+    images elsewhere set META_ADS_IMAGE_OUTPUT_DIR.
+    """
+    return os.path.realpath(os.environ.get("META_ADS_IMAGE_OUTPUT_DIR") or os.getcwd())
+
+
+def resolve_ad_image_save_path(ad_id: str, image_hash: str, output_dir: str) -> str:
+    """Build the destination path for a saved ad image, confined to the base dir.
+
+    Raises UnsafeSavePathError if `ad_id` is not a Meta object id, if the image
+    hash is not filename-safe, or if `output_dir` resolves outside the base
+    directory (absolute path, "..", or a symlink pointing out of it).
+    """
+    if not _AD_ID_PATTERN.match(ad_id or ""):
+        raise UnsafeSavePathError(
+            f"ad_id '{ad_id}' is not a Meta ad id; expected digits only"
+        )
+    if not _IMAGE_HASH_PATTERN.match(image_hash or ""):
+        raise UnsafeSavePathError(f"image hash '{image_hash}' is not a safe filename component")
+
+    base = _save_image_base_dir()
+    # An absolute output_dir makes join() discard the base, so realpath is what
+    # decides: the result must be the base itself or sit underneath it.
+    target_dir = os.path.realpath(os.path.join(base, output_dir or ""))
+    if target_dir != base and not target_dir.startswith(base + os.sep):
+        raise UnsafeSavePathError(
+            f"output_dir '{output_dir}' resolves outside the allowed directory "
+            f"({base}). Pass a path inside it, or set META_ADS_IMAGE_OUTPUT_DIR "
+            "to move the allowed directory."
+        )
+
+    filename = os.path.basename(f"{ad_id}_{image_hash}.jpg")
+    return os.path.join(target_dir, filename)
+
+
 @mcp_server.tool()
 @meta_api_tool
 async def get_ads(account_id: str, access_token: Optional[str] = None, limit: int = 10, 
@@ -1203,16 +1256,15 @@ async def save_ad_image_locally(ad_id: str, access_token: Optional[str] = None,
         if not image_bytes:
             return json.dumps({"error": "Failed to download image"}, indent=2)
 
+        # Resolve and confine the destination before anything is created on disk.
         try:
-            # Ensure output directory exists
-            if not os.path.exists(output_dir):
-                os.makedirs(output_dir)
-                
-            # Create a filename (e.g., using ad_id and image hash)
-            file_extension = ".jpg" # Default extension, could try to infer from headers later
-            filename = f"{ad_id}_{image_hashes[0]}{file_extension}"
-            filepath = os.path.join(output_dir, filename)
-            
+            filepath = resolve_ad_image_save_path(ad_id, image_hashes[0], output_dir)
+        except UnsafeSavePathError as e:
+            return json.dumps({"error": str(e)}, indent=2)
+
+        try:
+            os.makedirs(os.path.dirname(filepath), exist_ok=True)
+
             # Save the image bytes to the file
             with open(filepath, "wb") as f:
                 f.write(image_bytes)

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "hatchling.build"
 
 [project]
 name = "meta-ads-mcp"
-version = "1.0.125"
+version = "1.0.126"
 description = "Model Context Protocol (MCP) server for Meta Ads - Use Remote MCP at pipeboard.co for easiest setup"
 readme = "README.md"
 requires-python = ">=3.10"

server.json

@@ -2,7 +2,7 @@
   "$schema": "https://static.modelcontextprotocol.io/schemas/2025-12-11/server.schema.json",
   "name": "co.pipeboard/meta-ads-mcp",
   "description": "Facebook / Meta Ads automation with AI: analyze performance, test creatives, optimize spend.",
-  "version": "1.0.125",
+  "version": "1.0.126",
   "remotes": [
     {
       "type": "streamable-http",

tests/test_save_ad_image_path_confinement.py

@@ -0,0 +1,114 @@
+"""Regression tests for GHSA-2h5x-4qc8-3x27.
+
+`save_ad_image_locally` built its destination from two caller-supplied tool
+arguments with no validation: `os.path.join(output_dir, f"{ad_id}_{hash}.jpg")`.
+join() returns an absolute second argument unchanged and does not neutralize
+"..", and `os.makedirs(output_dir)` would build whatever tree was needed, so a
+caller could drop the downloaded image anywhere the process could write.
+
+`resolve_ad_image_save_path` now decides the path, and it is called before
+anything touches the disk.
+"""
+
+import os
+
+import pytest
+
+from meta_ads_mcp.core.ads import UnsafeSavePathError, resolve_ad_image_save_path
+
+AD_ID = "120250589548040262"
+IMAGE_HASH = "60dff700e54f59a38ec6e626c7b2f3f5"
+
+
+@pytest.fixture
+def base_dir(tmp_path, monkeypatch):
+    """Point the allowed base at a scratch directory."""
+    base = tmp_path / "workdir"
+    base.mkdir()
+    monkeypatch.setenv("META_ADS_IMAGE_OUTPUT_DIR", str(base))
+    return base
+
+
+def test_default_output_dir_lands_under_the_base(base_dir):
+    path = resolve_ad_image_save_path(AD_ID, IMAGE_HASH, "ad_images")
+
+    assert path == str(base_dir / "ad_images" / f"{AD_ID}_{IMAGE_HASH}.jpg")
+
+
+def test_nested_subdirectories_are_allowed(base_dir):
+    path = resolve_ad_image_save_path(AD_ID, IMAGE_HASH, "creatives/2026/q3")
+
+    assert path.startswith(str(base_dir) + os.sep)
+
+
+def test_empty_output_dir_writes_into_the_base(base_dir):
+    path = resolve_ad_image_save_path(AD_ID, IMAGE_HASH, "")
+
+    assert path == str(base_dir / f"{AD_ID}_{IMAGE_HASH}.jpg")
+
+
+@pytest.mark.parametrize(
+    "output_dir",
+    [
+        "/etc/cron.d",              # absolute path discards the base in join()
+        "/tmp",
+        "../escaped",               # traversal
+        "../../../../tmp/evil",
+        "ad_images/../../outside",  # traversal after a legitimate-looking prefix
+    ],
+)
+def test_output_dir_escapes_are_rejected(base_dir, output_dir):
+    with pytest.raises(UnsafeSavePathError):
+        resolve_ad_image_save_path(AD_ID, IMAGE_HASH, output_dir)
+
+
+@pytest.mark.parametrize(
+    "ad_id",
+    [
+        "/tmp/PWNED",               # absolute id discards the directory
+        "../../../../tmp/evil",     # traversal through the filename
+        "123/../../etc/passwd",
+        "not-an-id",
+        "",
+        "12 3",
+    ],
+)
+def test_non_numeric_ad_ids_are_rejected(base_dir, ad_id):
+    with pytest.raises(UnsafeSavePathError):
+        resolve_ad_image_save_path(ad_id, IMAGE_HASH, "ad_images")
+
+
+@pytest.mark.parametrize("image_hash", ["../../evil", "a/b", "", "hash with spaces"])
+def test_unsafe_image_hashes_are_rejected(base_dir, image_hash):
+    with pytest.raises(UnsafeSavePathError):
+        resolve_ad_image_save_path(AD_ID, image_hash, "ad_images")
+
+
+def test_symlink_out_of_the_base_is_rejected(base_dir, tmp_path):
+    """realpath resolution is what catches this, not string matching."""
+    outside = tmp_path / "outside"
+    outside.mkdir()
+    (base_dir / "escape").symlink_to(outside)
+
+    with pytest.raises(UnsafeSavePathError):
+        resolve_ad_image_save_path(AD_ID, IMAGE_HASH, "escape")
+
+
+def test_rejection_creates_nothing_on_disk(base_dir):
+    """The path is resolved before any makedirs, so a rejected call is inert."""
+    before = sorted(os.listdir(base_dir))
+
+    with pytest.raises(UnsafeSavePathError):
+        resolve_ad_image_save_path(AD_ID, IMAGE_HASH, "../../../../tmp/evil-tree")
+
+    assert sorted(os.listdir(base_dir)) == before
+    assert not os.path.exists("/tmp/evil-tree")
+
+
+def test_base_defaults_to_the_working_directory(tmp_path, monkeypatch):
+    monkeypatch.delenv("META_ADS_IMAGE_OUTPUT_DIR", raising=False)
+    monkeypatch.chdir(tmp_path)
+
+    path = resolve_ad_image_save_path(AD_ID, IMAGE_HASH, "ad_images")
+
+    assert path == os.path.join(os.path.realpath(tmp_path), "ad_images", f"{AD_ID}_{IMAGE_HASH}.jpg")