Comments inline.
(I'll send multiple emails per patch, where necessary)
On Wed, 5 Nov 2014 18:36:06 +0100
Jakub Hrozek <jhrozek(a)redhat.com> wrote:
From c9d484638009371cff3b1213b5640a7827b9e1ba Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Mon, 13 Oct 2014 21:13:38 +0200
Subject: [PATCH 2/6] KRB5: Drop privileges in the child, not the back
end
In future patches, sssd_be will be running as a non-privileged user,
who will execute the setuid krb5_child. In this case, the child will
start as root and drop the privileges as soon as possible.
However, we need to also remove the privilege drop in sssd_be, because
if we dropped to the user who is authenticating, we wouldn't be even
allowed to execute krb5_child. The krb5_child permissions should be
4750, owned by root.sssd, to make sure only root and sssd can execute
the child and if executed by sssd, the child will run as root.
Should we check that the parent process really is running either as
root or the sssd user and fail otherwise ?
---
src/providers/krb5/krb5_child.c | 57
+++++++++++++++++++++++++--------
src/providers/krb5/krb5_child_handler.c | 8 ----- 2 files changed,
44 insertions(+), 21 deletions(-)
diff --git a/src/providers/krb5/krb5_child.c
b/src/providers/krb5/krb5_child.c index
3234a4e6c740db5e05f7db8eb7f4ea0cc126e7ce..1c657de2d96d9fcea6d041af9cc9217863732b44
100644 --- a/src/providers/krb5/krb5_child.c +++
b/src/providers/krb5/krb5_child.c @@ -1840,11 +1840,52 @@ static int
k5c_setup_fast(struct krb5_req *kr, bool demand) return EOK;
}
-static int k5c_setup(struct krb5_req *kr, uint32_t offline)
+static errno_t check_use_fast(bool *_fast)
{
- krb5_error_code kerr;
char *use_fast_str;
+ bool fast = false;
+
+ use_fast_str = getenv(SSSD_KRB5_USE_FAST);
Although the launching user is somewhat trusted .. should we stop using
environment variables and instead pass info via command line options ?
The reason I ask is that I would like to see all getenv() calls changed
to secure_getenv() calls where possible.
+ if (use_fast_str == NULL || strcasecmp(use_fast_str,
"never") ==
0) {
+ DEBUG(SSSDBG_CONF_SETTINGS, "Not using FAST.\n");
+ } else if (strcasecmp(use_fast_str, "try") == 0 ||
+ strcasecmp(use_fast_str, "demand") == 0) {
+ fast = true;
+ } else {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Unsupported value [%s] for krb5_use_fast.\n",
+ use_fast_str);
+ return EINVAL;
+ }
+
+ *_fast = fast;
+ return EOK;
+}
+
+static int k5c_setup(struct krb5_req *kr, uint32_t offline)
+{
+ krb5_error_code kerr;
int parse_flags;
+ bool use_fast;
+
+ kerr = check_use_fast(&use_fast);
+ if (kerr != EOK) {
+ return kerr;
+ }
+
+ if (offline || (use_fast == false && kr->validate == false)) {
+ /* If krb5_child was started as setuid, but we don't need to
+ * perform either validation or FAST, just drop privileges to
+ * the SSSD user. The same applies to the offline case
Do I read this wrongly ? Or is this comment misleading ?
As far as I can tell you are dropping privileges to the user trying to
authenticate not the SSSD user? (Did I miss another change where SSSD's
user uid/gid values are set in kr ?)
+ */
+ kerr = become_user(kr->uid, kr->gid);
+ if (kerr != 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "become_user failed.\n");
+ return kerr;
+ }
+ }
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ "Running as [%"SPRIuid"][%"SPRIgid"].\n",
geteuid(),
getegid());
kr->realm = getenv(SSSD_KRB5_REALM);
if (kr->realm == NULL) {
@@ -1931,18 +1972,8 @@ static int k5c_setup(struct krb5_req *kr,
uint32_t offline) if (!offline) {
set_canonicalize_option(kr->options);
- use_fast_str = getenv(SSSD_KRB5_USE_FAST);
- if (use_fast_str == NULL || strcasecmp(use_fast_str,
"never") == 0) {
- DEBUG(SSSDBG_CONF_SETTINGS, "Not using FAST.\n");
- } else if (strcasecmp(use_fast_str, "try") == 0) {
+ if (use_fast) {
kerr = k5c_setup_fast(kr, false);
- } else if (strcasecmp(use_fast_str, "demand") == 0) {
- kerr = k5c_setup_fast(kr, true);
This change seem to be breaking k5c_setup_fast, in the sense that you
never pass along the "demand" option and therefore the code will always
fall back even though the configuration wanted to forbid downgrades.
I think this makes me NACK this patch (and I do not see any other
change that addresses this point in the other patches).
- } else {
- DEBUG(SSSDBG_CRIT_FAILURE,
- "Unsupported value [%s] for krb5_use_fast.\n",
- use_fast_str);
- return EINVAL;
}
}
diff --git a/src/providers/krb5/krb5_child_handler.c
b/src/providers/krb5/krb5_child_handler.c index
4ba939deb3e0e282b3ca9b2b21226ea7e64e311b..71c7f9c9f662e16b94afda0c8c0ae24666f0ba15
100644 --- a/src/providers/krb5/krb5_child_handler.c +++
b/src/providers/krb5/krb5_child_handler.c @@ -284,14 +284,6 @@ static
errno_t fork_child(struct tevent_req *req) pid = fork();
if (pid == 0) { /* child */
- if (state->kr->run_as_user) {
- ret = become_user(state->kr->uid, state->kr->gid);
- if (ret != EOK) {
- DEBUG(SSSDBG_CRIT_FAILURE, "become_user failed.\n");
- return ret;
- }
- }
-
err = exec_child(state,
pipefd_to_child, pipefd_from_child,
KRB5_CHILD,
state->kr->krb5_ctx->child_debug_fd);
--
Simo Sorce * Red Hat, Inc * New York