Re: [PATCH] ksys_mount: check for permissions before resource allocation

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

Just CC’ed to some of maintainers.

$ perl scripts/get_maintainer.pl fs/0001-ksys_mount-check-for-permissions-before-resource-all.patch
Alexander Viro <viro@zeniv.linux.org.uk> (maintainer:FILESYSTEMS (VFS and infrastructure))
linux-fsdevel@vger.kernel.org (open list:FILESYSTEMS (VFS and infrastructure))
linux-kernel@vger.kernel.org (open list)

> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <matvejchikov@gmail.com> wrote:
> 
> Early check for mount permissions prevents possible allocation of 3
> pages from kmalloc() pool by unpriveledged user which can be used for
> spraying the kernel heap.
> 
> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
> ---
> fs/namespace.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 5f75969adff1..1ef8feb2de2a 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -3046,6 +3046,9 @@ int ksys_mount(char __user *dev_name, char __user *dir_name, char __user *type,
> 	char *kernel_dev;
> 	void *options;
> 
> +	if (!may_mount())
> +		return -EPERM;
> +
> 	kernel_type = copy_mount_string(type);
> 	ret = PTR_ERR(kernel_type);
> 	if (IS_ERR(kernel_type))
> --
> 2.17.0
> 

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Al Viro]

On Tue, Jun 05, 2018 at 10:59:51AM +0400, Ilya Matveychikov wrote:
> Just CC’ed to some of maintainers.
> 
> $ perl scripts/get_maintainer.pl fs/0001-ksys_mount-check-for-permissions-before-resource-all.patch
> Alexander Viro <viro@zeniv.linux.org.uk> (maintainer:FILESYSTEMS (VFS and infrastructure))
> linux-fsdevel@vger.kernel.org (open list:FILESYSTEMS (VFS and infrastructure))
> linux-kernel@vger.kernel.org (open list)
> 
> > On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <matvejchikov@gmail.com> wrote:
> > 
> > Early check for mount permissions prevents possible allocation of 3
> > pages from kmalloc() pool by unpriveledged user which can be used for
> > spraying the kernel heap.

I'm sorry, but there are arseloads of unpriveleged syscalls that do the same,
starting with read() from procfs files.  So what the hell does it buy?

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 5, 2018, at 3:26 PM, Al Viro <viro@ZenIV.linux.org.uk> wrote:
>> 
>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <matvejchikov@gmail.com> wrote:
>>> 
>>> Early check for mount permissions prevents possible allocation of 3
>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>> spraying the kernel heap.
> 
> I'm sorry, but there are arseloads of unpriveleged syscalls that do the same,
> starting with read() from procfs files.  So what the hell does it buy?

Means that if all do the same shit no reason to fix it? Sounds weird...

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Al Viro]

On Tue, Jun 05, 2018 at 03:35:55PM +0400, Ilya Matveychikov wrote:
> 
> > On Jun 5, 2018, at 3:26 PM, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> >> 
> >>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <matvejchikov@gmail.com> wrote:
> >>> 
> >>> Early check for mount permissions prevents possible allocation of 3
> >>> pages from kmalloc() pool by unpriveledged user which can be used for
> >>> spraying the kernel heap.
> > 
> > I'm sorry, but there are arseloads of unpriveleged syscalls that do the same,
> > starting with read() from procfs files.  So what the hell does it buy?
> 
> Means that if all do the same shit no reason to fix it? Sounds weird...

Fix *what*?  You do realize that there's no permission checks to stop e.g.
stat(2) from copying the pathname in, right?  With user-supplied contents,
even...

