[PATCH] Add expandable sequences to krb5_ccachedir

[Sumit Bose]

Hi,

this patch should fix #406.

bye,
Sumit

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/24/2010 12:05 PM, Sumit Bose wrote:
>                        restricted deletion flag is created.

Isn't it better to also explicitly say "sticky bit" here? Perhaps with a
link to stat(2) if someone is not familiar with the term?
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkuFI0QACgkQHsardTLnvCW5YQCeLRqghxSgkljtkpVCxRCgk7fh
> mZoAoJ9RohJwQC4837drOTWNhRsUO/7T
> =iDsS
> -----END PGP SIGNATURE-----

[Simo Sorce]

On Wed, 24 Feb 2010 12:05:18 +0100
Sumit Bose  wrote:
> old_umask = umask(0000);
> ret = mkdir(dirname, 01777);
> umask(old_umask);
> if (ret != EOK) {
>    DEBUG(1, ("mkdir [%s] failed: [%d][%s].\n", dirname, errno,
>              strerror(errno)));
>    return errno;
> }

NACK.
We do not want to give every user on the system freedom to write what
they want in these directories.

All parents need to be root.root 0755

If the admins wants it differently they will have to pre-create the
parent directories themselves.

Simo.

[Sumit Bose]

On Wed, Feb 24, 2010 at 09:56:47AM -0500, Simo Sorce wrote:
> On Wed, 24 Feb 2010 12:05:18 +0100
> Sumit Bose  wrote:
> > old_umask = umask(0000);
> > ret = mkdir(dirname, 01777);
> > umask(old_umask);
> > if (ret != EOK) {
> >    DEBUG(1, ("mkdir [%s] failed: [%d][%s].\n", dirname, errno,
> >              strerror(errno)));
> >    return errno;
> > }
>
> NACK.
> We do not want to give every user on the system freedom to write what
> they want in these directories.
>
> All parents need to be root.root 0755
>
> If the admins wants it differently they will have to pre-create the
> parent directories themselves.
>
> Simo.

New version attached.

Additionally to the issues mentions above the templates are only parsed
once and the expanded path is checked for suspicious patterns like
'/../'. If a private directory is created it is chown'd immediately
after its creation.

There are two questions where I do not know a good answer:

- The parent directories are always created with 0755 permissions and
  the owner is root. Should we handle the case where the first existing
  directory does not belong to root, e.g. if we have
  /home/u1/a/b/c/ccache_dir/ccfile and a, b and c have to be created?

- The sequence '%P' is expanded to the client's PID. I think the PID
  does not qualify to be unique for a user and consequently a ccache
  directory with only this sequence and no other user specific sequences
  like '%u' is created as public directory. Do you agree?

bye,
Sumit

[Sumit Bose]

On Fri, Feb 26, 2010 at 04:43:56PM +0100, Sumit Bose wrote:
> On Wed, Feb 24, 2010 at 09:56:47AM -0500, Simo Sorce wrote:
> > On Wed, 24 Feb 2010 12:05:18 +0100
> > Sumit Bose  wrote:
> > > old_umask = umask(0000);
> > > ret = mkdir(dirname, 01777);
> > > umask(old_umask);
> > > if (ret != EOK) {
> > >    DEBUG(1, ("mkdir [%s] failed: [%d][%s].\n", dirname, errno,
> > >              strerror(errno)));
> > >    return errno;
> > > }
> >
> > NACK.
> > We do not want to give every user on the system freedom to write what
> > they want in these directories.
> >
> > All parents need to be root.root 0755
> >
> > If the admins wants it differently they will have to pre-create the
> > parent directories themselves.
> >
> > Simo.
>
> New version attached.
>
> Additionally to the issues mentions above the templates are only parsed
> once and the expanded path is checked for suspicious patterns like
> '/../'. If a private directory is created it is chown'd immediately
> after its creation.
>
> There are two questions where I do not know a good answer:
>
> The parent directories are always created with 0755 permissions and
> the owner is root. Should we handle the case where the first existing
> directory does not belong to root, e.g. if we have
> /home/u1/a/b/c/ccache_dir/ccfile and a, b and c have to be created?
>
> The sequence '%P' is expanded to the client's PID. I think the PID
> does not qualify to be unique for a user and consequently a ccache
> directory with only this sequence and no other user specific sequences
> like '%u' is created as public directory. Do you agree?

