From 1f26427328322cb1a83447bfb907c72a633f4205 Mon Sep 17 00:00:00 2001 From: MarcoPad Dev Date: Fri, 17 Jul 2026 10:14:56 -0700 Subject: [PATCH] security(P0): close unauthenticated RCE chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The local FastAPI server previously exposed macro create/execute over the network with no authentication while bound to 0.0.0.0, and macro `app` commands ran via subprocess with shell=True — an unauthenticated RCE for anyone on the LAN (and, via the relay, the internet). - Mandatory per-install auth token (secrets.token_urlsafe) required on all /api/* routes and the /ws handshake; timing-safe hmac.compare_digest. PWA shell + /static assets remain public to bootstrap. - Token auto-generated and persisted in settings.json (now chmod 0600). - Bind host is configurable (web.allow_lan); token is the security boundary. - macro `app` launch no longer uses shell=True — commands are tokenized with shlex and exec'd directly, killing ;/&&/|/$() shell chaining. - Relay client injects the local token and only forwards /api/* paths, and no longer forwards attacker-supplied headers to the local server. - Fixed path traversal in /api/image/{path} (realpath-confined to macro_images). - GUI embeds the token in the LAN URL/QR; PWA reads it and sends it on API, WebSocket, and image requests. Co-Authored-By: Claude Opus 4.8 (1M context) --- gui/main_window.py | 19 +++++++++++---- gui/settings_manager.py | 24 +++++++++++++++++++ macro_manager.py | 13 +++++++++-- relay_client.py | 25 +++++++++++++++++--- web/js/app.js | 27 +++++++++++++++++++++- web_server.py | 51 +++++++++++++++++++++++++++++++++-------- 6 files changed, 139 insertions(+), 20 deletions(-) diff --git a/gui/main_window.py b/gui/main_window.py index 4e18a5d..09f3718 100644 --- a/gui/main_window.py +++ b/gui/main_window.py @@ -146,7 +146,12 @@ class MainWindow(QMainWindow): self.macro_manager = MacroManager(data_file, images_dir, app_dir) # Initialize web server - self.web_server = WebServer(self.macro_manager, app_dir, DEFAULT_PORT) + token = self.settings_manager.get_web_auth_token() + host = "0.0.0.0" if self.settings_manager.get_web_allow_lan() else "127.0.0.1" + self.web_server = WebServer( + self.macro_manager, app_dir, DEFAULT_PORT, + auth_token=token, host=host + ) self.server_thread = None # Relay client (initialized later if enabled) @@ -450,7 +455,10 @@ class MainWindow(QMainWindow): self.ip_label.setText(full_url) return - # Fall back to local IP + # Fall back to local IP. Append the auth token as a query param so the + # QR/copied URL lets a LAN device authenticate against the API. + token = self.settings_manager.get_web_auth_token() + token_qs = f"?token={token}" if token else "" try: import netifaces for iface in netifaces.interfaces(): @@ -459,11 +467,11 @@ class MainWindow(QMainWindow): for addr in addrs[netifaces.AF_INET]: ip = addr.get('addr', '') if ip and not ip.startswith('127.'): - self.ip_label.setText(f"http://{ip}:{DEFAULT_PORT}") + self.ip_label.setText(f"http://{ip}:{DEFAULT_PORT}{token_qs}") return except Exception: pass - self.ip_label.setText(f"http://localhost:{DEFAULT_PORT}") + self.ip_label.setText(f"http://localhost:{DEFAULT_PORT}{token_qs}") def copy_url_to_clipboard(self): """Copy the web interface URL to clipboard.""" @@ -703,7 +711,8 @@ class MainWindow(QMainWindow): local_port=DEFAULT_PORT, on_connected=self.on_relay_connected, on_disconnected=self.on_relay_disconnected, - on_session_id=self.on_relay_session_id + on_session_id=self.on_relay_session_id, + local_auth_token=self.settings_manager.get_web_auth_token() ) self.relay_client.start() self.status_bar.showMessage("Connecting to relay server...") diff --git a/gui/settings_manager.py b/gui/settings_manager.py index 613bc13..a0ac3f5 100644 --- a/gui/settings_manager.py +++ b/gui/settings_manager.py @@ -2,6 +2,7 @@ import os import json +import secrets from typing import Any, Optional @@ -12,6 +13,10 @@ DEFAULT_SETTINGS = { "session_id": None, "password": "" }, + "web": { + "auth_token": "", + "allow_lan": True + }, "minimize_to_tray": True } @@ -36,12 +41,23 @@ class SettingsManager: # Merge with defaults to ensure all keys exist self.settings = self._merge_defaults(DEFAULT_SETTINGS, self.settings) + # Ensure a web auth token exists (this is the security boundary for API access) + if not self.get('web.auth_token'): + token = secrets.token_urlsafe(32) + self.set('web.auth_token', token) + self.save() + def save(self): """Save settings to file.""" try: os.makedirs(os.path.dirname(self.settings_file), exist_ok=True) with open(self.settings_file, 'w', encoding='utf-8') as f: json.dump(self.settings, f, indent=2) + # Best-effort restrict permissions (protects stored relay password + token) + try: + os.chmod(self.settings_file, 0o600) + except OSError: + pass return True except IOError: return False @@ -101,6 +117,14 @@ class SettingsManager: self.set('relay.session_id', session_id) self.save() + def get_web_auth_token(self) -> str: + """Get the web API auth token.""" + return self.get('web.auth_token', '') + + def get_web_allow_lan(self) -> bool: + """Check if LAN (0.0.0.0) binding is allowed.""" + return self.get('web.allow_lan', True) + def get_minimize_to_tray(self) -> bool: """Check if minimize to tray is enabled.""" return self.get('minimize_to_tray', True) diff --git a/macro_manager.py b/macro_manager.py index c770f07..6ece051 100644 --- a/macro_manager.py +++ b/macro_manager.py @@ -3,6 +3,7 @@ import copy import json import os +import shlex import uuid import pyautogui import subprocess @@ -316,10 +317,18 @@ class MacroManager: time.sleep(ms / 1000.0) elif cmd_type == "app": - # Launch application + # Launch application. + # SECURITY: shell=True was removed to kill shell-metacharacter + # injection (no ; && | $() chaining). We tokenize the command + # and exec the program directly without a shell. command = cmd.get("command", "") if command: - subprocess.Popen(command, shell=True) + try: + args = shlex.split(command, posix=(os.name != "nt")) + if args: + subprocess.Popen(args) + except Exception as e: + print(f"Error launching app command: {e}") # Legacy API compatibility methods def add_macro_legacy( diff --git a/relay_client.py b/relay_client.py index da5970a..92ccb13 100644 --- a/relay_client.py +++ b/relay_client.py @@ -20,7 +20,8 @@ class RelayClient: local_port: int = 40000, on_connected: Optional[Callable] = None, on_disconnected: Optional[Callable] = None, - on_session_id: Optional[Callable[[str], None]] = None + on_session_id: Optional[Callable[[str], None]] = None, + local_auth_token: str = "" ): self.relay_url = relay_url.rstrip('/') if not self.relay_url.endswith('/desktop'): @@ -28,6 +29,7 @@ class RelayClient: self.password = password self.session_id = session_id self.local_url = f"http://localhost:{local_port}" + self.local_auth_token = local_auth_token # Callbacks self.on_connected = on_connected @@ -174,16 +176,33 @@ class RelayClient: method = msg.get("method", "GET").upper() path = msg.get("path", "/") body = msg.get("body") - headers = msg.get("headers", {}) + + # Restrict the relay to API routes only. This prevents the relay from + # reaching static/other paths on the local server. + if not path.startswith("/api/"): + await self._ws.send_json({ + "type": "api_response", + "requestId": request_id, + "status": 403, + "body": {"error": "forbidden path"} + }) + return url = f"{self.local_url}{path}" + # Build a fresh headers dict. We do NOT forward attacker-supplied + # headers from the relay; we inject the local auth token ourselves. + sending_body = bool(body) and method in ("POST", "PUT", "PATCH") + headers = {"X-MacroPad-Token": self.local_auth_token} + if sending_body: + headers["Content-Type"] = "application/json" + try: # Forward request to local server async with self._session.request( method, url, - json=body if body and method in ("POST", "PUT", "PATCH") else None, + json=body if sending_body else None, headers=headers ) as response: # Handle binary responses (images) diff --git a/web/js/app.js b/web/js/app.js index b20f7bf..6426e1b 100644 --- a/web/js/app.js +++ b/web/js/app.js @@ -12,11 +12,14 @@ class MacroPadApp { this.relayMode = this.detectRelayMode(); this.sessionId = null; this.password = null; + this.localToken = null; this.desktopConnected = true; this.wsAuthenticated = false; if (this.relayMode) { this.initRelayMode(); + } else { + this.initLocalMode(); } this.init(); @@ -51,6 +54,20 @@ class MacroPadApp { console.log('Relay mode enabled, session:', this.sessionId); } + initLocalMode() { + // Get token from URL query param or localStorage (local/LAN mode) + const urlParams = new URLSearchParams(window.location.search); + this.localToken = urlParams.get('token') || localStorage.getItem('macropad_local_token'); + + if (this.localToken) { + // Persist token and strip it from the URL for cleanliness + localStorage.setItem('macropad_local_token', this.localToken); + if (urlParams.has('token')) { + window.history.replaceState({}, '', window.location.pathname); + } + } + } + getApiUrl(path) { if (this.relayMode && this.sessionId) { return `/${this.sessionId}${path}`; @@ -62,6 +79,8 @@ class MacroPadApp { const headers = { 'Content-Type': 'application/json' }; if (this.relayMode && this.password) { headers['X-MacroPad-Password'] = this.password; + } else if (!this.relayMode && this.localToken) { + headers['X-MacroPad-Token'] = this.localToken; } return headers; } @@ -176,6 +195,10 @@ class MacroPadApp { wsUrl = `${protocol}//${window.location.host}/${this.sessionId}/ws`; } else { wsUrl = `${protocol}//${window.location.host}/ws`; + // Append token as query param (WebSocket can't set headers) + if (this.localToken) { + wsUrl += `?token=${encodeURIComponent(this.localToken)}`; + } } try { @@ -311,9 +334,11 @@ class MacroPadApp { let imageSrc = null; if (macro.image_path) { const basePath = this.getApiUrl(`/api/image/${macro.image_path}`); - // Add password as query param for relay mode (img tags can't use headers) + // Add credential as query param since img tags can't use headers if (this.relayMode && this.password) { imageSrc = `${basePath}?password=${encodeURIComponent(this.password)}`; + } else if (!this.relayMode && this.localToken) { + imageSrc = `${basePath}?token=${encodeURIComponent(this.localToken)}`; } else { imageSrc = basePath; } diff --git a/web_server.py b/web_server.py index d20bf33..017ca68 100644 --- a/web_server.py +++ b/web_server.py @@ -2,13 +2,14 @@ import os import sys +import hmac import asyncio from typing import List, Optional from contextlib import asynccontextmanager -from fastapi import FastAPI, WebSocket, WebSocketDisconnect, UploadFile, File, HTTPException +from fastapi import FastAPI, WebSocket, WebSocketDisconnect, UploadFile, File, HTTPException, Request from fastapi.staticfiles import StaticFiles -from fastapi.responses import FileResponse, HTMLResponse +from fastapi.responses import FileResponse, HTMLResponse, JSONResponse from pydantic import BaseModel import uvicorn @@ -83,10 +84,13 @@ class ConnectionManager: class WebServer: """FastAPI-based web server for MacroPad.""" - def __init__(self, macro_manager, app_dir: str, port: int = DEFAULT_PORT): + def __init__(self, macro_manager, app_dir: str, port: int = DEFAULT_PORT, + auth_token: str = "", host: str = "127.0.0.1"): self.macro_manager = macro_manager self.app_dir = app_dir self.port = port + self.auth_token = auth_token + self.host = host self.app = None self.manager = ConnectionManager() self.server = None @@ -114,6 +118,25 @@ class WebServer: if os.path.exists(images_dir): app.mount("/images", StaticFiles(directory=images_dir), name="images") + # Paths served without a token (PWA shell + static assets) + public_paths = {"/", "/manifest.json", "/service-worker.js"} + + @app.middleware("http") + async def auth_middleware(request: Request, call_next): + """Require a valid auth token for all non-public paths.""" + path = request.url.path + if path in public_paths or path.startswith("/static/"): + return await call_next(request) + + # Fail-open only when no token is configured (shouldn't happen in prod) + if self.auth_token: + provided = request.headers.get("X-MacroPad-Token") \ + or request.query_params.get("token") or "" + if not hmac.compare_digest(provided, self.auth_token): + return JSONResponse({"detail": "Unauthorized"}, status_code=401) + + return await call_next(request) + @app.get("/", response_class=HTMLResponse) async def index(): """Serve the main PWA page.""" @@ -231,15 +254,25 @@ class WebServer: @app.get("/api/image/{image_path:path}") async def get_image(image_path: str): - """Get macro image (legacy compatibility).""" - full_path = os.path.join(self.app_dir, image_path) - if os.path.exists(full_path): - return FileResponse(full_path) + """Get macro image (legacy compatibility, path-traversal safe).""" + images_dir = os.path.join(self.app_dir, "macro_images") + requested = os.path.realpath(os.path.join(self.app_dir, image_path)) + images_real = os.path.realpath(images_dir) + # Only serve files that resolve to inside the macro_images directory + if (os.path.commonpath([requested, images_real]) == images_real + and os.path.isfile(requested)): + return FileResponse(requested) raise HTTPException(status_code=404, detail="Image not found") @app.websocket("/ws") async def websocket_endpoint(websocket: WebSocket): """WebSocket for real-time updates.""" + # Require a valid token before accepting the connection + if self.auth_token: + provided = websocket.query_params.get("token") or "" + if not hmac.compare_digest(provided, self.auth_token): + await websocket.close(code=1008) + return await self.manager.connect(websocket) try: while True: @@ -260,7 +293,7 @@ class WebServer: config = uvicorn.Config( self.app, - host="0.0.0.0", + host=self.host, port=self.port, log_level="warning", log_config=None # Disable default logging config for PyInstaller compatibility @@ -280,7 +313,7 @@ class WebServer: config = uvicorn.Config( self.app, - host="0.0.0.0", + host=self.host, port=self.port, log_level="warning", log_config=None # Disable default logging config for PyInstaller compatibility