On Fri, 2013-04-05 at 15:02 +0200, Lukas Slebodnik wrote:
> On (05/04/13 08:47), Simo Sorce wrote:
> >On Fri, 2013-04-05 at 12:26 +0200, Lukas Slebodnik wrote:
> >> On (04/04/13 12:24), Simo Sorce wrote:
> >> >Commit should say it all.
> >> >We do not have any security issue (that I know off) with the current
> >> >code, but I want to tighten up the privileges more given we do not need
> >> >the additional capabilities in the krb5_child anyway.
> >> >
> >> >Simo.
> >> >
> >> >--
> >> >Simo Sorce * Red Hat, Inc * New York
> >> Nack
> >>
> >> Patch make impossible user authentication.
> >>
> >> sh-4.2$ su - usersssd02
> >> Password:
> >> su: incorrect password
> >>
> >>
> >> From krb5_child.log:
> >> [become_user] (0x0200): Trying to become user [325600012][325600012].
> >> [create_ccache_in_dir] (0x0200): Creating ccache at
> >> [DIR:/run/user/325600012/krb5cc]
> >> [become_user] (0x0200): Trying to become user [325600012][325600012].
> >> [become_user] (0x0020): setgroups failed [1][Operation not permitted].
> >> ^^^^^
> >> The second call of function become_user fail with EPERM
> >>
> >> [create_ccache_in_dir] (0x0020): become_user failed.
> >> [get_and_save_tgt] (0x0020): 1140: [1][Operation not permitted]
> >> [map_krb5_error] (0x0020): 1160: [1][Operation not permitted]
> >>
> >> > errno_t become_user(uid_t uid, gid_t gid)
> >> > {
> >> > int ret;
> >> >
> >> > DEBUG(SSSDBG_FUNC_DATA, ("Trying to become user [%d][%d].\n", uid,
> >> > gid));
> >> >- ret = setgid(gid);
> >> >- if (ret == -1) {
> >> >- ret = errno;
> >> >- DEBUG(SSSDBG_CRIT_FAILURE,
> >> >- ("setgid failed [%d][%s].\n", ret, strerror(ret)));
> >> >- return ret;
> >> >- }
> >> >
> >> >- ret = setuid(uid);
> >> >+ /* drop supplmentary groups first */
> >> >+ ret = setgroups(0, NULL);
> >> > if (ret == -1) {
> >> > ret = errno;
> >> If errno is EPERM, than we should ignore this error and continue.
> >>
> >> > DEBUG(SSSDBG_CRIT_FAILURE,
> >> >- ("setuid failed [%d][%s].\n", ret, strerror(ret)));
> >> >+ ("setgroups failed [%d][%s].\n", ret, strerror(ret)));
> >> > return ret;
> >> > }
> >> >
> >
> >It is very odd that we get EPERM .. is this function beeing called
> >twice ? Once as root before the fork, and then again in the code ?
> >
> >Simo.
>
> Yes twice.
>
> 1st call:
> --------------------------------------------
> #0 become_user (uid=325600012, gid=325600012)
> at src/providers/krb5/krb5_become_user.c:29
> #1 0x00000000004101dd in get_and_save_tgt (kr=kr@entry=0x227f090,
> password=<optimized out>) at src/providers/krb5/krb5_child.c:1128
> #2 0x0000000000407972 in tgt_req_child (kr=0x227f090)
> at src/providers/krb5/krb5_child.c:1337
> #3 main (argc=<optimized out>, argv=<optimized out>)
> at src/providers/krb5/krb5_child.c:2126
>
>
> 2nd call:
> --------------------------------------------
> #0 become_user (uid=uid@entry=325600012, gid=gid@entry=325600012)
> at src/providers/krb5/krb5_become_user.c:29
> #1 0x000000000040b569 in create_ccache_in_dir (uid=uid@entry=325600012,
> gid=gid@entry=325600012, ctx=ctx@entry=0x2280010,
> princ=princ@entry=0x2287760,
> ccname=ccname@entry=0x227f280 "DIR:/run/user/325600012/krb5cc",
> creds=creds@entry=0x22807a0) at src/providers/krb5/krb5_child.c:659
> #2 0x000000000040eb7c in create_ccache (uid=325600012, gid=325600012,
> ctx=0x2280010, princ=0x2287760,
> ccname=0x227f280 "DIR:/run/user/325600012/krb5cc", creds=0x22807a0)
> at src/providers/krb5/krb5_child.c:732
> #3 0x000000000041020a in get_and_save_tgt (kr=kr@entry=0x227f090,
> password=<optimized out>) at src/providers/krb5/krb5_child.c:1136
> #4 0x0000000000407972 in tgt_req_child (kr=0x227f090)
> at src/providers/krb5/krb5_child.c:1337
> #5 main (argc=<optimized out>, argv=<optimized out>)
> at src/providers/krb5/krb5_child.c:2126
Ok, attached augmented patch should handle this case too.
Thanks for testing.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
>From 1719aa02504afa1c98f3227966ab0394e81bc50b Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Thu, 4 Apr 2013 11:32:51 -0400
Subject: [PATCH] Further restrict become_user drop of privileges.
We never need to regain root after we call become_user() so tighten up even
further our privilege drop.
Add a setgroups() call to remove all secondary groups root may have been given
for whateve reason. Then use the setres[ug]id function to also drop the saved
uid/gid so the process cannot regain back root id.
Capabilities are also implicitly dropped here, no more CAP_SETUID so this is a
Point of No Return, once changed to non-root the process can't get back.
Remove redefinition of sys/types.h and unistd.h, they are already defined in
util.h and they need to be included after _GNU_SOURCE/_BSD_SOURCE is defined
or the prototypes for setres[ug]id will not be found.
Add grp.h after util.h for the same reason.
---
src/providers/krb5/krb5_become_user.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/src/providers/krb5/krb5_become_user.c b/src/providers/krb5/krb5_become_user.c
index 082d141534cd12a02ee8a0ed5fa66114e7adfc73..70bc5630ece21505b58bd1a8795d7ab4b7864460 100644
--- a/src/providers/krb5/krb5_become_user.c
+++ b/src/providers/krb5/krb5_become_user.c
@@ -22,45 +22,48 @@
along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#include <sys/types.h>
-#include <unistd.h>
-
#include "util/util.h"
+#include <grp.h>
errno_t become_user(uid_t uid, gid_t gid)
{
+ uid_t cuid;
int ret;
DEBUG(SSSDBG_FUNC_DATA, ("Trying to become user [%d][%d].\n", uid, gid));
- ret = setgid(gid);
- if (ret == -1) {
- ret = errno;
- DEBUG(SSSDBG_CRIT_FAILURE,
- ("setgid failed [%d][%s].\n", ret, strerror(ret)));
- return ret;
+
+ /* skip call if we already are the requested user */
+ cuid = geteuid();
+ if (uid == cuid) {
+ DEBUG(SSSDBG_FUNC_DATA, ("Already user [%d].\n", uid));
+ return EOK;
}
- ret = setuid(uid);
+ /* drop supplmentary groups first */
+ ret = setgroups(0, NULL);
if (ret == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
- ("setuid failed [%d][%s].\n", ret, strerror(ret)));
+ ("setgroups failed [%d][%s].\n", ret, strerror(ret)));
return ret;
}
- ret = setegid(gid);
+ /* change gid so that root cannot be regained (changes saved gid too) */
+ ret = setresgid(gid, gid, gid);
if (ret == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
- ("setegid failed [%d][%s].\n", ret, strerror(ret)));
+ ("setresgid failed [%d][%s].\n", ret, strerror(ret)));
return ret;
}
- ret = seteuid(uid);
+ /* change uid so that root cannot be regained (changes saved uid too) */
+ /* this call also takes care of dropping CAP_SETUID, so this is a PNR */
+ ret = setresuid(uid, uid, uid);
if (ret == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
- ("seteuid failed [%d][%s].\n", ret, strerror(ret)));
+ ("setresuid failed [%d][%s].\n", ret, strerror(ret)));
return ret;
}
--
1.8.1.4
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
