Skip to content

Commit 3bbdd34

Browse files
committed
fix(sandbox): handle per-path Landlock errors instead of abandoning entire ruleset
A single missing path (e.g., /app in containers without that directory) caused PathFd::new() to propagate an error out of the entire Landlock setup closure. Under BestEffort mode, this silently disabled all filesystem restrictions for the sandbox. Changes: - Extract try_open_path() and classify_path_error() helpers that handle PathFd failures per-path instead of per-ruleset - BestEffort mode: skip inaccessible paths with a warning, apply remaining rules - HardRequirement mode: fail immediately on any inaccessible path - Add zero-rule safety check to prevent applying an empty ruleset that would block all filesystem access - Pre-filter system-injected baseline paths (e.g., /app) in enrichment functions so missing paths never reach Landlock - Add unit tests for try_open_path, classify_path_error, and error classification for ENOENT, EACCES, ELOOP, ENAMETOOLONG, ENOTDIR - Update user-facing docs and architecture docs with Landlock behavior tables, baseline path filtering, and compatibility mode semantics - Fix stale ABI::V1 references in docs (code uses ABI::V2) Closes #664
1 parent 0832f11 commit 3bbdd34

File tree

6 files changed

+267
-33
lines changed

6 files changed

+267
-33
lines changed

architecture/sandbox.md

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -431,15 +431,21 @@ Landlock restricts the child process's filesystem access to an explicit allowlis
431431
1. Build path lists from `filesystem.read_only` and `filesystem.read_write`
432432
2. If `include_workdir` is true, add the working directory to `read_write`
433433
3. If both lists are empty, skip Landlock entirely (no-op)
434-
4. Create a Landlock ruleset targeting ABI V1:
434+
4. Create a Landlock ruleset targeting ABI V2:
435435
- Read-only paths receive `AccessFs::from_read(abi)` rights
436436
- Read-write paths receive `AccessFs::from_all(abi)` rights
437-
5. Call `ruleset.restrict_self()` -- this applies to the calling process and all descendants
437+
5. For each path, attempt `PathFd::new()`. If it fails:
438+
- `BestEffort`: Log a warning with the error classification (not found, permission denied, symlink loop, etc.) and skip the path. Continue building the ruleset from remaining valid paths.
439+
- `HardRequirement`: Return a fatal error, aborting the sandbox.
440+
6. If all paths failed (zero rules applied), return an error rather than calling `restrict_self()` on an empty ruleset (which would block all filesystem access)
441+
7. Call `ruleset.restrict_self()` -- this applies to the calling process and all descendants
438442

439-
Error behavior depends on `LandlockCompatibility`:
443+
Kernel-level error behavior (e.g., Landlock ABI unavailable) depends on `LandlockCompatibility`:
440444
- `BestEffort`: Log a warning and continue without filesystem isolation
441445
- `HardRequirement`: Return a fatal error, aborting the sandbox
442446

447+
**Baseline path filtering**: System-injected baseline paths (e.g., `/app`) are pre-filtered by `enrich_proto_baseline_paths()` / `enrich_sandbox_baseline_paths()` using `Path::exists()` before they reach Landlock. User-specified paths are not pre-filtered -- they are evaluated at Landlock apply time so misconfigurations surface as warnings or errors.
448+
443449
### Seccomp syscall filtering
444450

445451
**File:** `crates/openshell-sandbox/src/sandbox/linux/seccomp.rs`

architecture/security-policy.md

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,7 @@ Controls which filesystem paths the sandboxed process can access. Enforced via L
320320
| `read_only` | `string[]` | `[]` | Paths accessible in read-only mode |
321321
| `read_write` | `string[]` | `[]` | Paths accessible in read-write mode |
322322

323-
**Enforcement mapping**: Each path becomes a Landlock `PathBeneath` rule. Read-only paths receive `AccessFs::from_read(ABI::V1)` permissions. Read-write paths receive `AccessFs::from_all(ABI::V1)` permissions (read, write, execute, create, delete, rename). All other paths are denied by the Landlock ruleset.
323+
**Enforcement mapping**: Each path becomes a Landlock `PathBeneath` rule. Read-only paths receive `AccessFs::from_read(ABI::V2)` permissions. Read-write paths receive `AccessFs::from_all(ABI::V2)` permissions (read, write, execute, create, delete, rename). All other paths are denied by the Landlock ruleset.
324324

