[PATCH] Move settig the SELinux context into a privileged child process

[Jakub Hrozek]

Hi,

this patchset moves setting the SELinux context from the sssd_be process
to a privileges child process in preparation for making the sssd_be process
running as the sssd user.

The first two patches just reduce code duplication between the child
processes we already have, the last one implements setting the context.

Please note that the Fedora and RHEL SELinux policy must be tweaked, so
currently the patches must be tested with SELinux set to Permissive.

Additionally, I wasn't able to run Coverity scan on these patches, because
our Coverity server was returning errors again for me. I'll run the tests
later, or, if the reviewer would find any issues, I'll be happy to fix them.

[Jakub Hrozek]

On Fri, Oct 24, 2014 at 10:57:02PM +0200, Jakub Hrozek wrote:
> Hi,
>
> this patchset moves setting the SELinux context from the sssd_be process
> to a privileges child process in preparation for making the sssd_be process
> running as the sssd user.
>
> The first two patches just reduce code duplication between the child
> processes we already have, the last one implements setting the context.
>
> Please note that the Fedora and RHEL SELinux policy must be tweaked, so
> currently the patches must be tested with SELinux set to Permissive.
>
> Additionally, I wasn't able to run Coverity scan on these patches, because
> our Coverity server was returning errors again for me. I'll run the tests
> later, or, if the reviewer would find any issues, I'll be happy to fix them.

Hi,

attached patches change the packaging a bit -- make distcheck didn't
pass previously and I forgot to package selinux_child in the RPM.

[Michal Židek]

On 10/31/2014 05:59 PM, Jakub Hrozek wrote:
> On Fri, Oct 24, 2014 at 10:57:02PM +0200, Jakub Hrozek wrote:
> > > Hi,
> > >
> > > this patchset moves setting the SELinux context from the sssd_be process
> > > to a privileges child process in preparation for making the sssd_be process
> > > running as the sssd user.
> > >
> > > The first two patches just reduce code duplication between the child
> > > processes we already have, the last one implements setting the context.
> > >
> > > Please note that the Fedora and RHEL SELinux policy must be tweaked, so
> > > currently the patches must be tested with SELinux set to Permissive.
> > >
> > > Additionally, I wasn't able to run Coverity scan on these patches, because
> > > our Coverity server was returning errors again for me. I'll run the tests
> > > later, or, if the reviewer would find any issues, I'll be happy to fix them.
>
> Hi,
>
> attached patches change the packaging a bit -- make distcheck didn't
> pass previously and I forgot to package selinux_child in the RPM.
>
> 0001-UTIL-Remove-code-duplication-of-struct-io.patch

Ack.
> 0002-UTIL-Remove-more-code-duplication-setting-up-child-p.patch

Ack.
> 0003-IPA-Move-setting-the-SELinux-context-to-a-child-proc.patch

The chgrp and chmod in Makefile.am should probably be called
only if the semanage is available:
--- a/Makefile.am
+++ b/Makefile.am
@@ -2852,10 +2852,12 @@ endif

if SSSD_USER
    chgrp $(SSSD_USER) $(sssdlibexecdir)/ldap_child
-       chgrp $(SSSD_USER) $(sssdlibexecdir)/selinux_child
         chmod 4750 $(sssdlibexecdir)/ldap_child
+if BUILD_SEMANAGE
+       chgrp $(SSSD_USER) $(sssdlibexecdir)/selinux_child
         chmod 4750 $(sssdlibexecdir)/selinux_child
  endif
+endif

---
In src/providers/ipa/ipa_selinux.c, the line 405 is too long:
  405     /* Update the SELinux context ...
  406      * unprivileged
  407      */

