diff --git a/src/agents/sandbox/ssh.test.ts b/src/agents/sandbox/ssh.test.ts index 3dd413073f2b..f5d1fd9ce263 100644 --- a/src/agents/sandbox/ssh.test.ts +++ b/src/agents/sandbox/ssh.test.ts @@ -108,6 +108,22 @@ describe("sandbox ssh helpers", () => { ); }); + it.each([ + ["identityFile", "IdentityFile"] as const, + ["certificateFile", "CertificateFile"] as const, + ["knownHostsFile", "UserKnownHostsFile"] as const, + ])("rejects %s values that would break ssh config directives", async (field, directive) => { + await expect( + createSshSandboxSessionFromSettings({ + command: "ssh", + target: "peter@example.com:2222", + strictHostKeyChecking: true, + updateHostKeys: false, + [field]: `/tmp/key\n ${directive} /tmp/injected`, + }), + ).rejects.toThrow(`SSH sandbox ${field} must not contain line breaks.`); + }); + it("wraps remote exec commands with env and workdir", () => { const command = buildExecRemoteCommand({ command: "pwd && printenv TOKEN", diff --git a/src/agents/sandbox/ssh.ts b/src/agents/sandbox/ssh.ts index d4eda5e707ca..ea2187db1729 100644 --- a/src/agents/sandbox/ssh.ts +++ b/src/agents/sandbox/ssh.ts @@ -619,6 +619,9 @@ export async function createSshSandboxSessionFromSettings( materializedCertificate ?? resolveOptionalLocalPath(settings.certificateFile); const knownHostsFile = materializedKnownHosts ?? resolveOptionalLocalPath(settings.knownHostsFile); + assertSshConfigLineValue(identityFile, "identityFile"); + assertSshConfigLineValue(certificateFile, "certificateFile"); + assertSshConfigLineValue(knownHostsFile, "knownHostsFile"); const hostAlias = "openclaw-sandbox"; const configPath = path.join(configDir, "config"); const lines = [ @@ -911,6 +914,12 @@ function resolveSshTmpRoot(): string { return path.resolve(resolvePreferredOpenClawTmpDir() ?? os.tmpdir()); } +function assertSshConfigLineValue(value: string | undefined, field: string): void { + if (value && /[\r\n]/.test(value)) { + throw new Error(`SSH sandbox ${field} must not contain line breaks.`); + } +} + function resolveOptionalLocalPath(value: string | undefined): string | undefined { const trimmed = value?.trim(); return trimmed ? resolveUserPath(trimmed) : undefined; diff --git a/src/infra/ssh-tunnel.test.ts b/src/infra/ssh-tunnel.test.ts index 9a16d09941f3..5170a4f32c35 100644 --- a/src/infra/ssh-tunnel.test.ts +++ b/src/infra/ssh-tunnel.test.ts @@ -38,6 +38,19 @@ describe("parseSshTarget", () => { }); }); + it("preserves OpenSSH alias and username tokens", () => { + expect(parseSshTarget("me+prod@prod+gpu:2222")).toEqual({ + user: "me+prod", + host: "prod+gpu", + port: 2222, + }); + expect(parseSshTarget(String.raw`DOMAIN\alice@jump+gpu`)).toEqual({ + user: String.raw`DOMAIN\alice`, + host: "jump+gpu", + port: 22, + }); + }); + it("rejects invalid hosts and ports", () => { expect(parseSshTarget("")).toBeNull(); expect(parseSshTarget("me@example.com:0")).toBeNull(); @@ -46,9 +59,19 @@ describe("parseSshTarget", () => { expect(parseSshTarget("me@example.com:not-a-port")).toBeNull(); expect(parseSshTarget("-V")).toBeNull(); expect(parseSshTarget("me@-badhost")).toBeNull(); + expect(parseSshTarget("-oProxyCommand=touch@example.com")).toBeNull(); expect(parseSshTarget("-oProxyCommand=echo")).toBeNull(); }); + it("rejects targets that cannot be embedded in ssh config directives", () => { + expect(parseSshTarget("example.com\n ProxyCommand touch marker")).toBeNull(); + expect(parseSshTarget("example.com\r ProxyCommand touch marker")).toBeNull(); + expect(parseSshTarget("example.com\n ProxyCommand touch marker:2222")).toBeNull(); + expect(parseSshTarget("me\nProxyCommand=touch@example.com")).toBeNull(); + expect(parseSshTarget("bad host")).toBeNull(); + expect(parseSshTarget("me name@example.com")).toBeNull(); + }); + it("rejects hostnames with stray leading or trailing colons", () => { // Default-port branch: the whole host part keeps the stray colon. expect(parseSshTarget("host:")).toBeNull(); diff --git a/src/infra/ssh-tunnel.ts b/src/infra/ssh-tunnel.ts index d9ed2908cee0..34d7d8d3ccb6 100644 --- a/src/infra/ssh-tunnel.ts +++ b/src/infra/ssh-tunnel.ts @@ -21,11 +21,31 @@ export type SshTunnel = { stop: () => Promise; }; +function hasControlOrWhitespace(value: string): boolean { + for (const char of value) { + const code = char.charCodeAt(0); + if (code <= 0x1f || code === 0x7f || /\s/.test(char)) { + return true; + } + } + return false; +} + +function isSafeSshTargetUser(user: string): boolean { + return !hasControlOrWhitespace(user) && !user.startsWith("-"); +} + // Reject hosts that would corrupt the SSH HostName field or enable argument -// injection: a leading '-' becomes an ssh option, and a stray leading/trailing -// ':' (e.g. sliced from "host::22") produces an invalid HostName. -function isMalformedHost(host: string): boolean { - return host.startsWith("-") || host.startsWith(":") || host.endsWith(":"); +// injection. Parsed targets are later interpolated into unquoted ssh_config +// directives and argv, so each accepted user/host must stay one SSH token. +function isSafeSshTargetHost(host: string): boolean { + return ( + !hasControlOrWhitespace(host) && + !host.startsWith("-") && + !host.startsWith(":") && + !host.endsWith(":") && + !host.includes("@") + ); } export function parseSshTarget(raw: string): SshParsedTarget | null { @@ -51,7 +71,10 @@ export function parseSshTarget(raw: string): SshParsedTarget | null { if (!host || port === undefined || port > 65535) { return null; } - if (isMalformedHost(host)) { + if (!isSafeSshTargetHost(host)) { + return null; + } + if (userPart !== undefined && !isSafeSshTargetUser(userPart)) { return null; } return { user: userPart, host, port }; @@ -60,7 +83,10 @@ export function parseSshTarget(raw: string): SshParsedTarget | null { if (!hostPart) { return null; } - if (isMalformedHost(hostPart)) { + if (!isSafeSshTargetHost(hostPart)) { + return null; + } + if (userPart !== undefined && !isSafeSshTargetUser(userPart)) { return null; } return { user: userPart, host: hostPart, port: 22 };