[SSSD] [PATCH] Move ccache operations to krb5_child, allow the krb5_auth code to run unprivileged
Jakub Hrozek
jhrozek at redhat.com
Mon Nov 10 16:11:43 UTC 2014
On Thu, Nov 06, 2014 at 09:56:13AM -0500, Simo Sorce wrote:
> Comments inline.
> (I'll send multiple emails per patch, where necessary)
Thanks for the review and sorry about the delay, some other work came
up...
>
> On Wed, 5 Nov 2014 18:36:06 +0100
> Jakub Hrozek <jhrozek at redhat.com> wrote:
>
> > From c9d484638009371cff3b1213b5640a7827b9e1ba Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek <jhrozek at 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 ?
We could only do that if we limit the user sssd runs as to either the
sssd user or root as well. Currently the 'user' configuration option
blindly accepts anything -- which might not be the best idea, because
realistically, only root or the sssd user would work.
So yeah, I'm fine with doing this change, but I also need to restrict
the user sssd runs at to root or the configured user.
>
> > ---
> > 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.
Yes, I agree and it's been something that we planned for some time, but
I think it should be a separate patch.
The work is tracked by https://fedorahosted.org/sssd/ticket/697 btw
>
> > + 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 ?)
The comment is misleading, thanks for catching that. Fixed.
>
> > + */
> > + 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).
Aah, thanks, I think I changed the code as a prototype to make sure FAST
worked at all and then forgot to finish this part..
I'll attach my patches to your other reply.
>
> > - } 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);
More information about the sssd-devel
mailing list