3:57 p.m.
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.
11:59 a.m.
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.
4:48 p.m.
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
11:30 a.m.
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.
12:41 p.m.
> 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
1:09 p.m.
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
5:09 a.m.
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 <jhrozek(a)redhat.com>
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
9:40 a.m.
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
10:27 a.m.
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 <mzidek(a)redhat.com>
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
12:21 p.m.
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 <mzidek(a)redhat.com>
>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
3452
days inactive
3465
days old
sssd-devel@lists.fedorahosted.org
9 comments
3 participants
participants (3)
-
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek