Page MenuHomeFreeBSD

namei: Make stackable filesystems check harder for jail roots
ClosedPublic

Authored by markj on Mon, May 19, 3:59 PM.
Tags
None
Referenced Files
F118966856: D50418.id155913.diff
Tue, Jun 3, 9:04 PM
Unknown Object (File)
Sat, May 31, 7:02 AM
Unknown Object (File)
Wed, May 28, 8:11 AM
Unknown Object (File)
Sat, May 24, 11:30 AM
Unknown Object (File)
Sat, May 24, 5:57 AM
Unknown Object (File)
Sat, May 24, 2:14 AM
Unknown Object (File)
Sat, May 24, 1:03 AM
Unknown Object (File)
Fri, May 23, 1:09 PM
Subscribers

Details

Summary

Suppose a process has its cwd pointing to a nullfs directory. Suppose
that the lower directory vnode is moved out from under the nullfs mount.
The nullfs vnode still shadows the lower vnode, and dotdot lookups
relative to that directory will instantiate new nullfs vnodes outside of
the nullfs mountpoint, effectively shadowing the lower filesystem.

This trick can be abused to escape a chroot, since the nullfs vnodes
instantiated by these dotdot lookups defeat the root vnode check in
vfs_lookup(), which uses vnode pointer equality to test for the process
root.

Fix this by extending nullfs and unionfs to perform the same check,
exploiting the fact that the passed componentname is embedded in a
nameidata structure to avoid changing the VOP_LOOKUP interface. That
is, add a flag to indicate that containerof can be used to get the full
nameidata structure, and perform the root vnode check on the lower vnode
when performing a dotdot lookup.

PR: 262180

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 64292
Build 61176: arc lint + arc unit

Event Timeline

markj requested review of this revision.Mon, May 19, 3:59 PM

I appreciate the single vfs_lookup_isroot function - it took me an embarrassingly long time to discover a second copy of that code had appeared in vfs_cache.c a mere five years ago.

A question for the uninitiated about the NAMEILOOKUP flag: what other context are names looked up in, that don't have nameidata?

I appreciate the single vfs_lookup_isroot function - it took me an embarrassingly long time to discover a second copy of that code had appeared in vfs_cache.c a mere five years ago.

A question for the uninitiated about the NAMEILOOKUP flag: what other context are names looked up in, that don't have nameidata?

There are some VOP_LOOKUP calls in individual filesystems which manually fill out a componentname: zfs_fhtovp() contains a couple of examples. I'm not yet completely convinced that it's safe to elide all of those cases, but I'm pretty sure it is.

The check is present for nullfs but not unionfs here. Could you rather wrap the __containerof(cnp, struct nameidata, ni_cnd) into a function that will check for NAMEILOOKUP positioned, and will return NULL if it is not?

sys/fs/nullfs/null_vnops.c
404–412

This comment really applies only to the VV_ROOT case, so I'd move it closer to the if ((ldvp->v_vflag & VV_ROOT) != 0) line.

kib added inline comments.
sys/kern/vfs_lookup.c
868

Please add {} around the body.

This revision is now accepted and ready to land.Mon, May 19, 10:34 PM
sys/sys/namei.h
155

Please keep namei.9 up to date.

olce requested changes to this revision.Tue, May 20, 10:02 AM

Just to be sure that my comment is not lost:

The check is present for nullfs but not unionfs here. Could you rather wrap the __containerof(cnp, struct nameidata, ni_cnd) into a function that will check for NAMEILOOKUP positioned, and will return NULL if it is not?

The change is not conceptually correct for unionfs.

This revision now requires changes to proceed.Tue, May 20, 10:02 AM
markj added inline comments.
sys/fs/nullfs/null_vnops.c
404–412

Most of the comment applies to both cases, so I just tweaked it slightly instead.

markj marked an inline comment as done.

Handle review comments:

  • update namei.8
  • use a helper function to extract nameidata
This revision is now accepted and ready to land.Thu, May 22, 3:56 PM