On 10/17/2014 06:47 PM, Jakub Hrozek wrote:
On Thu, Oct 16, 2014 at 10:25:12AM +0200, Jakub Hrozek wrote:
> On Wed, Oct 15, 2014 at 06:19:49PM -0400, Simo Sorce wrote:
> > On Wed, 15 Oct 2014 22:24:04 +0200
> > Jakub Hrozek <jhrozek@redhat.com> wrote:
> > 
> > > Attached are patches that perform changes in the monitor process and
> > > the low-level sbus and sysdb code required to run the NSS responder
> > > as a non-privileged user. Some of the patches call chmod/chown on
> > > files owned by the SSSD, so I'd like to request a very careful review.
> > 
> > Aside from the points raised in the emails already sent the rest looks
> > good to me.
> 
> Thank you very much for the review. I'll send out updated patches.
Attached are patches that implement the corrections Simo and Pavel asked
for with the exception of confdb being read-only for sssd processes,
Michal is still investigating that one.

I also merged some more Michal's patches that help spawn the PAM
privileged pipe before dropping privileges and also dropping privileges
in the other responders.

With this patchset, all responders are able to run as the SSSD user.

I have one question:
is it wise to also set the permissions of directories we create when
we "install -d" them? Or is this something typically done by the
downstream? Previously, we would just rely on the default system umask
when running "make install", maybe we should tighten the permissions in
Makefile.am as part of this effort?

And one comment:
One of Michal's patches moves creating the socket into a separate
function. This function also changes the umask, which I don't think
belongs into a utility function. I realize this is how the code worked
even before Michal's change, but I think it would be better to move the
umask setting into function like sss_process_init().

Thanks again for the review.

0001-UTIL-Add-a-function-to-convert-id_t-from-a-number-or.patch
[snip]

+++ b/src/tests/cwrap/test_usertools.c
@@ -0,0 +1,101 @@
+/*
+    Authors:
+        Jakub Hrozek <jhrozek@redhat.com>
+
+    Copyright (C) 2014 Red Hat
+
+    SSSD tests: User utilities
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 3 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#include <popt.h>
+#include "util/util.h"
+#include "tests/cmocka/common_mock.h"
+
+void test_get_user_num(void **state)
+{
+    uid_t uid;
+    gid_t gid;
+    errno_t ret;
+
+    ret = sss_user_by_name_or_uid("123", &uid, &gid);
+    assert_int_equal(ret, EOK);
+    assert_int_equal(uid, 123);
+    assert_int_equal(gid, 123);
+}
+
+void test_get_user_str(void **state)
+{
+    uid_t uid;
+    gid_t gid;
+    errno_t ret;
+
+    ret = sss_user_by_name_or_uid("sssd", &uid, &gid);
+    assert_int_equal(ret, EOK);
+    assert_int_equal(uid, 123);
+    assert_int_equal(gid, 456);
+}
I wasn't able to run the tests now, but why is gid supposed to 456 and in prev test 123?
+
+void test_get_user_nullparm(void **state)
+{
+    uid_t uid;
+    errno_t ret;
+
+    ret = sss_user_by_name_or_uid("sssd", &uid, NULL);
+    assert_int_equal(ret, EOK);
+    assert_int_equal(uid, 123);
+}
+
+int main(int argc, const char *argv[])
+{
+    poptContext pc;
+    int opt;
+    struct poptOption long_options[] = {
+        POPT_AUTOHELP
+        SSSD_DEBUG_OPTS
+        POPT_TABLEEND
+    };
+
+    const UnitTest tests[] = {
+        unit_test(test_get_user_num),
+        unit_test(test_get_user_str),
+        unit_test(test_get_user_nullparm),
+    };
+
+    /* Set debug level to invalid value so we can deside 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);
+            return 1;
+        }
+    }
+    poptFreeContext(pc);
+
+    DEBUG_CLI_INIT(debug_level);
+
+    tests_set_cwd();
+
+    return run_tests(tests);
+}


0004-SSSD-Load-a-user-to-run-a-service-as-from-configurat.patch

From 78066ed7cd43e0208f6d15b8b0489e3699b1b4cc Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek@redhat.com>
Date: Tue, 5 Aug 2014 13:52:48 +0200
Subject: [PATCH 04/19] SSSD: Load a user to run a service as from
 configuration

Adds two new options, user and group that are specified in the [sssd]
section. When these options are specified, SSSD will run as the user and
group. When these are not specified, SSSD will run as the configure-time
user and group.

The group option only takes effect when user is also specified.

Currently all services and providers are started as root. There is a
temporary svc_supported_as_nonroot() function that returns true for a
service if that service runs and was tested as nonroot and false
otherwise. Currently this function always returns false, but will be
amended in future patches.
---
[snip]
 
+static int get_service_user(struct mt_ctx *ctx)
+{
+    errno_t ret;
+    char *user_str;
+
+    ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
+                            CONFDB_MONITOR_USER_RUNAS,
+                            NULL, &user_str);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_FATAL_FAILURE, "Failed to get the user to run as");
'\n'
+        return ret;
+    }
+
+    ret = sss_user_by_name_or_uid(user_str, &ctx->uid, &ctx->gid);
+    talloc_free(user_str);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_FATAL_FAILURE, "Failed to set allowed UIDs.\n");
+        return ret;
+    }
+
+    return EOK;
+}
+


0011-responder_common-Create-fd-for-pipe-in-helper.patch

From 1a2d3bf47c0da1bb32be138fa8b2b33260f68a37 Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzidek@redhat.com>
Date: Wed, 15 Oct 2014 17:35:12 +0200
Subject: [PATCH 11/19] responder_common: Create fd for pipe in helper

Move creating of file descriptor for pipes into
helper function and make this function public.
---
 src/responder/common/responder.h        |   2 +
 src/responder/common/responder_common.c | 129 ++++++++++++++------------------
 2 files changed, 59 insertions(+), 72 deletions(-)

diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 97552ec472c5baa285b41cc48b51149f3ef6adb5..d233710782fe7df1bbcc338e3815d1701557519e 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -176,6 +176,8 @@ responder_get_domain(struct resp_ctx *rctx, const char *domain);
 errno_t responder_get_domain_by_id(struct resp_ctx *rctx, const char *id,
                                    struct sss_domain_info **_ret_dom);
 
+int create_pipe_fd(const char *sock_name, int *fd, mode_t umaskval);
+
 /* responder_cmd.c */
 int sss_cmd_empty_packet(struct sss_packet *packet);
 int sss_cmd_send_empty(struct cli_ctx *cctx, TALLOC_CTX *freectx);
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 0ec2372e8d08f1002b303b5edc6897f17cee9699..3b880647e45cf4d4191ff7a9166e82b11288204d 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -584,10 +584,63 @@ static int sss_dp_init(struct resp_ctx *rctx,
     return EOK;
 }
 
