Skip to content

Commit 8bc0764

Browse files
authored
Merge pull request #102 from tinyhumansai/tinybox-systemd-literal-env
fix(linux): preserve shell literals in systemd scopes
2 parents 3f0aa15 + cdb6a08 commit 8bc0764

4 files changed

Lines changed: 183 additions & 12 deletions

File tree

‎crates/tinybox-linux/src/sandbox/args.rs‎

Lines changed: 47 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
//! Building the `bwrap` command line, and the cgroup wrapper around it.
22
//!
3-
//! Pure functions, as with the Docker backend: what a sandbox binds, unshares,
4-
//! and limits *is* the security boundary, so every one of those decisions
5-
//! should be a value a test can assert on rather than something that only
6-
//! happens at runtime.
3+
//! Command construction, as with the Docker backend: what a sandbox binds,
4+
//! unshares, and limits *is* the security boundary, so every one of those
5+
//! decisions should be a value a test can assert on rather than something that
6+
//! only happens at runtime. Runtime capability probes are kept separate from
7+
//! the pure argument builders.
78
89
use std::path::Path;
9-
10-
use tinybox_core::{BoxSpec, Error, ExecRequest, Resources, Result, WorkspaceSource};
10+
use tinybox_core::{BoxSpec, Capability, Error, ExecRequest, Resources, Result, WorkspaceSource};
1111

1212
use super::NAME;
1313

@@ -21,6 +21,7 @@ pub const WORKSPACE_MOUNT: &str = "/workspace";
2121
/// user can read; this exposes the system's programs and libraries and nothing
2222
/// else.
2323
const USR: &str = "/usr";
24+
const SYSTEMD_EXPAND_ENVIRONMENT_FLAG: &[u8] = b"--expand-environment";
2425