rebased to current master

bye,
Sumit

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/08/2010 04:16 AM, Sumit Bose wrote:
> On Fri, Feb 26, 2010 at 04:43:56PM +0100, Sumit Bose wrote:
> > On Wed, Feb 24, 2010 at 09:56:47AM -0500, Simo Sorce wrote:
> > > On Wed, 24 Feb 2010 12:05:18 +0100
> > > Sumit Bose  wrote:
> > > > old_umask = umask(0000);
> > > > ret = mkdir(dirname, 01777);
> > > > umask(old_umask);
> > > > if (ret != EOK) {
> > > >    DEBUG(1, ("mkdir [%s] failed: [%d][%s].\n", dirname, errno,
> > > >              strerror(errno)));
> > > >    return errno;
> > > > }
> > >
> > > NACK.
> > > We do not want to give every user on the system freedom to write what
> > > they want in these directories.
> > >
> > > All parents need to be root.root 0755
> > >
> > > If the admins wants it differently they will have to pre-create the
> > > parent directories themselves.
> > >
> > > Simo.
> >
> > New version attached.
> >
> > Additionally to the issues mentions above the templates are only parsed
> > once and the expanded path is checked for suspicious patterns like
> > '/../'. If a private directory is created it is chown'd immediately
> > after its creation.
> >
> > There are two questions where I do not know a good answer:
> > The parent directories are always created with 0755 permissions and
> > the owner is root. Should we handle the case where the first existing
> > directory does not belong to root, e.g. if we have
> > /home/u1/a/b/c/ccache_dir/ccfile and a, b and c have to be created?

Yeah, if (and only if) the first existing directory belongs to the user
trying to create the cache file, then the directories it creates should
also belong to the user. If the highest existing directory is owned by
root, then the ownership should be retained.

If the highest existing directory is owned by some other user, it should
be an error.
> > The sequence '%P' is expanded to the client's PID. I think the PID
> > does not qualify to be unique for a user and consequently a ccache
> > directory with only this sequence and no other user specific sequences
> > like '%u' is created as public directory. Do you agree?

I personally contest that PID is a useful variable to handle here. I
can't really think of a time that you would really want a per-pid
credential cache.
> rebased to current master
>
> bye,
> Sumit

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkuVNwwACgkQeiVVYja6o6Mx5QCeMa+1jmIUgRXJVBKQME7q8S6z
> +JIAoJAkvH5tAHxTLEeNGygY7CQjkBbm
> =29w0
> -----END PGP SIGNATURE-----

[Sumit Bose]