+int create_pipe_fd(const char *sock_name, int *fd, mode_t umaskval)
+{
+    struct sockaddr_un addr;
+    errno_t ret;
+
+    *fd = socket(AF_UNIX, SOCK_STREAM, 0);
+    if (*fd == -1) {
+        return EIO;
+    }
+
+    umask(umaskval);
+
+    ret = set_nonblocking(*fd);
+    if (ret != EOK) {
+        goto done;
+    }
+
+    ret = set_close_on_exec(*fd);
+    if (ret != EOK) {
+        goto done;
+    }
+
+    memset(&addr, 0, sizeof(addr));
+    addr.sun_family = AF_UNIX;
+    strncpy(addr.sun_path, sock_name, sizeof(addr.sun_path)-1);
I think that spaces around '-' operator are more common in our codebase...but I don't argue about this.
+    addr.sun_path[sizeof(addr.sun_path)-1] = '\0';
same here
+
+    /* make sure we have no old sockets around */
+    unlink(sock_name);
check for failure?
+
+    if (bind(*fd, (struct sockaddr *)&addr, sizeof(addr)) == -1) {
+        DEBUG(SSSDBG_FATAL_FAILURE,
+              "Unable to bind on socket '%s'\n", sock_name);
+        ret = EIO;
+        goto done;
+    }
+    if (listen(*fd, 10) != 0) {
+        DEBUG(SSSDBG_FATAL_FAILURE,
+              "Unable to listen on socket '%s'\n", sock_name);
+        ret = EIO;
+        goto done;
+    }
Why not to have check for listen and bind failure in the same form ('== -1') ?
+
+    ret = EOK;
new line would be nice here
+done:
+    /* we want default permissions on created files to be very strict,
+       so set our umask to 0177 */
+    umask(0177);
+    if (ret != EOK) {
+        close(*fd);
+    }
+    return ret;
+}
+
I think it would be nicer to use local varibale fd  in this function and return its vales using output parameter _fd. Do you agree?