Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Securely create jails #57

Closed
skarekrow opened this issue Sep 1, 2017 · 12 comments · Fixed by #97
Closed

Securely create jails #57

skarekrow opened this issue Sep 1, 2017 · 12 comments · Fixed by #97
Assignees
Labels

Comments

@skarekrow
Copy link
Contributor

skarekrow commented Sep 1, 2017

iocage/iocage#347 was the original issue, iocage/iocage@54583ed commit that fixed it in iocage.

@gronke
Copy link
Member

gronke commented Sep 2, 2017

@croquagei posted:

A non root host user could conspire with a jailed root user to escalate privilege.

Requirement

  • Non-privileged users on the host must not be able to access filesystems that a jail ever had write access to

Possible Mitigations

A.) Directory mode 0700 on the jails dataset
B.) Directory mode 0700 on the iocage base dataset
C.) ZFS property setuid ?

@gronke gronke added the question label Sep 2, 2017
@skarekrow
Copy link
Contributor Author

I suggest D.) Directory mode 0700 on the jails parent dataset which is how iocage is currently doing it.

The reason I suggest that over A is a user may (like @rwestlund) want privileges to be more relaxed between jails and the host, and if we use A, that's a blanket approach. There is no fine-grain tuning.

So A, B are completely out of the question in my opinion as it will prevent users from doing what they want in the name of security. Which they should be able to decide ultimately, we just want a sane default per jail.

C doesn't work as then the jail cannot execute anything ;)

TLDR; This is over complicating the problem, just make each iocage/jails/JAIL 0700 by default and the user can manually change if they wish.

@skarekrow
Copy link
Contributor Author

skarekrow commented Sep 8, 2017

As I said in iocage/iocage#347, this mitigation comes with some serious repercussions that would require a whole redesign in iocage. It just so happens that kind of thing can happen here!

We should move all jail configuration to a common dataset, iocage/configs, let that be r/w/x, and squash all jails to a single jail dataset that will take the place of /root. That way we can securely implement this without breaking any functionality. We should apply the 700 perms to the base jails dataset at this point, as applying it to the jails root will break execution inside the jail. The chroot still needs to be rx.

The current tree looks similar to this:

iocage/
  jails/
    FOO/ <-- 700 perms is the right solution, but breaks anything wanting readonly to configuration
      root/

It should look like this:

iocage/
  configs/
    FOO/
  jails/ <-- 700 perms since everything underneath is actually a root dataset now.
    FOO/

EDIT: I just realized that would introduce the fine-grain tuning issue that A wanted, so my edited suggestion is this, we keep the current structure for security and fine-grain tuning, but the datasets are actually empty, as all the fstab, config.json, etc are really in configs/JAIL:

iocage/
  configs/
    FOO/
  jails/
    FOO/
      root/ <-- 700 perms

@rwestlund
Copy link

@skarekrow I think you're over-complicating it. I don't need to mix secure and insecure jails on the same host, and I can't imagine why anyone would; an insecure jail is only safe when there are no untrusted users, in which case you don't also need secure jails. Setting 0700 on jails/ seems reasonable (and simple) to me.

I like the idea of not needing root/ under each jail. But if we separate jails and configs, then we lose the ability to atomically snapshot a complete jail. Unless we can store the diff between the default config and a jail's config in zfs properties, like the legacy iocage did. That was nice.

@skarekrow
Copy link
Contributor Author

Unless we separate the configurations, we either have to trade the atomic snapshots, or the security of the jail. I'm not sure which is the better trade.

@rwestlund
Copy link

Why is it necessary for an unprivileged user to be able to access the jail's config files?

@skarekrow
Copy link
Contributor Author

@rwestlund for unprivileged access to iocage list and get which reads those files. We could require root to run iocage at all but that seems overkill.

@gronke
Copy link
Member

gronke commented Sep 9, 2017

It does not make sense to me to give unprivileged users access to iocage while this access can be used to escalate privileges. I think it's reasonable to say that only root may access this directory.

@skarekrow can you share a user scenario where it's mandatory to access jail properties from a non-privileged shell account?

Although if there's a reason to grant access to unprivileged users users, I'd give a group access to /iocage and/or specific jails. This comes with bad security implications and should be mentioned in the documentation, but not be the default.

@skarekrow
Copy link
Contributor Author

@gronke I don't like the groups idea. I would much rather separate the configuration like I suggested. It's clean and to the user an invisible change unless they care about configuration location.

I have a couple examples, so I'll repeat them again. list and get are two functions that rely on configuration. It's unreasonable to suggest root for those when they are read only operations.

@rwestlund
Copy link

cat /etc/master.password is also a read-only operation ;)

On a more serious note:

With separate configs, a zfs rollback on a jail will result in pairing an older jail with a newer configuration, which is likely to cause breakage. IMHO, The ability to atomically snapshot and rollback jail+config has always been iocage's best feature.

Until we find a better solution, it looks like one of these three features has to go:

  • Atomic snapshot and rollback
  • Secure (0700) jail permissions
  • Running iocage list as an unprivileged user

From where I stand, the third seems much less important than the other two.

@skarekrow
Copy link
Contributor Author

Yeah I've always viewed it as iocages best feature as well. I reverted the commit in iocage as I didn't feel people would appreciate having to run a tool as root for everything. If I'm wrong, I could certainly revert that.

@gronke
Copy link
Member

gronke commented Sep 11, 2017

I'm in favor of dropping the ability to run iocage as unprivileged user. There was some discussion around an iocage daemon or proxy script, which seem more suitable to grant non-root users access to specific iocage properties and controls.

Btw. why don't we just set 0700 on the top levle iocage dataset?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants