-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Serge.
Three comments in-line:
Serge E. Hallyn wrote:
>>From 24e8c76b50cc40b37ef65b69405cf6d6fa02865f Mon Sep 17 00:00:00 2001
> From: Serge E. Hallyn <[EMAIL PROTECTED]>
> Date: Thu, 12 Jul 2007 12:21:25 -0400
> Subject: [PATCH 1/1] file capabilities: change xattr format and eff meaning
>
> This patch implements three suggestions by Andrew Morgan. The first
> is to change the on disk format. The __le32 version, is renamed
> 'magic_etc', and contains a revision number (starting at 1), and
> a bit representing Fe. The Fe capset is no longer stored, so the
> xattr is now 3 ints instead of 4.
>
> The Fe was previously a full capset which was masked with
> the calculated new_permitted to get the process' new effective
> set. It is now a single bit. When that bit is off, the
> P'e is the empty set. When the bit is on, then P'e is set
> equal to P'p (new_permitted). The rationale for this is
> that either the application does not know about capabilities,
> and needs to start with all permitted caps in its effective
> set, or it does know about capabilities, and can start with
> an empty effective set and enable the caps it wants when it
> wants.
>
> Finally, future format compatibility is reduced. If
> a security.capability xattr is found with too new a version,
> don't run the binary. (It may be better to at least just ignore
> the xattr)
(FWIW I don't agree this would be better. If the running kernel sees a
privilege requirement that it cannot fulfill - because it doesn't
understand it - it should fail safe.)
>
> (I'll send an (17k) ltp patch to test this to anyone who asks.)
>
> Signed-off-by: Serge E. Hallyn <[EMAIL PROTECTED]>
> ---
> include/linux/capability.h | 35 ++++++---------------
> security/commoncap.c | 71 +++++++++++++++++--------------------------
> 2 files changed, 38 insertions(+), 68 deletions(-)
>
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index cdfaa10..0ee4f91 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -47,38 +47,23 @@ typedef struct __user_cap_data_struct {
> #define XATTR_CAPS_SUFFIX "capability"
> #define XATTR_NAME_CAPS XATTR_SECURITY_PREFIX XATTR_CAPS_SUFFIX
>
> -/* size of caps that we work with */
> -#define XATTR_CAPS_SZ (4*sizeof(__le32))
> +#define XATTR_CAPS_SZ (3*sizeof(__le32))
> +#define VFS_CAP_REVISION_MASK 0xFF000000
> +#define VFS_CAP_REVISION 0x01000000
>
> -/*
> - * data[] is organized as:
> - * effective[0]
> - * permitted[0]
> - * inheritable[0]
> - * effective[1]
> - * ...
> - * this way we can just read as much of the on-disk capability as
> - * we know should exist and know we'll get the data we'll need.
> - */
> -struct vfs_cap_data_disk {
> - __le32 version;
> - __le32 data[]; /* eff[0], perm[0], inh[0], eff[1], ... */
> -};
> -
> -struct vfs_cap_data_disk_v1 {
> - __le32 version;
> - __le32 data[3]; /* eff[0], perm[0], inh[0] */
> -};
> +#define VFS_CAP_FLAGS_MASK ~VFS_CAP_REVISION_MASK
> +#define VFS_CAP_FLAGS_EFFECTIVE 0x000001
>
> #ifdef __KERNEL__
>
> #include <asm/current.h>
>
> struct vfs_cap_data {
> - __u32 version;
> - __u32 effective;
> - __u32 permitted;
> - __u32 inheritable;
> + __u32 magic_etc; /* Little endian */
> + struct {
> + __u32 permitted; /* Little endian */
> + __u32 inheritable; /* Little endian */
> + } data[1];
> };
>
> /* #define STRICT_CAP_T_TYPECHECKS */
> diff --git a/security/commoncap.c b/security/commoncap.c
> index b08d250..284bbb5 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -118,27 +118,26 @@ static inline void bprm_clear_caps(struct linux_binprm
> *bprm)
>
> #ifdef CONFIG_SECURITY_FILE_CAPABILITIES
>
> -static inline int cap_from_disk(struct vfs_cap_data_disk *dcap,
> - struct linux_binprm *bprm, int size)
> +static inline int cap_from_disk(__le32 *caps, struct linux_binprm *bprm,
> + int size)
> {
> - int version;
magic_etc seems like it should be __u32:
> + int magic_etc;
>
> - version = le32_to_cpu(dcap->version);
> - if (version != _LINUX_CAPABILITY_VERSION)
> + if (size != XATTR_CAPS_SZ)
> return -EINVAL;
>
> - size /= sizeof(u32);
> - if ((size-1)%3) {
> - printk(KERN_WARNING "%s: size is an invalid size %d for %s\n",
> - __FUNCTION__, size,
> bprm->filename);
> + magic_etc = le32_to_cpu(caps[0]);
> +
> + switch ((magic_etc & VFS_CAP_REVISION_MASK)) {
> + case VFS_CAP_REVISION:
> + bprm->cap_effective = (magic_etc & VFS_CAP_FLAGS_EFFECTIVE)
> + ? CAP_FULL_SET : CAP_EMPTY_SET;
> + bprm->cap_permitted = to_cap_t( le32_to_cpu(caps[1]) );
> + bprm->cap_inheritable = to_cap_t( le32_to_cpu(caps[2]) );
> + return 0;
> + default:
> return -EINVAL;
> }
> -
> - bprm->cap_effective = le32_to_cpu(dcap->data[0]);
> - bprm->cap_permitted = le32_to_cpu(dcap->data[1]);
> - bprm->cap_inheritable = le32_to_cpu(dcap->data[2]);
> -
> - return 0;
> }
>
> /* Locate any VFS capabilities: */
> @@ -146,11 +145,9 @@ static int get_file_caps(struct linux_binprm *bprm)
> {
> struct dentry *dentry;
> int rc = 0;
> - struct vfs_cap_data_disk_v1 v1caps;
> - struct vfs_cap_data_disk *dcaps;
> + __le32 v1caps[XATTR_CAPS_SZ];
> struct inode *inode;
>
> - dcaps = (struct vfs_cap_data_disk *)&v1caps;
> if (bprm->file->f_vfsmnt->mnt_flags & MNT_NOSUID) {
> bprm_clear_caps(bprm);
> return 0;
> @@ -161,40 +158,23 @@ static int get_file_caps(struct linux_binprm *bprm)
> if (!inode->i_op || !inode->i_op->getxattr)
> goto out;
>
> - rc = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, dcaps,
> + rc = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, &v1caps,
> XATTR_CAPS_SZ);
> if (rc == -ENODATA || rc == -EOPNOTSUPP) {
> + /* no data, that's ok */
> rc = 0;
> goto out;
> }
> - if (rc == -ERANGE) {
> - int size;
> - size = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, NULL, 0);
> - if (size <= 0) { /* shouldn't ever happen */
> - rc = -EINVAL;
> - goto out;
> - }
> - dcaps = kmalloc(size, GFP_KERNEL);
> - if (!dcaps) {
> - rc = -ENOMEM;
> - goto out;
> - }
> - rc = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, dcaps,
> - size);
> - }
> if (rc < 0)
> goto out;
> - if (rc < sizeof(struct vfs_cap_data_disk_v1)) {
> - rc = -EINVAL;
> - goto out;
> - }
>
> - rc = cap_from_disk(dcaps, bprm, rc);
> + rc = cap_from_disk(v1caps, bprm, rc);
> + if (rc)
> + printk(KERN_NOTICE "%s: cap_from_disk returned %d for %s\n",
> + __FUNCTION__, rc, bprm->filename);
>
> out:
> dput(dentry);
> - if ((void *)dcaps != (void *)&v1caps)
> - kfree(dcaps);
> if (rc)
> bprm_clear_caps(bprm);
>
> @@ -214,6 +194,9 @@ int cap_bprm_set_security (struct linux_binprm *bprm)
> int ret;
>
> ret = get_file_caps(bprm);
> + if (ret)
> + printk(KERN_NOTICE "%s: get_file_caps returned %d for %s\n",
> + __FUNCTION__, ret, bprm->filename);
>
> /* To support inheritance of root-permissions and suid-root
> * executables under compatibility mode, we raise all three
> @@ -269,8 +252,10 @@ void cap_bprm_apply_creds (struct linux_binprm *bprm,
> int unsafe)
> * capability rules */
> if (!is_init(current)) {
> current->cap_permitted = new_permitted;
I'm not sure I agree with this next change. If you are going to change
this code, you can get by with an abbreviated (bool) bprm->cap_effective
(it doesn't need to be stored within the kernel as a full set).
For now, I would just leave this part of the patch out.
> - current->cap_effective =
> - cap_intersect (new_permitted, bprm->cap_effective);
> + if (cap_isclear(bprm->cap_effective))
> + cap_clear(current->cap_effective);
> + else
> + current->cap_effective = new_permitted;
> }
>
> /* AUD: Audit candidate if current->cap_effective is set */
Cheers
Andrew
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFGl5jh+bHCR3gb8jsRArXrAJ4sMbGTQiAdN3uZFiohOmKI50FcAQCfWJDw
a4CzlMKcjprEj/Lfxl2c0n0=
=uElm
-----END PGP SIGNATURE-----
-
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
