chaltroot(2)

[kib]

Summary

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

[kib]

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.

[dteske]

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.

[kib]

In D60035#1377627, @dteske wrote:

Yes, expose it. D59990 can configure the overlay from a native process, dropping the linux_base skip.

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.

[kib]

Extract chroot_unpriv() from kern_chroot.
Use the same privilege checks for chaltroot(2) as for chroot(2).

[dteske]

In D60035#1377674, @kib wrote:

Extract chroot_unpriv() from kern_chroot.
Use the same privilege checks for chaltroot(2) as for chroot(2).

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.

[kib]

In D60035#1377863, @dteske wrote:

In D60035#1377674, @kib wrote:

Extract chroot_unpriv() from kern_chroot.
Use the same privilege checks for chaltroot(2) as for chroot(2).

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.

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.

[kib]

Plug newpwd leak.

[kib]

Add fchaltroot(2)
Add mac callback
Documentation

[kib]

Teach chroot(8) about altroot.

[kib]

Bug fixes for disabling altroot mode:

• make chroot -A not bail with an error
• avoid NULL deref in MAC hook

[markj]

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.

[kib]

In D60035#1379038, @markj wrote:

Neat feature, what is the motivation?

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

What about jail_attach()? I would expect that to clear the altroot.

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.
Might be pwd_adir should be set to NULL instead?

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.

Ok.

I believe altroot is set automatically for Linux executables, right? What happens if a native application calls chaltroot() and then execs a Linux binary?

The altroot is set to the linuxolator alternative path, as it IMO should happen.

[kib]

Drop altroot for setid native binaries.
Man page fixes and clarifications.

[kib]

Teach procstat(1), fstat(1), and fuser(1) about altroot.

[markj]

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

Might be pwd_adir should be set to NULL instead?

The name lookup code checks pwd_adir != pwd_rdir to decide if a restart is needed, so we should avoid setting it to NULL.

The altroot is set to the linuxolator alternative path, as it IMO should happen.

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

4402

Isn't the type check racy? We do not hold the vnode lock here.

[kib]

sys/kern/kern_descrip.c

4402

It is but why would it matter?
The type can only change from VDIR to VBAD.

But I added the locking of the vnode for VACCESS() call, so it is 'fixed'.

[kib]

Change -1 to FCHALTROOT_DISABLE
Check VACCESS(EXEC) for altroot
Man page editings.

[kib]

Ping?

[dteske]

In D60035#1390426, @kib wrote:

Ping?

Was that to me, or markj? Or just in-general?

[kib]

In D60035#1390429, @dteske wrote:

In D60035#1390426, @kib wrote:

Ping?

Was that to me, or markj? Or just in-general?

Most specifically to markj, since the changes were to fix his findings.

[markj]

Could you please add some tests?

lib/libsys/chroot.2

113

sys/kern/kern_descrip.c

4390–4394

Please add a comment explaining what altroot_vp == NULL means.

sys/kern/vfs_syscalls.c

5424

Duplicate fdrop() call.

[kib]

In D60035#1390556, @markj wrote:

Could you please add some tests?

I believe dteske@ will add converted tests from b98d169d1f910e0b28c7

[kib]

Drop duplicate fdrop().
Update herald comment for pwd_altroot().

[dteske]

In D60035#1390644, @kib wrote:

In D60035#1390556, @markj wrote:

Could you please add some tests?

I believe dteske@ will add converted tests from b98d169d1f910e0b28c7

Yes, once this lands, happy to update my tests to use chaltroot