Skip to content

Commit 4da65e7

Browse files
miguelgilaclaude
andauthored
fix(security): harden runtime against critical/high vulnerabilities (#25)
* fix(security): harden runtime against 6 critical/high vulnerabilities - Gate REAPER_NO_OVERLAY behind #[cfg(test)] so overlay bypass is impossible in release builds (Critical) - Validate container/exec IDs against path traversal: reject empty, '..', slashes, and non-alphanumeric chars (Critical) - Restrict config file to REAPER_ prefixed keys only, preventing injection of LD_PRELOAD, PATH, etc. (High) - Set restrictive permissions (0700 dirs, 0600 files) on state files and directories (High) - Validate PID > 1 before sending kill signals to prevent signalling init or the caller's process group (High) - Always clear supplementary groups during privilege drop, even when additional_gids is empty, to prevent inheriting root's groups (High) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use debug_assertions gate for REAPER_NO_OVERLAY bypass #[cfg(test)] only applies to the test harness, not binary targets built alongside it. Integration tests spawn reaper-runtime as a subprocess, so the overlay bypass was inactive in CI. Switch to #[cfg(debug_assertions)] which is active in debug builds (cargo test) but not in release builds (production). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 389a642 commit 4da65e7

File tree

5 files changed

+178
-20
lines changed

5 files changed

+178
-20
lines changed

docs/TODO.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ List of tasks to do, not ordered in any specific way.
1414
- [x] Add examples that use jobs, deployments and daemonsets
1515
- [x] Add complex example (idea openldap server + sssd + something that uses users)
1616
- [x] Add quick-start guide with a playground kind cluster for doing fast testing
17-
- [ ] Evaluate if it would make sense to isolate overlays by namespace
17+
- [x] Evaluate if it would make sense to isolate overlays by namespace
1818
- [x] Manage DNS settings, currently relying on host DNS instead of k8s DNS settings.
1919
- [ ] Add certain configuration parameters as annotations, so users can influence how Reaper works (DNS, overlay name and mount point, etc.). But ensuring adminsistrator parameters cannot be overriden.
2020
- [ ] Introduce more complex examples, answer this question: can we have a sssd containerd pod expose its socks file so a sample reaper pod can utilize it?

src/bin/containerd-shim-reaper-v2/main.rs

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -345,14 +345,32 @@ fn extract_k8s_namespace(bundle: &str) -> Option<String> {
345345
})
346346
}
347347