If you depend upon preventing kmalloc'ed temporary allocations filled
with user-supplied data, you are screwed, plain and simple.  It really can't
be prevented, in a lot of ways that are much less exotic than mount(2).
Most of syscall arguments are copied in, before we get any permission
checks.  It does happen and it will happen - examining them while they are
still in userland is a nightmare in a lot of respects, starting with
security.

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 5, 2018, at 3:53 PM, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> On Tue, Jun 05, 2018 at 03:35:55PM +0400, Ilya Matveychikov wrote:
>> 
>>> On Jun 5, 2018, at 3:26 PM, Al Viro <viro@ZenIV.linux.org.uk> wrote:
>>>> 
>>>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <matvejchikov@gmail.com> wrote:
>>>>> 
>>>>> Early check for mount permissions prevents possible allocation of 3
>>>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>>>> spraying the kernel heap.
>>> 
>>> I'm sorry, but there are arseloads of unpriveleged syscalls that do the same,
>>> starting with read() from procfs files.  So what the hell does it buy?
>> 
>> Means that if all do the same shit no reason to fix it? Sounds weird...
> 
> Fix *what*?  You do realize that there's no permission checks to stop e.g.
> stat(2) from copying the pathname in, right?  With user-supplied contents,
> even...
> 
> If you depend upon preventing kmalloc'ed temporary allocations filled
> with user-supplied data, you are screwed, plain and simple.  It really can't
> be prevented, in a lot of ways that are much less exotic than mount(2).
> Most of syscall arguments are copied in, before we get any permission
> checks.  It does happen and it will happen - examining them while they are
> still in userland is a nightmare in a lot of respects, starting with
> security.

I agree that it’s impossible to completely avoid this kind of allocations
and examining data in user-land will be the bigger problem than copying
arguments to the kernel. But aside of that what’s wrong with the idea of
having the permission check before doing any kind of work?

BTW, sys_umount() has this check in the right place - before doing anything.
So, why not to have the same logic for mount/umount?

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[David Sterba]

On Tue, Jun 05, 2018 at 04:07:15PM +0400, Ilya Matveychikov wrote:
> > On Jun 5, 2018, at 3:53 PM, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> > On Tue, Jun 05, 2018 at 03:35:55PM +0400, Ilya Matveychikov wrote:
> >>> On Jun 5, 2018, at 3:26 PM, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> >>>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <matvejchikov@gmail.com> wrote:
> >>>>> Early check for mount permissions prevents possible allocation of 3
> >>>>> pages from kmalloc() pool by unpriveledged user which can be used for
> >>>>> spraying the kernel heap.
> >>> 
> >>> I'm sorry, but there are arseloads of unpriveleged syscalls that do the same,
> >>> starting with read() from procfs files.  So what the hell does it buy?
> >> 
> >> Means that if all do the same shit no reason to fix it? Sounds weird...
> > 
> > Fix *what*?  You do realize that there's no permission checks to stop e.g.
> > stat(2) from copying the pathname in, right?  With user-supplied contents,
> > even...
> > 
> > If you depend upon preventing kmalloc'ed temporary allocations filled
> > with user-supplied data, you are screwed, plain and simple.  It really can't
> > be prevented, in a lot of ways that are much less exotic than mount(2).
> > Most of syscall arguments are copied in, before we get any permission
> > checks.  It does happen and it will happen - examining them while they are
> > still in userland is a nightmare in a lot of respects, starting with
> > security.
> 
> I agree that it’s impossible to completely avoid this kind of allocations
> and examining data in user-land will be the bigger problem than copying
> arguments to the kernel. But aside of that what’s wrong with the idea of
> having the permission check before doing any kind of work?

Isn't there some sysctl knob or config option to sanitize freed memory?
I doubt that using kzfree everywhere unconditionally would be welcome,
also would not scale as there are too many of them. This IMHO leaves
only the build-time option for those willing to pay the performance hit.

> BTW, sys_umount() has this check in the right place - before doing anything.
> So, why not to have the same logic for mount/umount?

What if the check is not equivalent to the one done later? may_mount
needs namespace, it will be available at umount time but not necessarily
during mount due to the security hooks.

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Al Viro]

On Tue, Jun 05, 2018 at 04:07:15PM +0400, Ilya Matveychikov wrote:

> > If you depend upon preventing kmalloc'ed temporary allocations filled
> > with user-supplied data, you are screwed, plain and simple.  It really can't
> > be prevented, in a lot of ways that are much less exotic than mount(2).
> > Most of syscall arguments are copied in, before we get any permission
> > checks.  It does happen and it will happen - examining them while they are
> > still in userland is a nightmare in a lot of respects, starting with
> > security.
> 
> I agree that it’s impossible to completely avoid this kind of allocations
> and examining data in user-land will be the bigger problem than copying
> arguments to the kernel. But aside of that what’s wrong with the idea of
> having the permission check before doing any kind of work?

