On Wed, Apr 10, 2013 at 06:57:07PM +0200, Lukas Slebodnik wrote:
> On (10/04/13 08:48), Simo Sorce wrote:
> >On Wed, 2013-04-10 at 11:45 +0200, Lukas Slebodnik wrote:
> >> On (04/04/13 12:09), Simo Sorce wrote:
> >> >On Thu, 2013-04-04 at 12:14 +0200, Lukas Slebodnik wrote:
> >> >> ehlo,
> >> >>
> >> >> In krb5-libs 1.11, function krb5_cc_resolve verify if credential cache
> >> >> dir
> >> >> exists. If it doesn't exist, than it will be created. We
> >> >> have to ensure, that it will be created with right uid, gid and
> >> >> permissions.
> >> >>
> >> >> There is check in krb_auth_send, whether cached user data in ldb
> >> >> contains
> >> >> SYSDB_CCACHE_FILE attribute. If it is available, then old ccache is
> >> >> checked in
> >> >> function check_old_ccache. Gdm clean user directory (in /run/user/)
> >> >> after logout. This is the reason why SYSDB_CCACHE_FILE attribute was
> >> >> found
> >> >> in ldb cache and directory did not exist and then in function
> >> >> krb5_cc_resolve
> >> >> was created with wrong permissions.
> >> >>
> >> >> Patch attached.
> >> >
> >> >Although this approach may work I am not really comfortable with it.
> >> >
> >> >Here is an alternative approach:
> >> >
> >> >1. Before calling krb5_cc_resolve() do a stat.
> >> function cc_residual_is_used call stat, but nonexisting file(directory)
> >> was ignored (handled like EOK)
> >
> >Time to un-ignore it :-)
> >
> >> >2. If the file/dir does not exist just return an error w/o calling any
> >> >krb5 function at all.
> >> >
> >> >I think this should also solve the issue, and will not involve change of
> >> >uid/gid.
> >> >
> >> >This method has a very small race that can hardly be triggered though,
> >> >it would require someone to remove the cache after the stat and before
> >> >krb5_cc_resolve() is called. In that case auth would fail, but we should
> >> >be able to recover on the next authentication by removing the file if it
> >> >has the wrong permissions (I think this is already done, if not we can
> >> >fix it with a second patch that adds checking for uid/gid after the stat
> >> >and a call to cc_file_rmove/cc_dir_remove).
> >> cc_dir_remove call cc_file_remove, so directory will not be removed.
> >
> >That is fine, we nee to check for dir/primary in dir case I think.
> >
> >Simo.
> >
> >--
> >Simo Sorce * Red Hat, Inc * New York
> >
>
> Attaching updated patch, which also check dir/primary.
>
> LS
Just one nitpick, can you change this block:
+ primary_file = talloc_strdup_append(discard_const_p(char, dir),
+ "/primary");
+ if (!primary_file) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_strdup_append failed.\n"));
+ ret = ENOMEM;
+ goto done;
+ }
Change dir to be "char *" not "const char*" and use
talloc_asprintf(tmp_ctx, "%s/%s", dir);
I wonder how the code works correctly for you, I would have expected it
to work only if primary_file was already a talloc pointer.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
