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(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 ?
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);