fix(security): harden SSH target handling (#4001)
Thanks @YLChen-007. Co-authored-by: Edward-x <YLChen-007@users.noreply.github.com>
This commit is contained in:
parent
4b5514a259
commit
06289b36da
@ -6,6 +6,7 @@ Docs: https://docs.molt.bot
|
|||||||
Status: beta.
|
Status: beta.
|
||||||
|
|
||||||
### Changes
|
### Changes
|
||||||
|
- Security: harden SSH tunnel target parsing to prevent option injection/DoS. (#4001) Thanks @YLChen-007.
|
||||||
- Rebrand: rename the npm package/CLI to `moltbot`, add a `moltbot` compatibility shim, and move extensions to the `@moltbot/*` scope.
|
- Rebrand: rename the npm package/CLI to `moltbot`, add a `moltbot` compatibility shim, and move extensions to the `@moltbot/*` scope.
|
||||||
- Commands: group /help and /commands output with Telegram paging. (#2504) Thanks @hougangdev.
|
- Commands: group /help and /commands output with Telegram paging. (#2504) Thanks @hougangdev.
|
||||||
- macOS: limit project-local `node_modules/.bin` PATH preference to debug builds (reduce PATH hijacking risk).
|
- macOS: limit project-local `node_modules/.bin` PATH preference to debug builds (reduce PATH hijacking risk).
|
||||||
|
|||||||
@ -192,6 +192,45 @@ describe("gateway-status command", () => {
|
|||||||
expect(targets.some((t) => t.kind === "sshTunnel")).toBe(true);
|
expect(targets.some((t) => t.kind === "sshTunnel")).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("skips invalid ssh-auto discovery targets", async () => {
|
||||||
|
const runtimeLogs: string[] = [];
|
||||||
|
const runtime = {
|
||||||
|
log: (msg: string) => runtimeLogs.push(msg),
|
||||||
|
error: (_msg: string) => {},
|
||||||
|
exit: (code: number) => {
|
||||||
|
throw new Error(`__exit__:${code}`);
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
const originalUser = process.env.USER;
|
||||||
|
try {
|
||||||
|
process.env.USER = "steipete";
|
||||||
|
loadConfig.mockReturnValueOnce({
|
||||||
|
gateway: {
|
||||||
|
mode: "remote",
|
||||||
|
remote: {},
|
||||||
|
},
|
||||||
|
});
|
||||||
|
discoverGatewayBeacons.mockResolvedValueOnce([
|
||||||
|
{ tailnetDns: "-V" },
|
||||||
|
{ tailnetDns: "goodhost" },
|
||||||
|
]);
|
||||||
|
|
||||||
|
startSshPortForward.mockClear();
|
||||||
|
const { gatewayStatusCommand } = await import("./gateway-status.js");
|
||||||
|
await gatewayStatusCommand(
|
||||||
|
{ timeout: "1000", json: true, sshAuto: true },
|
||||||
|
runtime as unknown as import("../runtime.js").RuntimeEnv,
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(startSshPortForward).toHaveBeenCalledTimes(1);
|
||||||
|
const call = startSshPortForward.mock.calls[0]?.[0] as { target: string };
|
||||||
|
expect(call.target).toBe("steipete@goodhost");
|
||||||
|
} finally {
|
||||||
|
process.env.USER = originalUser;
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
it("infers SSH target from gateway.remote.url and ssh config", async () => {
|
it("infers SSH target from gateway.remote.url and ssh config", async () => {
|
||||||
const runtimeLogs: string[] = [];
|
const runtimeLogs: string[] = [];
|
||||||
const runtime = {
|
const runtime = {
|
||||||
|
|||||||
@ -107,7 +107,9 @@ export async function gatewayStatusCommand(
|
|||||||
const base = user ? `${user}@${host.trim()}` : host.trim();
|
const base = user ? `${user}@${host.trim()}` : host.trim();
|
||||||
return sshPort !== 22 ? `${base}:${sshPort}` : base;
|
return sshPort !== 22 ? `${base}:${sshPort}` : base;
|
||||||
})
|
})
|
||||||
.filter((x): x is string => Boolean(x));
|
.filter((candidate): candidate is string =>
|
||||||
|
Boolean(candidate && parseSshTarget(candidate)),
|
||||||
|
);
|
||||||
if (candidates.length > 0) sshTarget = candidates[0] ?? null;
|
if (candidates.length > 0) sshTarget = candidates[0] ?? null;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@ -54,6 +54,8 @@ describe("ssh-config", () => {
|
|||||||
expect(config?.host).toBe("peters-mac-studio-1.sheep-coho.ts.net");
|
expect(config?.host).toBe("peters-mac-studio-1.sheep-coho.ts.net");
|
||||||
expect(config?.port).toBe(2222);
|
expect(config?.port).toBe(2222);
|
||||||
expect(config?.identityFiles).toEqual(["/tmp/id_ed25519"]);
|
expect(config?.identityFiles).toEqual(["/tmp/id_ed25519"]);
|
||||||
|
const args = spawnMock.mock.calls[0]?.[1] as string[] | undefined;
|
||||||
|
expect(args?.slice(-2)).toEqual(["--", "me@alias"]);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("returns null when ssh -G fails", async () => {
|
it("returns null when ssh -G fails", async () => {
|
||||||
|
|||||||
@ -58,7 +58,8 @@ export async function resolveSshConfig(
|
|||||||
args.push("-i", opts.identity.trim());
|
args.push("-i", opts.identity.trim());
|
||||||
}
|
}
|
||||||
const userHost = target.user ? `${target.user}@${target.host}` : target.host;
|
const userHost = target.user ? `${target.user}@${target.host}` : target.host;
|
||||||
args.push(userHost);
|
// Use "--" so userHost can't be parsed as an ssh option.
|
||||||
|
args.push("--", userHost);
|
||||||
|
|
||||||
return await new Promise<SshResolvedConfig | null>((resolve) => {
|
return await new Promise<SshResolvedConfig | null>((resolve) => {
|
||||||
const child = spawn(sshPath, args, {
|
const child = spawn(sshPath, args, {
|
||||||
|
|||||||
27
src/infra/ssh-tunnel.test.ts
Normal file
27
src/infra/ssh-tunnel.test.ts
Normal file
@ -0,0 +1,27 @@
|
|||||||
|
import { describe, expect, it } from "vitest";
|
||||||
|
|
||||||
|
import { parseSshTarget } from "./ssh-tunnel.js";
|
||||||
|
|
||||||
|
describe("parseSshTarget", () => {
|
||||||
|
it("parses user@host:port targets", () => {
|
||||||
|
expect(parseSshTarget("me@example.com:2222")).toEqual({
|
||||||
|
user: "me",
|
||||||
|
host: "example.com",
|
||||||
|
port: 2222,
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("parses host-only targets with default port", () => {
|
||||||
|
expect(parseSshTarget("example.com")).toEqual({
|
||||||
|
user: undefined,
|
||||||
|
host: "example.com",
|
||||||
|
port: 22,
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("rejects hostnames that start with '-'", () => {
|
||||||
|
expect(parseSshTarget("-V")).toBeNull();
|
||||||
|
expect(parseSshTarget("me@-badhost")).toBeNull();
|
||||||
|
expect(parseSshTarget("-oProxyCommand=echo")).toBeNull();
|
||||||
|
});
|
||||||
|
});
|
||||||
@ -41,10 +41,14 @@ export function parseSshTarget(raw: string): SshParsedTarget | null {
|
|||||||
const portRaw = hostPart.slice(colonIdx + 1).trim();
|
const portRaw = hostPart.slice(colonIdx + 1).trim();
|
||||||
const port = Number.parseInt(portRaw, 10);
|
const port = Number.parseInt(portRaw, 10);
|
||||||
if (!host || !Number.isFinite(port) || port <= 0) return null;
|
if (!host || !Number.isFinite(port) || port <= 0) return null;
|
||||||
|
// Security: Reject hostnames starting with '-' to prevent argument injection
|
||||||
|
if (host.startsWith("-")) return null;
|
||||||
return { user: userPart, host, port };
|
return { user: userPart, host, port };
|
||||||
}
|
}
|
||||||
|
|
||||||
if (!hostPart) return null;
|
if (!hostPart) return null;
|
||||||
|
// Security: Reject hostnames starting with '-' to prevent argument injection
|
||||||
|
if (hostPart.startsWith("-")) return null;
|
||||||
return { user: userPart, host: hostPart, port: 22 };
|
return { user: userPart, host: hostPart, port: 22 };
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -134,7 +138,8 @@ export async function startSshPortForward(opts: {
|
|||||||
if (opts.identity?.trim()) {
|
if (opts.identity?.trim()) {
|
||||||
args.push("-i", opts.identity.trim());
|
args.push("-i", opts.identity.trim());
|
||||||
}
|
}
|
||||||
args.push(userHost);
|
// Security: Use '--' to prevent userHost from being interpreted as an option
|
||||||
|
args.push("--", userHost);
|
||||||
|
|
||||||
const stderr: string[] = [];
|
const stderr: string[] = [];
|
||||||
const child = spawn("/usr/bin/ssh", args, {
|
const child = spawn("/usr/bin/ssh", args, {
|
||||||
|
|||||||
@ -99,7 +99,7 @@ export function applyTestPluginDefaults(
|
|||||||
...plugins,
|
...plugins,
|
||||||
slots: {
|
slots: {
|
||||||
...plugins?.slots,
|
...plugins?.slots,
|
||||||
memory: null,
|
memory: "none",
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
@ -112,7 +112,7 @@ export function applyTestPluginDefaults(
|
|||||||
enabled: false,
|
enabled: false,
|
||||||
slots: {
|
slots: {
|
||||||
...plugins?.slots,
|
...plugins?.slots,
|
||||||
memory: null,
|
memory: "none",
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user