diff --git a/SECURITY.md b/SECURITY.md index 773fd9d4..d81a26e9 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -380,3 +380,39 @@ directory the server was started in, which is where the documented default matching `*_.jpg` outside your image directory. - Credited to [@Gal3m](https://github.com/Gal3m) and [@mohammad228](https://github.com/mohammad228). + +### GHSA-prmg-4fr3-mm6x — cached Meta access token was group/world-readable on disk + +- **Severity:** Medium +- **Affected versions:** `<= 1.0.126` +- **Fixed in:** `1.0.127` +- **Affected configurations:** Local installs that authenticate through the + OAuth flow and cache a token (`~/.config/meta-ads-mcp/token_cache.json`, or + `~/Library/Application Support/meta-ads-mcp/` on macOS). Matters most on + shared hosts — CI runners, bastion boxes, multi-UID containers. The hosted MCP + at `*.mcp.pipeboard.co` does not cache tokens to disk. + +**What went wrong.** The cache file was created with a bare `open(path, "w")` +and its directory with `mkdir()` with no mode, so under the usual `0022` umask +the long-lived Meta access token (~60 days) landed in a `0644` file inside a +`0755` directory. Any other local user could read it and act as the operator +against the Graph API. + +**Fix.** The file is created with `os.open(..., O_CREAT, 0o600)` and the +directory with mode `0o700`. Because `O_CREAT`'s mode applies only at creation +and `mkdir`'s mode is masked by the umask (and is a no-op for a directory that +already exists), both are also narrowed explicitly afterwards — which repairs a +cache written by an earlier version. The same narrowing runs when a cache is +loaded, so an existing install stops being readable as soon as the server +starts, with no need to re-authenticate. On Windows the chmod step is skipped: +it cannot express "owner only" there, where access is governed by ACLs. + +**Action for operators.** +- Upgrade to `1.0.127` or later; the permissions are repaired on the next load + or save. +- If you ran an earlier version on a shared host, treat the cached token as + exposed: rotate it at + `https://developers.facebook.com/tools/debug/accesstoken/` and review Graph + API access logs. +- Credited to [@Gal3m](https://github.com/Gal3m) and + [@mohammad228](https://github.com/mohammad228). diff --git a/meta_ads_mcp/__init__.py b/meta_ads_mcp/__init__.py index 38d8b5ac..cf0bfd62 100644 --- a/meta_ads_mcp/__init__.py +++ b/meta_ads_mcp/__init__.py @@ -6,7 +6,7 @@ from meta_ads_mcp.core.server import main -__version__ = "1.0.126" +__version__ = "1.0.127" __all__ = [ 'get_ad_accounts', diff --git a/meta_ads_mcp/core/auth.py b/meta_ads_mcp/core/auth.py index 31ea2bc7..9a073780 100644 --- a/meta_ads_mcp/core/auth.py +++ b/meta_ads_mcp/core/auth.py @@ -122,6 +122,30 @@ def deserialize(cls, data: Dict[str, Any]) -> 'TokenInfo': return token +# The token cache holds a long-lived Meta access token (~60 days), so it must be +# readable only by its owner. Before this, the file was created by a bare open() +# and the directory by mkdir() with no mode, landing at 0644 inside 0755 under +# the usual 0022 umask — readable by every local user. See GHSA-prmg-4fr3-mm6x. +TOKEN_CACHE_FILE_MODE = 0o600 +TOKEN_CACHE_DIR_MODE = 0o700 + + +def _restrict_permissions(path: pathlib.Path, mode: int) -> None: + """Narrow `path` to `mode` if it is wider, best effort. + + Also repairs caches written by earlier versions, so an operator does not + have to re-authenticate to get a private file. POSIX only: on Windows chmod + cannot express "owner only", and access is governed by ACLs instead. + """ + if platform.system() == "Windows": + return + try: + if (path.stat().st_mode & 0o777) != mode: + os.chmod(path, mode) + except OSError as e: + logger.warning(f"Could not restrict permissions on {path}: {e}") + + class AuthManager: """Manages authentication with Meta APIs""" def __init__(self, app_id: str, redirect_uri: str = AUTH_REDIRECT_URI): @@ -141,7 +165,10 @@ def _get_token_cache_path(self) -> pathlib.Path: # Create directory if it doesn't exist cache_dir = base_path / "meta-ads-mcp" - cache_dir.mkdir(parents=True, exist_ok=True) + cache_dir.mkdir(parents=True, exist_ok=True, mode=TOKEN_CACHE_DIR_MODE) + # mkdir's mode is masked by the umask and is a no-op for a directory that + # already exists, so narrow it explicitly either way. + _restrict_permissions(cache_dir, TOKEN_CACHE_DIR_MODE) return cache_dir / "token_cache.json" @@ -152,6 +179,8 @@ def _load_cached_token(self) -> bool: if not cache_path.exists(): return False + _restrict_permissions(cache_path, TOKEN_CACHE_FILE_MODE) + try: with open(cache_path, "r") as f: data = json.load(f) @@ -213,8 +242,21 @@ def _save_token_to_cache(self) -> None: cache_path = self._get_token_cache_path() try: - with open(cache_path, "w") as f: - json.dump(self.token_info.serialize(), f) + # O_CREAT's mode only applies when the file is created, so an + # existing cache from an earlier version is narrowed afterwards. + fd = os.open( + cache_path, + os.O_WRONLY | os.O_CREAT | os.O_TRUNC, + TOKEN_CACHE_FILE_MODE, + ) + try: + cache_file = os.fdopen(fd, "w") + except Exception: + os.close(fd) + raise + with cache_file: + json.dump(self.token_info.serialize(), cache_file) + _restrict_permissions(cache_path, TOKEN_CACHE_FILE_MODE) logger.info(f"Token cached at: {cache_path}") except Exception as e: logger.error(f"Error saving token to cache: {e}") diff --git a/pyproject.toml b/pyproject.toml index 8274f76a..0c7f9cb4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "meta-ads-mcp" -version = "1.0.126" +version = "1.0.127" 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" diff --git a/server.json b/server.json index d07946ce..682033f3 100644 --- a/server.json +++ b/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.126", + "version": "1.0.127", "remotes": [ { "type": "streamable-http", diff --git a/tests/test_token_cache_permissions.py b/tests/test_token_cache_permissions.py new file mode 100644 index 00000000..10a2d3dc --- /dev/null +++ b/tests/test_token_cache_permissions.py @@ -0,0 +1,122 @@ +"""Regression tests for GHSA-prmg-4fr3-mm6x. + +The token cache holds a long-lived Meta access token (~60 days). It was written +with a bare `open(path, "w")` into a directory created by `mkdir()` with no +mode, so under the usual 0022 umask it landed at 0644 inside 0755 — readable by +every other local user on a shared host. +""" + +import json +import os +import platform +import stat +import time + +import pytest + +from meta_ads_mcp.core.auth import ( + TOKEN_CACHE_DIR_MODE, + TOKEN_CACHE_FILE_MODE, + AuthManager, + TokenInfo, + _restrict_permissions, +) + +pytestmark = pytest.mark.skipif( + platform.system() == "Windows", + reason="POSIX permission bits do not apply on Windows", +) + + +def _mode(path) -> int: + return stat.S_IMODE(os.stat(path).st_mode) + + +@pytest.fixture +def home(tmp_path, monkeypatch): + """Point the platform cache lookup at a scratch HOME.""" + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setenv("APPDATA", str(tmp_path)) + monkeypatch.setattr("pathlib.Path.home", lambda: tmp_path) + (tmp_path / "Library" / "Application Support").mkdir(parents=True, exist_ok=True) + (tmp_path / ".config").mkdir(parents=True, exist_ok=True) + return tmp_path + + +@pytest.fixture +def manager(home): + auth_manager = AuthManager(app_id="test-app-id") + auth_manager.token_info = TokenInfo( + access_token="EAAG" + "x" * 60, + expires_in=60 * 24 * 3600, + user_id="123", + ) + return auth_manager + + +def test_token_cache_file_is_owner_only(manager): + manager._save_token_to_cache() + cache_path = manager._get_token_cache_path() + + assert _mode(cache_path) == TOKEN_CACHE_FILE_MODE + assert not _mode(cache_path) & 0o077, "group/other can read the Meta token" + + +def test_token_cache_directory_is_owner_only(manager): + cache_dir = manager._get_token_cache_path().parent + + assert _mode(cache_dir) == TOKEN_CACHE_DIR_MODE + assert not _mode(cache_dir) & 0o077 + + +def test_permissions_hold_under_a_permissive_umask(manager): + """0644 came from the umask, so the umask is what the fix must survive.""" + previous = os.umask(0o000) + try: + manager._save_token_to_cache() + finally: + os.umask(previous) + + assert _mode(manager._get_token_cache_path()) == TOKEN_CACHE_FILE_MODE + + +def test_existing_world_readable_cache_is_narrowed_on_save(manager): + cache_path = manager._get_token_cache_path() + cache_path.write_text("{}") + os.chmod(cache_path, 0o644) + + manager._save_token_to_cache() + + assert _mode(cache_path) == TOKEN_CACHE_FILE_MODE + + +def test_existing_world_readable_cache_is_narrowed_on_load(manager, home): + """A cache written by an older version is repaired without re-authenticating.""" + cache_path = manager._get_token_cache_path() + cache_path.write_text( + json.dumps( + { + "access_token": "EAAG" + "y" * 60, + "expires_in": 60 * 24 * 3600, + "created_at": int(time.time()), + "user_id": "123", + } + ) + ) + os.chmod(cache_path, 0o644) + + assert AuthManager(app_id="test-app-id")._load_cached_token() is True + assert _mode(cache_path) == TOKEN_CACHE_FILE_MODE + + +def test_saved_token_is_still_readable_by_its_owner(manager): + manager._save_token_to_cache() + + data = json.loads(manager._get_token_cache_path().read_text()) + + assert data["access_token"] == manager.token_info.access_token + + +def test_restrict_permissions_survives_a_missing_file(tmp_path): + """Best effort: a vanished path must not raise into the caller.""" + _restrict_permissions(tmp_path / "gone.json", TOKEN_CACHE_FILE_MODE)