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. Related: https://fedorahosted.org/sssd/ticket/2370 Reviewed-by: Sumit Bose <[email protected]> Reviewed-by: Lukáš Slebodník <[email protected]>
SSSD · Nov 18, 2014 · 35b4b21 · 35b4b21
1 parent 2745b01
commit 35b4b21
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 8 deletions.
diff --git 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
@@ -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,
@@ -141,7 +146,12 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
         }
     }
 
-    /* TODO: use prctl to get/set capabilities too ? */
+    /* TODO: use libcap-ng if we need to get/set capabilities too ? */
+
+    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
@@ -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 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
 
 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,