Presenting that as mitigating a vulnerability.  It's neither better nor worse
in that respect than the original.

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 5, 2018, at 4:28 PM, David Sterba <dsterba@suse.cz> wrote:
> 
>> BTW, sys_umount() has this check in the right place - before doing anything.
>> So, why not to have the same logic for mount/umount?
> 
> What if the check is not equivalent to the one done later? may_mount
> needs namespace, it will be available at umount time but not necessarily
> during mount due to the security hooks.

Might be the issue, you’re right. I can’t tell it for sure as I’m not so
familiar with linux/fs code.

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Eric W. Biederman]

Ilya Matveychikov <matvejchikov@gmail.com> writes:

> Just CC’ed to some of maintainers.
>
> $ perl scripts/get_maintainer.pl fs/0001-ksys_mount-check-for-permissions-before-resource-all.patch
> Alexander Viro <viro@zeniv.linux.org.uk> (maintainer:FILESYSTEMS (VFS and infrastructure))
> linux-fsdevel@vger.kernel.org (open list:FILESYSTEMS (VFS and infrastructure))
> linux-kernel@vger.kernel.org (open list)
>
>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <matvejchikov@gmail.com> wrote:
>> 
>> Early check for mount permissions prevents possible allocation of 3
>> pages from kmalloc() pool by unpriveledged user which can be used for
>> spraying the kernel heap.

*Snort*

You clearly have not read may_mount.  Your modified code still
let's unprivileged users in.  So even if all of Al's good objections
were not applicable this change would still be buggy and wrong.

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

>> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
>> ---
>> fs/namespace.c | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/fs/namespace.c b/fs/namespace.c
>> index 5f75969adff1..1ef8feb2de2a 100644
>> --- a/fs/namespace.c
>> +++ b/fs/namespace.c
>> @@ -3046,6 +3046,9 @@ int ksys_mount(char __user *dev_name, char __user *dir_name, char __user *type,
>> 	char *kernel_dev;
>> 	void *options;
>> 
>> +	if (!may_mount())
>> +		return -EPERM;
>> +
>> 	kernel_type = copy_mount_string(type);
>> 	ret = PTR_ERR(kernel_type);
>> 	if (IS_ERR(kernel_type))
>> --
>> 2.17.0
>> 

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 5, 2018, at 11:56 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> 
> Ilya Matveychikov <matvejchikov@gmail.com> writes:
> 
>> Just CC’ed to some of maintainers.
>> 
>> $ perl scripts/get_maintainer.pl fs/0001-ksys_mount-check-for-permissions-before-resource-all.patch
>> Alexander Viro <viro@zeniv.linux.org.uk> (maintainer:FILESYSTEMS (VFS and infrastructure))
>> linux-fsdevel@vger.kernel.org (open list:FILESYSTEMS (VFS and infrastructure))
>> linux-kernel@vger.kernel.org (open list)
>> 
>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <matvejchikov@gmail.com> wrote:
>>> 
>>> Early check for mount permissions prevents possible allocation of 3
>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>> spraying the kernel heap.
> 
> *Snort*
> 
> You clearly have not read may_mount.  Your modified code still
> let's unprivileged users in.  So even if all of Al's good objections
> were not applicable this change would still be buggy and wrong.
> 
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>


Don’t get me wrong but may_mount() is:

static inline bool may_mount(void)
{
        return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN);
}

What do you mean by "You clearly have not read may_mount”? The only thing that
can affect may_mount result (as mentioned earlier) is that task’s NS capability
might be changed by security_sb_mount() hook.

So, do you think that is’s possible to NOT have CAP_SYS_ADMIN while entering to
ksys_mount() but getting it with the security_sb_mount() hook?

This is the only case I see that using may_mount() before security_sb_mount()
is wrong. This was the point?

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Eric W. Biederman]

Ilya Matveychikov <matvejchikov@gmail.com> writes:

>> On Jun 5, 2018, at 11:56 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> 
>> Ilya Matveychikov <matvejchikov@gmail.com> writes:
>> 
>>> Just CC’ed to some of maintainers.
>>> 
>>> $ perl scripts/get_maintainer.pl fs/0001-ksys_mount-check-for-permissions-before-resource-all.patch
>>> Alexander Viro <viro@zeniv.linux.org.uk> (maintainer:FILESYSTEMS (VFS and infrastructure))
>>> linux-fsdevel@vger.kernel.org (open list:FILESYSTEMS (VFS and infrastructure))
>>> linux-kernel@vger.kernel.org (open list)
>>> 
>>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <matvejchikov@gmail.com> wrote:
>>>> 
>>>> Early check for mount permissions prevents possible allocation of 3
>>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>>> spraying the kernel heap.
>> 
>> *Snort*
>> 
>> You clearly have not read may_mount.  Your modified code still
>> let's unprivileged users in.  So even if all of Al's good objections
>> were not applicable this change would still be buggy and wrong.
>> 
>> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
>
> Don’t get me wrong but may_mount() is:
>
> static inline bool may_mount(void)
> {
>         return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN);
> }
>
> What do you mean by "You clearly have not read may_mount”? The only thing that
> can affect may_mount result (as mentioned earlier) is that task’s NS capability
> might be changed by security_sb_mount() hook.
>
> So, do you think that is’s possible to NOT have CAP_SYS_ADMIN while entering to
> ksys_mount() but getting it with the security_sb_mount() hook?

I mean it works for unprivileged users.

You can try "unshare -Urm" on a reasonably recent kernel and it will
work and you can then mount and unmount things.

Strictly speaking it only works if you have the appropriate set of
capabilities in your user namespace.  But that does not imply you are a
privileged user in the broader sense.

Any user can create a user namespace, and become the root user
in a user namespace.  The root user in a user namespace can create
a mount namespace.  The root user in a user namespace can mount
and unmount filesystems in their namespace.

Or in net anyone can call mount and get past the may_mount test.

Without reducing who can cause the kernel allocation moving the test is
pointless.

Eric

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 6, 2018, at 6:22 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> 
> Ilya Matveychikov <matvejchikov@gmail.com> writes:
> 
>>> On Jun 5, 2018, at 11:56 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>> 
>>> Ilya Matveychikov <matvejchikov@gmail.com> writes:
>>> 
>>>> Just CC’ed to some of maintainers.
>>>> 
>>>> $ perl scripts/get_maintainer.pl fs/0001-ksys_mount-check-for-permissions-before-resource-all.patch
>>>> Alexander Viro <viro@zeniv.linux.org.uk> (maintainer:FILESYSTEMS (VFS and infrastructure))
>>>> linux-fsdevel@vger.kernel.org (open list:FILESYSTEMS (VFS and infrastructure))
>>>> linux-kernel@vger.kernel.org (open list)
>>>> 
>>>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <matvejchikov@gmail.com> wrote:
>>>>> 
>>>>> Early check for mount permissions prevents possible allocation of 3
>>>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>>>> spraying the kernel heap.
>>> 
>>> *Snort*
>>> 
>>> You clearly have not read may_mount.  Your modified code still
>>> let's unprivileged users in.  So even if all of Al's good objections
>>> were not applicable this change would still be buggy and wrong.
>>> 
>>> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> 
>> 
>> Don’t get me wrong but may_mount() is:
>> 
>> static inline bool may_mount(void)
>> {
>>        return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN);
>> }
>> 
>> What do you mean by "You clearly have not read may_mount”? The only thing that
>> can affect may_mount result (as mentioned earlier) is that task’s NS capability
>> might be changed by security_sb_mount() hook.
>> 
>> So, do you think that is’s possible to NOT have CAP_SYS_ADMIN while entering to
>> ksys_mount() but getting it with the security_sb_mount() hook?
> 
> I mean it works for unprivileged users.
> 
> You can try "unshare -Urm" on a reasonably recent kernel and it will
> work and you can then mount and unmount things.
> 
> Strictly speaking it only works if you have the appropriate set of
> capabilities in your user namespace.  But that does not imply you are a
> privileged user in the broader sense.
> 
> Any user can create a user namespace, and become the root user
> in a user namespace.  The root user in a user namespace can create
> a mount namespace.  The root user in a user namespace can mount
> and unmount filesystems in their namespace.
> 
> Or in net anyone can call mount and get past the may_mount test.
> 
> Without reducing who can cause the kernel allocation moving the test is
> pointless.
> 

Ok, now I see. No reason to make change as it doesn’t really prevents users
of doing mount() using they own namespaces.

Thank you for the explanation.