325325
**Filesystem preparation**: Before the child process spawns, the supervisor creates any `read_write` directories that do not exist and sets their ownership to `process.run_as_user`:`process.run_as_group` via `chown()`. See `crates/openshell-sandbox/src/lib.rs` -- `prepare_filesystem()`.
326326

@@ -358,10 +358,16 @@ Controls Landlock LSM compatibility behavior. **Static field** -- immutable afte
358358

359359
| Value | Behavior |
360360
| ------------------ | --------------------------------------------------------------------------------------------------------------------------- |
361-
| `best_effort` | If Landlock is unavailable (older kernel, unprivileged container), log a warning and continue without filesystem sandboxing |
362-
| `hard_requirement` | If Landlock is unavailable, abort sandbox startup with an error |
361+
| `best_effort` | If Landlock is unavailable (older kernel, unprivileged container), log a warning and continue without filesystem sandboxing. Individual inaccessible paths (missing, permission denied, symlink loops) are skipped with a warning while remaining rules are still applied. If all paths fail, the sandbox continues without Landlock rather than applying an empty ruleset that would block all access. |
362+
| `hard_requirement` | If Landlock is unavailable or any configured path cannot be opened, abort sandbox startup with an error. |
363363

364-
See `crates/openshell-sandbox/src/sandbox/linux/landlock.rs` -- `compat_level()`.
364+
**Per-path error handling**: `PathFd::new()` (which wraps `open(path, O_PATH | O_CLOEXEC)`) can fail for several reasons beyond path non-existence: `EACCES` (permission denied), `ELOOP` (symlink loop), `ENAMETOOLONG`, `ENOTDIR`. Each failure is classified with a human-readable reason in logs. In `best_effort` mode, the path is skipped and ruleset construction continues. In `hard_requirement` mode, the error is fatal.
365+
366+
**Baseline path filtering**: The enrichment functions (`enrich_proto_baseline_paths`, `enrich_sandbox_baseline_paths`) pre-filter system-injected baseline paths (e.g., `/app`) by checking `Path::exists()` before adding them to the policy. This prevents missing baseline paths from reaching Landlock at all. User-specified paths are not pre-filtered — they are evaluated at Landlock apply time so that misconfigurations surface as warnings (`best_effort`) or errors (`hard_requirement`).
367+
368+
**Zero-rule safety check**: If all paths in the ruleset fail to open, `apply()` returns an error rather than calling `restrict_self()` on an empty ruleset. An empty Landlock ruleset with `restrict_self()` would block all filesystem access — the inverse of the intended degradation behavior. This error is caught by the outer `BestEffort` handler, which logs a warning and continues without Landlock.
369+
370+
See `crates/openshell-sandbox/src/sandbox/linux/landlock.rs` -- `compat_level()`, `try_open_path()`, `classify_path_fd_error()`, `classify_io_error()`.
365371

366372
```yaml
367373
landlock:

crates/openshell-sandbox/src/lib.rs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -899,12 +899,31 @@ fn enrich_proto_baseline_paths(proto: &mut openshell_core::proto::SandboxPolicy)
899899
let mut modified = false;
900900
for &path in PROXY_BASELINE_READ_ONLY {
901901
if !fs.read_only.iter().any(|p| p.as_str() == path) {
902+
// Baseline paths are system-injected, not user-specified. Skip
903+
// paths that do not exist in this container image to avoid noisy
904+
// warnings from Landlock and, more critically, to prevent a single
905+
// missing baseline path from abandoning the entire Landlock
906+
// ruleset under best-effort mode (see issue #664).
907+
if !std::path::Path::new(path).exists() {
908+
debug!(
909+
path,
910+
"Baseline read-only path does not exist, skipping enrichment"
911+
);
912+
continue;
913+
}
902914
fs.read_only.push(path.to_string());
903915
modified = true;
904916
}
905917
}
906918
for &path in PROXY_BASELINE_READ_WRITE {
907919
if !fs.read_write.iter().any(|p| p.as_str() == path) {
920+
if !std::path::Path::new(path).exists() {
921+
debug!(
922+
path,
923+
"Baseline read-write path does not exist, skipping enrichment"
924+
);
925+
continue;
926+
}
908927
fs.read_write.push(path.to_string());
909928
modified = true;
910929
}
@@ -929,13 +948,29 @@ fn enrich_sandbox_baseline_paths(policy: &mut SandboxPolicy) {
929948
for &path in PROXY_BASELINE_READ_ONLY {
930949
let p = std::path::PathBuf::from(path);
931950
if !policy.filesystem.read_only.contains(&p) {
951+
// Baseline paths are system-injected — skip non-existent paths to
952+
// avoid Landlock ruleset abandonment (issue #664).
953+
if !p.exists() {
954+
debug!(
955+
path,
956+
"Baseline read-only path does not exist, skipping enrichment"
957+
);
958+
continue;
959+
}
932960
policy.filesystem.read_only.push(p);
933961
modified = true;
934962
}
935963
}
936964
for &path in PROXY_BASELINE_READ_WRITE {
937965
let p = std::path::PathBuf::from(path);
938966
if !policy.filesystem.read_write.contains(&p) {
967+
if !p.exists() {
968+
debug!(
969+
path,
970+
"Baseline read-write path does not exist, skipping enrichment"
971+
);
972+
continue;
973+
}
939974
policy.filesystem.read_write.push(p);
940975
modified = true;
941976
}

crates/openshell-sandbox/src/sandbox/linux/landlock.rs

Lines changed: 185 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,11 @@
55
66
use crate::policy::{LandlockCompatibility, SandboxPolicy};
77
use landlock::{
8-
ABI, Access, AccessFs, CompatLevel, Compatible, PathBeneath, PathFd, Ruleset, RulesetAttr,
9-
RulesetCreatedAttr,
8+
ABI, Access, AccessFs, CompatLevel, Compatible, PathBeneath, PathFd, PathFdError, Ruleset,
9+
RulesetAttr, RulesetCreatedAttr,
1010
};
1111
use miette::{IntoDiagnostic, Result};
12-
use std::path::PathBuf;
12+
use std::path::{Path, PathBuf};
1313
use tracing::{debug, info, warn};
1414

1515
pub fn apply(policy: &SandboxPolicy, workdir: Option<&str>) -> Result<()> {
@@ -29,6 +29,7 @@ pub fn apply(policy: &SandboxPolicy, workdir: Option<&str>) -> Result<()> {
2929
return Ok(());
3030
}
3131

32+
let total_paths = read_only.len() + read_write.len();
3233
let abi = ABI::V2;
3334
info!(
3435
abi = ?abi,
@@ -38,47 +39,61 @@ pub fn apply(policy: &SandboxPolicy, workdir: Option<&str>) -> Result<()> {
3839
"Applying Landlock filesystem sandbox"
3940
);
4041

42+
let compatibility = &policy.landlock.compatibility;
43+
4144
let result: Result<()> = (|| {
4245
let access_all = AccessFs::from_all(abi);
4346
let access_read = AccessFs::from_read(abi);
4447

4548
let mut ruleset = Ruleset::default();
4649
ruleset = ruleset
47-
.set_compatibility(compat_level(&policy.landlock.compatibility))
50+
.set_compatibility(compat_level(compatibility))
4851
.handle_access(access_all)
4952
.into_diagnostic()?;
5053

5154
let mut ruleset = ruleset.create().into_diagnostic()?;
55+
let mut rules_applied: usize = 0;
56+
57+
for path in &read_only {
58+
if let Some(path_fd) = try_open_path(path, compatibility)? {
59+
debug!(path = %path.display(), "Landlock allow read-only");
60+
ruleset = ruleset
61+
.add_rule(PathBeneath::new(path_fd, access_read))
62+
.into_diagnostic()?;
63+
rules_applied += 1;
64+
}
65+
}
5266

53-
for path in read_only {
54-
debug!(path = %path.display(), "Landlock allow read-only");
55-
ruleset = ruleset
56-
.add_rule(PathBeneath::new(
57-
PathFd::new(path).into_diagnostic()?,
58-
access_read,
59-
))
60-
.into_diagnostic()?;
67+
for path in &read_write {
68+
if let Some(path_fd) = try_open_path(path, compatibility)? {
69+
debug!(path = %path.display(), "Landlock allow read-write");
70+
ruleset = ruleset
71+
.add_rule(PathBeneath::new(path_fd, access_all))
72+
.into_diagnostic()?;
73+
rules_applied += 1;
74+
}
6175
}
6276

63-
for path in read_write {
64-
debug!(path = %path.display(), "Landlock allow read-write");
65-
ruleset = ruleset
66-
.add_rule(PathBeneath::new(
67-
PathFd::new(path).into_diagnostic()?,
68-
access_all,
69-
))
70-
.into_diagnostic()?;
77+
if rules_applied == 0 {
78+
return Err(miette::miette!(
79+
"Landlock ruleset has zero valid paths — all {} path(s) failed to open. \
80+
Refusing to apply an empty ruleset that would block all filesystem access.",
81+
total_paths,
82+
));
7183
}
7284

85+
let skipped = total_paths - rules_applied;
86+
info!(
87+
rules_applied,
88+
skipped, "Landlock ruleset built successfully"
89+
);
90+
7391
ruleset.restrict_self().into_diagnostic()?;
7492
Ok(())
7593
})();
7694

7795
if let Err(err) = result {
78-
if matches!(
79-
policy.landlock.compatibility,
80-
LandlockCompatibility::BestEffort
81-
) {
96+
if matches!(compatibility, LandlockCompatibility::BestEffort) {
8297
warn!(
8398
error = %err,
8499
"Landlock filesystem sandbox is UNAVAILABLE — running WITHOUT filesystem restrictions. \
@@ -92,9 +107,155 @@ pub fn apply(policy: &SandboxPolicy, workdir: Option<&str>) -> Result<()> {
92107
Ok(())
93108
}
94109

110+
/// Attempt to open a path for Landlock rule creation.
111+
///
112+
/// In `BestEffort` mode, inaccessible paths (missing, permission denied, symlink
113+
/// loops, etc.) are skipped with a warning and `Ok(None)` is returned so the
114+
/// caller can continue building the ruleset from the remaining valid paths.
115+
///
116+
/// In `HardRequirement` mode, any failure is fatal — the caller propagates the
117+
/// error, which ultimately aborts sandbox startup.
118+
fn try_open_path(path: &Path, compatibility: &LandlockCompatibility) -> Result<Option<PathFd>> {
119+
match PathFd::new(path) {
120+
Ok(fd) => Ok(Some(fd)),
121+
Err(err) => {
122+
let reason = classify_path_fd_error(&err);
123+
match compatibility {
124+
LandlockCompatibility::BestEffort => {
125+
warn!(
126+
path = %path.display(),
127+
error = %err,
128+
reason = reason,
129+
"Skipping inaccessible Landlock path (best-effort mode)"
130+
);
131+
Ok(None)
132+
}
133+
LandlockCompatibility::HardRequirement => Err(miette::miette!(
134+
"Landlock path unavailable in hard_requirement mode: {} ({}): {}",
135+
path.display(),
136+
reason,
137+
err,
138+
)),
139+
}
140+
}
141+
}
142+
}
143+
144+
/// Classify a [`PathFdError`] into a human-readable reason.
145+
///
146+
/// `PathFd::new()` wraps `open(path, O_PATH | O_CLOEXEC)` which can fail for
147+
/// several reasons beyond simple non-existence. The `PathFdError::OpenCall`
148+
/// variant wraps the underlying `std::io::Error`.
149+
fn classify_path_fd_error(err: &PathFdError) -> &'static str {
150+
match err {
151+
PathFdError::OpenCall { source, .. } => classify_io_error(source),
152+
// PathFdError is #[non_exhaustive], handle future variants gracefully.
153+
_ => "unexpected error",
154+
}
155+
}
156+
157+
/// Classify a `std::io::Error` into a human-readable reason string.
158+
fn classify_io_error(err: &std::io::Error) -> &'static str {
159+
match err.kind() {
160+
std::io::ErrorKind::NotFound => "path does not exist",
161+
std::io::ErrorKind::PermissionDenied => "permission denied",
162+
_ => match err.raw_os_error() {
163+
Some(40) => "too many symlink levels", // ELOOP
164+
Some(36) => "path name too long", // ENAMETOOLONG
165+
Some(20) => "path component is not a directory", // ENOTDIR
166+
_ => "unexpected error",
167+
},
168+
}
169+
}
170+
95171
fn compat_level(level: &LandlockCompatibility) -> CompatLevel {
96172
match level {
97173
LandlockCompatibility::BestEffort => CompatLevel::BestEffort,
98174
LandlockCompatibility::HardRequirement => CompatLevel::HardRequirement,
99175
}
100176
}
177+
178+
#[cfg(test)]
179+
mod tests {
180+
use super::*;
181+
182+
#[test]
183+
fn try_open_path_best_effort_returns_none_for_missing_path() {
184+
let result = try_open_path(
185+
&PathBuf::from("/nonexistent/openshell/test/path"),
186+
&LandlockCompatibility::BestEffort,
187+
);
188+
assert!(result.is_ok());
189+
assert!(result.unwrap().is_none());
190+
}
191+
192+
#[test]
193+
fn try_open_path_hard_requirement_errors_for_missing_path() {
194+
let result = try_open_path(
195+
&PathBuf::from("/nonexistent/openshell/test/path"),
196+
&LandlockCompatibility::HardRequirement,
197+
);
198+
assert!(result.is_err());
199+
let err_msg = result.unwrap_err().to_string();
200+
assert!(
201+
err_msg.contains("hard_requirement"),
202+
"error should mention hard_requirement mode: {err_msg}"
203+
);
204+
assert!(
205+
err_msg.contains("does not exist"),
206+
"error should include the classified reason: {err_msg}"
207+
);
208+
}
209+
210+
#[test]
211+
fn try_open_path_succeeds_for_existing_path() {
212+
let dir = tempfile::tempdir().unwrap();
213+
let result = try_open_path(dir.path(), &LandlockCompatibility::BestEffort);
214+
assert!(result.is_ok());
215+
assert!(result.unwrap().is_some());
216+
}
217+
218+
#[test]
219+
fn classify_not_found() {
220+
let err = std::io::Error::from_raw_os_error(libc::ENOENT);
221+
assert_eq!(classify_io_error(&err), "path does not exist");
222+
}
223+
224+
#[test]
225+
fn classify_permission_denied() {
226+
let err = std::io::Error::from_raw_os_error(libc::EACCES);
227+
assert_eq!(classify_io_error(&err), "permission denied");
228+
}
229+
230+
#[test]
231+
fn classify_symlink_loop() {
232+
let err = std::io::Error::from_raw_os_error(libc::ELOOP);
233+
assert_eq!(classify_io_error(&err), "too many symlink levels");
234+
}
235+
236+
#[test]
237+
fn classify_name_too_long() {
238+
let err = std::io::Error::from_raw_os_error(libc::ENAMETOOLONG);
239+
assert_eq!(classify_io_error(&err), "path name too long");
240+
}
241+
242+
#[test]
243+
fn classify_not_a_directory() {
244+
let err = std::io::Error::from_raw_os_error(libc::ENOTDIR);
245+
assert_eq!(classify_io_error(&err), "path component is not a directory");
246+
}
247+
248+
#[test]
249+
fn classify_unknown_error() {
250+
let err = std::io::Error::from_raw_os_error(libc::EIO);
251+
assert_eq!(classify_io_error(&err), "unexpected error");
252+
}
253+
254+
#[test]
255+
fn classify_path_fd_error_extracts_io_error() {
256+
// Use PathFd::new on a non-existent path to get a real PathFdError
257+
// (the OpenCall variant is #[non_exhaustive] and can't be constructed directly).
258+
let err = PathFd::new("/nonexistent/openshell/classify/test").unwrap_err();
259+
assert_eq!(classify_path_fd_error(&err), "path does not exist");
260+
}
261+
}

docs/reference/policy-schema.md

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,20 @@ Configures [Landlock LSM](https://docs.kernel.org/security/landlock.html) enforc
105105

106106
| Field | Type | Required | Values | Description |
107107
|---|---|---|---|---|
108-
| `compatibility` | string | No | `best_effort`, `hard_requirement` | How OpenShell handles kernel ABI differences. `best_effort` uses the highest Landlock ABI the host kernel supports. `hard_requirement` fails if the required ABI is unavailable. |
108+
| `compatibility` | string | No | `best_effort`, `hard_requirement` | How OpenShell handles Landlock failures. See behavior table below. |
109+
110+
**Compatibility modes:**
111+
112+
| Value | Kernel ABI unavailable | Individual path inaccessible | All paths inaccessible |
113+
|---|---|---|---|
114+
| `best_effort` | Warns and continues without Landlock. | Skips the path, applies remaining rules. | Warns and continues without Landlock (refuses to apply an empty ruleset). |
115+
| `hard_requirement` | Aborts sandbox startup. | Aborts sandbox startup. | Aborts sandbox startup. |
116+
117+
`best_effort` (the default) is appropriate for most deployments. It handles missing paths gracefully -- for example, `/app` may not exist in every container image but is included in the baseline path set for containers that do have it. Individual missing paths are skipped while the remaining filesystem rules are still enforced.
118+
119+
`hard_requirement` is for environments where any gap in filesystem isolation is unacceptable. If a listed path cannot be opened for any reason (missing, permission denied, symlink loop), sandbox startup fails immediately rather than running with reduced protection.
120+
121+
When a path is skipped under `best_effort`, the sandbox logs a warning that includes the path, the specific error, and a human-readable reason (for example, "path does not exist" or "permission denied").
109122

110123
Example:
111124

0 commit comments

Comments
 (0)