348+
/// Validate that an ID is safe for use in filesystem paths.
349+
/// Rejects empty strings, path traversal (`..`), and characters outside `[a-zA-Z0-9._-]`.
350+
fn validate_id(id: &str) -> Result<(), Error> {
351+
if id.is_empty()
352+
|| id.len() > 256
353+
|| id == "."
354+
|| id == ".."
355+
|| !id
356+
.chars()
357+
.all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '_' || c == '-')
358+
{
359+
return Err(Error::InvalidArgument(format!("invalid ID: {:?}", id)));
360+
}
361+
Ok(())
362+
}
363+
348364
/// Build the file path for an exec state file.
349-
fn build_exec_state_path(container_id: &str, exec_id: &str) -> String {
350-
format!(
365+
fn build_exec_state_path(container_id: &str, exec_id: &str) -> Result<String, Error> {
366+
validate_id(container_id)?;
367+
validate_id(exec_id)?;
368+
Ok(format!(
351369
"{}/{}/exec-{}.json",
352370
runtime_state_dir(),
353371
container_id,
354372
exec_id
355-
)
373+
))
356374
}
357375

358376
/// Map a status string from runtime state JSON to the protobuf Status enum.
@@ -633,7 +651,7 @@ impl Task for ReaperTask {
633651
}
634652

635653
// Poll exec state file for PID
636-
let exec_path = build_exec_state_path(&req.id, &req.exec_id);
654+
let exec_path = build_exec_state_path(&req.id, &req.exec_id)?;
637655
let exec_path_clone = exec_path.clone();
638656

639657
let pid = tokio::task::spawn_blocking(move || {
@@ -769,7 +787,7 @@ impl Task for ReaperTask {
769787
if !req.exec_id.is_empty() {
770788
info!("delete() - EXEC process, exec_id={}", req.exec_id);
771789

772-
let exec_path = build_exec_state_path(&req.id, &req.exec_id);
790+
let exec_path = build_exec_state_path(&req.id, &req.exec_id)?;
773791
let _ = std::fs::remove_file(&exec_path);
774792

775793
return Ok(api::DeleteResponse {
@@ -871,12 +889,12 @@ impl Task for ReaperTask {
871889
if !req.exec_id.is_empty() {
872890
info!("kill() - EXEC process, exec_id={}", req.exec_id);
873891

874-
let exec_path = build_exec_state_path(&req.id, &req.exec_id);
892+
let exec_path = build_exec_state_path(&req.id, &req.exec_id)?;
875893

876894
if let Ok(data) = std::fs::read_to_string(&exec_path) {
877895
if let Ok(state) = serde_json::from_str::<serde_json::Value>(&data) {
878896
if let Some(pid) = state["pid"].as_i64() {
879-
if pid > 0 {
897+
if pid > 1 {
880898
let sig = nix::sys::signal::Signal::try_from(req.signal as i32)
881899
.unwrap_or(nix::sys::signal::Signal::SIGTERM);
882900
// Kill the entire process group so children are also signalled.
@@ -1002,7 +1020,7 @@ impl Task for ReaperTask {
10021020
if !req.exec_id.is_empty() {
10031021
info!("wait() - EXEC process, exec_id={}", req.exec_id);
10041022

1005-
let exec_path = build_exec_state_path(&req.id, &req.exec_id);
1023+
let exec_path = build_exec_state_path(&req.id, &req.exec_id)?;
10061024
let container_id = req.id.clone();
10071025
let exec_id_clone = req.exec_id.clone();
10081026

@@ -1136,7 +1154,7 @@ impl Task for ReaperTask {
11361154
if !req.exec_id.is_empty() {
11371155
info!("state() - EXEC process, exec_id={}", req.exec_id);
11381156

1139-
let exec_path = build_exec_state_path(&req.id, &req.exec_id);
1157+
let exec_path = build_exec_state_path(&req.id, &req.exec_id)?;
11401158

11411159
if let Ok(data) = std::fs::read_to_string(&exec_path) {
11421160
if let Ok(state) = serde_json::from_str::<serde_json::Value>(&data) {
@@ -1367,7 +1385,7 @@ impl Task for ReaperTask {
13671385
"stderr": if req.stderr.is_empty() { serde_json::Value::Null } else { serde_json::Value::String(req.stderr.clone()) },
13681386
});
13691387

1370-
let exec_path = build_exec_state_path(&req.id, &req.exec_id);
1388+
let exec_path = build_exec_state_path(&req.id, &req.exec_id)?;
13711389

13721390
std::fs::write(&exec_path, serde_json::to_vec_pretty(&exec_state).unwrap()).map_err(
13731391
|e| {
@@ -1814,7 +1832,7 @@ mod tests {
18141832
#[serial]
18151833
fn test_build_exec_state_path_format() {
18161834
std::env::set_var("REAPER_RUNTIME_ROOT", "/run/reaper");
1817-
let path = build_exec_state_path("my-container", "exec-123");
1835+
let path = build_exec_state_path("my-container", "exec-123").unwrap();
18181836
assert_eq!(path, "/run/reaper/my-container/exec-exec-123.json");
18191837
std::env::remove_var("REAPER_RUNTIME_ROOT");
18201838
}
@@ -1823,7 +1841,7 @@ mod tests {
18231841
#[serial]
18241842
fn test_build_exec_state_path_custom_root() {
18251843
std::env::set_var("REAPER_RUNTIME_ROOT", "/custom/path");
1826-
let path = build_exec_state_path("ctr-1", "e1");
1844+
let path = build_exec_state_path("ctr-1", "e1").unwrap();
18271845
assert_eq!(path, "/custom/path/ctr-1/exec-e1.json");
18281846
std::env::remove_var("REAPER_RUNTIME_ROOT");
18291847
}

src/bin/reaper-runtime/main.rs

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -434,9 +434,12 @@ fn do_start(id: &str, bundle: &Path) -> Result<()> {
434434
// REAPER_NO_OVERLAY=1 disables overlay for unit tests that lack CAP_SYS_ADMIN.
435435
#[cfg(target_os = "linux")]
436436
{
437+
#[cfg(debug_assertions)]
437438
let skip_overlay = std::env::var("REAPER_NO_OVERLAY")
438439
.map(|v| v == "1" || v.eq_ignore_ascii_case("true"))
439440
.unwrap_or(false);
441+
#[cfg(not(debug_assertions))]
442+
let skip_overlay = false;
440443

441444
if skip_overlay {
442445
info!("do_start() - overlay disabled via REAPER_NO_OVERLAY");
@@ -582,10 +585,9 @@ fn do_start(id: &str, bundle: &Path) -> Result<()> {
582585

583586
// Apply user/group configuration if present
584587
if let Some(ref user) = user_cfg_for_exec {
585-
// Set supplementary groups first (must be done while privileged)
586-
if !user.additional_gids.is_empty() {
587-
safe_setgroups(&user.additional_gids)?;
588-
}
588+
// Always clear/set supplementary groups (must be done while privileged).
589+
// When empty, this clears inherited root supplementary groups.
590+
safe_setgroups(&user.additional_gids)?;
589591

590592
// Set GID before UID (privilege dropping order matters)
591593
if nix::libc::setgid(user.gid) != 0 {
@@ -968,6 +970,9 @@ fn do_kill(id: &str, signal: Option<i32>) -> Result<()> {
968970
let signal = signal.unwrap_or(15); // Default to SIGTERM
969971
info!("do_kill() called - id={}, signal={}", id, signal);
970972
let pid = load_pid(id)?;
973+
if pid <= 1 {
974+
bail!("refusing to send signal to PID {} (must be > 1)", pid);
975+
}
971976
info!(
972977
"do_kill() - sending signal {} to process group (pgid={})",
973978
signal, pid

src/bin/reaper-runtime/state.rs

Lines changed: 100 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,33 @@
1+
use anyhow::bail;
12
use serde::{Deserialize, Serialize};
23
use std::fs;
34
use std::io::Write;
5+
#[cfg(unix)]
6+
use std::os::unix::fs::PermissionsExt;
47
use std::path::PathBuf;
58

9+
/// Validate that an ID is safe for use in filesystem paths.
10+
/// Rejects empty strings, path traversal (`..`), and characters outside `[a-zA-Z0-9._-]`.
11+
/// IDs longer than 256 characters are also rejected.
12+
pub fn validate_id(id: &str) -> anyhow::Result<()> {
13+
if id.is_empty() {
14+
bail!("ID must not be empty");
15+
}
16+
if id.len() > 256 {
17+
bail!("ID must not exceed 256 characters");
18+
}
19+
if id == "." || id == ".." {
20+
bail!("ID must not be '.' or '..'");
21+
}
22+
if !id
23+
.chars()
24+
.all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '_' || c == '-')
25+
{
26+
bail!("ID contains invalid characters (allowed: a-zA-Z0-9._-)");
27+
}
28+
Ok(())
29+
}
30+
631
/// OCI User specification for UID/GID switching
732
#[derive(Debug, Deserialize, Serialize, Clone)]
833
pub struct OciUser {
@@ -72,34 +97,49 @@ pub fn pid_path(id: &str) -> PathBuf {
7297
}
7398

7499
pub fn save_state(state: &ContainerState) -> anyhow::Result<()> {
100+
validate_id(&state.id)?;
75101
let dir = container_dir(&state.id);
76102
fs::create_dir_all(&dir)?;
103+
#[cfg(unix)]
104+
fs::set_permissions(&dir, fs::Permissions::from_mode(0o700))?;
105+
let path = state_path(&state.id);
77106
let json = serde_json::to_vec_pretty(&state)?;
78-
fs::write(state_path(&state.id), json)?;
107+
fs::write(&path, json)?;
108+
#[cfg(unix)]
109+
fs::set_permissions(&path, fs::Permissions::from_mode(0o600))?;
79110
Ok(())
80111
}
81112

82113
pub fn load_state(id: &str) -> anyhow::Result<ContainerState> {
114+
validate_id(id)?;
83115
let data = fs::read(state_path(id))?;
84116
let state: ContainerState = serde_json::from_slice(&data)?;
85117
Ok(state)
86118
}
87119

88120
pub fn save_pid(id: &str, pid: i32) -> anyhow::Result<()> {
121+
validate_id(id)?;
89122
let dir = container_dir(id);
90123
fs::create_dir_all(&dir)?;
91-
let mut f = fs::File::create(pid_path(id))?;
124+
#[cfg(unix)]
125+
fs::set_permissions(&dir, fs::Permissions::from_mode(0o700))?;
126+
let path = pid_path(id);
127+
let mut f = fs::File::create(&path)?;
92128
writeln!(f, "{}", pid)?;
129+
#[cfg(unix)]
130+
fs::set_permissions(&path, fs::Permissions::from_mode(0o600))?;
93131
Ok(())
94132
}
95133

96134
pub fn load_pid(id: &str) -> anyhow::Result<i32> {
135+
validate_id(id)?;
97136
let s = fs::read_to_string(pid_path(id))?;
98137
let pid: i32 = s.trim().parse()?;
99138
Ok(pid)
100139
}
101140

102141
pub fn delete(id: &str) -> anyhow::Result<()> {
142+
validate_id(id)?;
103143
let dir = container_dir(id);
104144
if dir.exists() {
105145
fs::remove_dir_all(dir)?;
@@ -138,14 +178,23 @@ pub fn exec_state_path(container_id: &str, exec_id: &str) -> PathBuf {
138178
}
139179

140180
pub fn save_exec_state(state: &ExecState) -> anyhow::Result<()> {
181+
validate_id(&state.container_id)?;
182+
validate_id(&state.exec_id)?;
141183
let dir = container_dir(&state.container_id);
142184
fs::create_dir_all(&dir)?;
185+
#[cfg(unix)]
186+
fs::set_permissions(&dir, fs::Permissions::from_mode(0o700))?;
187+
let path = exec_state_path(&state.container_id, &state.exec_id);
143188
let json = serde_json::to_vec_pretty(&state)?;
144-
fs::write(exec_state_path(&state.container_id, &state.exec_id), json)?;
189+
fs::write(&path, json)?;
190+
#[cfg(unix)]
191+
fs::set_permissions(&path, fs::Permissions::from_mode(0o600))?;
145192
Ok(())
146193
}
147194

148195
pub fn load_exec_state(container_id: &str, exec_id: &str) -> anyhow::Result<ExecState> {
196+
validate_id(container_id)?;
197+
validate_id(exec_id)?;
149198
let path = exec_state_path(container_id, exec_id);
150199
let data = fs::read(&path)?;
151200
let state: ExecState = serde_json::from_slice(&data)?;
@@ -425,4 +474,52 @@ mod tests {
425474
// .expect("Delete should not fail on nonexistent exec");
426475
// });
427476
// }
477+
478+
// --- validate_id tests ---
479+
480+
#[test]
481+
fn test_validate_id_valid() {
482+
assert!(validate_id("my-container").is_ok());
483+
assert!(validate_id("abc123").is_ok());
484+
assert!(validate_id("a.b_c-d").is_ok());
485+
assert!(validate_id("A").is_ok());
486+
}
487+
488+
#[test]
489+
fn test_validate_id_rejects_empty() {
490+
assert!(validate_id("").is_err());
491+
}
492+
493+
#[test]
494+
fn test_validate_id_rejects_path_traversal() {
495+
assert!(validate_id("..").is_err());
496+
assert!(validate_id("../etc/passwd").is_err());
497+
assert!(validate_id("foo/../bar").is_err());
498+
}
499+
500+
#[test]
501+
fn test_validate_id_rejects_dot() {
502+
assert!(validate_id(".").is_err());
503+
}
504+
505+
#[test]
506+
fn test_validate_id_rejects_slashes() {
507+
assert!(validate_id("foo/bar").is_err());
508+
assert!(validate_id("/absolute").is_err());
509+
}
510+
511+
#[test]
512+
fn test_validate_id_rejects_long() {
513+
let long_id = "a".repeat(257);
514+
assert!(validate_id(&long_id).is_err());
515+
let ok_id = "a".repeat(256);
516+
assert!(validate_id(&ok_id).is_ok());
517+
}
518+
519+
#[test]
520+
fn test_validate_id_rejects_special_chars() {
521+
assert!(validate_id("foo bar").is_err());
522+
assert!(validate_id("foo\nbar").is_err());
523+
assert!(validate_id("foo\0bar").is_err());
524+
}
428525
}

src/config.rs

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,16 @@ pub fn load_config() {
4444
continue;
4545
}
4646

47+
// Only allow REAPER_ prefixed keys to prevent injection of
48+
// dangerous env vars like LD_PRELOAD, PATH, etc.
49+
if !key.starts_with("REAPER_") {
50+
eprintln!(
51+
"reaper: config: ignoring non-REAPER_ key {:?} in {}",
52+
key, path
53+
);
54+
continue;
55+
}
56+
4757
// Only set if not already present in environment (env wins)
4858
if std::env::var(key).is_err() {
4959
std::env::set_var(key, value);
@@ -120,6 +130,34 @@ mod tests {
120130
std::env::remove_var("REAPER_TEST_PRECEDENCE");
121131
}
122132

133+
#[test]
134+
#[serial]
135+
fn test_rejects_non_reaper_keys() {
136+
let dir = tempfile::tempdir().unwrap();
137+
let conf = dir.path().join("reaper.conf");
138+
let mut f = std::fs::File::create(&conf).unwrap();
139+
writeln!(f, "LD_PRELOAD=/tmp/evil.so").unwrap();
140+
writeln!(f, "PATH=/tmp/evil").unwrap();
141+
writeln!(f, "REAPER_TEST_ALLOWED=yes").unwrap();
142+
143+
std::env::remove_var("LD_PRELOAD");
144+
std::env::remove_var("REAPER_TEST_ALLOWED");
145+
std::env::set_var("REAPER_CONFIG", conf.to_str().unwrap());
146+
147+
load_config();
148+
149+
// Non-REAPER_ keys must NOT be set
150+
assert!(
151+
std::env::var("LD_PRELOAD").is_err(),
152+
"LD_PRELOAD should not be set from config"
153+
);
154+
// REAPER_ keys should be set
155+
assert_eq!(std::env::var("REAPER_TEST_ALLOWED").unwrap(), "yes");
156+
157+
std::env::remove_var("REAPER_CONFIG");
158+
std::env::remove_var("REAPER_TEST_ALLOWED");
159+
}
160+
123161
#[test]
124162
#[serial]
125163
fn test_skips_malformed_lines() {

0 commit comments

Comments
 (0)