diff --git a/Dockerfile b/Dockerfile index e078c1bfef..79f9d05734 100644 --- a/Dockerfile +++ b/Dockerfile @@ -18,7 +18,9 @@ RUN apt-get -y update \ pkg-config=1.8.* \ protobuf-compiler=3.21.* -RUN git clone -b master --single-branch https://github.com/google/nsjail.git . && git checkout dccf911fd2659e7b08ce9507c25b2b38ec2c5800 +COPY docker/nsjail-patches/ /nsjail-patches/ +RUN git clone -b master --single-branch https://github.com/google/nsjail.git . && git checkout dccf911fd2659e7b08ce9507c25b2b38ec2c5800 \ + && git apply /nsjail-patches/*.patch RUN make FROM ${RUST_IMAGE} AS rust_base diff --git a/backend/windmill-worker/nsjail/probe.config.proto b/backend/windmill-worker/nsjail/probe.config.proto new file mode 100644 index 0000000000..2fdc5abdca --- /dev/null +++ b/backend/windmill-worker/nsjail/probe.config.proto @@ -0,0 +1,47 @@ +name: "nsjail startup probe" + +mode: ONCE +hostname: "probe" +log_level: WARNING +time_limit: 10 + +# Same namespaces and rlimit handling as the job profiles (nsjail's default rlimits, a cgroup +# namespace, the user namespace unless DISABLE_NUSER): a worker whose kernel or runtime refuses +# any of them cannot run jobs either. +clone_newnet: false +clone_newuser: {CLONE_NEWUSER} + +skip_setsid: true +keep_caps: false +mount_proc: true + +mount { + src: "/bin" + dst: "/bin" + is_bind: true +} +mount { + src: "/lib" + dst: "/lib" + is_bind: true +} +mount { + src: "/lib64" + dst: "/lib64" + is_bind: true + mandatory: false +} +mount { + src: "/usr" + dst: "/usr" + is_bind: true +} +mount { + dst: "/tmp" + fstype: "tmpfs" + rw: true +} + +exec_bin { + path: "/bin/true" +} diff --git a/backend/windmill-worker/src/common.rs b/backend/windmill-worker/src/common.rs index 462fb2a686..c0ae372e38 100644 --- a/backend/windmill-worker/src/common.rs +++ b/backend/windmill-worker/src/common.rs @@ -986,12 +986,26 @@ pub fn build_command_with_isolation(program: &str, args: &[&str]) -> Command { } } +/// Under `NSJAIL_GVISOR_COMPAT`, rewrites the config of an nsjail `cmd` for gVisor. A spawn of +/// nsjail that bypasses `start_child_process` must call this itself. +pub fn apply_nsjail_gvisor_compat(cmd: &Command) -> Result<(), Error> { + if *crate::NSJAIL_GVISOR_COMPAT && cmd.as_std().get_program() == crate::NSJAIL_PATH.as_str() { + crate::nsjail_compat::apply(cmd.as_std()).map_err(|e| { + Error::ExecutionErr(format!( + "NSJAIL_GVISOR_COMPAT could not prepare nsjail: {e}" + )) + })?; + } + Ok(()) +} + pub async fn start_child_process( cmd: Command, executable: &str, disable_process_group: bool, ) -> Result, Error> { use process_wrap::tokio::*; + apply_nsjail_gvisor_compat(&cmd)?; let mut cmd = TokioCommandWrap::from(cmd); // On Windows, put the child in its own console process group so a CTRL_BREAK_EVENT diff --git a/backend/windmill-worker/src/dbt_executor.rs b/backend/windmill-worker/src/dbt_executor.rs index dbef02bec3..28db2753e1 100644 --- a/backend/windmill-worker/src/dbt_executor.rs +++ b/backend/windmill-worker/src/dbt_executor.rs @@ -4053,6 +4053,7 @@ pub(crate) async fn run_captured( ) -> error::Result { use tokio::io::AsyncReadExt; + crate::common::apply_nsjail_gvisor_compat(&cmd)?; let mut child = cmd .stdout(Stdio::piped()) .stderr(Stdio::piped()) diff --git a/backend/windmill-worker/src/duckdb_runner.rs b/backend/windmill-worker/src/duckdb_runner.rs index 46f5c5cbde..b7c0bc6321 100644 --- a/backend/windmill-worker/src/duckdb_runner.rs +++ b/backend/windmill-worker/src/duckdb_runner.rs @@ -23,8 +23,7 @@ use windmill_common::error::{Error, Result}; use crate::{ common::{build_command_with_isolation, resolve_nsjail_tmp_mount_block, DEV_CONF_NSJAIL}, - is_sandboxing_enabled, is_unshare_enabled, DISABLE_NUSER, HOME_ENV, NSJAIL_AVAILABLE, - NSJAIL_PATH, PATH_ENV, + is_sandboxing_enabled, is_unshare_enabled, DISABLE_NUSER, HOME_ENV, NSJAIL_PATH, PATH_ENV, }; const NSJAIL_CONFIG_RUN_DUCKDB_CONTENT: &str = include_str!("../nsjail/run.duckdb.config.proto"); @@ -42,7 +41,9 @@ pub(crate) enum RunnerMode { /// Decided once per job, so every connection a job opens runs the same way. pub(crate) fn runner_mode() -> RunnerMode { - if cfg!(unix) && is_sandboxing_enabled() && NSJAIL_AVAILABLE.is_some() { + // Not gated on `NSJAIL_AVAILABLE`: a sandboxed worker whose nsjail cannot jail must fail + // the job, as every other jailed executor does, not run it outside the jail. + if cfg!(unix) && is_sandboxing_enabled() { RunnerMode::Jailed } else if is_unshare_enabled() { // `build_command_with_isolation` logs and runs unisolated when `unshare` is missing, @@ -475,6 +476,7 @@ async fn spawn_runner( }); } } + crate::common::apply_nsjail_gvisor_compat(&cmd)?; let mut child = cmd.spawn().map_err(|e| { Error::InternalErr(format!("Could not start the DuckDB runner ({mode:?}): {e}")) })?; diff --git a/backend/windmill-worker/src/lib.rs b/backend/windmill-worker/src/lib.rs index e56334df67..1945d16346 100644 --- a/backend/windmill-worker/src/lib.rs +++ b/backend/windmill-worker/src/lib.rs @@ -62,6 +62,7 @@ pub mod memory_ee; pub mod memory_oss; #[cfg(feature = "mysql")] mod mysql_executor; +mod nsjail_compat; #[cfg(feature = "nu")] mod nu_executor; #[cfg(feature = "oracledb")] diff --git a/backend/windmill-worker/src/nsjail_compat.rs b/backend/windmill-worker/src/nsjail_compat.rs new file mode 100644 index 0000000000..3069dcee52 --- /dev/null +++ b/backend/windmill-worker/src/nsjail_compat.rs @@ -0,0 +1,49 @@ +//! `NSJAIL_GVISOR_COMPAT=true`: run nsjail inside gVisor (runsc). Each config is rewritten +//! right before nsjail starts, with `gvisor_compat: true` (an option of the nsjail the images +//! build, from `docker/nsjail-patches/`) and no cgroup namespace, which gVisor refuses. + +use std::ffi::OsStr; +use std::io; +use std::path::PathBuf; +use std::process::Command; + +pub(crate) fn compat_config(config: &str) -> String { + let mut out = config + .lines() + .filter(|l| { + let l = l.trim_start(); + !l.starts_with("clone_newcgroup") && !l.starts_with("gvisor_compat") + }) + .collect::>() + .join("\n"); + out.push_str("\nclone_newcgroup: false\ngvisor_compat: true\n"); + out +} + +/// Rewrites the config `cmd` (an nsjail command line) passes with `--config`. +pub(crate) fn apply(cmd: &Command) -> io::Result<()> { + let args = cmd.get_args().collect::>(); + let Some(config) = args.windows(2).find(|w| w[0] == "--config").map(|w| w[1]) else { + return Ok(()); + }; + let path = match cmd.get_current_dir() { + Some(dir) => dir.join(config), + None => PathBuf::from(config), + }; + let rewritten = compat_config(&std::fs::read_to_string(&path)?); + std::fs::write(&path, rewritten) +} + +#[cfg(test)] +mod tests { + // nsjail rejects a config that sets a scalar field twice, which would fail every jailed job. + #[test] + fn compat_config_sets_each_field_once() { + let bun = include_str!("../nsjail/run.bun.config.proto"); + assert!(bun.contains("clone_newcgroup: false")); + let twice = super::compat_config(&super::compat_config(bun)); + assert_eq!(twice.matches("clone_newcgroup").count(), 1); + assert_eq!(twice.matches("gvisor_compat").count(), 1); + assert!(twice.ends_with("\nclone_newcgroup: false\ngvisor_compat: true\n")); + } +} diff --git a/backend/windmill-worker/src/worker.rs b/backend/windmill-worker/src/worker.rs index cef3b1bbfc..4048a3603c 100644 --- a/backend/windmill-worker/src/worker.rs +++ b/backend/windmill-worker/src/worker.rs @@ -261,6 +261,7 @@ const NUM_SECS_PING: u64 = 5; const NUM_SECS_READINGS: u64 = 60; const INCLUDE_DEPS_PY_SH_CONTENT: &str = include_str!("../nsjail/download_deps.py.sh"); +const NSJAIL_CONFIG_PROBE_CONTENT: &str = include_str!("../nsjail/probe.config.proto"); const WORKER_SHELL_NAP_TIME_DURATION: u64 = 15; const WORKER_SHELL_INITIAL_NAP_TIME_DURATION: u64 = 5; @@ -335,6 +336,14 @@ pub struct PowershellRepo { pub pat: String, } +/// Runs nsjail in a form gVisor (runsc) accepts, see `nsjail_compat`. +pub static NSJAIL_GVISOR_COMPAT: std::sync::LazyLock = std::sync::LazyLock::new(|| { + std::env::var("NSJAIL_GVISOR_COMPAT") + .ok() + .and_then(|x| x.parse::().ok()) + .unwrap_or(false) +}); + lazy_static::lazy_static! { static ref SLEEP_QUEUE_BASE: u64 = std::env::var("SLEEP_QUEUE") @@ -546,29 +555,41 @@ lazy_static::lazy_static! { pub static ref NSJAIL_AVAILABLE: Option = { let nsjail_path = NSJAIL_PATH.as_str(); - let test_result = std::process::Command::new(nsjail_path) - .arg("--help") - .output(); - - match test_result { + match probe_nsjail(nsjail_path) { Ok(output) if output.status.success() => { - tracing::info!("nsjail available at: {}", nsjail_path); + tracing::info!( + "nsjail available at: {}{}", + nsjail_path, + if *NSJAIL_GVISOR_COMPAT { " (NSJAIL_GVISOR_COMPAT)" } else { "" } + ); Some(nsjail_path.to_string()) }, Ok(output) => { - let stderr = String::from_utf8_lossy(&output.stderr); - tracing::error!( - "nsjail test failed (exit code: {}). stderr: '{}'. path: '{}'. \ - Nsjail sandboxing will NOT be available. \ - nsjail should be included in all standard windmill images. \ - If job_isolation is set to 'nsjail_sandboxing' in Instance Settings, jobs will fail.", + let msg = format!( + "nsjail could not run a test jail (exit code: {}). stderr: '{}'. path: '{}'. \ + Nsjail sandboxing will NOT be available: the worker reports its job isolation \ + without nsjail, and jobs that require nsjail fail. Under gVisor (runsc), see \ + NSJAIL_GVISOR_COMPAT.", output.status, - stderr.trim(), + String::from_utf8_lossy(&output.stderr).trim(), nsjail_path ); + // Most workers never jail anything, and most containers cannot. + if *DISABLE_NSJAIL { + tracing::info!("{msg}"); + } else { + tracing::error!("{msg}"); + } + None + }, + Err(NsjailProbeError::Setup(e)) => { + tracing::error!( + "Could not prepare the nsjail test jail: {}. Nsjail sandboxing will NOT be available.", + e + ); None }, - Err(e) => { + Err(NsjailProbeError::Start(e)) => { if e.kind() == std::io::ErrorKind::NotFound { tracing::error!( "nsjail not found at '{}'. Nsjail sandboxing will NOT be available. \ @@ -1021,6 +1042,43 @@ pub(crate) fn artifact_cache_name( format!("mod-{}", windmill_common::utils::calculate_hash(&keyed)) } +/// Runs `/bin/true` in a jail built like a job's, through the same spawn path, so a worker +/// whose runtime lets nsjail start but not jail (a container without the privileges, gVisor) +/// does not report nsjail isolation. +enum NsjailProbeError { + /// The probe could not be prepared: nothing is known about nsjail. + Setup(std::io::Error), + /// nsjail itself could not be executed. + Start(std::io::Error), +} + +fn probe_nsjail(nsjail_path: &str) -> Result { + // A fresh private directory: the worker runs what this config says, as itself. + let dir = std::env::temp_dir().join(format!("windmill-nsjail-probe-{}", uuid::Uuid::new_v4())); + let mut builder = std::fs::DirBuilder::new(); + #[cfg(unix)] + std::os::unix::fs::DirBuilderExt::mode(&mut builder, 0o700); + builder.create(&dir).map_err(NsjailProbeError::Setup)?; + let output = (|| { + std::fs::write( + dir.join("probe.config.proto"), + NSJAIL_CONFIG_PROBE_CONTENT.replace("{CLONE_NEWUSER}", &(!*DISABLE_NUSER).to_string()), + ) + .map_err(NsjailProbeError::Setup)?; + let mut cmd = std::process::Command::new(nsjail_path); + cmd.current_dir(&dir) + .env_clear() + .env("PATH", PATH_ENV.as_str()) + .args(["--config", "probe.config.proto"]); + if *NSJAIL_GVISOR_COMPAT { + crate::nsjail_compat::apply(&cmd).map_err(NsjailProbeError::Setup)?; + } + cmd.output().map_err(NsjailProbeError::Start) + })(); + let _ = std::fs::remove_dir_all(&dir); + output +} + pub fn is_sandboxing_enabled() -> bool { if !*DISABLE_NSJAIL { return true; diff --git a/docker/DockerfileSlim b/docker/DockerfileSlim index ef976f83d3..630f454f01 100644 --- a/docker/DockerfileSlim +++ b/docker/DockerfileSlim @@ -23,7 +23,9 @@ RUN apt-get -y update \ && apt-get clean \ && rm -rf /var/lib/apt/lists/* -RUN git clone -b master --single-branch https://github.com/google/nsjail.git . && git checkout dccf911fd2659e7b08ce9507c25b2b38ec2c5800 +COPY docker/nsjail-patches/ /nsjail-patches/ +RUN git clone -b master --single-branch https://github.com/google/nsjail.git . && git checkout dccf911fd2659e7b08ce9507c25b2b38ec2c5800 \ + && git apply /nsjail-patches/*.patch RUN make FROM ${DEBIAN_IMAGE} diff --git a/docker/DockerfileSlimEe b/docker/DockerfileSlimEe index 67de36eac7..8189e74bf9 100644 --- a/docker/DockerfileSlimEe +++ b/docker/DockerfileSlimEe @@ -23,7 +23,9 @@ RUN apt-get -y update \ && apt-get clean \ && rm -rf /var/lib/apt/lists/* -RUN git clone -b master --single-branch https://github.com/google/nsjail.git . && git checkout dccf911fd2659e7b08ce9507c25b2b38ec2c5800 +COPY docker/nsjail-patches/ /nsjail-patches/ +RUN git clone -b master --single-branch https://github.com/google/nsjail.git . && git checkout dccf911fd2659e7b08ce9507c25b2b38ec2c5800 \ + && git apply /nsjail-patches/*.patch RUN make FROM ${DEBIAN_IMAGE} diff --git a/docker/nsjail-patches/gvisor-compat.patch b/docker/nsjail-patches/gvisor-compat.patch new file mode 100644 index 0000000000..70176b7272 --- /dev/null +++ b/docker/nsjail-patches/gvisor-compat.patch @@ -0,0 +1,153 @@ +gvisor_compat: an opt-in nsjail option (config field and --gvisor_compat) for running inside +gVisor (runsc). With it false, nsjail behaves as upstream. With it true: +- no rtnetlink link cache when no interface is moved into the jail: gVisor acks NLM_F_DUMP + requests before NLMSG_DONE and libnl rejects that (google/gvisor#15945); +- an EPERM from setrlimit(RLIMIT_MSGQUEUE) is a warning: gVisor refuses that limit for any + value (google/gvisor#15944); +- an EPERM from PR_SET_SECUREBITS is tolerated when the UID does not change, where the bits + have no effect (SECBIT_NO_SETUID_FIXUP lands in gVisor with google/gvisor#15130). +Applied to google/nsjail dccf911fd2659e7b08ce9507c25b2b38ec2c5800 by the Dockerfiles. + +diff --git a/cmdline.cc b/cmdline.cc +index 1b52b33..b49ce16 100644 +--- a/cmdline.cc ++++ b/cmdline.cc +@@ -115,6 +115,7 @@ struct custom_option custom_opts[] = { + { { "rlimit_rtprio", required_argument, NULL, 0x0210 }, "RLIMIT_RTPRIO, 'max' or 'hard' for the current hard limit, 'def' or 'soft' for the current soft limit, 'inf' for RLIM64_INFINITY (default: 'soft')" }, + { { "rlimit_msgqueue", required_argument, NULL, 0x0211 }, "RLIMIT_MSGQUEUE in bytes, 'max' or 'hard' for the current hard limit, 'def' or 'soft' for the current soft limit, 'inf' for RLIM64_INFINITY (default: 'soft')" }, + { { "disable_rlimits", no_argument, NULL, 0x0208 }, "Disable all rlimits, default to limits set by parent" }, ++ { { "gvisor_compat", no_argument, NULL, 0x0510 }, "Tolerate what gVisor (runsc) does not implement (see config.proto)" }, + { { "persona_addr_compat_layout", no_argument, NULL, 0x0301 }, "personality(ADDR_COMPAT_LAYOUT)" }, + { { "persona_mmap_page_zero", no_argument, NULL, 0x0302 }, "personality(MMAP_PAGE_ZERO)" }, + { { "persona_read_implies_exec", no_argument, NULL, 0x0303 }, "personality(READ_IMPLIES_EXEC)" }, +@@ -437,6 +438,7 @@ std::unique_ptr parseArgs(int argc, char* argv[]) { + nsjconf->rl_rtpr = parseRLimit(RLIMIT_RTPRIO, "soft", 1); + nsjconf->rl_msgq = parseRLimit(RLIMIT_MSGQUEUE, "soft", 1); + nsjconf->disable_rl = false; ++ nsjconf->gvisor_compat = false; + nsjconf->personality = 0; + nsjconf->clone_newnet = true; + nsjconf->clone_newuser = true; +@@ -603,6 +605,9 @@ std::unique_ptr parseArgs(int argc, char* argv[]) { + case 0x0208: + nsjconf->disable_rl = true; + break; ++ case 0x0510: ++ nsjconf->gvisor_compat = true; ++ break; + case 0x0301: + nsjconf->personality |= ADDR_COMPAT_LAYOUT; + break; +diff --git a/config.cc b/config.cc +index 620d08e..9427069 100644 +--- a/config.cc ++++ b/config.cc +@@ -166,6 +166,7 @@ static bool configParseInternal(nsjconf_t* nsjconf, const nsjail::NsJailConfig& + configRLimit(RLIMIT_MSGQUEUE, njc.rlimit_msgqueue_type(), njc.rlimit_msgqueue()); + + nsjconf->disable_rl = njc.disable_rl(); ++ nsjconf->gvisor_compat = njc.gvisor_compat(); + + if (njc.persona_addr_compat_layout()) { + nsjconf->personality |= ADDR_COMPAT_LAYOUT; +diff --git a/config.proto b/config.proto +index cd297ae..96b477c 100644 +--- a/config.proto ++++ b/config.proto +@@ -266,4 +266,10 @@ message NsJailConfig { + /* Binary path (with arguments) to be executed. If not specified here, it + can be specified with cmd-line as "-- /path/to/command arg1 arg2" */ + optional Exe exec_bin = 90; ++ ++ /* Tolerate what gVisor (runsc) does not implement: an EPERM from ++ setrlimit(RLIMIT_MSGQUEUE) or from PR_SET_SECUREBITS when the UID does ++ not change, and no rtnetlink link cache when no interface is moved ++ into the jail */ ++ optional bool gvisor_compat = 93 [default = false]; + } +diff --git a/contain.cc b/contain.cc +index b5120cc..41e69f8 100644 +--- a/contain.cc ++++ b/contain.cc +@@ -172,8 +172,12 @@ static bool containSetLimits(nsjconf_t* nsjconf) { + } + rl.rlim_cur = rl.rlim_max = nsjconf->rl_msgq; + if (setrlimit64(RLIMIT_MSGQUEUE, &rl) == -1) { +- PLOG_E("setrlimit64(0, RLIMIT_MSGQUEUE , %" PRIu64 ")", nsjconf->rl_msgq); +- return false; ++ if (!nsjconf->gvisor_compat || errno != EPERM) { ++ PLOG_E("setrlimit64(0, RLIMIT_MSGQUEUE , %" PRIu64 ")", nsjconf->rl_msgq); ++ return false; ++ } ++ PLOG_W("setrlimit64(0, RLIMIT_MSGQUEUE , %" PRIu64 "), ignored (gvisor_compat)", ++ nsjconf->rl_msgq); + } + return true; + } +diff --git a/net.cc b/net.cc +index 87c3df9..b4c64f4 100644 +--- a/net.cc ++++ b/net.cc +@@ -138,6 +138,10 @@ bool initNsFromParent(nsjconf_t* nsjconf, int pid) { + if (!nsjconf->clone_newnet) { + return true; + } ++ /* gVisor's rtnetlink replies are rejected by libnl's link cache, which is unused here */ ++ if (nsjconf->gvisor_compat && nsjconf->ifaces.empty() && nsjconf->iface_vs.empty()) { ++ return true; ++ } + struct nl_sock* sk = nl_socket_alloc(); + if (!sk) { + LOG_E("Could not allocate socket with nl_socket_alloc()"); +diff --git a/nsjail.h b/nsjail.h +index 5687ca7..fa2b9fa 100644 +--- a/nsjail.h ++++ b/nsjail.h +@@ -119,6 +119,7 @@ struct nsjconf_t { + uint64_t rl_rtpr; + uint64_t rl_msgq; + bool disable_rl; ++ bool gvisor_compat; + unsigned long personality; + bool clone_newnet; + bool clone_newuser; +diff --git a/user.cc b/user.cc +index a335e2d..f3e578b 100644 +--- a/user.cc ++++ b/user.cc +@@ -260,10 +260,24 @@ bool initNsFromChild(nsjconf_t* nsjconf) { + * Make sure all capabilities are retained after the subsequent setuid/setgid, as they will + * be needed for privileged operations: mounts, uts change etc. + */ ++ bool securebits_set = true; + if (prctl(PR_SET_SECUREBITS, SECBIT_KEEP_CAPS | SECBIT_NO_SETUID_FIXUP, 0UL, 0UL, 0UL) == + -1) { +- PLOG_E("prctl(PR_SET_SECUREBITS, SECBIT_KEEP_CAPS | SECBIT_NO_SETUID_FIXUP)"); +- return false; ++ /* ++ * The bits only matter when setresuid() below changes the UID: without them, ++ * leaving UID 0 clears the capabilities still needed by the jail setup. ++ */ ++ uid_t ruid, euid, suid; ++ const uid_t uid = nsjconf->uids[0].inside_id; ++ if (!nsjconf->gvisor_compat || errno != EPERM || ++ getresuid(&ruid, &euid, &suid) == -1 || ruid != uid || euid != uid || ++ suid != uid) { ++ PLOG_E("prctl(PR_SET_SECUREBITS, SECBIT_KEEP_CAPS | SECBIT_NO_SETUID_FIXUP)"); ++ return false; ++ } ++ PLOG_W("prctl(PR_SET_SECUREBITS, SECBIT_KEEP_CAPS | SECBIT_NO_SETUID_FIXUP), " ++ "ignored (gvisor_compat, UID unchanged)"); ++ securebits_set = false; + } + + /* +@@ -306,7 +320,7 @@ bool initNsFromChild(nsjconf_t* nsjconf) { + * unexpectedly retaining capabilities after a UID/GID + * change. + */ +- if (prctl(PR_SET_SECUREBITS, 0UL, 0UL, 0UL, 0UL) == -1) { ++ if (securebits_set && prctl(PR_SET_SECUREBITS, 0UL, 0UL, 0UL, 0UL) == -1) { + PLOG_E("prctl(PR_SET_SECUREBITS, 0)"); + return false; + }