mirror of
https://github.com/REMnux/remnux-mcp-server
synced 2026-06-21 13:45:33 +00:00
fix(security): confine upload_from_host source path under --sandbox
upload_from_host accepted any absolute host_path with no base-directory confinement. In docker/ssh mode it reads from the host where the server runs (the analyst's workstation), outside the container/VM isolation that bounds the rest of the server, so a prompt-injected client could stage a host file such as ~/.ssh/id_rsa into REMnux. Add opt-in confinement: with --sandbox, the source is realpath-resolved and must reside under --ingest-root (defaults to the samples dir). The resolved path is used for the read and the copy, so the validated and read paths match (closes the check-vs-read race) and a symlinked parent cannot redirect the read. In docker/ssh mode --ingest-root is required when --sandbox is set; the server fails closed at startup otherwise. Default behavior (no --sandbox) is unchanged: local mode already grants arbitrary read via run_tool by design, so confinement stays opt-in. Documents the connector-mode boundary in the Security Model. Verified end to end in local, docker, and ssh modes.
This commit is contained in:
@@ -122,6 +122,16 @@ docker run -d --name remnux remnux/remnux-distro:noble
|
||||
claude mcp add remnux -- npx @remnux/mcp-server --mode=docker --container=remnux
|
||||
```
|
||||
|
||||
To confine `upload_from_host` to a host-side sample directory (so a prompt-injected client cannot read other files off your workstation), add `--sandbox --ingest-root`:
|
||||
|
||||
```bash
|
||||
mkdir -p "$HOME/remnux-samples"
|
||||
claude mcp add remnux -- npx @remnux/mcp-server --mode=docker --container=remnux \
|
||||
--sandbox --ingest-root="$HOME/remnux-samples"
|
||||
```
|
||||
|
||||
See [Security Model](#security-model) for the reasoning. This is optional hardening. Without it, `upload_from_host` can read any file your user account can read.
|
||||
|
||||
**With a VM (SSH):**
|
||||
|
||||
```bash
|
||||
@@ -230,6 +240,7 @@ claude mcp add remnux --transport http http://REMNUX_IP:3000/mcp \
|
||||
| `--output-dir` | Output directory path inside REMnux | `/home/remnux/files/output` |
|
||||
| `--timeout` | Default command timeout in seconds | `300` |
|
||||
| `--sandbox` | Enable path sandboxing (restrict files to samples/output dirs) | off |
|
||||
| `--ingest-root` | With `--sandbox`, confine `upload_from_host` source reads to this directory (required in docker/ssh mode) | samples dir |
|
||||
| `--transport` | Transport mode: `stdio` or `http` | `stdio` |
|
||||
| `--http-port` | HTTP server port (for http transport) | `3000` |
|
||||
| `--http-host` | HTTP bind address (for http transport) | `127.0.0.1` |
|
||||
@@ -317,8 +328,11 @@ All three connection modes (docker, ssh, local) execute commands inside a dispos
|
||||
| Resource exhaustion (tools hang or consume excessive resources) | AI assistant / analysis session | Timeout enforcement (default 5 min), output budgets (40KB/tool default, 120KB total) |
|
||||
| Archive zip-slip (path traversal in archives) | Analysis session | Post-extraction validation rejects path escape attempts |
|
||||
| SSH injection | SSH connection | Proper shell escaping using single quotes |
|
||||
| Host-side file read via `upload_from_host` (docker/ssh mode) | Analyst's workstation (outside isolation) | Opt-in `--sandbox` confines the source to `--ingest-root` (realpath-resolved). See the disclosure below. |
|
||||
|
||||
**Other considerations:** A theoretical TOCTOU race exists between path validation and tool execution; container isolation is the primary mitigation (use immutable sample storage for high-security contexts). Tool description poisoning is mitigated by using build-time constants rather than runtime lookups from external sources.
|
||||
**Where `upload_from_host` reads from, and why it matters.** The relevant boundary is **connector mode (`local` vs `docker`/`ssh`), not transport**. In `local` mode (including HTTP transport with the local connector), the AI already has shell-level read on the REMnux box by design: `run_tool` executes arbitrary commands there, so `upload_from_host` reading a file outside the samples directory adds nothing beyond what the model already grants. In `docker`/`ssh` mode, `upload_from_host` is the one tool that reads from the machine where the server runs, the analyst's workstation, via `docker cp` or SFTP. That read happens outside the container/VM isolation that bounds everything else, so a prompt-injected client could stage a host file such as `~/.ssh/id_rsa` or `~/.aws/credentials` into REMnux. Enable `--sandbox` with `--ingest-root=<host staging dir>` to confine that read. In docker/ssh mode, `--ingest-root` is required when `--sandbox` is set, because the samples directory lives inside REMnux rather than on the host.
|
||||
|
||||
**Other considerations:** A theoretical TOCTOU race exists between path validation and tool execution; container isolation is the primary mitigation (use immutable sample storage for high-security contexts). The `upload_from_host` confinement closes its own check-vs-read race by reading the realpath it validated. Tool description poisoning is mitigated by using build-time constants rather than runtime lookups from external sources.
|
||||
|
||||
**What does NOT need protection (container/VM's job):** REMnux filesystem, packages, services, privileges, network config, devices, mounts, and path traversal inside REMnux — all disposable and container-isolated.
|
||||
|
||||
|
||||
@@ -2,8 +2,11 @@
|
||||
* Tests for file-upload module
|
||||
*/
|
||||
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { validateFilename, validateHostPath } from "../file-upload.js";
|
||||
import { describe, it, expect, beforeAll, afterAll } from "vitest";
|
||||
import { confineHostPath, validateFilename, validateHostPath } from "../file-upload.js";
|
||||
import { mkdtempSync, mkdirSync, writeFileSync, symlinkSync, rmSync } from "fs";
|
||||
import { tmpdir } from "os";
|
||||
import { join } from "path";
|
||||
|
||||
describe("validateFilename", () => {
|
||||
describe("valid filenames", () => {
|
||||
@@ -158,3 +161,60 @@ describe("validateHostPath", () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("confineHostPath", () => {
|
||||
let root: string;
|
||||
let outside: string;
|
||||
|
||||
beforeAll(() => {
|
||||
root = mkdtempSync(join(tmpdir(), "remnux-ingest-"));
|
||||
outside = mkdtempSync(join(tmpdir(), "remnux-outside-"));
|
||||
writeFileSync(join(root, "sample.exe"), "data");
|
||||
mkdirSync(join(root, "sub"));
|
||||
writeFileSync(join(root, "sub", "nested.bin"), "data");
|
||||
writeFileSync(join(outside, "secret.txt"), "secret");
|
||||
// root/escape is a symlink to the outside directory
|
||||
symlinkSync(outside, join(root, "escape"));
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
rmSync(root, { recursive: true, force: true });
|
||||
rmSync(outside, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("accepts a file directly inside the ingest root and returns its realPath", () => {
|
||||
const result = confineHostPath(join(root, "sample.exe"), root);
|
||||
expect(result.valid).toBe(true);
|
||||
expect(result.realPath).toBeDefined();
|
||||
});
|
||||
|
||||
it("accepts a file in a subdirectory of the root", () => {
|
||||
expect(confineHostPath(join(root, "sub", "nested.bin"), root).valid).toBe(true);
|
||||
});
|
||||
|
||||
it("rejects a file outside the ingest root", () => {
|
||||
const result = confineHostPath(join(outside, "secret.txt"), root);
|
||||
expect(result.valid).toBe(false);
|
||||
expect(result.error).toContain("escapes");
|
||||
});
|
||||
|
||||
it("rejects a symlinked-parent escape out of the root", () => {
|
||||
// The final component is a regular file, but its parent (root/escape) is a symlink
|
||||
// pointing outside the root — realpath resolution must detect the escape.
|
||||
const result = confineHostPath(join(root, "escape", "secret.txt"), root);
|
||||
expect(result.valid).toBe(false);
|
||||
expect(result.error).toContain("escapes");
|
||||
});
|
||||
|
||||
it("rejects a nonexistent source (cannot be resolved)", () => {
|
||||
const result = confineHostPath(join(root, "does-not-exist.bin"), root);
|
||||
expect(result.valid).toBe(false);
|
||||
expect(result.error).toContain("host_path could not be resolved");
|
||||
});
|
||||
|
||||
it("rejects when the ingest root itself cannot be resolved", () => {
|
||||
const result = confineHostPath(join(root, "sample.exe"), join(root, "no-such-root"));
|
||||
expect(result.valid).toBe(false);
|
||||
expect(result.error).toContain("ingest root could not be resolved");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -76,6 +76,10 @@ function parseArgs(): ServerConfig {
|
||||
config.outputDir = value;
|
||||
consumeValue();
|
||||
break;
|
||||
case "--ingest-root":
|
||||
config.ingestRoot = value;
|
||||
consumeValue();
|
||||
break;
|
||||
case "--timeout":
|
||||
config.timeout = parseIntOrExit(value, "--timeout");
|
||||
consumeValue();
|
||||
@@ -147,6 +151,8 @@ OPTIONS:
|
||||
--timeout <seconds> Default command timeout (default: 300)
|
||||
--sandbox Enable path sandboxing (restrict files to samples/output dirs)
|
||||
--no-sandbox No-op (sandbox is already off by default)
|
||||
--ingest-root <path> With --sandbox, confine upload_from_host source reads to this
|
||||
directory (defaults to samples dir; required in docker/ssh mode)
|
||||
--transport <mode> Transport: stdio (default) or http
|
||||
--http-port <port> HTTP port (default: 3000)
|
||||
--http-host <host> HTTP bind address (default: 127.0.0.1)
|
||||
|
||||
+61
-4
@@ -6,9 +6,9 @@
|
||||
*/
|
||||
|
||||
import { createHash } from "crypto";
|
||||
import { createReadStream, lstatSync } from "fs";
|
||||
import { createReadStream, lstatSync, realpathSync } from "fs";
|
||||
import { pipeline } from "stream/promises";
|
||||
import { basename, isAbsolute } from "path";
|
||||
import { basename, isAbsolute, relative, resolve, sep } from "path";
|
||||
import type { Connector } from "./connectors/index.js";
|
||||
|
||||
// 200MB limit for uploaded files (stat-based check, no in-memory buffering)
|
||||
@@ -87,6 +87,50 @@ export function validateHostPath(hostPath: string): { valid: boolean; error?: st
|
||||
return { valid: true };
|
||||
}
|
||||
|
||||
/**
|
||||
* Confine a host source path to an allowed ingest root (opt-in, --sandbox only).
|
||||
*
|
||||
* Resolves the real paths of both the root and the candidate so a symlinked parent
|
||||
* directory cannot redirect the read outside the root. Returns the canonical realPath
|
||||
* for the caller to read/copy, so the validated path and the read path are identical
|
||||
* (closes the check-vs-read race).
|
||||
*
|
||||
* @param hostPath - Absolute host path supplied by the caller
|
||||
* @param ingestRoot - Directory the source must reside within
|
||||
* @returns valid flag, error message on failure, and the resolved realPath on success
|
||||
*/
|
||||
export function confineHostPath(
|
||||
hostPath: string,
|
||||
ingestRoot: string,
|
||||
): { valid: boolean; error?: string; realPath?: string } {
|
||||
let root: string;
|
||||
try {
|
||||
root = realpathSync(resolve(ingestRoot));
|
||||
} catch (err) {
|
||||
return {
|
||||
valid: false,
|
||||
error: `ingest root could not be resolved (${ingestRoot}): ${err instanceof Error ? err.message : "unknown error"}`,
|
||||
};
|
||||
}
|
||||
|
||||
let target: string;
|
||||
try {
|
||||
target = realpathSync(resolve(hostPath));
|
||||
} catch (err) {
|
||||
return {
|
||||
valid: false,
|
||||
error: `host_path could not be resolved: ${err instanceof Error ? err.message : "unknown error"}`,
|
||||
};
|
||||
}
|
||||
|
||||
const rel = relative(root, target);
|
||||
if (rel === ".." || rel.startsWith(".." + sep) || isAbsolute(rel)) {
|
||||
return { valid: false, error: `host_path escapes the allowed ingest root (${ingestRoot})` };
|
||||
}
|
||||
|
||||
return { valid: true, realPath: target };
|
||||
}
|
||||
|
||||
/**
|
||||
* Upload a file from the host filesystem to the samples directory
|
||||
*
|
||||
@@ -95,6 +139,7 @@ export function validateHostPath(hostPath: string): { valid: boolean; error?: st
|
||||
* @param hostPath - Absolute path on the host filesystem
|
||||
* @param filename - Override filename (defaults to basename of hostPath)
|
||||
* @param overwrite - Whether to overwrite if file exists (default: false)
|
||||
* @param ingestRoot - When set (opt-in via --sandbox), confine the source to this root
|
||||
* @returns Upload result with file path, size, and SHA256 hash
|
||||
*/
|
||||
export async function uploadSampleFromHost(
|
||||
@@ -104,6 +149,7 @@ export async function uploadSampleFromHost(
|
||||
filename?: string,
|
||||
overwrite: boolean = false,
|
||||
mode: "docker" | "ssh" | "local" = "docker",
|
||||
ingestRoot?: string,
|
||||
): Promise<UploadResult> {
|
||||
// Validate host path
|
||||
const pathValidation = validateHostPath(hostPath);
|
||||
@@ -145,6 +191,17 @@ export async function uploadSampleFromHost(
|
||||
};
|
||||
}
|
||||
|
||||
// Opt-in confinement (--sandbox): restrict the source to an allowed ingest root.
|
||||
// Resolves symlinks and reads the canonical path so the validated and read paths match.
|
||||
let readPath = hostPath;
|
||||
if (ingestRoot) {
|
||||
const confinement = confineHostPath(hostPath, ingestRoot);
|
||||
if (!confinement.valid) {
|
||||
return { success: false, error: confinement.error };
|
||||
}
|
||||
readPath = confinement.realPath!;
|
||||
}
|
||||
|
||||
// Determine target filename
|
||||
const targetFilename = filename ?? basename(hostPath);
|
||||
|
||||
@@ -158,7 +215,7 @@ export async function uploadSampleFromHost(
|
||||
let sha256: string;
|
||||
try {
|
||||
const hash = createHash("sha256");
|
||||
await pipeline(createReadStream(hostPath), hash);
|
||||
await pipeline(createReadStream(readPath), hash);
|
||||
sha256 = hash.digest("hex");
|
||||
} catch (err) {
|
||||
return {
|
||||
@@ -196,7 +253,7 @@ export async function uploadSampleFromHost(
|
||||
|
||||
// Write file using connector's streaming path-based method
|
||||
try {
|
||||
await connector.writeFileFromPath(filePath, hostPath);
|
||||
await connector.writeFileFromPath(filePath, readPath);
|
||||
} catch (err) {
|
||||
return {
|
||||
success: false,
|
||||
|
||||
@@ -63,6 +63,7 @@ describe("handleUploadFromHost", () => {
|
||||
undefined,
|
||||
true,
|
||||
"docker",
|
||||
"/samples", // sandbox on by default in mock → confine to samplesDir
|
||||
);
|
||||
});
|
||||
|
||||
@@ -88,6 +89,51 @@ describe("handleUploadFromHost", () => {
|
||||
"renamed.exe",
|
||||
false,
|
||||
"docker",
|
||||
"/samples", // sandbox on by default in mock → confine to samplesDir
|
||||
);
|
||||
});
|
||||
|
||||
it("passes ingestRoot=undefined when sandbox is disabled (noSandbox)", async () => {
|
||||
const deps = createMockDeps({ noSandbox: true });
|
||||
vi.mocked(uploadSampleFromHost).mockResolvedValue({
|
||||
success: true,
|
||||
path: "/samples/test.exe",
|
||||
sha256: "abc",
|
||||
size_bytes: 100,
|
||||
});
|
||||
|
||||
await handleUploadFromHost(deps, { host_path: "/tmp/test.exe", overwrite: false });
|
||||
|
||||
expect(uploadSampleFromHost).toHaveBeenCalledWith(
|
||||
deps.connector,
|
||||
"/samples",
|
||||
"/tmp/test.exe",
|
||||
undefined,
|
||||
false,
|
||||
"docker",
|
||||
undefined, // no confinement when sandbox is off
|
||||
);
|
||||
});
|
||||
|
||||
it("confines to --ingest-root when sandbox is enabled", async () => {
|
||||
const deps = createMockDeps({ noSandbox: false, ingestRoot: "/srv/ingest" });
|
||||
vi.mocked(uploadSampleFromHost).mockResolvedValue({
|
||||
success: true,
|
||||
path: "/samples/test.exe",
|
||||
sha256: "abc",
|
||||
size_bytes: 100,
|
||||
});
|
||||
|
||||
await handleUploadFromHost(deps, { host_path: "/srv/ingest/test.exe", overwrite: false });
|
||||
|
||||
expect(uploadSampleFromHost).toHaveBeenCalledWith(
|
||||
deps.connector,
|
||||
"/samples",
|
||||
"/srv/ingest/test.exe",
|
||||
undefined,
|
||||
false,
|
||||
"docker",
|
||||
"/srv/ingest", // explicit ingest root takes precedence over samplesDir
|
||||
);
|
||||
});
|
||||
|
||||
|
||||
@@ -8,6 +8,7 @@ export interface HandlerConfig {
|
||||
noSandbox: boolean;
|
||||
mode: "docker" | "ssh" | "local";
|
||||
transport?: "stdio" | "http";
|
||||
ingestRoot?: string;
|
||||
}
|
||||
|
||||
export interface HandlerDeps {
|
||||
|
||||
@@ -36,6 +36,11 @@ export async function handleUploadFromHost(
|
||||
), startTime);
|
||||
}
|
||||
|
||||
// Opt-in confinement: only when --sandbox is enabled (config.noSandbox === false).
|
||||
// Defaults to samplesDir; docker/ssh deployments set --ingest-root to a host-side dir
|
||||
// (enforced at startup so this default is never silently wrong there).
|
||||
const ingestRoot = config.noSandbox ? undefined : (config.ingestRoot ?? config.samplesDir);
|
||||
|
||||
try {
|
||||
const result = await uploadSampleFromHost(
|
||||
connector,
|
||||
@@ -44,6 +49,7 @@ export async function handleUploadFromHost(
|
||||
args.filename,
|
||||
args.overwrite,
|
||||
config.mode,
|
||||
ingestRoot,
|
||||
);
|
||||
|
||||
if (result.success) {
|
||||
|
||||
+20
-2
@@ -47,6 +47,7 @@ export interface ServerConfig extends ConnectorConfig {
|
||||
outputDir: string;
|
||||
timeout: number;
|
||||
noSandbox?: boolean;
|
||||
ingestRoot?: string;
|
||||
transport?: "stdio" | "http";
|
||||
httpPort?: number;
|
||||
httpHost?: string;
|
||||
@@ -92,6 +93,7 @@ export async function createServer(config: ServerConfig) {
|
||||
noSandbox: config.noSandbox ?? false,
|
||||
mode: config.mode,
|
||||
transport: config.transport,
|
||||
ingestRoot: config.ingestRoot,
|
||||
},
|
||||
sessionState,
|
||||
};
|
||||
@@ -395,6 +397,18 @@ export async function createServer(config: ServerConfig) {
|
||||
export async function startServer(config: ServerConfig) {
|
||||
const transportMode = config.transport ?? "stdio";
|
||||
|
||||
// Fail closed: --sandbox confinement in docker/ssh mode reads from the host where the
|
||||
// server runs, but samplesDir is the REMnux-side path and is meaningless there. Require
|
||||
// an explicit --ingest-root rather than silently confining to a path that rejects uploads.
|
||||
const sandboxOn = !(config.noSandbox ?? false);
|
||||
if (sandboxOn && (config.mode === "docker" || config.mode === "ssh") && !config.ingestRoot) {
|
||||
console.error(
|
||||
`Error: --sandbox in ${config.mode} mode requires --ingest-root=<host directory> to ` +
|
||||
"confine upload_from_host reads (the samples directory is inside REMnux, not on the host).",
|
||||
);
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
if (transportMode === "http") {
|
||||
await startHttpServer(config);
|
||||
} else {
|
||||
@@ -412,7 +426,9 @@ export async function startServer(config: ServerConfig) {
|
||||
process.on("SIGINT", shutdown);
|
||||
process.on("SIGTERM", shutdown);
|
||||
|
||||
const warnings = config.noSandbox ? " (WARNING: sandbox disabled)" : "";
|
||||
const warnings = sandboxOn
|
||||
? ` (upload_from_host confined to ${config.ingestRoot ?? config.samplesDir})`
|
||||
: " (WARNING: sandbox disabled)";
|
||||
console.error(`REMnux MCP server started${warnings}`);
|
||||
}
|
||||
}
|
||||
@@ -515,7 +531,9 @@ async function startHttpServer(config: ServerConfig) {
|
||||
}
|
||||
});
|
||||
|
||||
const warnings = config.noSandbox ? " (WARNING: sandbox disabled)" : "";
|
||||
const warnings = !(config.noSandbox ?? false)
|
||||
? ` (upload_from_host confined to ${config.ingestRoot ?? config.samplesDir})`
|
||||
: " (WARNING: sandbox disabled)";
|
||||
const authStatus = token ? "auth enabled" : "NO AUTH";
|
||||
|
||||
return new Promise<void>((resolve) => {
|
||||
|
||||
@@ -25,7 +25,7 @@ export const extractArchiveSchema = z.object({
|
||||
export type ExtractArchiveArgs = z.infer<typeof extractArchiveSchema>;
|
||||
|
||||
export const uploadFromHostSchema = z.object({
|
||||
host_path: z.string().describe("Absolute path to the file on the machine where the MCP server runs (not the remote client in HTTP deployments)"),
|
||||
host_path: z.string().describe("Absolute path to the file on the machine where the MCP server runs (not the remote client in HTTP deployments). When the server is started with --sandbox, the resolved path must reside inside the configured --ingest-root (defaults to the samples directory)."),
|
||||
filename: z.string().optional().describe("Override filename in samples dir (defaults to basename of host_path)"),
|
||||
overwrite: z.boolean().optional().default(false).describe("Whether to overwrite if file exists. Default: false"),
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user