* Re: Patch: Add support for chroot to Worker#user V2
2017-04-05 3:44 99% ` Eric Wong
@ 2017-04-05 4:57 99% ` Jeremy Evans
0 siblings, 0 replies; 6+ results
From: Jeremy Evans @ 2017-04-05 4:57 UTC (permalink / raw)
To: Eric Wong; +Cc: unicorn-public
On 04/05 03:44, Eric Wong wrote:
> Jeremy Evans <code@jeremyevans.net> wrote:
> > @@ -134,6 +136,11 @@ def user(user, group = nil)
> > Process.initgroups(user, gid)
> > Process::GID.change_privilege(gid)
> > end
> > + if chroot
> > + chroot = Dir.pwd if chroot == true
> > + Dir.chroot(chroot)
> > + Dir.chdir('/')
> > + end
>
> By the way, I noticed in configurator.rb (for
> working_directory), we also update ENV['PWD'] after chdir.
>
> Perhaps we should do so, here?
That makes sense to me, though I don't use ENV['PWD'] personally.
> For working_directory, I preserved ENV['PWD'] in case Dir.pwd
> wasn't aware of symlinks; or there's code which relies on
> env['PWD'] without caring for making the syscalls required for
> Dir.pwd...
>
> I'm not sure how much it matters in practice...
I'm not either, but I don't see how it could hurt.
Thanks,
Jeremy
^ permalink raw reply [relevance 99%]
* Re: Patch: Add support for chroot to Worker#user V2
2017-02-23 18:46 73% Patch: Add support for chroot to Worker#user V2 Jeremy Evans
@ 2017-04-05 3:44 99% ` Eric Wong
2017-04-05 4:57 99% ` Jeremy Evans
0 siblings, 1 reply; 6+ results
From: Eric Wong @ 2017-04-05 3:44 UTC (permalink / raw)
To: Jeremy Evans; +Cc: unicorn-public
Jeremy Evans <code@jeremyevans.net> wrote:
> @@ -134,6 +136,11 @@ def user(user, group = nil)
> Process.initgroups(user, gid)
> Process::GID.change_privilege(gid)
> end
> + if chroot
> + chroot = Dir.pwd if chroot == true
> + Dir.chroot(chroot)
> + Dir.chdir('/')
> + end
By the way, I noticed in configurator.rb (for
working_directory), we also update ENV['PWD'] after chdir.
Perhaps we should do so, here?
For working_directory, I preserved ENV['PWD'] in case Dir.pwd
wasn't aware of symlinks; or there's code which relies on
env['PWD'] without caring for making the syscalls required for
Dir.pwd...
I'm not sure how much it matters in practice...
^ permalink raw reply [relevance 99%]
* Patch: Add support for chroot to Worker#user V2
@ 2017-02-23 18:46 73% Jeremy Evans
2017-04-05 3:44 99% ` Eric Wong
0 siblings, 1 reply; 6+ results
From: Jeremy Evans @ 2017-02-23 18:46 UTC (permalink / raw)
To: unicorn-public
Here's V2 of the chroot support patch.
This changes the commit message language, and supports chrooting to
a directory that isn't the current directory.
From 9bd82792d57f54a868c9a0e9af2bd452f3ef298d Mon Sep 17 00:00:00 2001
From: Jeremy Evans <code@jeremyevans.net>
Date: Tue, 21 Feb 2017 08:44:34 -0800
Subject: [PATCH] Add support for chroot to Worker#user
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 adds an extra layer of security, so that if the unicorn
process is exploited, file system access is limited to the chroot
directory instead of the entire file system.
---
lib/unicorn/worker.rb | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/lib/unicorn/worker.rb b/lib/unicorn/worker.rb
index 6748a2f..e22c1bf 100644
--- a/lib/unicorn/worker.rb
+++ b/lib/unicorn/worker.rb
@@ -111,9 +111,11 @@ def close # :nodoc:
# In most cases, you should be using the Unicorn::Configurator#user
# directive instead. This method should only be used if you need
# fine-grained control of exactly when you want to change permissions
- # in your after_fork hooks.
+ # in your after_fork or after_worker_ready hooks, or if you want to
+ # use the chroot support.
#
- # 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 +125,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 +136,11 @@ def user(user, group = nil)
Process.initgroups(user, gid)
Process::GID.change_privilege(gid)
end
+ if chroot
+ chroot = Dir.pwd if chroot == true
+ Dir.chroot(chroot)
+ Dir.chdir('/')
+ end
Process.euid != uid and Process::UID.change_privilege(uid)
@switched = true
end
--
2.11.0
^ permalink raw reply related [relevance 73%]
* Re: Patch: Add support for chroot to Worker#user
2017-02-21 19:53 99% ` Eric Wong
@ 2017-02-21 20:15 98% ` Jeremy Evans
0 siblings, 0 replies; 6+ results
From: Jeremy Evans @ 2017-02-21 20:15 UTC (permalink / raw)
To: Eric Wong; +Cc: unicorn-public
On 02/21 07:53, Eric Wong wrote:
> Jeremy Evans <code@jeremyevans.net> 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
^ permalink raw reply [relevance 98%]
* Re: Patch: Add support for chroot to Worker#user
2017-02-21 19:24 76% Patch: Add support for chroot to Worker#user Jeremy Evans
@ 2017-02-21 19:53 99% ` Eric Wong
2017-02-21 20:15 98% ` Jeremy Evans
0 siblings, 1 reply; 6+ results
From: Eric Wong @ 2017-02-21 19:53 UTC (permalink / raw)
To: Jeremy Evans; +Cc: unicorn-public
Jeremy Evans <code@jeremyevans.net> 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.
> 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.
^ permalink raw reply [relevance 99%]
* Patch: Add support for chroot to Worker#user
@ 2017-02-21 19:24 76% Jeremy Evans
2017-02-21 19:53 99% ` Eric Wong
0 siblings, 1 reply; 6+ results
From: Jeremy Evans @ 2017-02-21 19:24 UTC (permalink / raw)
To: unicorn-public
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.
This is not a complete patch as it does not include tests. I can
add tests if you would consider accepting this feature.
---
lib/unicorn/worker.rb | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/lib/unicorn/worker.rb b/lib/unicorn/worker.rb
index 6748a2f..db418ea 100644
--- a/lib/unicorn/worker.rb
+++ b/lib/unicorn/worker.rb
@@ -111,9 +111,10 @@ def close # :nodoc:
# In most cases, you should be using the Unicorn::Configurator#user
# directive instead. This method should only be used if you need
# fine-grained control of exactly when you want to change permissions
- # in your after_fork hooks.
+ # in your after_fork hooks, or if you want to use the chroot support.
#
- # 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
Process.euid != uid and Process::UID.change_privilege(uid)
@switched = true
end
--
2.11.0
^ permalink raw reply related [relevance 76%]
Results 1-6 of 6 | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2017-02-21 19:24 76% Patch: Add support for chroot to Worker#user Jeremy Evans
2017-02-21 19:53 99% ` Eric Wong
2017-02-21 20:15 98% ` Jeremy Evans
2017-02-23 18:46 73% Patch: Add support for chroot to Worker#user V2 Jeremy Evans
2017-04-05 3:44 99% ` Eric Wong
2017-04-05 4:57 99% ` Jeremy Evans
Code repositories for project(s) associated with this public inbox
https://yhbt.net/unicorn.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).