On Fri, Nov 14, 2014 at 01:52:20PM +0100, Jakub Hrozek wrote:
On Thu, Nov 13, 2014 at 07:30:41PM +0100, Jakub Hrozek wrote:
> On Wed, Nov 12, 2014 at 05:08:09PM +0100, Lukas Slebodnik wrote:
>
...
> Thank you, see the attached patches.
I forgot to remove the extra find_uid.c from Makefile.am
I only have minor comments, see below. I understand that most part of
the patches are refactorings to make sure the related code is run with
the right permissions. (See next mail for some additional suggestions).
The ccache handling always was delicate, e.g. directory creation with
DIR type and determine if FILE with _XXXXXX should be recreated or
reused. I run couple to tests without an issue but I think only
regression testing might give clarity here. So ACK from my side.
bye,
Sumit
From 242b9273014ade6d507a2f3e0d78e7e2a66f084e 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.
---
src/providers/krb5/krb5_child.c | 71 ++++++++++++++++++++++++++-------
src/providers/krb5/krb5_child_handler.c | 8 ----
2 files changed, 57 insertions(+), 22 deletions(-)
...
- 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) {
- kerr = k5c_setup_fast(kr, false);
- } else if (strcasecmp(use_fast_str, "demand") == 0) {
- kerr = k5c_setup_fast(kr, true);
- } else {
- DEBUG(SSSDBG_CRIT_FAILURE,
- "Unsupported value [%s] for krb5_use_fast.\n",
- use_fast_str);
- return EINVAL;
+ if (fast_val > K5C_FAST_NEVER) {
I think if (fast_val != K5C_FAST_NEVER) {
would be more readable here
+ kerr = k5c_setup_fast(kr, fast_val == K5C_FAST_DEMAND);
+ if (kerr != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, "Cannot set up FAST\n");
+ return kerr;
+ }
}
}
...
From 4fc2b7fd08c2f2ecfff8d559c06d62dd600655f7 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Sat, 18 Oct 2014 20:52:43 +0200
Subject: [PATCH 3/6] KRB5: Move ccache-related functions to krb5_ccache.c
Add a new module krb5_ccache.c that contains all ccache-related
operations. The only user of this module shall be krb5_child.c as the
other modules will run unprivileged and accessing the keytab requires
typo: ^ccache^
either privileges of root or the ccache owner.
---
Makefile.am | 4 +
src/providers/krb5/krb5_auth.c | 16 +-
src/providers/krb5/krb5_auth.h | 1 +
src/providers/krb5/{krb5_utils.c => krb5_ccache.c} | 720 +++++----------------
src/providers/krb5/{krb5_utils.h => krb5_ccache.h} | 49 +-
src/providers/krb5/krb5_child.c | 1 +
src/providers/krb5/krb5_common.h | 7 -
src/providers/krb5/krb5_renew_tgt.c | 1 +
src/providers/krb5/krb5_utils.c | 672 +------------------
src/providers/krb5/krb5_utils.h | 15 -
src/tests/krb5_child-test.c | 1 +
src/tests/krb5_utils-tests.c | 1 +
12 files changed, 197 insertions(+), 1291 deletions(-)
copy src/providers/krb5/{krb5_utils.c => krb5_ccache.c} (57%)
copy src/providers/krb5/{krb5_utils.h => krb5_ccache.h} (53%)
...
- /* Additional syntax from krb5.conf default_ccache_name
*/
- case '{':
- if (strncmp(n , S_EXP_UID, L_EXP_UID) == 0) {
- action = 'U';
- n += L_EXP_UID - 1;
- rerun = true;
- continue;
- } else if (strncmp(n , S_EXP_USERID, L_EXP_USERID) == 0) {
- action = 'U';
- n += L_EXP_USERID - 1;
- rerun = true;
- continue;
- } else if (strncmp(n , S_EXP_EUID, L_EXP_EUID) == 0) {
- /* SSSD does not distinguish betwen uid and euid,
- * so we treat both the same way */
- action = 'U';
- n += L_EXP_EUID - 1;
- rerun = true;
- continue;
- } else if (strncmp(n , S_EXP_USERNAME, L_EXP_USERNAME) == 0) {
- action = 'u';
- n += L_EXP_USERNAME - 1;
- rerun = true;
- continue;
- } else {
- /* ignore any expansion variable we do not understand and
- * let libkrb5 hndle it or fail */
- name = n;
- n = strchr(name, '}');
- if (!n) {
- DEBUG(SSSDBG_CRIT_FAILURE,
trailing whitespace ^
- "Invalid substitution sequence in
cache "
- "template. Missing closing '}' in
[%s].\n",
- template);
- goto done;
- }
- result = talloc_asprintf_append(result, "%s%%%.*s", p,
- (int)(n - name + 1), name);
- }
- break;
- default:
- DEBUG(SSSDBG_CRIT_FAILURE,
- "format error, unknown template [%%%c].\n", *n);
- goto done;
- }
- }
-
- if (result == NULL) {
- DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf_append failed.\n");
- goto done;
- }
-
- p = n + 1;
- }
-
...
From 9a3732a1622fe4a172e0b753d1b5837691a7bd6d Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Sat, 18 Oct 2014 22:03:01 +0200
Subject: [PATCH 4/6] KRB5: Move checking for illegal RE to krb5_utils.c
Otherwise we would have to link krb5_child with pcre and transfer the
regex, which wold be cumbersome. Check for illegal patterns when
typo: would
expanding the template instead.
---
src/providers/krb5/krb5_auth.c | 5 +--
src/providers/krb5/krb5_ccache.c | 38 ++------------------
src/providers/krb5/krb5_ccache.h | 7 +---
src/providers/krb5/krb5_utils.c | 36 +++++++++++++++++--
src/providers/krb5/krb5_utils.h | 4 +--
src/tests/krb5_child-test.c | 2 +-
src/tests/krb5_utils-tests.c | 78 ++++++++++++++++------------------------
7 files changed, 73 insertions(+), 97 deletions(-)
...
From f608986724901a441ec87cdd0e170d9bfb175c21 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Sat, 18 Oct 2014 22:03:13 +0200
Subject: [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
The credential cache operations must be now performed by the krb5_child
completely, because the sssd_be process might be running as the sssd
user who doesn't have access to the ccaches.
src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
until we fix Kerberos ticket renewal as non-root.
Also includes a new error code that indicates that the back end should
remove the old ccache attribute -- the child can't do that if it's
running as the user.
---
Makefile.am | 13 +-
src/providers/krb5/krb5_auth.c | 223 ++++----------------------------
src/providers/krb5/krb5_ccache.c | 62 ++++-----
src/providers/krb5/krb5_ccache.h | 5 +-
src/providers/krb5/krb5_child.c | 208 +++++++++++++++++++++++++++--
src/providers/krb5/krb5_child_handler.c | 13 ++
src/tests/krb5_child-test.c | 3 +-
src/util/util_errors.c | 1 +
src/util/util_errors.h | 1 +
9 files changed, 281 insertions(+), 248 deletions(-)
...
From 8f0f7aba473f243e369589db46b09e690648e42b Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Sun, 19 Oct 2014 12:28:13 +0200
Subject: [PATCH 6/6] KRB5: Do not switch_creds() if already the specified user
The code didn't have to handle this case previously as sssd_be was always
running as root and switching to the ccache as the user logging in.
Also handle NULL creds on restore_creds() in case there was no switch.
One less if-condition and fewer indentation levels.
---
src/tests/cwrap/test_become_user.c | 7 +++++++
src/util/become_user.c | 27 ++++++++++++++++++++-------
2 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/src/tests/cwrap/test_become_user.c b/src/tests/cwrap/test_become_user.c
index 06d3ad425c4928e9e9bff661fbb8f7b4536b8896..7ecea5aac34bb73ca81d94ad481f05b338e65ed0
100644
--- a/src/tests/cwrap/test_become_user.c
+++ b/src/tests/cwrap/test_become_user.c
@@ -76,6 +76,7 @@ void test_switch_user(void **state)
struct passwd *sssd;
TALLOC_CTX *tmp_ctx;
struct sss_creds *saved_creds;
+ struct sss_creds *saved_creds2 = NULL;
check_leaks_push(global_talloc_context);
tmp_ctx = talloc_new(global_talloc_context);
@@ -102,6 +103,12 @@ void test_switch_user(void **state)
assert_int_equal(saved_creds->uid, 0);
assert_int_equal(saved_creds->gid, 0);
+ /* Attempt to restore creds again */
+ ret = switch_creds(tmp_ctx, sssd->pw_uid, sssd->pw_gid,
+ 0, NULL, &saved_creds2);
+ assert_int_equal(ret, EOK);
+ assert_null(saved_creds2);
+
/* restore root */
ret = restore_creds(saved_creds);
assert_int_equal(ret, EOK);
diff --git a/src/util/become_user.c b/src/util/become_user.c
index b5f94f993cd2c23bd3340fc502d36a530aa729fa..8fbc228ad93d16bd076d7920e22fb4b2f83417cb
100644
--- a/src/util/become_user.c
+++ b/src/util/become_user.c
@@ -90,9 +90,14 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
struct sss_creds *ssc = NULL;
int size;
int ret;
+ uid_t myuid;
+ uid_t mygid;
DEBUG(SSSDBG_FUNC_DATA, "Switch user to [%d][%d].\n", uid, gid);
+ myuid = geteuid();
+ mygid = getegid();
+
if (saved_creds) {
/* save current user credentials */
size = getgroups(0, NULL);
@@ -124,8 +129,8 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
}
/* we care only about effective ids */
- ssc->uid = geteuid();
- ssc->gid = getegid();
+ ssc->uid = myuid;
+ ssc->gid = mygid;
}
/* if we are regaining root set euid first so that we have CAP_SETUID back,
@@ -143,6 +148,11 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
/* TODO: use prctl to get/set capabilities too ? */
not your patch, but maybe libcap should be added as an alternative to
the comment?
+ if (myuid == uid && mygid == gid) {
+ DEBUG(SSSDBG_FUNC_DATA, "Already user [%"SPRIuid"].\n",
uid);
+ return EOK;
+ }
+
/* try to setgroups first should always work if CAP_SETUID is set,
* otherwise it will always fail, failure is not critical though as
* generally we only really care about uid and at mot primary gid */
@@ -177,11 +187,9 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
done:
if (ret) {
- if (ssc) {
- /* attempt to restore creds first */
- restore_creds(ssc);
- talloc_free(ssc);
- }
+ /* attempt to restore creds first */
+ restore_creds(ssc);
+ talloc_free(ssc);
} else if (saved_creds) {
*saved_creds = ssc;
}
@@ -190,6 +198,11 @@ done:
errno_t restore_creds(struct sss_creds *saved_creds)
{
+ if (saved_creds == NULL) {
+ /* In case save_creds was saved with the UID already dropped */
+ return EOK;
+ }
+
return switch_creds(saved_creds,
saved_creds->uid,
saved_creds->gid,
--
1.9.3
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel