Re: [PATCH] selinux: Generalize support for NNP/nosuid SELinux domain transitions

[Stephen Smalley]

On Tue, 2017-07-18 at 09:17 -0400, Stephen Smalley wrote:
> On Mon, 2017-07-17 at 15:54 -0400, Paul Moore wrote:
> > On Sun, Jul 16, 2017 at 11:15 AM, Jason Zaman 
> > wrote:
> > > On Fri, Jul 14, 2017 at 12:46:47PM -0400, Stephen Smalley wrote:
> > > > As systemd ramps up enabling NNP (NoNewPrivileges) for system
> > > > services,
> > > > it is increasingly breaking SELinux domain transitions for
> > > > those
> > > > services
> > > > and their descendants.  systemd enables NNP not only for
> > > > services
> > > > whose
> > > > unit files explicitly specify NoNewPrivileges=yes but also for
> > > > services
> > > > whose unit files specify any of the following options in
> > > > combination with
> > > > running without CAP_SYS_ADMIN (e.g. specifying User= or a
> > > > CapabilityBoundingSet= without CAP_SYS_ADMIN):
> > > > SystemCallFilter=,
> > > > SystemCallArchitectures=, RestrictAddressFamilies=,
> > > > RestrictNamespaces=,
> > > > PrivateDevices=, ProtectKernelTunables=, ProtectKernelModules=,
> > > > MemoryDenyWriteExecute=, or RestrictRealtime= as per the
> > > > systemd.exec(5)
> > > > man page.
> > > > 
> > > > The end result is bad for the security of both SELinux-disabled
> > > > and
> > > > SELinux-enabled systems.  Packagers have to turn off these
> > > > options in the unit files to preserve SELinux domain
> > > > transitions.  For
> > > > users who choose to disable SELinux, this means that they miss
> > > > out on
> > > > at least having the systemd-supported protections.  For users
> > > > who
> > > > keep
> > > > SELinux enabled, they may still be missing out on some
> > > > protections
> > > > because it isn't necessarily guaranteed that the SELinux policy
> > > > for
> > > > that service provides the same protections in all cases.
> > > > 
> > > > commit 7b0d0b40cd78 ("selinux: Permit bounded transitions under
> > > > NO_NEW_PRIVS or NOSUID.") allowed bounded transitions under NNP
> > > > in
> > > > order to support limited usage for sandboxing
> > > > programs.  However,
> > > > defining typebounds for all of the affected service domains
> > > > is impractical to implement in policy, since typebounds
> > > > requires
> > > > us
> > > > to ensure that each domain is allowed everything all of its
> > > > descendant
> > > > domains are allowed, and this has to be repeated for the entire
> > > > chain
> > > > of domain transitions.  There is no way to clone all allow
> > > > rules
> > > > from
> > > > descendants to their ancestors in policy currently, and doing
> > > > so
> > > > would
> > > > be undesirable even if it were practical, as it requires
> > > > leaking
> > > > permissions to objects and operations into ancestor domains
> > > > that
> > > > could
> > > > weaken their own security in order to allow them to the
> > > > descendants
> > > > (e.g. if a descendant requires execmem permission, then so do
> > > > all
> > > > of
> > > > its ancestors; if a descendant requires execute permission to a
> > > > file,
> > > > then so do all of its ancestors; if a descendant requires read
> > > > to
> > > > a
> > > > symbolic link or temporary file, then so do all of its
> > > > ancestors...).
> > > > SELinux domains are intentionally not hierarchical / bounded in
> > > > this
> > > > manner normally, and making them so would undermine their
> > > > protections
> > > > and least privilege.
> > > > 
> > > > We have long had a similar tension with SELinux transitions and
> > > > nosuid
> > > > mounts, albeit not as severe.  Users often have had to choose
> > > > between
> > > > retaining nosuid on a mount and allowing SELinux domain
> > > > transitions on
> > > > files within those mounts.  This likewise leads to unfortunate
> > > > tradeoffs
> > > > in security.
> > > > 
> > > > Decouple NNP/nosuid from SELinux transitions, so that we don't
> > > > have to
> > > > make a choice between them. Introduce a nnp_nosuid_transition
> > > > policy
> > > > capability that causes the ability to transition under
> > > > NNP/nosuid
> > > > to
> > > > be based on a nnp_nosuid_transition permission between the old
> > > > and new
> > > > contexts rather than typebounds.  Domain transitions can then
> > > > be
> > > > allowed
> > > > in policy without requiring the parent to be a strict superset
> > > > of
> > > > all of
> > > > its children.
> > > > 
> > > > With this change, systemd unit files can be left unmodified
> > > > from
> > > > upstream.
> > > > SELinux-disabled and SELinux-enabled users will benefit from
> > > > retaining any
> > > > of the systemd-provided protections.  SELinux policy will only
> > > > need to
> > > > be adapted to enable the new policy capability and to allow the
> > > > new permission between domain pairs as appropriate.
> > > 
> > > A few things, the name is pretty awkward. my main issue with it
> > > is
> > > what
> > > if some other similar kind of thing is added in the future then
> > > we'll
> > > have nnp_nosuid_transition that should also cover 

ANN: SELinux userspace 2.7-rc5 release candidate

[Stephen Smalley]

A fifth release candidate for the SELinux userspace is now available
at:
https://github.com/SELinuxProject/selinux/wiki/Releases

Please give it a test and let us know if there are any issues.

Changes from the -rc4 release:

Stephen Smalley (4):
  open_init_pty: Do not make stdin and stdout non-blocking
  Revert "open_init_pty: Do not make stdin and stdout non-blocking"
  open_init_pty: restore stdin/stdout to blocking upon exit
  Update VERSION files for 2.7-rc5

Re: [PATCH] selinux: Generalize support for NNP/nosuid SELinux domain transitions

[Stephen Smalley]

On Mon, 2017-07-17 at 15:54 -0400, Paul Moore wrote:
> On Sun, Jul 16, 2017 at 11:15 AM, Jason Zaman 
> wrote:
> > On Fri, Jul 14, 2017 at 12:46:47PM -0400, Stephen Smalley wrote:
> > > As systemd ramps up enabling NNP (NoNewPrivileges) for system
> > > services,
> > > it is increasingly breaking SELinux domain transitions for those
> > > services
> > > and their descendants.  systemd enables NNP not only for services
> > > whose
> > > unit files explicitly specify NoNewPrivileges=yes but also for
> > > services
> > > whose unit files specify any of the following options in
> > > combination with
> > > running without CAP_SYS_ADMIN (e.g. specifying User= or a
> > > CapabilityBoundingSet= without CAP_SYS_ADMIN): SystemCallFilter=,
> > > SystemCallArchitectures=, RestrictAddressFamilies=,
> > > RestrictNamespaces=,
> > > PrivateDevices=, ProtectKernelTunables=, ProtectKernelModules=,
> > > MemoryDenyWriteExecute=, or RestrictRealtime= as per the
> > > systemd.exec(5)
> > > man page.
> > > 
> > > The end result is bad for the security of both SELinux-disabled
> > > and
> > > SELinux-enabled systems.  Packagers have to turn off these
> > > options in the unit files to preserve SELinux domain
> > > transitions.  For
> > > users who choose to disable SELinux, this means that they miss
> > > out on
> > > at least having the systemd-supported protections.  For users who
> > > keep
> > > SELinux enabled, they may still be missing out on some
> > > protections
> > > because it isn't necessarily guaranteed that the SELinux policy
> > > for
> > > that service provides the same protections in all cases.
> > > 
> > > commit 7b0d0b40cd78 ("selinux: Permit bounded transitions under
> > > NO_NEW_PRIVS or NOSUID.") allowed bounded transitions under NNP
> > > in
> > > order to support limited usage for sandboxing programs.  However,
> > > defining typebounds for all of the affected service domains
> > > is impractical to implement in policy, since typebounds requires
> > > us
> > > to ensure that each domain is allowed everything all of its
> > > descendant
> > > domains are allowed, and this has to be repeated for the entire
> > > chain
> > > of domain transitions.  There is no way to clone all allow rules
> > > from
> > > descendants to their ancestors in policy currently, and doing so
> > > would
> > > be undesirable even if it were practical, as it requires leaking
> > > permissions to objects and operations into ancestor domains that
> > > could
> > > weaken their own security in order to allow them to the
> > > descendants
> > > (e.g. if a descendant requires execmem permission, then so do all
> > > of
> > > its ancestors; if a descendant requires execute permission to a
> > > file,
> > > then so do all of its ancestors; if a descendant requires read to
> > > a
> > > symbolic link or temporary file, then so do all of its
> > > ancestors...).
> > > SELinux domains are intentionally not hierarchical / bounded in
> > > this
> > > manner normally, and making them so would undermine their
> > > protections
> > > and least privilege.
> > > 
> > > We have long had a similar tension with SELinux transitions and
> > > nosuid
> > > mounts, albeit not as severe.  Users often have had to choose
> > > between
> > > retaining nosuid on a mount and allowing SELinux domain
> > > transitions on
> > > files within those mounts.  This likewise leads to unfortunate
> > > tradeoffs
> > > in security.
> > > 
> > > Decouple NNP/nosuid from SELinux transitions, so that we don't
> > > have to
> > > make a choice between them. Introduce a nnp_nosuid_transition
> > > policy
> > > capability that causes the ability to transition under NNP/nosuid
> > > to
> > > be based on a nnp_nosuid_transition permission between the old
> > > and new
> > > contexts rather than typebounds.  Domain transitions can then be
> > > allowed
> > > in policy without requiring the parent to be a strict superset of
> > > all of
> > > its children.
> > > 
> > > With this change, systemd unit files can be left unmodified from
> > > upstream.
> > > SELinux-disabled and SELinux-enabled users will benefit from
> > > retaining any
> > > of the systemd-provided protections.  SELinux policy will only
> > > need to
> > > be adapted to enable the new policy capability and to allow the
> > > new permission between domain pairs as appropriate.
> > 
> > A few things, the name is pretty awkward. my main issue with it is
> > what
> > if some other similar kind of thing is added in the future then
> > we'll
> > have nnp_nosuid_transition that should also cover it but the name
> > wont
> > cover it.
> 
> Yes, I don't think anyone is in love with the name, but no one
> offered
> anything better; see the previous patch posting for some of that
> discussion.  If you have a better suggestion I would love to hear it.

Agreed, although I'm skeptical that there will be any "other similar
kind of thing" added in the future that we would want to cover under
the same 

[PATCH 2/2] open_init_pty: restore stdin/stdout to blocking upon exit

[Stephen Smalley]

At exit, restore stdin and stdout to blocking.

Test: run_init id && run_init id
Test: open_init_pty bash -c 'echo hello; exec >&- 2>&- <&-; sleep 1;'

Fixes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=863187
Fixes: https://bugs.gentoo.org/show_bug.cgi?id=621062
Signed-off-by: Stephen Smalley 
---
 policycoreutils/run_init/open_init_pty.c | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/policycoreutils/run_init/open_init_pty.c 
b/policycoreutils/run_init/open_init_pty.c
index 6e25ea3..150cb45 100644
--- a/policycoreutils/run_init/open_init_pty.c
+++ b/policycoreutils/run_init/open_init_pty.c
@@ -191,6 +191,28 @@ static void setfd_nonblock(int fd)
}
 }
 
+static void setfd_block(int fd)
+{
+   int fsflags = fcntl(fd, F_GETFL);
+
+   if (fsflags < 0) {
+   fprintf(stderr, "fcntl(%d, F_GETFL): %s\n", fd, 
strerror(errno));
+   exit(EX_IOERR);
+   }
+
+   if (fcntl(fd, F_SETFL, fsflags & ~O_NONBLOCK) < 0) {
+   fprintf(stderr, "fcntl(%d, F_SETFL, ... & ~O_NONBLOCK): %s\n", 
fd, strerror(errno));
+   exit(EX_IOERR);
+   }
+}
+
+static void setfd_atexit(void)
+{
+   setfd_block(STDIN_FILENO);
+   setfd_block(STDOUT_FILENO);
+   return;
+}
+
 static void sigchld_handler(int asig __attribute__ ((unused)))
 {
 }
@@ -280,6 +302,10 @@ int main(int argc, char *argv[])
setfd_nonblock(pty_master);
setfd_nonblock(STDIN_FILENO);
setfd_nonblock(STDOUT_FILENO);
+   if (atexit(setfd_atexit) < 0) {
+   perror("atexit()");
+   exit(EXIT_FAILURE);
+   }
 
if (isatty(STDIN_FILENO)) {
if (tty_semi_raw(STDIN_FILENO) < 0) {
-- 
2.9.4

[PATCH 1/2] Revert "open_init_pty: Do not make stdin and stdout non-blocking"

[Stephen Smalley]

Making stdin/stdout non-blocking causes open_init_pty to hang if
they are closed, ala
./open_init_pty bash -c 'echo hello; exec >&- 2>&- <&-; sleep 1; '
and per
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=474956#10

This reverts commit fb081eb64b36a9de5a43f3d69d9e628b6eb1afc7.

Reported-by: Laurent Bigonville 
Signed-off-by: Stephen Smalley 
---
 policycoreutils/run_init/open_init_pty.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/policycoreutils/run_init/open_init_pty.c 
b/policycoreutils/run_init/open_init_pty.c
index b37ae4d..6e25ea3 100644
--- a/policycoreutils/run_init/open_init_pty.c
+++ b/policycoreutils/run_init/open_init_pty.c
@@ -276,8 +276,10 @@ int main(int argc, char *argv[])
}
}
 
-   /* Non blocking mode for the pty master. */
+   /* Non blocking mode for all file descriptors. */
setfd_nonblock(pty_master);
+   setfd_nonblock(STDIN_FILENO);
+   setfd_nonblock(STDOUT_FILENO);
 
if (isatty(STDIN_FILENO)) {
if (tty_semi_raw(STDIN_FILENO) < 0) {
-- 
2.9.4

Re: [PATCH v2 2/8] exec: Move security_bprm_secureexec() earlier

[Kees Cook]

On Mon, Jul 10, 2017 at 7:07 PM, Kees Cook  wrote:
> On Mon, Jul 10, 2017 at 10:18 AM, Eric W. Biederman
>  wrote:
>> Kees Cook  writes:
>>
>>> On Mon, Jul 10, 2017 at 1:57 AM, Eric W. Biederman
>>>  wrote:
 Kees Cook  writes:

> There are several places where exec needs to know if a privilege-gain has
> happened. These should be using the results of security_bprm_secureexec()
> but it is getting (needlessly) called very late.

 It is hard to tell at a glance but I believe this introduces a
 regression.

 cap_bprm_set_creds is currently called before cap_bprm_secureexec and
 it has a number of cases such as no_new_privs and ptrace that can result
 in some of the precomputed credential changes not happening.

 Without accounting for that I believe your cap_bprm_securexec now
 returns a postive value too early.
>>>
>>> It's still before cap_bprm_secureexec. cap_brpm_set_creds() is in
>>> prepare_binprm(), which is well before exec_binprm() and it's eventual
>>> call to setup_new_exec().
>>
>> Good point.  I didn't double check and the set in the name had me
>> thinking it was setting the creds on current.
>>
>> Is there any reason we need a second security hook?  It feels like we
>> should be able to just fold the secureexec hook into the set_creds hook.
>>
>> The two are so interrelated I fear that having them separate only
>> encourages them to diverge in trivial ways as it is easy to forget about
>> some detail or other.
>>
>> Certainly having them called from different functions seems wrong.  If
>> we know enough in prepare_binprm we know enough.
>
> Hmmm, yes. That would let us have the secureexec-ness knowledge before
> copy_strings(), in case we ever need to make that logic
> secureexec-aware.
>
> I'll dig through the LSMs to examine the set_creds hooks to see if
> this could be possible.

So, yes, after digging, this is very possible. In fact, it's highly
desirable. Both commoncaps and AppArmor save state into the bprm
struct between bprm_set_creds and bprm_secureexec explicitly to return
a sane value from bprm_secureexec. (And Smack and SELinux both have
trivial tests that just repeat from bprm_set_creds.)

I've reworked the series to just remove bprm_secureexec entirely. It
comes out nicely, removing more than it adds:

 14 files changed, 54 insertions(+), 144 deletions(-)

I'll send the patches in the morning (perhaps to go through -mm since
it touches fs/exec.c, binfmt_elf*.c, and security/).

-Kees

-- 
Kees Cook
Pixel Security