On Mon, Mar 08, 2010 at 12:42:36PM -0500, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 03/08/2010 04:16 AM, Sumit Bose wrote:
> > On Fri, Feb 26, 2010 at 04:43:56PM +0100, Sumit Bose wrote:
> > > On Wed, Feb 24, 2010 at 09:56:47AM -0500, Simo Sorce wrote:
> > > > On Wed, 24 Feb 2010 12:05:18 +0100
> > > > Sumit Bose  wrote:
> > > > > old_umask = umask(0000);
> > > > > ret = mkdir(dirname, 01777);
> > > > > umask(old_umask);
> > > > > if (ret != EOK) {
> > > > >    DEBUG(1, ("mkdir [%s] failed: [%d][%s].\n", dirname, errno,
> > > > >              strerror(errno)));
> > > > >    return errno;
> > > > > }
> > > >
> > > > NACK.
> > > > We do not want to give every user on the system freedom to write what
> > > > they want in these directories.
> > > >
> > > > All parents need to be root.root 0755
> > > >
> > > > If the admins wants it differently they will have to pre-create the
> > > > parent directories themselves.
> > > >
> > > > Simo.
> > >
> > > New version attached.
> > >
> > > Additionally to the issues mentions above the templates are only parsed
> > > once and the expanded path is checked for suspicious patterns like
> > > '/../'. If a private directory is created it is chown'd immediately
> > > after its creation.
> > >
> > > There are two questions where I do not know a good answer:
> > > The parent directories are always created with 0755 permissions and
> > > the owner is root. Should we handle the case where the first existing
> > > directory does not belong to root, e.g. if we have
> > > /home/u1/a/b/c/ccache_dir/ccfile and a, b and c have to be created?
>
> Yeah, if (and only if) the first existing directory belongs to the user
> trying to create the cache file, then the directories it creates should
> also belong to the user. If the highest existing directory is owned by
> root, then the ownership should be retained.
>
> If the highest existing directory is owned by some other user, it should
> be an error.
> > > The sequence '%P' is expanded to the client's PID. I think the PID
> > > does not qualify to be unique for a user and consequently a ccache
> > > directory with only this sequence and no other user specific sequences
> > > like '%u' is created as public directory. Do you agree?
>
> I personally contest that PID is a useful variable to handle here. I
> can't really think of a time that you would really want a per-pid
> credential cache.

I have disabled the '%P' sequence for the directory expansion and change
setting of the permissions as described above.

bye,
Sumit
> > rebased to current master
> >
> > bye,
> > Sumit
>
> Stephen Gallagher
> RHCE 804006346421761
>
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkuVNwwACgkQeiVVYja6o6Mx5QCeMa+1jmIUgRXJVBKQME7q8S6z
> +JIAoJAkvH5tAHxTLEeNGygY7CQjkBbm
> =29w0
> -----END PGP SIGNATURE-----
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/09/2010 12:30 PM, Sumit Bose wrote:
> > I have disabled the '%P' sequence for the directory expansion and change
> > setting of the permissions as described above.
> > bye,
> > Sumit

Nack

Using "expand_ccache_dir" as the boolean for disallowing %P is
confusing. Please rename the variable to make it more obvious why you
can't use %P. (Maybe "bool restrict_substitions")

Using lstat() in find_ccdir_parent_data() is a bad idea. We should just
be using stat() to determine if we're ultimately dealing with a
directory, unless your intent here is to mandate that krb5_ccachedir
must never contain a symlink, in which case this needs to be documented
in the manpage, along with why.

Please save errno before calling DEBUG(). It won't always be guaranteed
that errno is unchanged within the DEBUG macro. This occurs in both
find_ccdir_parent_data() and create_ccache_dir().

li->s = talloc_strdup(mem_ctx, dirname);
Instead of mem_ctx, you should be using li for the memory parent.

Please talloc_zfree(parent) before returning from
find_ccdir_parent_data(), otherwise you'll be carrying that unnecessary
memory around.

You have similar memory issues in create_ccache_dir(). You create
dirname with the mem_ctx as the parent, but you never free it. And since
you're moving the pointer on it, you probably want to save the start of
the string so you can call talloc_free() on that.

if (!private_path && (parent_stat.st_uid != 0 || parent_stat.st_gid != 0))

This check doesn't make sense to me. You're only going to allow the
creation of a public directory if the parent dir is owned by root and
has group root? I think the check should be for
(parent_stat.st_mode & (S_ISVTX | S_IWOTH | S_IXOTH)) (aka
world-writeable and world-executable and sticky-bit)

Similarly, the private path needs to check for
(parent_stat.st_uid == uid && parent_stat.st_mode & (S_IWUSR | S_IXUSR))
 ||
(parent_stat.st_mode & (S_ISVTX | S_IWOTH | S_IXOTH))

