On Wed, 2013-04-10 at 15:09 +0200, Jakub Hrozek wrote:
On Wed, Apr 10, 2013 at 08:48:08AM -0400, 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.
This patch fixes the error by the way. Basic sanity testing also went
fine and the code looks good to me.
Sorry, meant to say it looks good to me too, but haven't tested.
Simo.
--
Simo Sorce * Red Hat, Inc * New York