From 1c7adbdbd377586f36dd835b1b59d4175c2a25ad Mon Sep 17 00:00:00 2001 From: stefanbaxter Date: Sat, 31 Oct 2026 14:16:48 +0000 Subject: [PATCH] fix(cubejs): fourth review of the confinement (MotherDuck and Databricks options) Found by a fourth adversarial review of the merged confinement (#103), all with default operator config: - MotherDuck: md: connection strings take options (motherduck_host, ports, download URLs) that send the pod's requests to any host -- at instance creation, before the sandbox -- and returned the response body in the error. Only a database name and a token-shaped motherduck_token are accepted; a token can no longer carry options into the URL Cube builds. - Databricks: the in-JVM driver's connection options include ones that read or write pod files (Volume/Staging allowed local paths, SSL key stores) and reach other hosts (proxies). Only the options a connection needs are accepted, in the URL and the properties, whichever JDBC type loads it. - DuckDB: bounded by default (memory 1GB, 2 threads, 4GB spill), raisable with CUBEJS_DB_DUCKDB_MEMORY_LIMIT / _THREADS / _MAX_TEMP_DIRECTORY_SIZE, so one tenant query can't take down the shared process or disk. The review's exploits rerun against this image: refused. 760/760. Co-Authored-By: Claude Opus 5.5 --- .../cubejs/scripts/patchCubeYamlCompiler.mjs | 5 ++ .../src/__tests__/cube17Regression.test.js | 3 + .../utils/__tests__/confineDbParams.test.js | 19 ++++++ services/cubejs/src/utils/confineDbParams.js | 59 +++++++++++++++++-- 4 files changed, 82 insertions(+), 4 deletions(-) diff --git a/services/cubejs/scripts/patchCubeYamlCompiler.mjs b/services/cubejs/scripts/patchCubeYamlCompiler.mjs index 9ad2a85e..683f4362 100644 --- a/services/cubejs/scripts/patchCubeYamlCompiler.mjs +++ b/services/cubejs/scripts/patchCubeYamlCompiler.mjs @@ -115,6 +115,11 @@ const DUCKDB_CREATE_PATCHED = ` const safeExtensions = ${DUCKDB_SAFE_EXTE allow_community_extensions: 'false', allow_persistent_secrets: 'false', temp_directory: require('fs').mkdtempSync(require('path').join(require('os').tmpdir(), 'duckdb-spill-')), + // Every tenant's DuckDB shares the cube process and its disk: + // bounded unless the operator raises them. + memory_limit: process.env.CUBEJS_DB_DUCKDB_MEMORY_LIMIT || '1GB', + threads: process.env.CUBEJS_DB_DUCKDB_THREADS || '2', + max_temp_directory_size: process.env.CUBEJS_DB_DUCKDB_MAX_TEMP_DIRECTORY_SIZE || '4GB', }; const instance = await node_api_1.DuckDBInstance.create(dbUrl, dbOptions);`; diff --git a/services/cubejs/src/__tests__/cube17Regression.test.js b/services/cubejs/src/__tests__/cube17Regression.test.js index a972d9f2..4fac87fd 100644 --- a/services/cubejs/src/__tests__/cube17Regression.test.js +++ b/services/cubejs/src/__tests__/cube17Regression.test.js @@ -376,6 +376,9 @@ y`); } const driver = ServerCore.createDriver("duckdb", {}); try { + const [limits] = await driver.query("SELECT current_setting('threads') AS t, current_setting('max_temp_directory_size') AS m", []); + assert.equal(Number(limits.t), 2, "bounded threads by default"); + assert.match(String(limits.m), /GiB|GB/); await assert.rejects(driver.query("CALL enable_logging(storage='stdout')", []), /not allowed/); } finally { await driver.release(); diff --git a/services/cubejs/src/utils/__tests__/confineDbParams.test.js b/services/cubejs/src/utils/__tests__/confineDbParams.test.js index 1e66cb0c..56f789e6 100644 --- a/services/cubejs/src/utils/__tests__/confineDbParams.test.js +++ b/services/cubejs/src/utils/__tests__/confineDbParams.test.js @@ -20,6 +20,18 @@ describe("tenant datasource params are confined, not refused", () => { assert.deepEqual(await confineDbParams("duckdb", { duckdbS3UseCredentialChain: true, initSql: "x" }, ds), { initSql: "x" }); assert.equal((await confineDbParams("duckdb", { databasePath: "md:my_db?motherduck_token=t" }, ds)).databasePath, "md:my_db?motherduck_token=t"); await refused("duckdb", { databasePath: "md:my_db" }, /MotherDuck needs a token/); + // MotherDuck options would point the pod's requests anywhere, before the sandbox + for (const databasePath of [ + "md:x?motherduck_token=t&motherduck_host=169.254.169.254/latest/meta-data?", + "md:x?motherduck_token=t&motherduck_port=9999", + "md:?motherduck_extension_download_url=http://10.0.0.1/", + ]) { + await refused("duckdb", { databasePath }, /options other than the token/); + } + await refused("duckdb", { databasePath: "md:x/../y?motherduck_token=t" }, /database names/); + await refused("duckdb", { motherDuckToken: "t&motherduck_host=10.0.0.1" }, /malformed MotherDuck token/); + await refused("duckdb", { databasePath: "md:x", motherDuckToken: "t&x=y" }, /malformed MotherDuck token/); + assert.equal((await confineDbParams("duckdb", { databasePath: "md:x", motherDuckToken: "eyJ.abc_-1" }, ds)).motherDuckToken, "eyJ.abc_-1"); assert.equal((await confineDbParams("duckdb", { databasePath: "a.duckdb" }, ds)).databasePath, "/data/local-db/ds-1/a.duckdb"); await refused("duckdb", { databasePath: "/etc/passwd" }, /under \/data\/local-db\/ds-1/); await refused("duckdb", { databasePath: "../ds-2/b.duckdb" }, /under \/data\/local-db\/ds-1/); @@ -69,6 +81,13 @@ describe("tenant datasource params are confined, not refused", () => { // the Databricks driver class through the generic type, by URL or property await refused("jdbc", { url: `${url};LogLevel=6`, drivername: "com.databricks.client.jdbc.Driver" }, /logging/); await refused("jdbc", { url, properties: { LogPath: "/tmp/x" } }, /logging/); + // options that read or write pod files, or reach other hosts, whichever type loads the driver + for (const option of ["VolumeOperationAllowedLocalPaths=/", "StagingAllowedLocalPaths=/", "SSLKeyStore=/app/k.jks", "UseProxy=1;ProxyHost=10.0.0.1"]) { + await refused("databricks-jdbc", { url: `${url};${option}`, token: "t" }, /options not allowed/); + await refused("jdbc", { url: `${url};${option}` }, /options not allowed/); + } + await refused("jdbc", { url, properties: { VolumeOperationAllowedLocalPaths: "/" } }, /options not allowed/); + assert.equal((await confineDbParams("databricks-jdbc", { url: `${url};AuthMech=3;ConnCatalog=main`, token: "t" }, ds)).url, `${url};AuthMech=3;ConnCatalog=main`); }); it("cubestore: never ours, never private, websocket only, every plain connection pinned", async () => { diff --git a/services/cubejs/src/utils/confineDbParams.js b/services/cubejs/src/utils/confineDbParams.js index 4d297a5f..7425628e 100644 --- a/services/cubejs/src/utils/confineDbParams.js +++ b/services/cubejs/src/utils/confineDbParams.js @@ -72,15 +72,40 @@ const addresses = async (host) => { } }; +// MotherDuck connection strings take options (motherduck_host, download URLs, +// ports) that point the pod's requests anywhere -- at instance creation, +// before the sandbox applies -- and a token is pasted into one. Only a +// database name and a token-shaped token. +const MOTHERDUCK_TOKEN = /^[A-Za-z0-9._-]+$/; +const confineMotherDuck = (file, token) => { + const [name, query = ""] = String(file).split(/\?(.*)/s); + if (!/^(md|motherduck):[A-Za-z0-9_.-]*$/i.test(name)) { + refuse("duckdb", "MotherDuck database names are letters, digits, '_', '.' and '-'"); + } + let urlToken = null; + for (const pair of query ? query.split("&") : []) { + const [key, value = ""] = pair.split("="); + if (key.trim().toLowerCase() !== "motherduck_token") { + refuse("duckdb", "MotherDuck connection options other than the token are not allowed"); + } + urlToken = decodeURIComponent(value); + } + for (const t of [urlToken, token]) { + if (t != null && !MOTHERDUCK_TOKEN.test(String(t))) refuse("duckdb", "malformed MotherDuck token"); + } + // Without a token MotherDuck falls back to an interactive device login that + // blocks the request for a minute. + if (!urlToken && !token) refuse("duckdb", "MotherDuck needs a token"); +}; + const confineDuckDb = (params, { dataSourceId }) => { // The credential chain hands tenant SQL this pod's own AWS credentials. const { duckdbS3UseCredentialChain, ...rest } = params; const file = rest.databasePath; const motherDuck = /^(md|motherduck):/i.test(String(file ?? "")); - // Without a token MotherDuck falls back to an interactive device login that - // blocks the request for a minute. - if (motherDuck && !rest.motherDuckToken && !/motherduck_token=/i.test(file)) { - refuse("duckdb", "MotherDuck needs a token"); + if (motherDuck) confineMotherDuck(file, rest.motherDuckToken); + else if (rest.motherDuckToken != null && !MOTHERDUCK_TOKEN.test(String(rest.motherDuckToken))) { + refuse("duckdb", "malformed MotherDuck token"); } if (file && file !== ":memory:" && !motherDuck) { rest.databasePath = localFile("duckdb", file, dataSourceId); @@ -122,6 +147,30 @@ const sharedJdbcClassPath = () => { return [...jarsIn(jdbcDriversDir()), ...databricks]; }; +// The Databricks JDBC driver runs in our JVM, and its connection options +// include ones that read or write pod files (Volume/Staging allowed local +// paths, SSL key stores, log files) or reach other hosts (proxies). Only the +// options a connection needs, in the URL and in the properties, whichever +// JDBC-family type loads the driver. +const DATABRICKS_OPTIONS = new Set([ + "httppath", "authmech", "uid", "pwd", "ssl", "transportmode", "conncatalog", + "connschema", "usenativequery", "enablearrow", "auth_flow", "oauth2clientid", + "oauth2secret", "useragententry", "enablegeospatialsupport", "sockettimeout", + "rowsfetchedperblock", +]); +const refuseDatabricksOptions = (dbType, params) => { + const url = String(params.url ?? ""); + if (!/^\s*jdbc:(databricks|spark):\/\//i.test(url) && dbType !== "databricks-jdbc") return; + const keys = [ + ...url.split(";").slice(1).map((kv) => kv.split("=")[0].trim()).filter(Boolean), + ...Object.keys(params.properties ?? {}), + ]; + const disallowed = keys.filter((key) => !DATABRICKS_OPTIONS.has(key.toLowerCase())); + if (disallowed.length) { + refuse(dbType, `Databricks connection options not allowed: ${disallowed.join(", ")}`); + } +}; + // JDBC driver logging (LogPath, LogLevel, ... in the URL or properties) makes // the JVM-wide driver write files on this pod and trace every tenant's // connections. Any JDBC-family type can load the Databricks driver. @@ -139,6 +188,7 @@ const confineJdbc = (params) => { refuse("jdbc", "in-process database URLs are not allowed"); } refuseDriverLogging("jdbc", params); + refuseDatabricksOptions("jdbc", params); const dir = jdbcDriversDir(); for (const entry of [params.customClassPath ?? []].flat()) { // One entry, one jar: a class-path separator would add others. @@ -156,6 +206,7 @@ const confineDatabricks = (params) => { refuse("databricks-jdbc", "url must be a jdbc:databricks:// URL"); } refuseDriverLogging("databricks-jdbc", params); + refuseDatabricksOptions("databricks-jdbc", params); return params; };