(aka owned by the current user and writeable/executable or
(world-writeable, world-executable and sticky-bit))

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkuXqmEACgkQeiVVYja6o6N7GACgkBeQm+NcZ4l0BCOKhlNgiaka
> op4AoKXik40TL9yPpQUOwKxV0JJqDi5b
> =GnHu
> -----END PGP SIGNATURE-----

[Sumit Bose]

On Wed, Mar 10, 2010 at 09:19:14AM -0500, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 03/09/2010 12:30 PM, Sumit Bose wrote:
> > > I have disabled the '%P' sequence for the directory expansion and change
> > > setting of the permissions as described above.
> > > bye,
> > > Sumit
>
> Nack
>
> Using "expand_ccache_dir" as the boolean for disallowing %P is
> confusing. Please rename the variable to make it more obvious why you
> can't use %P. (Maybe "bool restrict_substitions")

done, renamed to file_mode
> Using lstat() in find_ccdir_parent_data() is a bad idea. We should just
> be using stat() to determine if we're ultimately dealing with a
> directory, unless your intent here is to mandate that krb5_ccachedir
> must never contain a symlink, in which case this needs to be documented
> in the manpage, along with why.

done
> Please save errno before calling DEBUG(). It won't always be guaranteed
> that errno is unchanged within the DEBUG macro. This occurs in both
> find_ccdir_parent_data() and create_ccache_dir().

done
> li->s = talloc_strdup(mem_ctx, dirname);
> Instead of mem_ctx, you should be using li for the memory parent.

done
> Please talloc_zfree(parent) before returning from
> find_ccdir_parent_data(), otherwise you'll be carrying that unnecessary
> memory around.
>
> You have similar memory issues in create_ccache_dir(). You create
> dirname with the mem_ctx as the parent, but you never free it. And since
> you're moving the pointer on it, you probably want to save the start of
> the string so you can call talloc_free() on that.

done, additionally I introduced a temporary talloc context to get rid of
other allocated memory, too.
> if (!private_path && (parent_stat.st_uid != 0 || parent_stat.st_gid != 0))
>
> This check doesn't make sense to me. You're only going to allow the
> creation of a public directory if the parent dir is owned by root and
> has group root? I think the check should be for
> (parent_stat.st_mode & (S_ISVTX | S_IWOTH | S_IXOTH)) (aka
> world-writeable and world-executable and sticky-bit)
>
> Similarly, the private path needs to check for
> (parent_stat.st_uid == uid && parent_stat.st_mode & (S_IWUSR | S_IXUSR))
>  ||
> (parent_stat.st_mode & (S_ISVTX | S_IWOTH | S_IXOTH))
>
> (aka owned by the current user and writeable/executable or
> (world-writeable, world-executable and sticky-bit))

the sticky-bit won't help here if the directory does not belong to root.
However I added the x-bit check and removed the gid check.

New version attached.

bye,
Sumit
> Stephen Gallagher
> RHCE 804006346421761
>
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkuXqmEACgkQeiVVYja6o6N7GACgkBeQm+NcZ4l0BCOKhlNgiaka
> op4AoKXik40TL9yPpQUOwKxV0JJqDi5b
> =GnHu
> -----END PGP SIGNATURE-----
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/11/2010 07:02 AM, Sumit Bose wrote:
> > New version attached.
> > bye,
> > Sumit

Ack.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkuY/FcACgkQeiVVYja6o6OrgwCeI3AvzipubIqNDVQkGkwzLlPr
> NoAAn3ikr85npjBlEwKHGxQ2aT/XDPz6
> =EVnS
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/11/2010 09:21 AM, Stephen Gallagher wrote:
> On 03/11/2010 07:02 AM, Sumit Bose wrote:
> > > New version attached.
> > > bye,
> > > Sumit
>
> Ack.

Pushed to master and 1-1-0

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkuY/McACgkQeiVVYja6o6OR9wCfZUvMYCa6HVz4kgxht+6tTFrh
> KWEAoInmCOrWutFSrl553UIXxzvrRYeA
> =wRyJ
> -----END PGP SIGNATURE-----