On Wed 19-09-12 18:52:18, Eric W. Biederman wrote:
> From: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
>
> - In setxattr if we are setting a posix acl convert uids and gids from
> the current user namespace into the initial user namespace, before
> the xattrs are passed to the underlying filesystem.
>
> Untranslatable uids and gids are represented as -1 which
> posix_acl_from_xattr will represent as INVALID_UID or INVALID_GID.
> posix_acl_valid will fail if an acl from userspace has any
> INVALID_UID or INVALID_GID values. In net this guarantees that
> untranslatable posix acls will not be stored by filesystems.
>
> - In getxattr if we are reading a posix acl convert uids and gids from
> the initial user namespace into the current user namespace.
>
> Uids and gids that can not be tranlsated into the current user namespace
> will be represented as -1.
>
> - Replace e_id in struct posix_acl_entry with an anymouns union of
> e_uid and e_gid. For the short term retain the e_id field
> until all of the users are converted.
>
> - Don't set struct posix_acl.e_id in the cases where the acl type
> does not use e_id. Greatly reducing the use of ACL_UNDEFINED_ID.
>
> - Rework the ordering checks in posix_acl_valid so that I use kuid_t
> and kgid_t types throughout the code, and so that I don't need
> arithmetic on uid and gid types.
>
> Cc: Theodore Tso <tytso@xxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Andreas Dilger <adilger.kernel@xxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> ---
I know this got merged but I got to seriously looking into it only now
and frankly I think the push was a hurried one...
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 4d45b71..c111745 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -20,6 +20,7 @@
> #include <linux/fsnotify.h>
> #include <linux/audit.h>
> #include <linux/vmalloc.h>
> +#include <linux/posix_acl_xattr.h>
>
> #include <asm/uaccess.h>
>
> @@ -347,6 +348,9 @@ setxattr(struct dentry *d, const char __user *name, const void __user *value,
> error = -EFAULT;
> goto out;
> }
> + if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> + (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> + posix_acl_fix_xattr_from_user(kvalue, size);
> }
>
> error = vfs_setxattr(d, kname, kvalue, size, flags);
> @@ -450,6 +454,9 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
>
> error = vfs_getxattr(d, kname, kvalue, size);
> if (error > 0) {
> + if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> + (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> + posix_acl_fix_xattr_to_user(kvalue, size);
> if (size && copy_to_user(value, kvalue, error))
> error = -EFAULT;
> } else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
> diff --git a/fs/xattr_acl.c b/fs/xattr_acl.c
> index 69d06b0..bf472ca 100644
> --- a/fs/xattr_acl.c
> +++ b/fs/xattr_acl.c
> @@ -9,7 +9,65 @@
> #include <linux/fs.h>
> #include <linux/posix_acl_xattr.h>
> #include <linux/gfp.h>
> +#include <linux/user_namespace.h>
>
> +/*
> + * Fix up the uids and gids in posix acl extended attributes in place.
> + */
> +static void posix_acl_fix_xattr_userns(
> + struct user_namespace *to, struct user_namespace *from,
> + void *value, size_t size)
> +{
> + posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> + posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
> + int count;
> + kuid_t uid;
> + kgid_t gid;
> +
> + if (!value)
> + return;
> + if (size < sizeof(posix_acl_xattr_header))
> + return;
> + if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
> + return;
> +
> + count = posix_acl_xattr_count(size);
> + if (count < 0)
> + return;
> + if (count == 0)
> + return;
> +
> + for (end = entry + count; entry != end; entry++) {
> + switch(le16_to_cpu(entry->e_tag)) {
> + case ACL_USER:
> + uid = make_kuid(from, le32_to_cpu(entry->e_id));
This should have some error checking I guess... The initial checks done
in posix_acl_from_xattr() are for init_user_ns (why?) and only duplicated
in posix_acl_valid().
> + entry->e_id = cpu_to_le32(from_kuid(to, uid));
> + break;
> + case ACL_GROUP:
> + gid = make_kgid(from, le32_to_cpu(entry->e_id));
> + entry->e_id = cpu_to_le32(from_kuid(to, uid));
here should be gid ^^^
> + break;
> + default:
> + break;
> + }
> + }
> +}
Also what about the following scenario:
We have namespace A with user U1 and namespace B which does not have a
valid representation for U1. There is a file F which can be seen from both
namespaces. In namespace A we create acl for user U1 attached to F. Now in
namespace B we modify the acl via setfacl(1) command. What it does is
getxattr(2) - returns mangled acl because U1 has no representation in B. We
add something to xattr and call setxattr(2) - results in removing the
original acl for U1 and instead adding acl for uid -1. That is a security
bug I'd say.
Honza
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
