jail: add allow.mount.unsafe.all to allow mounting any filesystem
AcceptedPublic
Actions

Authored by oshogbo on Tue, May 12, 1:36 PM.

Details

Reviewers

mckusick
kib
jamie

Summary

Add a new jail parameter allow.mount.unsafe.all that allows mounting
any filesystem type. This is useful for build jails or development
environments where the jail root user is trusted. It is especially
useful when we want to restric the /dev visibility inside the jail,
when working on some scripts and we dont want to accidentally
overwritten other disks.

Most file systems trust their on-disk format and are not hardened
against malicous input (unlike, for example, allow.mount.zfs or
allow.mount.nullfs), which might lead to security issues. The
"unsafe" naming makes this risk explicit.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 73093
Build 69976: arc lint + arc unit

Event Timeline

oshogbo created this revision.Tue, May 12, 1:36 PM

Herald added a subscriber: imp. · View Herald TranscriptTue, May 12, 1:36 PM

oshogbo requested review of this revision.Tue, May 12, 1:36 PM

Harbormaster completed remote builds in B73028: Diff 177652.Tue, May 12, 1:36 PM

oshogbo added reviewers: mckusick, kib.Tue, May 12, 1:40 PM

UFS cannot be used like this. The code trusts the on-disk format, and malicious jail user could cause almost everything to kernel code, like arbitrary memory overwrite.

I see your point.

However, I have a use case where I'm building a UFS image inside a jail using md devices, etc., and I don't want to expose the entire system to that subsystem. I'd prefer to limit its visibility to disks and related resources. Not because I don't the generated filesystem, but mainly to reduce the risk of accidental user mistakes.

I was thinking this kind of flag could follow an "enable at your own risk" model.
Do you think it we can change this and add flag like a dangerous_allow_mount_ufs flag?

Docker on Linux has a similar concept with the --privileged flag, which allows users to shoot themselves in the foot if they choose to.

Might be, instead of adding per-filesystem VFCF_JAIL flag, add one single flag allowing user-mounting anything. Then document it properly.

oshogbo retitled this revision from ufs: allow mounting UFS inside jails to jail: add allow.mount.unsafe.all to allow mounting any filesystem.Thu, May 14, 8:40 AM

Introduce the single flag allowing mounting anything.

Herald added subscribers: ziaee, olce. · View Herald TranscriptThu, May 14, 8:42 AM

Harbormaster completed remote builds in B73093: Diff 177822.Thu, May 14, 8:42 AM

oshogbo added a reviewer: jamie.Thu, May 14, 8:44 AM

oshogbo edited the summary of this revision. (Show Details)

kib accepted this revision.Thu, May 14, 9:16 PM

kib added inline comments.

sys/sys/jail.h
284	This is really awful. Can it be converted to the (long) or-ed line of symbolic names?

This revision is now accepted and ready to land.Thu, May 14, 9:16 PM

There's no need for the two-layer name "unsafe.all". If you really want both "unsafe" and "all" in the name allow.mount.unsafe_all should do. Better yet would be to keep is simple with allow.mount.all to allow all filesystems, with the understanding that such a thing might be unsafe. As it stands, there's this "allow.mount.unsafe" hierarchy, which suggests that allowing all filesystem types is unsafe, which each of those filesystem types is implicitly labeled as safe.

My thinking was that we might later discover the feature needs more granularity, and therefore introduce options like:

allow.mount.unsafe.all
allow.mount.unsafe.ufs
allow.mount.unsafe.zfs
ect.

In my opinion its more elegant.

allow.mount.all to allow all filesystems

I would prefer to keep it explicit that this is unsafe and that you are potentially shooting yourself in the foot, since allow.mount.all suggests this is a normal or recommended operation.