vfs: guard against altroot pointing to not a directory pwd_altroot(): avoid creating new pwd structure if altroot does not change chaltroot(2): new syscall to change the altroot AKA ABI root for native processes chaltroot(2): make accessible to userspace
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
This is a proposal for now, I want to discuss do we want this feature exposed to the native processes at all.
I thought about putting it under e.g. procctl(2), but it seems to not be a natural place.
I believe its immediate use is to allow much easier testing of the namei(9) changes around altroot behavior, in particular for the fixes dteske@ is working on in D58855 and D59990.
On the other hand, it is not obvious if it is useful for something else, but who knows where users end up with.
The changes to pwd_altroot(9) itself could be considered as an independent bug fixes anyway.
Comment Actions
Yes, expose it. D59990 can configure the overlay from a native process, dropping the linux_base skip.
pwd_altroot fixes look right on their own (ENOTDIR, no-op when adir is unchanged, error back to linux_pwd_onexec).
Issue: privilege it like chroot. sys_chaltroot has no priv_check. pwd_adir survives native exec (only linux_pwd_onexec_native clears it), so an unprivileged chaltroot then exec of a setuid helper overlays that helper's absolute lookups. Same reason chroot is not free.
Agree that procctl is the wrong home; this belongs next to chroot.
D59990 can stay as-is until this lands. Happy to retarget the tests onto chaltroot after.
Comment Actions
I think that the linux tests should go regardless of this proposal. If chaltroot(2) goes in, then more tests can be added that utilize the native interface. But the linux-ABI tests are really independent from this and are useful on its own.
pwd_altroot fixes look right on their own (ENOTDIR, no-op when adir is unchanged, error back to linux_pwd_onexec).
Issue: privilege it like chroot. sys_chaltroot has no priv_check. pwd_adir survives native exec (only linux_pwd_onexec_native clears it), so an unprivileged chaltroot then exec of a setuid helper overlays that helper's absolute lookups. Same reason chroot is not free.
Hmm, I thought that it would be enough to drop altroot for native if setugid binary is executed, but might be you are correct and altroot should be treated same as chroot. In principle it allows the same actions to be taken as chroot, and trying to formulate rules that would prevent that from abusing is too risky.
So I will add the same permission checks as for chroot.
Agree that procctl is the wrong home; this belongs next to chroot.
D59990 can stay as-is until this lands. Happy to retarget the tests onto chaltroot after.
No, please go ahead and commit as is.
Comment Actions
Extract chroot_unpriv() from kern_chroot.
Use the same privilege checks for chaltroot(2) as for chroot(2).
Comment Actions
Privilege check matches. Thanks for adding that.
Issue: pwd_altroot no-op leaks newpwd. Alloc is unconditional; when altroot_vp == oldpwd->pwd_adir the else nullifies oldpwd and the unused newpwd is never dropped.
MAC: kern_chroot still does mac_vnode_check_chroot; sys_chaltroot does not, even when path is not NULL.
Comment Actions
Fixed, thanks.
MAC: kern_chroot still does mac_vnode_check_chroot; sys_chaltroot does not, even when path is not NULL.
There is no mac callback for chaltroot. I am not sure that we need it, perhaps yes. Also there is no audit event allocated for chaltroot. As well as no man page.
I want to get some more feedback.
Comment Actions
Bug fixes for disabling altroot mode:
- make chroot -A not bail with an error
- avoid NULL deref in MAC hook
Comment Actions
Neat feature, what is the motivation?
Could you please modify kern_proc_filedesc_out() and procstat to show the altroot in procstat files output? I am not sure whether the altroot should be shown unconditionally, or only if it is set. Probably the latter?
I do not see the point in introducing a MAC hook if nothing is using it, but ok, it may save someone some time later.
| lib/libsys/chroot.2 | ||
|---|---|---|
| 110 | ||
| 111 | I do not see where this check for executable permissions occurs, BTW. For plain chroot it is done in change_dir(). | |
| 112 | This applies only when the path is absolute, I presume? | |
| 114 | ||
| 117 | ||
| 119 | ||
| 130 | What about jail_attach()? I would expect that to clear the altroot. What about setuid executables? Suppose an application calls chaltroot() and then drops its privileges. It can then invoke setuid programs with the altroot set. If the application can write to the altroot somehow, then it can trivially regain its privileges, e.g., by loading an alternate DSO. I believe altroot is set automatically for Linux executables, right? What happens if a native application calls chaltroot() and then execs a Linux binary? | |
| 133 | ||
| 134 | ||
| usr.sbin/chroot/chroot.c | ||
| 110–111 | You did not initialize clear_altroot. | |
Comment Actions
It came out from the discussions in D58855, I realized that it is useful at least for testing. But might be it is useful for more things. We have it so why not expose? We support that for very long time for linuxolator.
Could you please modify kern_proc_filedesc_out() and procstat to show the altroot in procstat files output? I am not sure whether the altroot should be shown unconditionally, or only if it is set. Probably the latter?
Ok, will do.
I do not see the point in introducing a MAC hook if nothing is using it, but ok, it may save someone some time later.
It is only for completeness, same as the audit id for the syscall. If somebody can write a policy for chroot(), then he should be able to write a policy for chaltroot().
| lib/libsys/chroot.2 | ||
|---|---|---|
| 111 | Well, it is the regular lookup rules. I can remove the 'executable' part. | |
| 130 |
It does not, it sets altroot to the chroot/new root. See pwd_chroot_chdir(). The behavior is not changed from the existing linuxolator case.
Ok.
The altroot is set to the linuxolator alternative path, as it IMO should happen. | |
| lib/libsys/chroot.2 | ||
|---|---|---|
| 110 | ||
| 111 | The man page implies that chaltroot /foo/bar/baz requires execute permission for /foo/bar/baz, but name lookup only requires execute permissions for /foo and /foo/bar. So I think you need to explicitly check, like regular chroot() does. | |
| 112 | ||
| 113 | ||
| 119 | I would not mention chroot specifically, the process root directory can be modified by other means. Maybe, "a second lookup is automatically initiated, starting from the process' true root directory." | |
| 128 | ||
| 129 | ||
| 130 |
The name lookup code checks pwd_adir != pwd_rdir to decide if a restart is needed, so we should avoid setting it to NULL.
This should be documented. | |
| 132 | ||
| 135 | ||
| 144 | ||
| 149 | I wonder if we should use a more obscure value instead, since -1 is a common sentinel value. Maybe AT_FDCWD or similar. | |
| 151 | ||
| 157 | ||
| sys/kern/kern_descrip.c | ||
| 4345 | Isn't the type check racy? We do not hold the vnode lock here. | |
| sys/kern/kern_descrip.c | ||
|---|---|---|
| 4345 | It is but why would it matter? But I added the locking of the vnode for VACCESS() call, so it is 'fixed'. | |