* Re: [PATCH v3 05/25] user_namespace: refactor map_write()
From: Serge E. Hallyn @ 2020-02-19 2:35 UTC (permalink / raw)
To: Christian Brauner
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn,
smbarber, Seth Forshee, Alexander Viro, Alexey Dobriyan,
Serge Hallyn, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, linux-kernel, linux-fsdevel, containers,
linux-security-module, linux-api
In-Reply-To: <20200218143411.2389182-6-christian.brauner@ubuntu.com>
On Tue, Feb 18, 2020 at 03:33:51PM +0100, Christian Brauner wrote:
> Refactor map_write() to prepare for adding fsid mappings support. This mainly
> factors out various open-coded parts into helpers that can be reused in the
> follow up patch.
>
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
> /* v2 */
> patch not present
>
> /* v3 */
> patch added
> - Jann Horn <jannh@google.com>:
> - Split changes to map_write() to implement fsid mappings into three separate
> patches: basic fsid helpers, preparatory changes to map_write(), actual
> fsid mapping support in map_write().
> ---
> kernel/user_namespace.c | 117 +++++++++++++++++++++++++---------------
> 1 file changed, 74 insertions(+), 43 deletions(-)
>
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 2cfd1e519cc4..e91141262bcc 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -1038,10 +1038,10 @@ static int cmp_extents_reverse(const void *a, const void *b)
> }
>
> /**
> - * sort_idmaps - Sorts an array of idmap entries.
> + * sort_map - Sorts an array of idmap entries.
> * Can only be called if number of mappings exceeds UID_GID_MAP_MAX_BASE_EXTENTS.
> */
> -static int sort_idmaps(struct uid_gid_map *map)
> +static int sort_map(struct uid_gid_map *map)
> {
> if (map->nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
> return 0;
> @@ -1064,6 +1064,71 @@ static int sort_idmaps(struct uid_gid_map *map)
> return 0;
> }
>
> +static int sort_idmaps(struct uid_gid_map *map)
> +{
> + return sort_map(map);
> +}
> +
> +static int map_from_parent(struct uid_gid_map *new_map,
> + struct uid_gid_map *parent_map)
> +{
> + unsigned idx;
> +
> + /* Map the lower ids from the parent user namespace to the
> + * kernel global id space.
> + */
> + for (idx = 0; idx < new_map->nr_extents; idx++) {
> + struct uid_gid_extent *e;
> + u32 lower_first;
> +
> + if (new_map->nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
> + e = &new_map->extent[idx];
> + else
> + e = &new_map->forward[idx];
> +
> + lower_first = map_id_range_down(parent_map, e->lower_first, e->count);
> +
> + /* Fail if we can not map the specified extent to
> + * the kernel global id space.
> + */
> + if (lower_first == (u32)-1)
> + return -EPERM;
> +
> + e->lower_first = lower_first;
> + }
> +
> + return 0;
> +}
> +
> +static int map_into_kids(struct uid_gid_map *id_map,
> + struct uid_gid_map *parent_id_map)
> +{
> + return map_from_parent(id_map, parent_id_map);
> +}
> +
> +static void install_idmaps(struct uid_gid_map *id_map,
> + struct uid_gid_map *new_id_map)
> +{
> + if (new_id_map->nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS) {
> + memcpy(id_map->extent, new_id_map->extent,
> + new_id_map->nr_extents * sizeof(new_id_map->extent[0]));
> + } else {
> + id_map->forward = new_id_map->forward;
> + id_map->reverse = new_id_map->reverse;
> + }
> +}
> +
> +static void free_idmaps(struct uid_gid_map *new_id_map)
> +{
> + if (new_id_map->nr_extents > UID_GID_MAP_MAX_BASE_EXTENTS) {
> + kfree(new_id_map->forward);
> + kfree(new_id_map->reverse);
> + new_id_map->forward = NULL;
> + new_id_map->reverse = NULL;
> + new_id_map->nr_extents = 0;
> + }
> +}
> +
> static ssize_t map_write(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos,
> int cap_setid,
> @@ -1073,7 +1138,6 @@ static ssize_t map_write(struct file *file, const char __user *buf,
> struct seq_file *seq = file->private_data;
> struct user_namespace *ns = seq->private;
> struct uid_gid_map new_map;
> - unsigned idx;
> struct uid_gid_extent extent;
> char *kbuf = NULL, *pos, *next_line;
> ssize_t ret;
> @@ -1191,61 +1255,28 @@ static ssize_t map_write(struct file *file, const char __user *buf,
> if (!new_idmap_permitted(file, ns, cap_setid, &new_map))
> goto out;
>
> - ret = -EPERM;
> - /* Map the lower ids from the parent user namespace to the
> - * kernel global id space.
> - */
> - for (idx = 0; idx < new_map.nr_extents; idx++) {
> - struct uid_gid_extent *e;
> - u32 lower_first;
> -
> - if (new_map.nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
> - e = &new_map.extent[idx];
> - else
> - e = &new_map.forward[idx];
> -
> - lower_first = map_id_range_down(parent_map,
> - e->lower_first,
> - e->count);
> -
> - /* Fail if we can not map the specified extent to
> - * the kernel global id space.
> - */
> - if (lower_first == (u32) -1)
> - goto out;
> -
> - e->lower_first = lower_first;
> - }
> + ret = map_into_kids(&new_map, parent_map);
> + if (ret)
> + goto out;
>
> /*
> * If we want to use binary search for lookup, this clones the extent
> * array and sorts both copies.
> */
> ret = sort_idmaps(&new_map);
> - if (ret < 0)
> + if (ret)
> goto out;
>
> /* Install the map */
> - if (new_map.nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS) {
> - memcpy(map->extent, new_map.extent,
> - new_map.nr_extents * sizeof(new_map.extent[0]));
> - } else {
> - map->forward = new_map.forward;
> - map->reverse = new_map.reverse;
> - }
> + install_idmaps(map, &new_map);
> smp_wmb();
> map->nr_extents = new_map.nr_extents;
>
> *ppos = count;
> ret = count;
> out:
> - if (ret < 0 && new_map.nr_extents > UID_GID_MAP_MAX_BASE_EXTENTS) {
> - kfree(new_map.forward);
> - kfree(new_map.reverse);
> - map->forward = NULL;
> - map->reverse = NULL;
> - map->nr_extents = 0;
> - }
> + if (ret < 0)
> + free_idmaps(&new_map);
>
> mutex_unlock(&userns_state_mutex);
> kfree(kbuf);
> --
> 2.25.0
^ permalink raw reply
* Re: [PATCH v3 07/25] proc: task_state(): use from_kfs{g,u}id_munged
From: Serge E. Hallyn @ 2020-02-19 2:36 UTC (permalink / raw)
To: Christian Brauner
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn,
smbarber, Seth Forshee, Alexander Viro, Alexey Dobriyan,
Serge Hallyn, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, linux-kernel, linux-fsdevel, containers,
linux-security-module, linux-api
In-Reply-To: <20200218143411.2389182-8-christian.brauner@ubuntu.com>
On Tue, Feb 18, 2020 at 03:33:53PM +0100, Christian Brauner wrote:
> If fsid mappings have been written, this will cause proc to look at fsid
> mappings for the user namespace. If no fsid mappings have been written the
> behavior is as before.
>
> Here is part of the output from /proc/<pid>/status from the initial user
> namespace for systemd running in an unprivileged container as user namespace
> root with id mapping 0 100000 100000 and fsid mapping 0 300000 100000:
>
> Name: systemd
> Umask: 0000
> State: S (sleeping)
> Tgid: 13023
> Ngid: 0
> Pid: 13023
> PPid: 13008
> TracerPid: 0
> Uid: 100000 100000 100000 300000
> Gid: 100000 100000 100000 300000
> FDSize: 64
> Groups:
>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
> /* v2 */
> unchanged
>
> /* v3 */
> unchanged
> ---
> fs/proc/array.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 5efaf3708ec6..d4a04f85a67e 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -91,6 +91,7 @@
> #include <linux/string_helpers.h>
> #include <linux/user_namespace.h>
> #include <linux/fs_struct.h>
> +#include <linux/fsuidgid.h>
>
> #include <asm/pgtable.h>
> #include <asm/processor.h>
> @@ -193,11 +194,11 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> seq_put_decimal_ull(m, "\nUid:\t", from_kuid_munged(user_ns, cred->uid));
> seq_put_decimal_ull(m, "\t", from_kuid_munged(user_ns, cred->euid));
> seq_put_decimal_ull(m, "\t", from_kuid_munged(user_ns, cred->suid));
> - seq_put_decimal_ull(m, "\t", from_kuid_munged(user_ns, cred->fsuid));
> + seq_put_decimal_ull(m, "\t", from_kfsuid_munged(user_ns, cred->fsuid));
> seq_put_decimal_ull(m, "\nGid:\t", from_kgid_munged(user_ns, cred->gid));
> seq_put_decimal_ull(m, "\t", from_kgid_munged(user_ns, cred->egid));
> seq_put_decimal_ull(m, "\t", from_kgid_munged(user_ns, cred->sgid));
> - seq_put_decimal_ull(m, "\t", from_kgid_munged(user_ns, cred->fsgid));
> + seq_put_decimal_ull(m, "\t", from_kfsgid_munged(user_ns, cred->fsgid));
> seq_put_decimal_ull(m, "\nFDSize:\t", max_fds);
>
> seq_puts(m, "\nGroups:\t");
> --
> 2.25.0
^ permalink raw reply
* Re: [PATCH v3 09/25] fs: add is_userns_visible() helper
From: Serge E. Hallyn @ 2020-02-19 2:42 UTC (permalink / raw)
To: Christian Brauner
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn,
smbarber, Seth Forshee, Alexander Viro, Alexey Dobriyan,
Serge Hallyn, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, linux-kernel, linux-fsdevel, containers,
linux-security-module, linux-api
In-Reply-To: <20200218143411.2389182-10-christian.brauner@ubuntu.com>
On Tue, Feb 18, 2020 at 03:33:55PM +0100, Christian Brauner wrote:
> Introduce a helper which makes it possible to detect fileystems whose
> superblock is visible in multiple user namespace. This currently only
> means proc and sys. Such filesystems usually have special semantics so their
> behavior will not be changed with the introduction of fsid mappings.
Hi,
I'm afraid I've got a bit of a hangup about the terminology here. I
*think* what you mean is that SB_I_USERNS_VISIBLE is an fs whose uids are
always translated per the id mappings, not fsid mappings. But when I see
the name it seems to imply that !SB_I_USERNS_VISIBLE filesystems can't
be seen by other namespaces at all.
Am I right in my first interpretation? If so, can we talk about the
naming?
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> /* v2 */
> unchanged
>
> /* v3 */
> unchanged
> ---
> include/linux/fs.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3cd4fe6b845e..fdc8fb2d786b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3651,4 +3651,9 @@ static inline int inode_drain_writes(struct inode *inode)
> return filemap_write_and_wait(inode->i_mapping);
> }
>
> +static inline bool is_userns_visible(unsigned long iflags)
> +{
> + return (iflags & SB_I_USERNS_VISIBLE);
> +}
> +
> #endif /* _LINUX_FS_H */
> --
> 2.25.0
^ permalink raw reply
* Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
From: Jordan Hand @ 2020-02-19 3:26 UTC (permalink / raw)
To: Jarkko Sakkinen, linux-kernel, x86, linux-sgx
Cc: akpm, dave.hansen, sean.j.christopherson, nhorman, npmccallum,
haitao.huang, andriy.shevchenko, tglx, kai.svahn, bp, josh, luto,
kai.huang, rientjes, cedric.xing, puiterwijk,
linux-security-module, Suresh Siddha, Haitao Huang
In-Reply-To: <20200209212609.7928-11-jarkko.sakkinen@linux.intel.com>
I ran our validation tests for the Open Enclave SDK against this patch
set and came across a potential issue.
On 2/9/20 1:25 PM, Jarkko Sakkinen wrote:
> +/**
> + * sgx_encl_may_map() - Check if a requested VMA mapping is allowed
> + * @encl: an enclave
> + * @start: lower bound of the address range, inclusive
> + * @end: upper bound of the address range, exclusive
> + * @vm_prot_bits: requested protections of the address range
> + *
> + * Iterate through the enclave pages contained within [@start, @end) to verify
> + * the permissions requested by @vm_prot_bits do not exceed that of any enclave
> + * page to be mapped. Page addresses that do not have an associated enclave
> + * page are interpreted to zero permissions.
> + *
> + * Return:
> + * 0 on success,
> + * -EACCES if VMA permissions exceed enclave page permissions
> + */
> +int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> + unsigned long end, unsigned long vm_prot_bits)
> +{
> + unsigned long idx, idx_start, idx_end;
> + struct sgx_encl_page *page;
> +
> + /* PROT_NONE always succeeds. */
> + if (!vm_prot_bits)
> + return 0;
> +
> + idx_start = PFN_DOWN(start);
> + idx_end = PFN_DOWN(end - 1);
> +
> + for (idx = idx_start; idx <= idx_end; ++idx) {
> + mutex_lock(&encl->lock);
> + page = radix_tree_lookup(&encl->page_tree, idx);
> + mutex_unlock(&encl->lock);
> +
> + if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
> + return -EACCES;
> + }
> +
> + return 0;
> +}
> +static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
> + unsigned long offset,
> + u64 secinfo_flags)
> +{
> + struct sgx_encl_page *encl_page;
> + unsigned long prot;
> +
> + encl_page = kzalloc(sizeof(*encl_page), GFP_KERNEL);
> + if (!encl_page)
> + return ERR_PTR(-ENOMEM);
> +
> + encl_page->desc = encl->base + offset;
> + encl_page->encl = encl;
> +
> + prot = _calc_vm_trans(secinfo_flags, SGX_SECINFO_R, PROT_READ) |
> + _calc_vm_trans(secinfo_flags, SGX_SECINFO_W, PROT_WRITE) |
> + _calc_vm_trans(secinfo_flags, SGX_SECINFO_X, PROT_EXEC);
> +
> + /*
> + * TCS pages must always RW set for CPU access while the SECINFO
> + * permissions are *always* zero - the CPU ignores the user provided
> + * values and silently overwrites them with zero permissions.
> + */
> + if ((secinfo_flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
> + prot |= PROT_READ | PROT_WRITE;
> +
> + /* Calculate maximum of the VM flags for the page. */
> + encl_page->vm_max_prot_bits = calc_vm_prot_bits(prot, 0);
During mprotect (in mm/mprotect.c line 525) the following checks if
READ_IMPLIES_EXECUTE and a PROT_READ is being requested. If so and
VM_MAYEXEC is set, it also adds PROT_EXEC to the request.
if (rier && (vma->vm_flags & VM_MAYEXEC))
prot |= PROT_EXEC;
But if we look at sgx_encl_page_alloc(), we see vm_max_prot_bits is set
without taking VM_MAYEXEC into account:
encl_page->vm_max_prot_bits = calc_vm_prot_bits(prot, 0);
sgx_encl_may_map() checks that the requested protection can be added with:
if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
return -EACCESS
This means that for any process where READ_IMPLIES_EXECUTE is set and
page where (vma->vm_flags & VM_MAYEXEC) == true, mmap/mprotect calls to
that request PROT_READ on a page that was not added with PROT_EXEC will
fail.
- Jordan
^ permalink raw reply
* Re: [PATCH v7 01/12] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-02-19 7:54 UTC (permalink / raw)
To: James Morris
Cc: Serge Hallyn, Stephen Smalley, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Helge Deller, Thomas Gleixner, Andi Kleen,
Stephane Eranian, Igor Lubashev, Jiri Olsa, linux-kernel,
intel-gfx@lists.freedesktop.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
linux-arm-kernel, linuxppc-dev@lists.ozlabs.org,
linux-parisc@vger.kernel.org, oprofile-list,
linux-doc@vger.kernel.org, linux-man
In-Reply-To: <alpine.LRH.2.21.2002190621180.10165@namei.org>
On 18.02.2020 22:21, James Morris wrote:
> On Mon, 17 Feb 2020, Alexey Budankov wrote:
>
>>
>> Introduce CAP_PERFMON capability designed to secure system performance
>> monitoring and observability operations so that CAP_PERFMON would assist
>> CAP_SYS_ADMIN capability in its governing role for performance
>> monitoring and observability subsystems.
>
>
> Acked-by: James Morris <jamorris@linux.microsoft.com>
Thanks James!
I appreciate your involvement and collaboration
w.r.t to the whole patch set.
Gratefully,
Alexey
^ permalink raw reply
* [PATCH] module support: during lockdown, log name of unsigned module
From: Martin Haass @ 2020-02-19 9:02 UTC (permalink / raw)
To: Jessica Yu; +Cc: linux-kernel, linux-security-module, linux-modules
during lockdown loading of unsigned modules is restricted to signed
modules only. The old error message does not show which module misses
the signature, making it very difficult for a user to determine which
module is at fault.
This patch adds a line to the logs which additionally contains the
module name that caused the error message. The old message cannot
be replaced as it is generated by lockdown_is_locked_down
---
kernel/module.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index 33569a01d6e..6dcb28139a0 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2807,7 +2807,8 @@ static int module_sig_check(struct load_info *info,
int flags)
const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
const char *reason;
const void *mod = info->hdr;
-
+ int is_locked = -EPERM;
+
/*
* Require flags == 0, as a module with version information
* removed is no longer the module that was signed
@@ -2843,7 +2844,12 @@ static int module_sig_check(struct load_info *info,
int flags)
return -EKEYREJECTED;
}
- return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
+ is_locked = security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
+ if (is_locked == -EPERM) {
+ pr_notice("Lockdown: %s: rejected module '%s' cause: %s",
+ current->comm, info->name, reason);
+ }
+ return is_locked;
/* All other errors are fatal, including nomem, unparseable
* signatures and signature check failures - even if signatures
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v3 09/25] fs: add is_userns_visible() helper
From: Christian Brauner @ 2020-02-19 12:06 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn,
smbarber, Seth Forshee, Alexander Viro, Alexey Dobriyan,
James Morris, Kees Cook, Jonathan Corbet, Phil Estes,
linux-kernel, linux-fsdevel, containers, linux-security-module,
linux-api
In-Reply-To: <20200219024233.GA19334@mail.hallyn.com>
On Tue, Feb 18, 2020 at 08:42:33PM -0600, Serge Hallyn wrote:
> On Tue, Feb 18, 2020 at 03:33:55PM +0100, Christian Brauner wrote:
> > Introduce a helper which makes it possible to detect fileystems whose
> > superblock is visible in multiple user namespace. This currently only
> > means proc and sys. Such filesystems usually have special semantics so their
> > behavior will not be changed with the introduction of fsid mappings.
>
> Hi,
>
> I'm afraid I've got a bit of a hangup about the terminology here. I
> *think* what you mean is that SB_I_USERNS_VISIBLE is an fs whose uids are
> always translated per the id mappings, not fsid mappings. But when I see
Correct!
> the name it seems to imply that !SB_I_USERNS_VISIBLE filesystems can't
> be seen by other namespaces at all.
>
> Am I right in my first interpretation? If so, can we talk about the
> naming?
Yep, your first interpretation is right. What about: wants_idmaps()
^ permalink raw reply
* Re: [PATCH v3 00/25] user_namespace: introduce fsid mappings
From: Christian Brauner @ 2020-02-19 12:27 UTC (permalink / raw)
To: James Bottomley
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn,
Kees Cook, Jonathan Corbet, linux-api, containers, linux-kernel,
smbarber, Seth Forshee, linux-security-module, Alexander Viro,
linux-fsdevel, Alexey Dobriyan
In-Reply-To: <1582069856.16681.59.camel@HansenPartnership.com>
On Tue, Feb 18, 2020 at 03:50:56PM -0800, James Bottomley wrote:
> On Tue, 2020-02-18 at 15:33 +0100, Christian Brauner wrote:
> > In the usual case of running an unprivileged container we will have
> > setup an id mapping, e.g. 0 100000 100000. The on-disk mapping will
> > correspond to this id mapping, i.e. all files which we want to appear
> > as 0:0 inside the user namespace will be chowned to 100000:100000 on
> > the host. This works, because whenever the kernel needs to do a
> > filesystem access it will lookup the corresponding uid and gid in the
> > idmapping tables of the container. Now think about the case where we
> > want to have an id mapping of 0 100000 100000 but an on-disk mapping
> > of 0 300000 100000 which is needed to e.g. share a single on-disk
> > mapping with multiple containers that all have different id mappings.
> > This will be problematic. Whenever a filesystem access is requested,
> > the kernel will now try to lookup a mapping for 300000 in the id
> > mapping tables of the user namespace but since there is none the
> > files will appear to be owned by the overflow id, i.e. usually
> > 65534:65534 or nobody:nogroup.
> >
> > With fsid mappings we can solve this by writing an id mapping of 0
> > 100000 100000 and an fsid mapping of 0 300000 100000. On filesystem
> > access the kernel will now lookup the mapping for 300000 in the fsid
> > mapping tables of the user namespace. And since such a mapping
> > exists, the corresponding files will have correct ownership.
>
> So I did compile this up in order to run the shiftfs tests over it to
> see how it coped with the various corner cases. However, what I find
> is it simply fails the fsid reverse mapping in the setup. Trying to
> use a simple uid of 0 100000 1000 and a fsid of 100000 0 1000 fails the
> entry setuid(0) call because of this code:
This is easy to fix. But what's the exact use-case?
^ permalink raw reply
* Re: [PATCH v3 11/25] inode: inode_owner_or_capable(): handle fsid mappings
From: Christian Brauner @ 2020-02-19 12:29 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn,
smbarber, Seth Forshee, Alexander Viro, Alexey Dobriyan,
Serge Hallyn, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, linux-kernel, linux-fsdevel, containers,
linux-security-module, linux-api
In-Reply-To: <20200218222523.GA9535@infradead.org>
On Tue, Feb 18, 2020 at 02:25:23PM -0800, Christoph Hellwig wrote:
> On Tue, Feb 18, 2020 at 03:33:57PM +0100, Christian Brauner wrote:
> > + if (is_userns_visible(inode->i_sb->s_iflags)) {
> > + if (kuid_has_mapping(ns, inode->i_uid) && ns_capable(ns, CAP_FOWNER))
> > + return true;
> > + } else if (kfsuid_has_mapping(ns, inode->i_uid) && ns_capable(ns, CAP_FOWNER)) {
>
> This adds some crazy long unreadable lines..
I'll ad a helper in the next version or wrap those lines depending on
what makes more sense.
^ permalink raw reply
* Re: [PATCH v3 15/25] posix_acl: handle fsid mappings
From: Christian Brauner @ 2020-02-19 12:56 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn,
smbarber, Seth Forshee, Alexander Viro, Alexey Dobriyan,
Serge Hallyn, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, linux-kernel, linux-fsdevel, containers,
linux-security-module, linux-api
In-Reply-To: <20200218222631.GB9535@infradead.org>
On Tue, Feb 18, 2020 at 02:26:31PM -0800, Christoph Hellwig wrote:
> On Tue, Feb 18, 2020 at 03:34:01PM +0100, Christian Brauner wrote:
> > diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> > index 249672bf54fe..ed6112c9b804 100644
> > --- a/fs/posix_acl.c
> > +++ b/fs/posix_acl.c
> > @@ -22,6 +22,7 @@
> > #include <linux/xattr.h>
> > #include <linux/export.h>
> > #include <linux/user_namespace.h>
> > +#include <linux/fsuidgid.h>
> >
> > static struct posix_acl **acl_by_type(struct inode *inode, int type)
> > {
> > @@ -692,12 +693,12 @@ static void posix_acl_fix_xattr_userns(
> > 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));
> > - entry->e_id = cpu_to_le32(from_kuid(to, uid));
> > + uid = make_kfsuid(from, le32_to_cpu(entry->e_id));
> > + entry->e_id = cpu_to_le32(from_kfsuid(to, uid));
> > break;
> > case ACL_GROUP:
> > - gid = make_kgid(from, le32_to_cpu(entry->e_id));
> > - entry->e_id = cpu_to_le32(from_kgid(to, gid));
> > + gid = make_kfsgid(from, le32_to_cpu(entry->e_id));
> > + entry->e_id = cpu_to_le32(from_kfsgid(to, gid));
> > break;
>
> Before we touch this code any more it needs to move to the right place.
> Poking into ACLs from generic xattr code is a complete layering
git history shows that it was deliberately placed after the fs specific
xattr handlers have been called so individual filesystems don't need to
be aware of id mappings to make maintenance easier. Same goes for vfs
capabilities. Moving this down into individual filesystem seems like a
maintenance nightmare where now each individual filesystem will have to
remember to fixup their ids. For namespaced vfs caps which are handled
at the same level in setxattr() it will also mean breaking backwards
compatible translation from non-namespaced vfs caps aware userspace to
namespaced vfs-caps aware kernels.
^ permalink raw reply
* Re: [PATCH v3 00/25] user_namespace: introduce fsid mappings
From: Jann Horn @ 2020-02-19 15:33 UTC (permalink / raw)
To: Christian Brauner
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai,
Stephen Barber, Seth Forshee, Alexander Viro, Alexey Dobriyan,
Serge Hallyn, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, kernel list, linux-fsdevel, Linux Containers,
linux-security-module, Linux API
In-Reply-To: <20200218143411.2389182-1-christian.brauner@ubuntu.com>
On Tue, Feb 18, 2020 at 3:35 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
[...]
> - Let the keyctl infrastructure only operate on kfsid which are always
> mapped/looked up in the id mappings similar to what we do for
> filesystems that have the same superblock visible in multiple user
> namespaces.
>
> This version also comes with minimal tests which I intend to expand in
> the future.
>
> From pings and off-list questions and discussions at Google Container
> Security Summit there seems to be quite a lot of interest in this
> patchset with use-cases ranging from layer sharing for app containers
> and k8s, as well as data sharing between containers with different id
> mappings. I haven't Cced all people because I don't have all the email
> adresses at hand but I've at least added Phil now. :)
>
> This is the implementation of shiftfs which was cooked up during lunch at
> Linux Plumbers 2019 the day after the container's microconference. The
> idea is a design-stew from Stéphane, Aleksa, Eric, and myself (and by
> now also Jann.
> Back then we all were quite busy with other work and couldn't really sit
> down and implement it. But I took a few days last week to do this work,
> including demos and performance testing.
> This implementation does not require us to touch the VFS substantially
> at all. Instead, we implement shiftfs via fsid mappings.
> With this patch, it took me 20 mins to port both LXD and LXC to support
> shiftfs via fsid mappings.
[...]
Can you please grep through the kernel for all uses of ->fsuid and
->fsgid and fix them up appropriately? Some cases I still see:
The SafeSetID LSM wants to enforce that you can only use CAP_SETUID to
gain the privileges of a specific set of IDs:
static int safesetid_task_fix_setuid(struct cred *new,
const struct cred *old,
int flags)
{
/* Do nothing if there are no setuid restrictions for our old RUID. */
if (setuid_policy_lookup(old->uid, INVALID_UID) == SIDPOL_DEFAULT)
return 0;
if (uid_permitted_for_cred(old, new->uid) &&
uid_permitted_for_cred(old, new->euid) &&
uid_permitted_for_cred(old, new->suid) &&
uid_permitted_for_cred(old, new->fsuid))
return 0;
/*
* Kill this process to avoid potential security vulnerabilities
* that could arise from a missing whitelist entry preventing a
* privileged process from dropping to a lesser-privileged one.
*/
force_sig(SIGKILL);
return -EACCES;
}
This could theoretically be bypassed through setfsuid() if the kuid
based on the fsuid mappings is permitted but the kuid based on the
normal mappings is not.
fs/coredump.c in suid dump mode uses "cred->fsuid = GLOBAL_ROOT_UID";
this should probably also fix up the other uid, even if there is no
scenario in which it would actually be used at the moment?
The netfilter xt_owner stuff makes packet filtering decisions based on
the ->fsuid; it might be better to filter on the ->kfsuid so that you
can filter traffic from different user namespaces differently?
audit_log_task_info() is doing "from_kuid(&init_user_ns, cred->fsuid)".
^ permalink raw reply
* Re: [PATCH v3 00/25] user_namespace: introduce fsid mappings
From: James Bottomley @ 2020-02-19 15:36 UTC (permalink / raw)
To: Christian Brauner
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn,
Kees Cook, Jonathan Corbet, linux-api, containers, linux-kernel,
smbarber, Seth Forshee, linux-security-module, Alexander Viro,
linux-fsdevel, Alexey Dobriyan
In-Reply-To: <20200219122752.jalnsmsotigwxwsw@wittgenstein>
On Wed, 2020-02-19 at 13:27 +0100, Christian Brauner wrote:
> On Tue, Feb 18, 2020 at 03:50:56PM -0800, James Bottomley wrote:
> > On Tue, 2020-02-18 at 15:33 +0100, Christian Brauner wrote:
[...]
> > > With fsid mappings we can solve this by writing an id mapping of
> > > 0 100000 100000 and an fsid mapping of 0 300000 100000. On
> > > filesystem access the kernel will now lookup the mapping for
> > > 300000 in the fsid mapping tables of the user namespace. And
> > > since such a mapping exists, the corresponding files will have
> > > correct ownership.
> >
> > So I did compile this up in order to run the shiftfs tests over it
> > to see how it coped with the various corner cases. However, what I
> > find is it simply fails the fsid reverse mapping in the
> > setup. Trying to use a simple uid of 0 100000 1000 and a fsid of
> > 100000 0 1000 fails the entry setuid(0) call because of this code:
>
> This is easy to fix. But what's the exact use-case?
Well, the use case I'm looking to solve is the same one it's always
been: getting a deprivileged fake root in a user_ns to be able to write
an image at fsuid 0.
I don't think it's solvable in your current framework, although
allowing the domain to be disjoint might possibly hack around it. The
problem with the proposed framework is that there are no backshifts
from the filesystem view, there are only forward shifts to the
filesystem view. This means that to get your framework to write a
filesystem at fsuid 0 you have to have an identity map for fsuid. Which
I can do: I tested uid shift 0 100000 1000 and fsuid shift 0 0 1000.
It does all work, as you'd expect because the container has real fs
root not a fake root. And that's the whole problem: Firstly, I'm fs
root for any filesystem my userns can see, so any imprecision in
setting up the mount namespace of the container and I own your host and
secondly any containment break and I'm privileged with respect to the
fs uid wherever I escape to so I will likewise own your host.
The only way to keep containment is to have a zero fsuid inside the
container corresponding to a non-zero one outside. And the only way to
solve the imprecision in mount namespace issue is to strictly control
the entry point at which the writing at fsuid 0 becomes active.
James
^ permalink raw reply
* Re: [PATCH v3 24/25] sys: handle fsid mappings in set*id() calls
From: Jann Horn @ 2020-02-19 15:42 UTC (permalink / raw)
To: Christian Brauner
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai,
Stephen Barber, Seth Forshee, Alexander Viro, Alexey Dobriyan,
Serge Hallyn, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, kernel list, linux-fsdevel, Linux Containers,
linux-security-module, Linux API
In-Reply-To: <20200218143411.2389182-25-christian.brauner@ubuntu.com>
On Tue, Feb 18, 2020 at 3:37 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
> Switch set*id() calls to lookup fsids in the fsid mappings. If no fsid mappings
> are setup the behavior is unchanged, i.e. fsids are looked up in the id
> mappings.
[...]
> @@ -353,7 +354,7 @@ long __sys_setregid(gid_t rgid, gid_t egid)
> const struct cred *old;
> struct cred *new;
> int retval;
> - kgid_t krgid, kegid;
> + kgid_t krgid, kegid, kfsgid;
>
> krgid = make_kgid(ns, rgid);
> kegid = make_kgid(ns, egid);
> @@ -385,12 +386,20 @@ long __sys_setregid(gid_t rgid, gid_t egid)
> new->egid = kegid;
> else
> goto error;
> + kfsgid = make_kfsgid(ns, egid);
> + } else {
> + kfsgid = kgid_to_kfsgid(new->user_ns, new->egid);
> + }
Here the "kfsgid" is the new filesystem GID as translated by the
special fsgid mapping...
> + if (!gid_valid(kfsgid)) {
> + retval = -EINVAL;
> + goto error;
> }
>
> if (rgid != (gid_t) -1 ||
> (egid != (gid_t) -1 && !gid_eq(kegid, old->gid)))
> new->sgid = new->egid;
> - new->fsgid = new->egid;
> + new->kfsgid = new->egid;
... but the "kfsgid" of the creds struct is translated by the normal
gid mapping...
> + new->fsgid = kfsgid;
... and the local "kfsgid" is stored into the "fsgid" member.
This is pretty hard to follow. Can you come up with some naming scheme
that is clearer and where one name is always used for the
normally-translated fsgid and another name is always used for the
specially-translated fsgid? E.g. something like "pfsgid" (with the "p"
standing for "process", because it uses the same id mappings as used
for process identities) for the IDs translated via the normal gid
mapping?
^ permalink raw reply
* Re: [PATCH v3 19/25] commoncap: handle fsid mappings with vfs caps
From: Jann Horn @ 2020-02-19 15:53 UTC (permalink / raw)
To: Christian Brauner
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai,
Stephen Barber, Seth Forshee, Alexander Viro, Alexey Dobriyan,
Serge Hallyn, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, kernel list, linux-fsdevel, Linux Containers,
linux-security-module, Linux API
In-Reply-To: <20200218143411.2389182-20-christian.brauner@ubuntu.com>
On Tue, Feb 18, 2020 at 3:35 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
> Switch vfs cap helpers to lookup fsids in the fsid mappings. If no fsid
> mappings are setup the behavior is unchanged, i.e. fsids are looked up in the
> id mappings.
[...]
> diff --git a/security/commoncap.c b/security/commoncap.c
[...]
> @@ -328,7 +328,7 @@ static bool rootid_owns_currentns(kuid_t kroot)
> return false;
>
> for (ns = current_user_ns(); ; ns = ns->parent) {
> - if (from_kuid(ns, kroot) == 0)
> + if (from_kfsuid(ns, kroot) == 0)
> return true;
> if (ns == &init_user_ns)
> break;
Nit: Maybe change the name of this function to something that makes it
clear that this operates in the fsuid mapping domain.
^ permalink raw reply
* Re: [PATCH v3 06/25] user_namespace: make map_write() support fsid mappings
From: Jann Horn @ 2020-02-19 16:18 UTC (permalink / raw)
To: Christian Brauner
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai,
Stephen Barber, Seth Forshee, Alexander Viro, Alexey Dobriyan,
Serge Hallyn, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, kernel list, linux-fsdevel, Linux Containers,
linux-security-module, Linux API
In-Reply-To: <20200218143411.2389182-7-christian.brauner@ubuntu.com>
On Tue, Feb 18, 2020 at 3:35 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
> Based on discussions with Jann we decided in order to cleanly handle nested
> user namespaces that fsid mappings can only be written before the corresponding
> id mappings have been written. Writing id mappings before writing the
> corresponding fsid mappings causes fsid mappings to mirror id mappings.
>
> Consider creating a user namespace NS1 with the initial user namespace as
> parent. Assume NS1 receives id mapping 0 100000 100000 and fsid mappings 0
> 300000 100000. Files that root in NS1 will create will map to kfsuid=300000 and
> kfsgid=300000 and will hence be owned by uid=300000 and gid 300000 on-disk in
> the initial user namespace.
> Now assume user namespace NS2 is created in user namespace NS1. Assume that NS2
> receives id mapping 0 10000 65536 and an fsid mapping of 0 10000 65536. Files
> that root in NS2 will create will map to kfsuid=10000 and kfsgid=10000 in NS1.
> hence, files created by NS2 will hence be appear to be be owned by uid=10000
> and gid=10000 on-disk in NS1. Looking at the initial user namespace, files
> created by NS2 will map to kfsuid=310000 and kfsgid=310000 and hence will be
> owned by uid=310000 and gid=310000 on-disk.
[...]
> static bool new_idmap_permitted(const struct file *file,
> struct user_namespace *ns, int cap_setid,
> - struct uid_gid_map *new_map)
> + struct uid_gid_map *new_map,
> + enum idmap_type idmap_type)
> {
> const struct cred *cred = file->f_cred;
> +
> + /* Don't allow writing fsuid maps when uid maps have been written. */
> + if (idmap_type == FSUID_MAP && idmap_exists(&ns->uid_map))
> + return false;
> +
> + /* Don't allow writing fsgid maps when gid maps have been written. */
> + if (idmap_type == FSGID_MAP && idmap_exists(&ns->gid_map))
> + return false;
Why are these checks necessary? Shouldn't an fs*id map have already
been implicitly created?
^ permalink raw reply
* Re: [PATCH v3 09/25] fs: add is_userns_visible() helper
From: Andy Lutomirski @ 2020-02-19 17:18 UTC (permalink / raw)
To: Christian Brauner
Cc: Serge E. Hallyn, Stéphane Graber, Eric W. Biederman,
Aleksa Sarai, Jann Horn, smbarber, Seth Forshee, Alexander Viro,
Alexey Dobriyan, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, LKML, Linux FS Devel, Linux Containers, LSM List,
Linux API
In-Reply-To: <20200219120604.vqudwaeppebvisco@wittgenstein>
On Wed, Feb 19, 2020 at 4:06 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Tue, Feb 18, 2020 at 08:42:33PM -0600, Serge Hallyn wrote:
> > On Tue, Feb 18, 2020 at 03:33:55PM +0100, Christian Brauner wrote:
> > > Introduce a helper which makes it possible to detect fileystems whose
> > > superblock is visible in multiple user namespace. This currently only
> > > means proc and sys. Such filesystems usually have special semantics so their
> > > behavior will not be changed with the introduction of fsid mappings.
> >
> > Hi,
> >
> > I'm afraid I've got a bit of a hangup about the terminology here. I
> > *think* what you mean is that SB_I_USERNS_VISIBLE is an fs whose uids are
> > always translated per the id mappings, not fsid mappings. But when I see
>
> Correct!
>
> > the name it seems to imply that !SB_I_USERNS_VISIBLE filesystems can't
> > be seen by other namespaces at all.
> >
> > Am I right in my first interpretation? If so, can we talk about the
> > naming?
>
> Yep, your first interpretation is right. What about: wants_idmaps()
Maybe fsidmap_exempt()?
I still haven't convinced myself that any of the above is actually
correct behavior, especially when people do things like creating
setuid binaries.
^ permalink raw reply
* [PATCH 9/9] integrity: check properly whether EFI GetVariable() is available
From: Ard Biesheuvel @ 2020-02-19 17:19 UTC (permalink / raw)
To: linux-efi
Cc: Ard Biesheuvel, Leif Lindholm, Peter Jones, Alexander Graf,
Heinrich Schuchardt, Jeff Brasen, Atish Patra, x86, James Morris,
Serge E. Hallyn, linux-security-module
In-Reply-To: <20200219171907.11894-1-ardb@kernel.org>
Testing the value of the efi.get_variable function pointer is not
the right way to establish whether the platform supports EFI
variables at runtime. Instead, use the newly added granular check
that can test for the presence of each EFI runtime service
individually.
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
security/integrity/platform_certs/load_uefi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 111898aad56e..e2fe1bd3abb9 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -76,7 +76,7 @@ static int __init load_uefi_certs(void)
unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
int rc = 0;
- if (!efi.get_variable)
+ if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
return false;
/* Get db, MokListRT, and dbx. They might not exist, so it isn't
--
2.17.1
^ permalink raw reply related
* Re: SELinux support in virtio-fs
From: Casey Schaufler @ 2020-02-19 17:50 UTC (permalink / raw)
To: Stephen Smalley, Ondrej Mosnacek, Daniel Walsh
Cc: Stefan Hajnoczi, virtio-fs, SElinux list,
Linux Security Module list
In-Reply-To: <6df9b58c-fe9b-28f3-c151-f77aa6dd67e7@tycho.nsa.gov>
On 2/19/2020 8:44 AM, Stephen Smalley wrote:
> On 2/19/20 11:11 AM, Ondrej Mosnacek wrote:
>> On Wed, Feb 12, 2020 at 4:29 PM Daniel Walsh <dwalsh@redhat.com> wrote:
>>> On 2/10/20 11:06 AM, Stefan Hajnoczi wrote:
>>>> Hi Dan,
>>>> I've CCed the public virtio-fs mailing list because SELinux support in
>>>> virtio-fs has been asked about recently.
>>>>
>>>> It's time to figure out what level of SELinux support will be available
>>>> in virtio-fs. The file system client shares most of its code with FUSE
>>>> and SELinux labels on files are currently not supported in FUSE.
>>>>
>>>> It would be possible to pass through extended attributes to the
>>>> virtiofsd daemon running on the host. However, passing through xattrs
>>>> allows the client to relabel files on the host file system and this
>>>> could pose a security problem. virtiofsd already allows the client to
>>>> set the uid/gid and permissions, but is passing through SELinux xattrs a
>>>> bad idea?
>>>>
>>>> virtiofsd is in a position to mangle extended attribute names
>>>> ("security.selinux" -> "virtiofs.security.selinux") in order to separate
>>>> guest SELinux labels from host SELinux labels.
>>>>
>>>> As someone who knows very little about SELinux I'm eager to hear what
>>>> you think would be a good approach. Secure containers (e.g. Kata
>>>> Containers) are an important use case but virtio-fs can also be used as
>>>> the root file system for a guest (a scenario where full SELinux support
>>>> is needed).
>>>>
>>>> Thanks,
>>>> Stefan
>>>
>>> I am traveling right now. We should add in the SELinux team, and I will
>>> be able to look at this on Friday.
>>
>> Cc'ing the upstream SELinux mailing list for more insight. Here is a
>> public archive of the full thread:
>>
>> https://www.redhat.com/archives/virtio-fs/2020-February/msg00005.html
Also adding the LSM mailing list. This could be interesting for
any security module that uses extended attributes.
>
> FWIW, there were previous attempts to add FUSE support for per-file SELinux labeling (rather than just a single genfscon-based or context= mount option label for all files in the mount) but there were problems:
>
> - deadlock during mount with userspace waiting for mount(2) to complete and the kernel blocked on requesting the security.selinux xattr of the root directory as part of superblock setup during mount [1] [2].
> [1] https://lore.kernel.org/selinux/1280234607.4789.6.camel@moss-pluto.epoch.ncsc.mil/
> [2] https://lore.kernel.org/selinux/20120824195928.22970.16209.stgit@paris.rdu.redhat.com/
>
> - there was an attempt to introduce distinctions based on filesystem "subtype" so that whitelisted fuse filesystems could have xattr support enabled when it was known that their userspace would handle mount(2) safely [3] but this was apparently always broken and later reverted [4].
> [3] https://lore.kernel.org/selinux/20130416225619.GA30164@sh-el5.eng.rdu2.redhat.com/
> [4] https://lore.kernel.org/selinux/20131213195520.11231.81980.stgit@localhost/.
>
> - there is the issue of trusting the fuse filesystem for its labeling information and for domain/context transitions. At the least, we'd need a permission check to gate which contexts a fuse filesystem could supply (e.g. the filesystem associate check), and by default nosuid mounts disable domain transitions (although it is possible to selectively allow them via nosuid_transition now). Also, if a non-init user namespace mount, even if policy is configured to use xattrs (SECURITY_FS_USE_XATTR), we flip to using mountpoint labeling (i.e. implicit context= mount with the context derived from the mounter's context and matching type_transition rule if any) and we don't permit use of context mount options.
^ permalink raw reply
* Re: [PATCH v3 00/25] user_namespace: introduce fsid mappings
From: Serge E. Hallyn @ 2020-02-19 19:35 UTC (permalink / raw)
To: Christian Brauner
Cc: Stéphane Graber, Eric W. Biederman, Aleksa Sarai, Jann Horn,
smbarber, Seth Forshee, Alexander Viro, Alexey Dobriyan,
Serge Hallyn, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, linux-kernel, linux-fsdevel, containers,
linux-security-module, linux-api
In-Reply-To: <20200218143411.2389182-1-christian.brauner@ubuntu.com>
On Tue, Feb 18, 2020 at 03:33:46PM +0100, Christian Brauner wrote:
> With fsid mappings we can solve this by writing an id mapping of 0
> 100000 100000 and an fsid mapping of 0 300000 100000. On filesystem
> access the kernel will now lookup the mapping for 300000 in the fsid
> mapping tables of the user namespace. And since such a mapping exists,
> the corresponding files will have correct ownership.
So if I have
/proc/self/uid_map: 0 100000 100000
/proc/self/fsid_map: 1000 1000 1
1. If I read files from the rootfs which have host uid 101000, they
will appear as uid 100 to me?
2. If I read host files with uid 1000, they will appear as uid 1000 to me?
3. If I create a new file, as uid 1000, what will be the inode owning uid?
^ permalink raw reply
* Re: [PATCH 9/9] integrity: check properly whether EFI GetVariable() is available
From: Serge E. Hallyn @ 2020-02-19 20:46 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi, Leif Lindholm, Peter Jones, Alexander Graf,
Heinrich Schuchardt, Jeff Brasen, Atish Patra, x86, James Morris,
Serge E. Hallyn, linux-security-module
In-Reply-To: <20200219171907.11894-10-ardb@kernel.org>
On Wed, Feb 19, 2020 at 06:19:07PM +0100, Ard Biesheuvel wrote:
> Testing the value of the efi.get_variable function pointer is not
> the right way to establish whether the platform supports EFI
> variables at runtime. Instead, use the newly added granular check
> that can test for the presence of each EFI runtime service
> individually.
>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> security/integrity/platform_certs/load_uefi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> index 111898aad56e..e2fe1bd3abb9 100644
> --- a/security/integrity/platform_certs/load_uefi.c
> +++ b/security/integrity/platform_certs/load_uefi.c
> @@ -76,7 +76,7 @@ static int __init load_uefi_certs(void)
> unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
> int rc = 0;
>
> - if (!efi.get_variable)
> + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
Sorry, where is this defined?
> return false;
>
> /* Get db, MokListRT, and dbx. They might not exist, so it isn't
> --
> 2.17.1
^ permalink raw reply
* Re: [PATCH 9/9] integrity: check properly whether EFI GetVariable() is available
From: Ard Biesheuvel @ 2020-02-19 21:00 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: linux-efi, Leif Lindholm, Peter Jones, Alexander Graf,
Heinrich Schuchardt, Jeff Brasen, Atish Patra,
the arch/x86 maintainers, James Morris, linux-security-module
In-Reply-To: <20200219204603.GA28639@mail.hallyn.com>
On Wed, 19 Feb 2020 at 21:46, Serge E. Hallyn <serge@hallyn.com> wrote:
>
> On Wed, Feb 19, 2020 at 06:19:07PM +0100, Ard Biesheuvel wrote:
> > Testing the value of the efi.get_variable function pointer is not
> > the right way to establish whether the platform supports EFI
> > variables at runtime. Instead, use the newly added granular check
> > that can test for the presence of each EFI runtime service
> > individually.
> >
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: linux-security-module@vger.kernel.org
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > security/integrity/platform_certs/load_uefi.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > index 111898aad56e..e2fe1bd3abb9 100644
> > --- a/security/integrity/platform_certs/load_uefi.c
> > +++ b/security/integrity/platform_certs/load_uefi.c
> > @@ -76,7 +76,7 @@ static int __init load_uefi_certs(void)
> > unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
> > int rc = 0;
> >
> > - if (!efi.get_variable)
> > + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
>
> Sorry, where is this defined?
>
Apologies, I failed to cc everyone on the whole series.
It is defined in the first patch.
https://lore.kernel.org/linux-efi/20200219171907.11894-1-ardb@kernel.org/
^ permalink raw reply
* Re: [PATCH v3 00/25] user_namespace: introduce fsid mappings
From: Serge E. Hallyn @ 2020-02-19 21:48 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Christian Brauner, Stéphane Graber, Eric W. Biederman,
Aleksa Sarai, Jann Horn, smbarber, Seth Forshee, Alexander Viro,
Alexey Dobriyan, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, linux-kernel, linux-fsdevel, containers,
linux-security-module, linux-api
In-Reply-To: <20200219193558.GA27641@mail.hallyn.com>
On Wed, Feb 19, 2020 at 01:35:58PM -0600, Serge E. Hallyn wrote:
> On Tue, Feb 18, 2020 at 03:33:46PM +0100, Christian Brauner wrote:
> > With fsid mappings we can solve this by writing an id mapping of 0
> > 100000 100000 and an fsid mapping of 0 300000 100000. On filesystem
> > access the kernel will now lookup the mapping for 300000 in the fsid
> > mapping tables of the user namespace. And since such a mapping exists,
> > the corresponding files will have correct ownership.
>
> So if I have
>
> /proc/self/uid_map: 0 100000 100000
> /proc/self/fsid_map: 1000 1000 1
Oh, sorry. Your explanation in 20/25 i think set me straight, though I need
to think through a few more examples.
...
> 3. If I create a new file, as nsuid 1000, what will be the inode owning kuid?
(Note - I edited the quoted txt above to be more precise)
I'm still not quite clear on this. I believe the fsid mapping will take
precedence so it'll be uid 1000 ? Per mount behavior would be nice there,
but perhaps unwieldy.
^ permalink raw reply
* Re: [PATCH v3 00/25] user_namespace: introduce fsid mappings
From: Tycho Andersen @ 2020-02-19 21:56 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Christian Brauner, Stéphane Graber, Eric W. Biederman,
Aleksa Sarai, Jann Horn, smbarber, Seth Forshee, Alexander Viro,
Alexey Dobriyan, James Morris, Kees Cook, Jonathan Corbet,
Phil Estes, linux-kernel, linux-fsdevel, containers,
linux-security-module, linux-api
In-Reply-To: <20200219214837.GA29159@mail.hallyn.com>
On Wed, Feb 19, 2020 at 03:48:37PM -0600, Serge E. Hallyn wrote:
> On Wed, Feb 19, 2020 at 01:35:58PM -0600, Serge E. Hallyn wrote:
> > On Tue, Feb 18, 2020 at 03:33:46PM +0100, Christian Brauner wrote:
> > > With fsid mappings we can solve this by writing an id mapping of 0
> > > 100000 100000 and an fsid mapping of 0 300000 100000. On filesystem
> > > access the kernel will now lookup the mapping for 300000 in the fsid
> > > mapping tables of the user namespace. And since such a mapping exists,
> > > the corresponding files will have correct ownership.
> >
> > So if I have
> >
> > /proc/self/uid_map: 0 100000 100000
> > /proc/self/fsid_map: 1000 1000 1
>
> Oh, sorry. Your explanation in 20/25 i think set me straight, though I need
> to think through a few more examples.
>
> ...
>
> > 3. If I create a new file, as nsuid 1000, what will be the inode owning kuid?
>
> (Note - I edited the quoted txt above to be more precise)
>
> I'm still not quite clear on this. I believe the fsid mapping will take
> precedence so it'll be uid 1000 ? Per mount behavior would be nice there,
> but perhaps unwieldy.
The is_userns_visible() bits seems to be an attempt at understanding
what people would want per-mount, with a policy hard coded in the
kernel.
But maybe per-mount behavior can be solved more elegantly with shifted
bind mounts, so we can drop all that from this series, and ignore
per-mount settings here?
Tycho
^ permalink raw reply
* Re: [RFC PATCH] security: <linux/lsm_hooks.h>: fix all kernel-doc warnings
From: James Morris @ 2020-02-20 0:10 UTC (permalink / raw)
To: Randy Dunlap
Cc: LKML, linux-security-module, John Johansen, Kees Cook,
Micah Morton, Serge E. Hallyn, Paul Moore, Stephen Smalley,
Eric Paris, Casey Schaufler, Kentaro Takeda, Tetsuo Handa
In-Reply-To: <fb2c98bd-b579-6ad0-721a-56a4f81f0d6e@infradead.org>
On Sat, 15 Feb 2020, Randy Dunlap wrote:
> From: Randy Dunlap <rdunlap@infradead.org>
>
> Fix all kernel-doc warnings in <linux/lsm_hooks.h>.
> Fixes the following warnings:
>
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'quotactl' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'quota_on' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'getprocattr' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'setprocattr' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'locked_down' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'perf_event_open' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'perf_event_alloc' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'perf_event_free' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'perf_event_read' not described in 'security_list_options'
> ../include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'perf_event_write' not described in 'security_list_options'
>
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Thanks, applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 9/9] integrity: check properly whether EFI GetVariable() is available
From: Serge E. Hallyn @ 2020-02-20 3:19 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Serge E. Hallyn, linux-efi, Leif Lindholm, Peter Jones,
Alexander Graf, Heinrich Schuchardt, Jeff Brasen, Atish Patra,
the arch/x86 maintainers, James Morris, linux-security-module
In-Reply-To: <CAKv+Gu_c4mhMN5LBoH5jJWwMHaMxKY7zcp4hiqdRFiadPT8Nww@mail.gmail.com>
On Wed, Feb 19, 2020 at 10:00:11PM +0100, Ard Biesheuvel wrote:
> On Wed, 19 Feb 2020 at 21:46, Serge E. Hallyn <serge@hallyn.com> wrote:
> >
> > On Wed, Feb 19, 2020 at 06:19:07PM +0100, Ard Biesheuvel wrote:
> > > Testing the value of the efi.get_variable function pointer is not
> > > the right way to establish whether the platform supports EFI
> > > variables at runtime. Instead, use the newly added granular check
> > > that can test for the presence of each EFI runtime service
> > > individually.
> > >
> > > Cc: James Morris <jmorris@namei.org>
> > > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > > Cc: linux-security-module@vger.kernel.org
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > > security/integrity/platform_certs/load_uefi.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > > index 111898aad56e..e2fe1bd3abb9 100644
> > > --- a/security/integrity/platform_certs/load_uefi.c
> > > +++ b/security/integrity/platform_certs/load_uefi.c
> > > @@ -76,7 +76,7 @@ static int __init load_uefi_certs(void)
> > > unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
> > > int rc = 0;
> > >
> > > - if (!efi.get_variable)
> > > + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
> >
> > Sorry, where is this defined?
> >
>
> Apologies, I failed to cc everyone on the whole series.
>
> It is defined in the first patch.
>
> https://lore.kernel.org/linux-efi/20200219171907.11894-1-ardb@kernel.org/
Gotcha, thanks, I shoulda get-lore-mbox'ed it :)
Acked-by: Serge Hallyn <serge@hallyn.com>
thanks,
-serge
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox