google / google/nsjail

mnt: new-API path silently drops nosuid/nodev/noexec on nested submounts if the recursive mount_setattr fails

Open
#312 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
4.1k
Forks
371
Avg merge
4d 7h
Merged PRs (30d)
7

Description

## Summary

In the new mount-API path, the per-mount `nosuid`/`nodev`/`noexec` attributes of a recursive bind are applied with a single `mount_setattr()` call whose result is discarded (warning only). The later remount pass re-applies flags with a non-recursive `mount(2)` (top mount only), so if `mount_setattr()` fails, nested submounts of the cloned subtree stay suid/dev/exec-permitting inside the jail — including for mandatory mount points.

## Details (at f100fd9)

- `doBindMountAt()` clones the whole source subtree with `open_tree(..., OPEN_TREE_CLONE | AT_RECURSIVE)` and applies the requested flags via `applyMountFlags(..., recursive=true)`; the return value is only logged (`mnt_newapi.cc:497-499`).
- The final remount goes through `remountWithLegacyMount()` → `mount(dst, dst, MS_REMOUNT|MS_BIND|...)` without `MS_REC` — top mount only (`mnt_newapi.cc:170-186`; `computeLegacyRemountFlags()` adds no `MS_REC`).
- `remountPt()` therefore "succeeds", and `init()` only aborts when a *mandatory* mount's remount fails (`mnt.cc:330-332`) — a failed recursive `mount_setattr()` never surfaces.
- The recursive read-only pass on the jail root is not an equivalent backstop: it only sets `MOUNT_ATTR_RDONLY` (`mnt_newapi.cc:745-754`), so nosuid/nodev/noexec have no second recursive layer.
- The same discarded-return pattern exists for dynamic-content mounts (`mnt_newapi.cc:456-458`) and fs-mounts (`mnt_newapi.cc:568-570`).

## Impact

A config like "recursive-bind /usr with nosuid" over a source subtree containing a nested mount leaves that nested mount without nosuid inside the jail when `mount_setattr()` fails (fs-specific restrictions, permissions). A setuid binary on the nested mount stays setuid-effective for jailed processes. To be clear: this is fail-open hardening behavior rather than an exploitable bug — I did not find a way to force `mount_setattr()` to fail on a normal system.

## Suggested direction

Make `applyMountFlags()` failure fatal, at least for mandatory mount points, consistent with the recent fail-closed hardening (#306, #310, and #284 which introduced the recursive application). Happy to send a patch if the direction sounds right — equally fine to close as by-design best-effort.

Contributor guide

Open the contributing guide

Research direction

Start with applyMountFlags() and its call sites in mnt_newapi.cc:456-458, 497-499, and 568-570, then trace remountWithLegacyMount() and init() in mnt_newapi.cc and mnt.cc:330-332. Review the fail-closed precedent in issues #306, #310, and #284 and determine the expected handling for mandatory versus non-mandatory mounts. Done means recursive flag-application failures are handled consistently and nested mounts cannot silently lose the requested hardening.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, linux
Domain
operating-systems, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.