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 <[email protected]>
---
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
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 <[email protected]> (maintainer:FILESYSTEMS (VFS and infrastructure))
[email protected] (open list:FILESYSTEMS (VFS and infrastructure))
[email protected] (open list)
> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <[email protected]> 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 <[email protected]>
> ---
> 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
>
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 <[email protected]> (maintainer:FILESYSTEMS (VFS and infrastructure))
> [email protected] (open list:FILESYSTEMS (VFS and infrastructure))
> [email protected] (open list)
>
> > On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <[email protected]> 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?
> On Jun 5, 2018, at 3:26 PM, Al Viro <[email protected]> wrote:
>>
>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <[email protected]> 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...
On Tue, Jun 05, 2018 at 03:35:55PM +0400, Ilya Matveychikov wrote:
>
> > On Jun 5, 2018, at 3:26 PM, Al Viro <[email protected]> wrote:
> >>
> >>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <[email protected]> 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.
> On Jun 5, 2018, at 3:53 PM, Al Viro <[email protected]> wrote:
>
> On Tue, Jun 05, 2018 at 03:35:55PM +0400, Ilya Matveychikov wrote:
>>
>>> On Jun 5, 2018, at 3:26 PM, Al Viro <[email protected]> wrote:
>>>>
>>>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <[email protected]> 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?
On Tue, Jun 05, 2018 at 04:07:15PM +0400, Ilya Matveychikov wrote:
> > On Jun 5, 2018, at 3:53 PM, Al Viro <[email protected]> wrote:
> > On Tue, Jun 05, 2018 at 03:35:55PM +0400, Ilya Matveychikov wrote:
> >>> On Jun 5, 2018, at 3:26 PM, Al Viro <[email protected]> wrote:
> >>>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <[email protected]> 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.
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.
> On Jun 5, 2018, at 4:28 PM, David Sterba <[email protected]> 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.
Ilya Matveychikov <[email protected]> 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 <[email protected]> (maintainer:FILESYSTEMS (VFS and infrastructure))
> [email protected] (open list:FILESYSTEMS (VFS and infrastructure))
> [email protected] (open list)
>
>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <[email protected]> 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" <[email protected]>
>> Signed-off-by: Ilya V. Matveychikov <[email protected]>
>> ---
>> 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
>>
> On Jun 5, 2018, at 11:56 PM, Eric W. Biederman <[email protected]> wrote:
>
> Ilya Matveychikov <[email protected]> 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 <[email protected]> (maintainer:FILESYSTEMS (VFS and infrastructure))
>> [email protected] (open list:FILESYSTEMS (VFS and infrastructure))
>> [email protected] (open list)
>>
>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <[email protected]> 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" <[email protected]>
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?
Ilya Matveychikov <[email protected]> writes:
>> On Jun 5, 2018, at 11:56 PM, Eric W. Biederman <[email protected]> wrote:
>>
>> Ilya Matveychikov <[email protected]> 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 <[email protected]> (maintainer:FILESYSTEMS (VFS and infrastructure))
>>> [email protected] (open list:FILESYSTEMS (VFS and infrastructure))
>>> [email protected] (open list)
>>>
>>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <[email protected]> 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" <[email protected]>
>
>
> 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
> On Jun 6, 2018, at 6:22 PM, Eric W. Biederman <[email protected]> wrote:
>
> Ilya Matveychikov <[email protected]> writes:
>
>>> On Jun 5, 2018, at 11:56 PM, Eric W. Biederman <[email protected]> wrote:
>>>
>>> Ilya Matveychikov <[email protected]> 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 <[email protected]> (maintainer:FILESYSTEMS (VFS and infrastructure))
>>>> [email protected] (open list:FILESYSTEMS (VFS and infrastructure))
>>>> [email protected] (open list)
>>>>
>>>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov <[email protected]> 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" <[email protected]>
>>
>>
>> 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.