GHSA-p3h2-2j4p-p83g: MCPHub has Path Traversal via Malicious MCPB Manifest Name
**MCPB File Upload Handler** extracts a ZIP file and reads `manifest.json` from it. The `name` field in the manifest is directly concatenated into a file path (line 107) without any sanitization or path traversal character validation. An attacker can craft a malicious MCPB file where `manifest.name` is set to something like `../../../etc/malicious`, causing the file to be extracted to an arbitrary location on the file system. The `cleanupOldMcpbServer` function (line 110) also uses the unsanitized name, potentially allowing deletion of arbitrary directories.1. Summary**Vulnerability Type**: Path Traversal (CWE-22)**Sink Location**: src/controllers/mcpbController.ts:107**Vulnerability Description**: The `name` field from an uploaded MCPB manifest is used directly, without sanitization or normalization, to construct a file system path for directory creation and move operations, which may lead to path traversal attacks.2. Analysis LogicStep 1: Inspect the identified sink (src/controllers/mcpbController.ts:106-116)I examined the upload handler and located the file system sink where `manifest.name` is used to build the final extraction path and write files to that path.Analysis: `manifest.name` is used to build `finalExtractDir`, which is then operated on by `fs.mkdirSync` and `fs.renameSync`. These are file system write/move operations, so if `name` is user-controlled and unsanitized, this is a path traversal sink. Next, I traced the origin of `manifest.name`.Step 2: Trace the source of `manifest.name` in the upload handler (src/controllers/mcpbController.ts:83-104)I traced back the data flow to see how the manifest is read and validated.Analysis: `manifest` is parsed directly from `manifest.json` inside the uploaded archive. The only check on `manifest.name` is that it is non‑empty; there is no sanitization, normalization, or allow‑list validation. Next, I confirmed the entry point for uploading MCPB files to verify user control.Step 3: Trace the HTTP entry point in src/routes/index.ts:297-299I located the route that exposes the upload handler.Analysis: The `/mcpb/upload` endpoint invokes `uploadMiddleware` and `uploadMcpbFile`, so user‑supplied uploads are the source of the manifest content. Next, I verified the upload middleware behavior.Step 4: Confirm the upload middleware (src/controllers/mcpbController.ts:8-38)I inspected how the uploaded file is received and stored.Analysis: The upload middleware only checks file extension and size. It does not restrict or validate the contents of the archive or `manifest.name`. Therefore, `manifest.name` is user‑controlled input. Next, I checked whether any sanitization or normalization is applied before reaching the sink.Step 5: Verify lack of path validation on `manifest.name` in src/controllers/mcpbController.ts:92-110I verified that no path sanitization occurs between parsing and usage.Analysis: Before using `manifest.name` to construct a file system path, there is no `path.resolve`/`realpath` check, no use of `basename()`, and no allow‑list validation. This confirms that the path is built from untrusted input without defenses.Step 6: Examine cleanup behavior using the unsanitized name (src/controllers/mcpbController.ts:41-52)I verified how `cleanupOldMcpbServer` uses the same input.Analysis: `serverName` is used without validation, but the deletion is limited to directories already present in `uploadDir` as returned by `readdirSync`. The main traversal risk remains in constructing the path for `finalExtractDir` and the subsequent file system operations.Analysis WalkthroughQ1: Does user‑controllable input affect the file path? → **Yes**. `manifest.name` is read from the uploaded archive’s `manifest.json` and used in `path.join(...)` to build `finalExtractDir` (src/controllers/mcpbController.ts:89-110).Q2: Is the path normalized and validated against a base directory? → **No**. There is no `resolve`/`realpath` + `startsWith` check before `fs.mkdirSync`/`fs.renameSync` (src/controllers/mcpbController.ts:106-116).Q3: Is `basename()`/`getName()` used to strip directory components? → **No**. `manifest.name` is used directly in a template string (src/controllers/mcpbController.ts:106-107).Q4: Is there a valid allow‑list for allowed names? → **No**. Only an existence check is performed on `manifest.name` (src/controllers/mcpbController.ts:92-97).Q5: Is the code in a test/demo/deprecated/generated context? → **No**. This is a production controller and route (src/controllers/mcpbController.ts:64-130, src/routes/index.ts:297-299).→ Reached leaf node: **True Positive**3. Conclusion**True Positive****Key evidence:**`manifest.name` flows directly into `finalExtractDir` and is used by `fs.mkdirSync` and `fs.renameSync` without sanitization (src/controllers/mcpbController.ts:106-116).`manifest.name` is parsed from `manifest.json` inside an uploaded archive, with only a non‑empty check (src/controllers/mcpbController.ts:89-97).The `/mcpb/upload` endpoint exposes the upload handler that processes user‑supplied archives (src/routes/index.ts:297-299).4. Remediation RecommendationsAdd normalization and base directory validation before using `manifest.name` to construct `finalExtractDir` (e.g., `const resolved = path.resolve(baseDir, `server-${safeName}`); if (!resolved.startsWith(baseDir)) reject;`).Use `path.basename()` to strip directory components from `manifest.name` and enforce a strict character allow‑list (alphanumeric, `_`, `-`, `.`) before use.Consider rejecting any `manifest.name` that contains path separators or traversal sequences, and add unit tests for malicious traversal inputs.
CVSS v4.0: 7.2