2526
/// Files a normal command expects to exist.
2627
///
@@ -72,15 +73,20 @@ fn unsupported(kind: &'static str) -> Error {
7273
/// Returns [`Error::EmptyCommand`] when the request names no program, and
7374
/// [`Error::UnsupportedWorkspaceSource`] when the spec names a source this
7475
/// backend cannot bind.
75-
pub(super) fn exec(spec: &BoxSpec, request: &ExecRequest, limits: bool) -> Result<Vec<String>> {
76+
pub(super) fn exec(
77+
spec: &BoxSpec,
78+
request: &ExecRequest,
79+
limits: bool,
80+
supports_expand_environment: bool,
81+
) -> Result<Vec<String>> {
7682
if request.argv.is_empty() {
7783
return Err(Error::EmptyCommand {
7884
sandbox: NAME.to_owned(),
7985
});
8086
}
8187

8288
let mut argv = if limits {
83-
cgroup_prefix(&spec.resources)
89+
cgroup_prefix(&spec.resources, supports_expand_environment)
8490
} else {
8591
Vec::new()
8692
};
@@ -95,8 +101,11 @@ pub(super) fn exec(spec: &BoxSpec, request: &ExecRequest, limits: bool) -> Resul
95101
/// also why limits are opt-in: a machine with no systemd user session cannot
96102
/// provide them, and a sandbox that accepted a memory cap it could not apply
97103
/// would be claiming something untrue.
98-
fn cgroup_prefix(resources: &Resources) -> Vec<String> {
99-
vec![
104+
pub(super) fn cgroup_prefix(
105+
resources: &Resources,
106+
supports_expand_environment: bool,
107+
) -> Vec<String> {
108+
let mut argv = vec![
100109
"systemd-run".to_owned(),
101110
"--user".to_owned(),
102111
"--scope".to_owned(),
@@ -120,7 +129,34 @@ fn cgroup_prefix(resources: &Resources) -> Vec<String> {
120129
"--property".to_owned(),
121130
format!("TasksMax={}", resources.pids_max),
122131
"--".to_owned(),
123-
]
132+
];
133+
if supports_expand_environment {
134+
// The shell receives the command as an argument, but newer systemd
135+
// releases expand `${VAR}` references before the shell can interpret
136+
// its own quoting. Keep arbitrary user commands byte-for-byte intact.
137+
// This option was added in v254; older releases do not expand scope
138+
// command arguments by default, so omitting it preserves compatibility.
139+
argv.splice(3..3, ["--expand-environment=no".to_owned()]);
140+
}
141+
argv
142+
}
143+
144+
pub(super) fn parse_expand_environment_help(
145+
success: bool,
146+
stdout: &[u8],
147+
stderr: &[u8],
148+
) -> Result<bool> {
149+
if !success {
150+
return Err(Error::Unsupported {
151+
sandbox: NAME.to_owned(),
152+
capability: Capability::ResourceLimits,
153+
});
154+
}
155+
Ok([stdout, stderr].into_iter().any(|stream| {
156+
stream
157+
.windows(SYSTEMD_EXPAND_ENVIRONMENT_FLAG.len())
158+
.any(|window| window == SYSTEMD_EXPAND_ENVIRONMENT_FLAG)
159+
}))
124160
}
125161

126162
/// The `bwrap` invocation itself.

‎crates/tinybox-linux/src/sandbox/mod.rs‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,21 @@ impl Sandbox for NamespaceSandbox {
145145
});
146146
}
147147

148-
let argv = args::exec(&info.spec, request, self.limits)?;
148+
let supports_expand_environment = if self.limits {
149+
let probe = self
150+
.host
151+
.run(&ExecRequest::new(["systemd-run", "--help"]).with_env("LC_ALL", "C"))
152+
.await?;
153+
args::parse_expand_environment_help(probe.succeeded(), &probe.stdout, &probe.stderr)?
154+
} else {
155+
false
156+
};
157+
let argv = args::exec(
158+
&info.spec,
159+
request,
160+
self.limits,
161+
supports_expand_environment,
162+
)?;
149163
// A command that runs and fails is a result, so the raw output is
150164
// returned rather than being turned into a backend error.
151165
self.host.run(&ExecRequest::new(argv)).await

‎crates/tinybox-linux/src/sandbox/mod_tests.rs‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,37 @@ impl Host for RecordingHost {
4444

4545
async fn run(&self, request: &ExecRequest) -> Result<ExecOutput> {
4646
self.seen().push(request.argv.clone());
47+
if request.argv == ["systemd-run", "--help"] {
48+
return Ok(ExecOutput::new(
49+
0,
50+
b"--expand-environment=BOOL".to_vec(),
51+
Vec::new(),
52+
));
53+
}
4754
Ok(ExecOutput::new(0, b"ran".to_vec(), Vec::new()))
4855
}
4956
}
5057

58+
#[derive(Debug, Default)]
59+
struct FailedProbeHost {
60+
seen: Mutex<Vec<Vec<String>>>,
61+
}
62+
63+
#[async_trait]
64+
impl Host for FailedProbeHost {
65+
fn name(&self) -> &'static str {
66+
"failed-probe"
67+
}
68+
69+
async fn run(&self, request: &ExecRequest) -> Result<ExecOutput> {
70+
self.seen
71+
.lock()
72+
.unwrap_or_else(PoisonError::into_inner)
73+
.push(request.argv.clone());
74+
Ok(ExecOutput::new(1, Vec::new(), b"probe failed".to_vec()))
75+
}
76+
}
77+
5178
fn sandbox() -> (NamespaceSandbox, Arc<RecordingHost>) {
5279
let host = Arc::new(RecordingHost::default());
5380
(
@@ -300,6 +327,7 @@ async fn limits_wrap_the_sandbox_in_a_systemd_scope() -> Result<()> {
300327
assert_eq!(argv.first().map(String::as_str), Some("systemd-run"));
301328
assert!(has(&argv, "--user"));
302329
assert!(has(&argv, "--scope"));
330+
assert!(has(&argv, "--expand-environment=no"));
303331
let properties = argv
304332
.iter()
305333
.enumerate()
@@ -324,6 +352,71 @@ async fn limits_wrap_the_sandbox_in_a_systemd_scope() -> Result<()> {
324352
Ok(())
325353
}
326354

355+
#[test]
356+
fn systemd_scope_environment_flag_is_added_only_when_supported() {
357+
let resources = Resources {
358+
cpu_millis: 1_000,
359+
memory_bytes: 64 * 1024 * 1024,
360+
pids_max: 16,
361+
disk_bytes: 1024 * 1024,
362+
};
363+
364+
let supported = args::cgroup_prefix(&resources, true);
365+
assert!(has(&supported, "--expand-environment=no"));
366+
assert_eq!(
367+
supported.get(3).map(String::as_str),
368+
Some("--expand-environment=no")
369+
);
370+
371+
let legacy = args::cgroup_prefix(&resources, false);
372+
assert!(!has(&legacy, "--expand-environment=no"));
373+
assert_eq!(legacy.get(3).map(String::as_str), Some("--quiet"));
374+
}
375+
376+
#[test]
377+
fn systemd_environment_flag_probe_distinguishes_legacy_and_fails_closed() {
378+
assert_eq!(
379+
args::parse_expand_environment_help(true, b"--help", b""),
380+
Ok(false)
381+
);
382+
assert_eq!(
383+
args::parse_expand_environment_help(true, b"--expand-environment=BOOL", b""),
384+
Ok(true)
385+
);
386+
assert_eq!(
387+
args::parse_expand_environment_help(false, b"--expand-environment=BOOL", b""),
388+
Err(Error::Unsupported {
389+
sandbox: NAME.to_owned(),
390+
capability: tinybox_core::Capability::ResourceLimits,
391+
})
392+
);
393+
}
394+
395+
#[tokio::test]
396+
async fn failed_systemd_option_probe_refuses_without_launching_workload() -> Result<()> {
397+
let host = Arc::new(FailedProbeHost::default());
398+
let sandbox =
399+
NamespaceSandbox::new(host.clone(), Arc::new(MemoryStore::new())).with_cgroup_limits();
400+
let info = sandbox.create(&spec()?).await?;
401+
402+
let result = sandbox
403+
.exec(&info.id, &ExecRequest::new(["echo", "must not run"]))
404+
.await;
405+
406+
assert_eq!(
407+
result,
408+
Err(Error::Unsupported {
409+
sandbox: NAME.to_owned(),
410+
capability: Capability::ResourceLimits,
411+
})
412+
);
413+
assert_eq!(
414+
*host.seen.lock().unwrap_or_else(PoisonError::into_inner),
415+
[vec!["systemd-run".to_owned(), "--help".to_owned()]]
416+
);
417+
Ok(())
418+
}
419+
327420
#[tokio::test]
328421
async fn without_limits_there_is_no_scope_at_all() -> Result<()> {
329422
let (sandbox, host) = sandbox();

‎crates/tinybox-linux/tests/live_namespaces.rs‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,34 @@ async fn live_a_memory_limit_is_actually_enforced() -> Result<()> {
409409
Ok(())
410410
}
411411

412+
#[tokio::test]
413+
async fn live_systemd_scope_preserves_shell_dollar_literals() -> Result<()> {
414+
if !enabled() {
415+
return Ok(());
416+
}
417+
let dir = workspace()?;
418+
let sandbox = NamespaceSandbox::new(Arc::new(LocalHost::new()), Arc::new(MemoryStore::new()))
419+
.with_cgroup_limits();
420+
let info = sandbox.create(&spec(dir.path())?).await?;
421+
let output = sandbox
422+
.exec(
423+
&info.id,
424+
&ExecRequest::new(["/bin/sh", "-c", "printf '%s\\n' '${cgroup_path}'"]),
425+
)
426+
.await?;
427+
sandbox.destroy(&info.id).await?;
428+
429+
if !output.succeeded() {
430+
return Err(Error::Backend {
431+
sandbox: "namespace".to_owned(),
432+
operation: "preserve a literal dollar reference in a systemd scope",
433+
message: output.stderr_lossy().trim().to_owned(),
434+
});
435+
}
436+
assert_eq!(output.stdout_lossy(), "${cgroup_path}\n");
437+
Ok(())
438+
}
439+
412440
#[tokio::test]
413441
async fn live_a_box_is_only_a_record_so_destroying_it_leaves_the_workspace() -> Result<()> {
414442
if !enabled() {

0 commit comments

Comments
 (0)