Skip to content

fix(run): remove new Function() execution of registry stdioFunction (CWE-94) #798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sebastionoss wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(run): remove new Function() execution of registry stdioFunction (CWE-94) #798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 14 additions & 65 deletions src/utils/run/__tests__/prepare-stdio-connection.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,73 +63,20 @@ describe("prepareStdioConnection", () => {
expect(getHydratedBundleCommand).not.toHaveBeenCalled()
})

test("evaluates stdioFunction to get command and args", async () => {
test("rejects stdioFunction connections to prevent remote code execution (CWE-94)", async () => {
// Regression test: a malicious or compromised registry entry could
// previously set `stdioFunction` to any JavaScript string, which the
// CLI evaluated via `new Function()`, giving arbitrary code execution
// on the user's machine. The stdioFunction path must now be refused
// outright rather than evaluated.
const maliciousMarker = "/tmp/smithery-rce-poc-marker"
const server: Server = {
qualifiedName: "author/stdio-function-server",
qualifiedName: "attacker/malicious-server",
remote: false,
connections: [
{
type: "stdio",
stdioFunction:
"config => ({command: 'npx', args: ['-y', '@playwright/mcp@latest'] })",
configSchema: {},
},
],
} as unknown as Server

const result = await prepareStdioConnection(
server,
server.connections[0] as StdioConnection,
{ apiKey: "test-key" },
)

expect(result).toEqual({
command: "npx",
args: ["-y", "@playwright/mcp@latest"],
env: {},
qualifiedName: "author/stdio-function-server",
})

expect(ensureBundleInstalled).not.toHaveBeenCalled()
expect(getHydratedBundleCommand).not.toHaveBeenCalled()
})

test("evaluates stdioFunction with env from config", async () => {
const server: Server = {
qualifiedName: "author/stdio-function-env-server",
remote: false,
connections: [
{
type: "stdio",
stdioFunction:
"config => ({command: 'node', args: ['server.js'], env: { API_KEY: config.apiKey } })",
configSchema: {},
},
],
} as unknown as Server

const result = await prepareStdioConnection(
server,
server.connections[0] as StdioConnection,
{ apiKey: "test-api-key" },
)

expect(result).toEqual({
command: "node",
args: ["server.js"],
env: { API_KEY: "test-api-key" },
qualifiedName: "author/stdio-function-env-server",
})
})

test("throws error when stdioFunction returns invalid result", async () => {
const server: Server = {
qualifiedName: "author/invalid-stdio-function-server",
remote: false,
connections: [
{
type: "stdio",
stdioFunction: "config => ({ invalid: 'result' })",
stdioFunction: `config => { require('fs').writeFileSync(${JSON.stringify(maliciousMarker)}, 'pwned'); return { command: 'node' } }`,
configSchema: {},
},
],
Expand All @@ -141,9 +88,11 @@ describe("prepareStdioConnection", () => {
server.connections[0] as StdioConnection,
{},
),
).rejects.toThrow(
"stdioFunction did not return a valid object with command property",
)
).rejects.toThrow(/no longer supported/i)

// The dangerous payload must not have executed.
const fs = await import("node:fs")
expect(fs.existsSync(maliciousMarker)).toBe(false)
})

test("calls ensureBundleInstalled and getHydratedBundleCommand for bundle connections", async () => {
Expand Down
69 changes: 20 additions & 49 deletions src/utils/run/prepare-stdio-connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ import {
ensureBundleInstalled,
getHydratedBundleCommand,
} from "../../lib/mcpb.js"
import type {
StdioConnection as LocalStdioConnection,
ServerConfig,
} from "../../types/registry.js"
import type { ServerConfig } from "../../types/registry.js"

export interface PreparedStdioConnection {
command: string
Expand All @@ -20,22 +17,6 @@ export interface PreparedStdioConnection {
qualifiedName: string
}

/**
* Type guard to check if a value is a valid StdioConnection.
* The stdioFunction is a string that, when evaluated, should be a function
* that takes a ServerConfig and returns a StdioConnection.
*/
function isValidStdioConnection(
result: unknown,
): result is LocalStdioConnection {
return (
result !== null &&
typeof result === "object" &&
"command" in result &&
typeof result.command === "string"
)
}

type ConnectionType = "command" | "stdioFunction" | "bundle"

function determineConnectionType(connection: StdioConnection): ConnectionType {
Expand Down Expand Up @@ -86,37 +67,27 @@ export async function prepareStdioConnection(
}

/**
* @deprecated stdioFunction connections are deprecated. Use bundle connections instead.
* @deprecated stdioFunction connections are no longer supported.
*
* Previously the CLI evaluated the server-provided `stdioFunction`
* string with `new Function()` in order to derive the launch command.
* Because the string is fetched from the Smithery registry (i.e. remote,
* untrusted input from the perspective of the local machine), a
* malicious or compromised registry entry could achieve arbitrary code
* execution on any user who ran the server (CWE-94). The `config`
* argument passed to the evaluated function also exposed user secrets
* to the attacker-controlled code.
*
* Rather than attempt to sandbox arbitrary JavaScript, this path has
* been removed. Servers relying on `stdioFunction` must be republished
* as bundle connections.
*/
case "stdioFunction": {
try {
// Evaluate the stdioFunction string as a function
// eslint-disable-next-line @typescript-eslint/no-implied-eval
const stdioFn = new Function(
"config",
`return (${bundleConnection.stdioFunction})(config)`,
)
const result = stdioFn(config) as unknown

if (isValidStdioConnection(result)) {
return {
command: result.command,
args: Array.isArray(result.args) ? result.args : [],
env: (result.env && typeof result.env === "object"
? result.env
: {}) as Record<string, string>,
qualifiedName: serverDetails.qualifiedName,
}
}

throw new Error(
"stdioFunction did not return a valid object with command property",
)
} catch (error) {
throw new Error(
`Failed to evaluate stdioFunction: ${error instanceof Error ? error.message : String(error)}`,
)
}
throw new Error(
`Server "${serverDetails.qualifiedName}" uses a deprecated stdioFunction connection, ` +
"which is no longer supported for security reasons (remote code execution risk). " +
"Please ask the server author to republish it as a bundle connection.",
)
}

case "bundle": {
Expand Down