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