On Fri, 2013-09-06 at 15:04 +0200, Sumit Bose wrote:
On Tue, Sep 03, 2013 at 10:07:13PM -0400, Simo Sorce wrote:
> After the recent patches to explicitly enable the KEYRING type in SSSD I
> realized that the code that manipulates ccaches had grown too much, and,
> most importantly, was doing unnecessary operations already performed in
> an abstract way by krb5 functions.
>
> This patch set mostly addresses ticket #2061
>
> The aims has been to remove as much as possible type-specific code,
> resorting to type specific behavior only as an explicit exception where
> necessary due to historical or other reasons.
>
> The combined diff gives a nice total stat of:
> 815 insertions(+)
> 1529 deletions(-)
>
>
> The last patch is an attempt to address ticket #2071,
>
> It was necessary to add it here otherwise sssd will fail to operate
> correctly with some templates (as noted in #2071).
> However I am not sure that's the way we want to resolve the problem.
> The patch aimed at maintaining as much as possible a reasonable
> behavior, although the intended behavior was not really written
> anywhere. Personally I would rather scrap the patch and instead provide
> a new one that would simply stop creating public directories at all, I
> do not think it is sssd's role to fix/create directories that should be
> set up by the admin appropriately ahead of time (either manually of via
> tmpfiles.d or whatever).
>
> I will try to follow up with a proposed patch that 'simplifies' sssd
> behavior instead of fixing it for #2071
>
> Simo.
>
> --
> Simo Sorce * Red Hat, Inc * New York
I've reviewed the first 14 patches and my comments and the result of a
discussion on irc about how security can be improved when running as a
user are below.
Thank you, comments inline and new patchset attached.
I'll review the 15th patch together with the alternative one and
send my
comments separately.
bye,
Sumit
[..]
ret, strerror(ret)));
> + /* free ssc immediately otherwise the code will try to restore
> + * wrong creds */
> + free(ssc);
talloc_free or talloc_zfree
ouch thanks a lot for catching this one.
[..]
> + /* change gid now, (leaves saved gid to current, so we can
restore) */
> + ret = setresgid(gid, gid, -1);
only setting the egid should be sufficient and safer
[..]
> + if (uid != 0) {
> + /* change uid, (leaves saved uid to current, so we can restore) */
> + ret = setresuid(uid, uid, -1);
only setting the euid should be sufficient and safer
As discussed on IRC change both the above to retain the real uid/gid by
leaving -1 in there, this way a user process cannot successfully send
SIGKILL to sssd_be.
[..]
> +static errno_t handle_randomized(TALLOC_CTX *mem_ctx, char
*in)
mem_ctx is not used in handle_randomized()
Ah remnants of a previous version, removed the mem_ctx here and also
removed the tmp_ctx from the calling function, as handle_randomized()
was actually the only user.
Simo.
--
Simo Sorce * Red Hat, Inc * New York