On 02/21 07:53, Eric Wong wrote: > Jeremy Evans <[email protected]> wrote: > > Any chrooting would need to happen inside Worker#user, because > > you can't chroot until after you have parsed the list of groups, > > and you must chroot before dropping root privileges. > > > > chroot is an important security feature, so that if the unicorn > > process is exploited, file system access is limited to the chroot > > directory instead of the entire system. > > I'm hesitant to document this as "an important security feature". > > Perhaps "an extra layer of security" is more accurate, > as there are ways of breaking out of a chroot.
That's fine. Note that while breaking out of a chroot is easy if you have root privileges, it is difficult to break out of a chroot if root privileges have been dropped. There are a couple of possibilities listed at https://github.com/earthquake/chw00t, neither of which is likely in environments where unicorn is typically deployed. > > > This is not a complete patch as it does not include tests. I can > > add tests if you would consider accepting this feature. > > I wouldn't worry about automated tests if elevated privileges > are required. > > > - # Changes the worker process to the specified +user+ and +group+ > > + # Changes the worker process to the specified +user+ and +group+, > > + # and chroots to the current working directory if +chroot+ is set. > > # This is only intended to be called from within the worker > > # process from the +after_fork+ hook. This should be called in > > # the +after_fork+ hook after any privileged functions need to be > > @@ -123,7 +124,7 @@ def close # :nodoc: > > # directly back to the caller (usually the +after_fork+ hook. > > # These errors commonly include ArgumentError for specifying an > > # invalid user/group and Errno::EPERM for insufficient privileges > > - def user(user, group = nil) > > + def user(user, group = nil, chroot = false) > > # we do not protect the caller, checking Process.euid == 0 is > > # insufficient because modern systems have fine-grained > > # capabilities. Let the caller handle any and all errors. > > @@ -134,6 +135,10 @@ def user(user, group = nil) > > Process.initgroups(user, gid) > > Process::GID.change_privilege(gid) > > end > > + if chroot > > + Dir.chroot(Dir.pwd) > > + Dir.chdir('/') > > + end > > Perhaps this can be made more flexible by allowing chroot to > any specified directory, not just pwd. Something like: > > chroot = Dir.pwd if chroot == true > if chroot > Dir.chroot(chroot) > Dir.chdir('/') > end > > Anyways I'm inclined to accept this feature. I had basically the same code originally, but wanted to minimize the API surface to increase the likelihood for acceptance. So I'm definitely in favor of that change. Thanks, Jeremy -- unsubscribe: [email protected] archive: https://bogomips.org/unicorn-public/