---
Also in src/providers/ipa/ipa_selinux.c, the indentation
on line 872 should be aligned with the line above:
  871     if (sci->seuser == NULL || sci->mls_range == NULL
  872             || sci->username == NULL) {

Should be:
  871     if (sci->seuser == NULL || sci->mls_range == NULL
  872         || sci->username == NULL) {
(Or at least I am doing it like this, and it is style that
was proposed IIRC by Stephen in some old discussion).

---

Otherwise the patch looks good to me and the selinux
context is retrieved properly.

Michal

[Jakub Hrozek]

On Fri, Oct 31, 2014 at 10:48:09PM +0100, Michal Židek wrote:
> On 10/31/2014 05:59 PM, Jakub Hrozek wrote:
> > On Fri, Oct 24, 2014 at 10:57:02PM +0200, Jakub Hrozek wrote:
> > > > Hi,
> > > >
> > > > this patchset moves setting the SELinux context from the sssd_be process
> > > > to a privileges child process in preparation for making the sssd_be process
> > > > running as the sssd user.
> > > >
> > > > The first two patches just reduce code duplication between the child
> > > > processes we already have, the last one implements setting the context.
> > > >
> > > > Please note that the Fedora and RHEL SELinux policy must be tweaked, so
> > > > currently the patches must be tested with SELinux set to Permissive.
> > > >
> > > > Additionally, I wasn't able to run Coverity scan on these patches, because
> > > > our Coverity server was returning errors again for me. I'll run the tests
> > > > later, or, if the reviewer would find any issues, I'll be happy to fix them.
> >
> > Hi,
> >
> > attached patches change the packaging a bit -- make distcheck didn't
> > pass previously and I forgot to package selinux_child in the RPM.
> >
> > 0001-UTIL-Remove-code-duplication-of-struct-io.patch
>
> Ack.
> > 0002-UTIL-Remove-more-code-duplication-setting-up-child-p.patch
>
> Ack.
> > 0003-IPA-Move-setting-the-SELinux-context-to-a-child-proc.patch
>
> The chgrp and chmod in Makefile.am should probably be called
> only if the semanage is available:
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -2852,10 +2852,12 @@ endif
>
> if SSSD_USER
>    chgrp $(SSSD_USER) $(sssdlibexecdir)/ldap_child
>   chgrp $(SSSD_USER) $(sssdlibexecdir)/selinux_child
>   chmod 4750 $(sssdlibexecdir)/ldap_child
>
> +if BUILD_SEMANAGE
>   chgrp $(SSSD_USER) $(sssdlibexecdir)/selinux_child
>   chmod 4750 $(sssdlibexecdir)/selinux_child
>
> endif
> +endif

Fixed
> In src/providers/ipa/ipa_selinux.c, the line 405 is too long:
>  405     /* Update the SELinux context ...
>  406      * unprivileged
>  407      */

Fixed
> Also in src/providers/ipa/ipa_selinux.c, the indentation
> on line 872 should be aligned with the line above:
>  871     if (sci->seuser == NULL || sci->mls_range == NULL
>  872             || sci->username == NULL) {
>
> Should be:
>  871     if (sci->seuser == NULL || sci->mls_range == NULL
>  872         || sci->username == NULL) {
> (Or at least I am doing it like this, and it is style that
> was proposed IIRC by Stephen in some old discussion).

Fixed. I thought the leading tabs were also proposed in that thread..
> Otherwise the patch looks good to me and the selinux
> context is retrieved properly.
>
> Michal

Thank you for the review, new patches are attached.

[Michal Židek]

> > Also in src/providers/ipa/ipa_selinux.c, the indentation
> > on line 872 should be aligned with the line above:
> >   871     if (sci->seuser == NULL || sci->mls_range == NULL
> >   872             || sci->username == NULL) {
> >
> > Should be:
> >   871     if (sci->seuser == NULL || sci->mls_range == NULL
> >   872         || sci->username == NULL) {
> > (Or at least I am doing it like this, and it is style that
> > was proposed IIRC by Stephen in some old discussion).
>
> Fixed. I thought the leading tabs were also proposed in that thread..

Now, I am not sure... anyway, I think the way it is
now is better :)

Ack to all patches.

Michal

[Jakub Hrozek]

On Wed, Nov 05, 2014 at 07:41:43PM +0100, Michal Židek wrote:
> > > Also in src/providers/ipa/ipa_selinux.c, the indentation
> > > on line 872 should be aligned with the line above:
> > >  871     if (sci->seuser == NULL || sci->mls_range == NULL
> > >  872             || sci->username == NULL) {
> > >
> > > Should be:
> > >  871     if (sci->seuser == NULL || sci->mls_range == NULL
> > >  872         || sci->username == NULL) {
> > > (Or at least I am doing it like this, and it is style that
> > > was proposed IIRC by Stephen in some old discussion).
> >
> > Fixed. I thought the leading tabs were also proposed in that thread..
>
> Now, I am not sure... anyway, I think the way it is
> now is better :)
>
> Ack to all patches.
>
> Michal

* master:
    5eef3da14cb34e4cb6356f0b291c066db946f936
    0348c74bad010d35f92400c749a7acc2fea8b2cb
    45414c12aa933a33d9a635cc212c448c858c6bab

[Lukas Slebodnik]

On (05/11/14 18:30), Jakub Hrozek wrote:
> On Fri, Oct 31, 2014 at 10:48:09PM +0100, Michal Židek wrote:
> > On 10/31/2014 05:59 PM, Jakub Hrozek wrote:
> > > On Fri, Oct 24, 2014 at 10:57:02PM +0200, Jakub Hrozek wrote:
> > > > > Hi,
> > > > >
> > > > > this patchset moves setting the SELinux context from the sssd_be process
> > > > > to a privileges child process in preparation for making the sssd_be process
> > > > > running as the sssd user.
> > > > >
> > > > > The first two patches just reduce code duplication between the child
> > > > > processes we already have, the last one implements setting the context.
> > > > >
> > > > > Please note that the Fedora and RHEL SELinux policy must be tweaked, so
> > > > > currently the patches must be tested with SELinux set to Permissive.
> > > > >
> > > > > Additionally, I wasn't able to run Coverity scan on these patches, because
> > > > > our Coverity server was returning errors again for me. I'll run the tests
> > > > > later, or, if the reviewer would find any issues, I'll be happy to fix them.
> > >
> > > Hi,
> > >
> > > attached patches change the packaging a bit -- make distcheck didn't
> > > pass previously and I forgot to package selinux_child in the RPM.
> > >
> > > 0001-UTIL-Remove-code-duplication-of-struct-io.patch
> >
> > Ack.
> > > 0002-UTIL-Remove-more-code-duplication-setting-up-child-p.patch
> >
> > Ack.
> > > 0003-IPA-Move-setting-the-SELinux-context-to-a-child-proc.patch
> >
> > The chgrp and chmod in Makefile.am should probably be called
> > only if the semanage is available:
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -2852,10 +2852,12 @@ endif
> >
> > if SSSD_USER
> >    chgrp $(SSSD_USER) $(sssdlibexecdir)/ldap_child
> >   chgrp $(SSSD_USER) $(sssdlibexecdir)/selinux_child
> >   chmod 4750 $(sssdlibexecdir)/ldap_child
> >
> > +if BUILD_SEMANAGE
> >   chgrp $(SSSD_USER) $(sssdlibexecdir)/selinux_child
> >   chmod 4750 $(sssdlibexecdir)/selinux_child
> >
> > endif
> > +endif
>
> Fixed
> > In src/providers/ipa/ipa_selinux.c, the line 405 is too long:
> >  405     /* Update the SELinux context ...
> >  406      * unprivileged
> >  407      */
>
> Fixed
> > Also in src/providers/ipa/ipa_selinux.c, the indentation
> > on line 872 should be aligned with the line above:
> >  871     if (sci->seuser == NULL || sci->mls_range == NULL
> >  872             || sci->username == NULL) {
> >
> > Should be:
> >  871     if (sci->seuser == NULL || sci->mls_range == NULL
> >  872         || sci->username == NULL) {
> > (Or at least I am doing it like this, and it is style that
> > was proposed IIRC by Stephen in some old discussion).
>
> Fixed. I thought the leading tabs were also proposed in that thread..
> > Otherwise the patch looks good to me and the selinux
> > context is retrieved properly.
> >
> > Michal
>
> Thank you for the review, new patches are attached.
> From 0c4dd8767998d0fbf09be3a0e6d5ec96a50443aa Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Mon, 20 Oct 2014 23:16:40 +0200
> Subject: [PATCH 3/3] IPA: Move setting the SELinux context to a child process
>
> In order for the sssd_be process to run as unprivileged user, we need to
> move the semanage processing to a process that runs as the root user
> using setuid privileges.
>
> Makefile.am                       |  27 +++
> contrib/sssd.spec.in              |   1 +
> src/providers/ipa/ipa_selinux.c   | 425 +++++++++++++++++++++++++++++++++++---
> src/providers/ipa/selinux_child.c | 272 ++++++++++++++++++++++++
> src/util/util_errors.c            |   1 +
> src/util/util_errors.h            |   1 +
> 6 files changed, 699 insertions(+), 28 deletions(-)
> create mode 100644 src/providers/ipa/selinux_child.c

//snip
> +int main(int argc, const char *argv[])
> +{
> int opt;
> poptContext pc;
> int debug_fd = -1;
> errno_t ret;
> TALLOC_CTX *main_ctx = NULL;
> uint8_t *buf = NULL;
> ssize_t len = 0;
> struct input_buffer *ibuf = NULL;
> struct response *resp = NULL;
> size_t written;
> struct poptOption long_options[] = {
>    POPT_AUTOHELP
>    {"debug-level", 'd', POPT_ARG_INT, &debug_level, 0,
>     _("Debug level"), NULL},
>    {"debug-timestamps", 0, POPT_ARG_INT, &debug_timestamps, 0,
>     _("Add debug timestamps"), NULL},
>    {"debug-microseconds", 0, POPT_ARG_INT, &debug_microseconds, 0,
>     _("Show timestamps with microseconds"), NULL},
>    {"debug-fd", 0, POPT_ARG_INT, &debug_fd, 0,
>     _("An open file descriptor for the debug logs"), NULL},
>    {"debug-to-stderr", 0, POPT_ARG_NONE | POPT_ARGFLAG_DOC_HIDDEN,
>     &debug_to_stderr, 0,
>     _("Send the debug output to stderr directly."), NULL },
>    POPT_TABLEEND
> };
> /* Set debug level to invalid value so we can decide if -d 0 was used. */
> debug_level = SSSDBG_INVALID;
> pc = poptGetContext(argv[0], argc, argv, long_options, 0);
> while((opt = poptGetNextOpt(pc)) != -1) {
>    switch(opt) {
>    default:
>    fprintf(stderr, "\nInvalid option %s: %s\n\n",
>              poptBadOption(pc, 0), poptStrerror(opt));
>        poptPrintUsage(pc, stderr, 0);
>        _exit(-1);
>    }
> }
> poptFreeContext(pc);
> DEBUG_INIT(debug_level);
> debug_prg_name = talloc_asprintf(NULL, "[sssd[selinux_child[%d]]]", getpid());
> if (debug_prg_name == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
>    goto fail;
> }
> if (debug_fd != -1) {
>    ret = set_debug_file_from_fd(debug_fd);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "set_debug_file_from_fd failed.\n");
>    }
> }
> DEBUG(SSSDBG_TRACE_FUNC, "selinux_child started.\n");
> DEBUG(SSSDBG_TRACE_INTERNAL,
>      "Running as [%"SPRIuid"][%"SPRIgid"].\n", geteuid(), getegid());
> main_ctx = talloc_new(NULL);
> if (main_ctx == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new failed.\n");
>    talloc_free(discard_const(debug_prg_name));
>    goto fail;
> }
> talloc_steal(main_ctx, debug_prg_name);
> buf = talloc_size(main_ctx, sizeof(uint8_t)*IN_BUF_SIZE);
> if (buf == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
>    goto fail;
> }
> ibuf = talloc_zero(main_ctx, struct input_buffer);
> if (ibuf == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n");
>    goto fail;
> }
> DEBUG(SSSDBG_TRACE_FUNC, "context initialized\n");
> errno = 0;
> len = sss_atomic_read_s(STDIN_FILENO, buf, IN_BUF_SIZE);
> if (len == -1) {
>    ret = errno;
>    DEBUG(SSSDBG_CRIT_FAILURE, "read failed [%d][%s].\n", ret, strerror(ret));
>    goto fail;
> }
> close(STDIN_FILENO);
> ret = unpack_buffer(buf, len, ibuf);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE,
>          "unpack_buffer failed.[%d][%s].\n", ret, strerror(ret));
>    goto fail;
> }
> DEBUG(SSSDBG_TRACE_FUNC, "performing selinux operations\n");
> ret = set_seuser(ibuf->username, ibuf->seuser, ibuf->mls_range);
> ret = prepare_response(main_ctx, ret, &resp);

^^^
return value is not tested. It might happen that resp will not be used
initialized in sss_atomic_write_s.
> errno = 0;
> written = sss_atomic_write_s(STDOUT_FILENO, resp->buf, resp->size);
> if (written == -1) {
>    ret = errno;
>    DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
>                strerror(ret));
>    goto fail;
> }

html report from clang is attached.

LS

[Michal Židek]

On 11/06/2014 12:09 PM, Lukas Slebodnik wrote:
> > DEBUG(SSSDBG_TRACE_FUNC, "performing selinux operations\n");
> > ret = set_seuser(ibuf->username, ibuf->seuser, ibuf->mls_range);
> > ret = prepare_response(main_ctx, ret, &resp);
>    ^^^
>
> return value is not tested. It might happen that resp will not be used
> initialized in sss_atomic_write_s.
> > errno = 0;
> > written = sss_atomic_write_s(STDOUT_FILENO, resp->buf, resp->size);
> > if (written == -1) {
> >    ret = errno;
> >    DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
> >                strerror(ret));
> >    goto fail;
> > }
>
> html report from clang is attached.
>
> LS

Thanks, please see the attached simple patch.

Michal

[Lukas Slebodnik]

On (06/11/14 16:40), Michal Židek wrote:
> On 11/06/2014 12:09 PM, Lukas Slebodnik wrote:
> > > DEBUG(SSSDBG_TRACE_FUNC, "performing selinux operations\n");
> > > ret = set_seuser(ibuf->username, ibuf->seuser, ibuf->mls_range);
> > > ret = prepare_response(main_ctx, ret, &resp);
> >   ^^^
> >
> > return value is not tested. It might happen that resp will not be used
> > initialized in sss_atomic_write_s.
> > > errno = 0;
> > > written = sss_atomic_write_s(STDOUT_FILENO, resp->buf, resp->size);
> > > if (written == -1) {
> > >    ret = errno;
> > >    DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
> > >                strerror(ret));
> > >    goto fail;
> > > }
> >
> > html report from clang is attached.
> >
> > LS
>
> Thanks, please see the attached simple patch.
>
> Michal
> From 3942a3acfa1a46b68163ecbc2566b8c2599f3c61 Mon Sep 17 00:00:00 2001
> From: Michal Zidek 
> Date: Thu, 6 Nov 2014 16:35:11 +0100
> Subject: [PATCH] selinux_child: Do not ignore return values.
>
> src/providers/ipa/selinux_child.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/src/providers/ipa/selinux_child.c b/src/providers/ipa/selinux_child.c
> index a624cfd..a38ffcb 100644

ACK

LS

[Jakub Hrozek]

On Thu, Nov 06, 2014 at 05:27:30PM +0100, Lukas Slebodnik wrote:
> On (06/11/14 16:40), Michal Židek wrote:
> > On 11/06/2014 12:09 PM, Lukas Slebodnik wrote:
> > > > DEBUG(SSSDBG_TRACE_FUNC, "performing selinux operations\n");
> > > > ret = set_seuser(ibuf->username, ibuf->seuser, ibuf->mls_range);
> > > > ret = prepare_response(main_ctx, ret, &resp);
> > >   ^^^
> > >
> > > return value is not tested. It might happen that resp will not be used
> > > initialized in sss_atomic_write_s.
> > > > errno = 0;
> > > > written = sss_atomic_write_s(STDOUT_FILENO, resp->buf, resp->size);
> > > > if (written == -1) {
> > > >    ret = errno;
> > > >    DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
> > > >                strerror(ret));
> > > >    goto fail;
> > > > }
> > >
> > > html report from clang is attached.
> > >
> > > LS
> >
> > Thanks, please see the attached simple patch.
> >
> > Michal
> > From 3942a3acfa1a46b68163ecbc2566b8c2599f3c61 Mon Sep 17 00:00:00 2001
> > From: Michal Zidek 
> > Date: Thu, 6 Nov 2014 16:35:11 +0100
> > Subject: [PATCH] selinux_child: Do not ignore return values.
> >
> > src/providers/ipa/selinux_child.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/src/providers/ipa/selinux_child.c b/src/providers/ipa/selinux_child.c
> > index a624cfd..a38ffcb 100644
>
> ACK
>
> LS

* master: 013c01bd491b535e1705dbb3dbd8424cffc66b7a