[PATCH] prctl: fix validation of an address

[PATCH] prctl: fix validation of an address

[Andrey Vagin]

The address should be bigger than dac_mmap_min_addr, because
a process with CAP_RAWIO can map a vma bellow mmap_min_addr.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Eric Paris <eparis@redhat.com>
Cc: James Morris <jmorris@namei.org>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 kernel/sys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 265b376..e0e1bbd 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1868,7 +1868,7 @@ static int prctl_set_mm(int opt, unsigned long addr,
 	if (opt == PR_SET_MM_EXE_FILE)
 		return prctl_set_mm_exe_file(mm, (unsigned int)addr);
 
-	if (addr >= TASK_SIZE || addr < mmap_min_addr)
+	if (addr >= TASK_SIZE || addr < dac_mmap_min_addr)
 		return -EINVAL;
 
 	error = -EINVAL;
-- 
1.7.11.7

Re: [PATCH] prctl: fix validation of an address

[Eric Paris]

On Sat, 2012-12-29 at 15:00 +0400, Andrey Vagin wrote:
> The address should be bigger than dac_mmap_min_addr, because
> a process with CAP_RAWIO can map a vma bellow mmap_min_addr.

NAK

This doesn't make any sense.  dac_mmap_min_addr should ONLY be used in
security/min_addr.c and security/commoncap.c.  Period.  You should not
be allowed to circumvent LSM protections.  Maybe you are missing that
mmap_min_addr = max(dac_mmap_min_addr, CONFIG_LSM_MMAP_MIN_ADDR)  ?

But this patch is absolutely unacceptable.  Maybe you can help me
understand what problem you had and what you were hoping for?

-Eric
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Eric Paris <eparis@redhat.com>
> Cc: James Morris <jmorris@namei.org>
> Signed-off-by: Andrey Vagin <avagin@openvz.org>
> ---
>  kernel/sys.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 265b376..e0e1bbd 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1868,7 +1868,7 @@ static int prctl_set_mm(int opt, unsigned long addr,
>  	if (opt == PR_SET_MM_EXE_FILE)
>  		return prctl_set_mm_exe_file(mm, (unsigned int)addr);
>  
> -	if (addr >= TASK_SIZE || addr < mmap_min_addr)
> +	if (addr >= TASK_SIZE || addr < dac_mmap_min_addr)
>  		return -EINVAL;
>  
>  	error = -EINVAL;

Re: [PATCH] prctl: fix validation of an address

[Andrew Vagin]

On Sun, Dec 30, 2012 at 05:03:07PM -0500, Eric Paris wrote:
> On Sat, 2012-12-29 at 15:00 +0400, Andrey Vagin wrote:
> > The address should be bigger than dac_mmap_min_addr, because
> > a process with CAP_RAWIO can map a vma bellow mmap_min_addr.
> 
> NAK

Currently prctl(PR_SET_MM_*, addr, ) returns EINVAL for valid addresses.
I think it's a bug. Are you agree?

> 
> This doesn't make any sense.  dac_mmap_min_addr should ONLY be used in
> security/min_addr.c and security/commoncap.c.  Period.  You should not

We can add a function to security/commoncap.c...

> be allowed to circumvent LSM protections.  Maybe you are missing that
> mmap_min_addr = max(dac_mmap_min_addr, CONFIG_LSM_MMAP_MIN_ADDR)  ?

Or maybe you are missing ;) that
/*
 * cap_mmap_addr - check if able to map given addr
 * @addr: address attempting to be mapped
 *
 * If the process is attempting to map memory below dac_mmap_min_addr
 * they need  CAP_SYS_RAWIO.

It was a real case. I have an application, which is failed due to
prlctl. This application uses vm.mmap_min_addr to calculate an adress
for a new vma.

$ sysctl -a | grep min_add
vm.mmap_min_addr = 4096

CONFIG_LSM_MMAP_MIN_ADDR could not be got from user space.

This application can use a real value of mmap_min_addr, but it is not
provided into userspace.

> 
> But this patch is absolutely unacceptable.  Maybe you can help me
> understand what problem you had and what you were hoping for?

Currently a task can have user memory area bellow dac_mmap_min_addr,
but prctl returns -EINVAL for such addresses.
How can I understand the reason, if I know that the address is valid?

I like Linux, because I always can predict its behavior, but this
dac_mmap_min_addr vs mmap_min_addr looks stange for me.

> 
> -Eric
> > 
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> > Cc: Serge Hallyn <serge.hallyn@canonical.com>
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > Cc: Eric Paris <eparis@redhat.com>
> > Cc: James Morris <jmorris@namei.org>
> > Signed-off-by: Andrey Vagin <avagin@openvz.org>
> > ---
> >  kernel/sys.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/sys.c b/kernel/sys.c
> > index 265b376..e0e1bbd 100644
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -1868,7 +1868,7 @@ static int prctl_set_mm(int opt, unsigned long addr,
> >  	if (opt == PR_SET_MM_EXE_FILE)
> >  		return prctl_set_mm_exe_file(mm, (unsigned int)addr);
> >  
> > -	if (addr >= TASK_SIZE || addr < mmap_min_addr)
> > +	if (addr >= TASK_SIZE || addr < dac_mmap_min_addr)
> >  		return -EINVAL;
> >  
> >  	error = -EINVAL;
> 
> 

Re: [PATCH] prctl: fix validation of an address

[Eric Paris]

On Mon, 2012-12-31 at 14:14 +0400, Andrew Vagin wrote:
> On Sun, Dec 30, 2012 at 05:03:07PM -0500, Eric Paris wrote:
> > On Sat, 2012-12-29 at 15:00 +0400, Andrey Vagin wrote:
> > > The address should be bigger than dac_mmap_min_addr, because
> > > a process with CAP_RAWIO can map a vma bellow mmap_min_addr.
> > 
> > NAK
> 
> Currently prctl(PR_SET_MM_*, addr, ) returns EINVAL for valid addresses.
> I think it's a bug. Are you agree?

Can you help me understand how prctl(PR_SET_MM_*, relates to
checkpoint/restore?  My worry here is that somehow this interface could
be used to bypass the security properties of mmap_min_addr.    I have no
idea how the interface is used, so I don't know if my fears are founded.
When I hear 'restore' I think of a privileged application setting up
some unprivileged application based on untrusted data.  My fear is that
some unpriv application, that doesn't have permission to map below
mmap_min_addr, may be able to trick the privileged application, which
would have this permission, into doing it on its behalf.  Does that make
sense?  Is that a realistic scenario with how this interface is used?

> CONFIG_LSM_MMAP_MIN_ADDR could not be got from user space.
> 
> This application can use a real value of mmap_min_addr, but it is not
> provided into userspace.

Unrelated to this patch issue, but I guess either could be exposed if
there is a need.

> Currently a task can have user memory area bellow dac_mmap_min_addr,
> but prctl returns -EINVAL for such addresses.
> How can I understand the reason, if I know that the address is valid?

Talking about dac_mmap_min_addr is wrong.  The capabilities security
module uses dac_mmap_min_addr but other LSMs can (and obviously do) use
other things.  mmap_min_addr is just the shorthand to make sure you
clear all hurdles.  Breaking those hurdles up outside of the security
subsystem is wrong.

The kernel makes the decision on what is valid via security_mmap_addr().
Assuming there are no security fears of an untrusted application
tricking some priviledged application to set up these maps the answer is
just calling security_mmap_addr() instead of doing if(addr <
mmap_min_addr) return -EINVAL;

I don't know if it is a good idea to allow this interface to ever go
below mmap_min_addr, but I do know that using (or even thinking about)
dac_mmap_min_addr is wrong and you should be looking at
security_mmap_addr() if you look at anything...

-Eric

Re: [PATCH] prctl: fix validation of an address

[Cyrill Gorcunov]

On Mon, Dec 31, 2012 at 09:27:14AM -0500, Eric Paris wrote:
> On Mon, 2012-12-31 at 14:14 +0400, Andrew Vagin wrote:
> > On Sun, Dec 30, 2012 at 05:03:07PM -0500, Eric Paris wrote:
> > > On Sat, 2012-12-29 at 15:00 +0400, Andrey Vagin wrote:
> > > > The address should be bigger than dac_mmap_min_addr, because
> > > > a process with CAP_RAWIO can map a vma bellow mmap_min_addr.
> > > 
> > > NAK
> > 
> > Currently prctl(PR_SET_MM_*, addr, ) returns EINVAL for valid addresses.
> > I think it's a bug. Are you agree?
> 
> Can you help me understand how prctl(PR_SET_MM_*, relates to
> checkpoint/restore?  My worry here is that somehow this interface could

Here how we use it (from userspace code)

	ret |= sys_prctl_safe(PR_SET_MM, PR_SET_MM_START_CODE, (long)args->mm.mm_start_code, 0);
	ret |= sys_prctl_safe(PR_SET_MM, PR_SET_MM_END_CODE, (long)args->mm.mm_end_code, 0);
	...

the values of mm.mm_start_code and such are saved in image file and obtained
during checkpoint stage. Note the prctl_set_mm requires the caller to have
CAP_SYS_RESOURCE privilege granted.

> be used to bypass the security properties of mmap_min_addr.    I have no
> idea how the interface is used, so I don't know if my fears are founded.
> When I hear 'restore' I think of a privileged application setting up
> some unprivileged application based on untrusted data.  My fear is that
> some unpriv application, that doesn't have permission to map below
> mmap_min_addr, may be able to trick the privileged application, which
> would have this permission, into doing it on its behalf.  Does that make
> sense?  Is that a realistic scenario with how this interface is used?

> 
> > CONFIG_LSM_MMAP_MIN_ADDR could not be got from user space.
> > 
> > This application can use a real value of mmap_min_addr, but it is not
> > provided into userspace.
> 
> Unrelated to this patch issue, but I guess either could be exposed if
> there is a need.
> 
> > Currently a task can have user memory area bellow dac_mmap_min_addr,
> > but prctl returns -EINVAL for such addresses.
> > How can I understand the reason, if I know that the address is valid?
> 
> Talking about dac_mmap_min_addr is wrong.  The capabilities security
> module uses dac_mmap_min_addr but other LSMs can (and obviously do) use
> other things.  mmap_min_addr is just the shorthand to make sure you
> clear all hurdles.  Breaking those hurdles up outside of the security
> subsystem is wrong.
> 
> The kernel makes the decision on what is valid via security_mmap_addr().
> Assuming there are no security fears of an untrusted application
> tricking some priviledged application to set up these maps the answer is
> just calling security_mmap_addr() instead of doing if(addr <
> mmap_min_addr) return -EINVAL;

If only I've not missed something obvious, the check for security_mmap_addr() here
instead of poking the mmap_min_addr looks more correct for me. Andrew?

> I don't know if it is a good idea to allow this interface to ever go
> below mmap_min_addr, but I do know that using (or even thinking about)
> dac_mmap_min_addr is wrong and you should be looking at
> security_mmap_addr() if you look at anything...

	Cyrill

Re: [PATCH] prctl: fix validation of an address

[Eric Paris]

On Mon, 2012-12-31 at 19:13 +0400, Cyrill Gorcunov wrote:
> On Mon, Dec 31, 2012 at 09:27:14AM -0500, Eric Paris wrote:
> > On Mon, 2012-12-31 at 14:14 +0400, Andrew Vagin wrote:
> > > On Sun, Dec 30, 2012 at 05:03:07PM -0500, Eric Paris wrote:
> > > > On Sat, 2012-12-29 at 15:00 +0400, Andrey Vagin wrote:
> > > > > The address should be bigger than dac_mmap_min_addr, because
> > > > > a process with CAP_RAWIO can map a vma bellow mmap_min_addr.
> > > > 
> > > > NAK
> > > 
> > > Currently prctl(PR_SET_MM_*, addr, ) returns EINVAL for valid addresses.
> > > I think it's a bug. Are you agree?
> > 
> > Can you help me understand how prctl(PR_SET_MM_*, relates to
> > checkpoint/restore?  My worry here is that somehow this interface could
> 
> Here how we use it (from userspace code)
> 
> 	ret |= sys_prctl_safe(PR_SET_MM, PR_SET_MM_START_CODE, (long)args->mm.mm_start_code, 0);
> 	ret |= sys_prctl_safe(PR_SET_MM, PR_SET_MM_END_CODE, (long)args->mm.mm_end_code, 0);
> 	...
> 
> the values of mm.mm_start_code and such are saved in image file and obtained
> during checkpoint stage. Note the prctl_set_mm requires the caller to have
> CAP_SYS_RESOURCE privilege granted.

Is there anything which prevents an unpriv application from changing
mm.mm_start_code and mm.mm_end_code in the image, thus taking advantage
of the privileged restore code to bypass the mmap_min_addr
restrictions? 

Re: [PATCH] prctl: fix validation of an address

[Cyrill Gorcunov]

On Mon, Dec 31, 2012 at 10:20:47AM -0500, Eric Paris wrote:
> 
> Is there anything which prevents an unpriv application from changing
> mm.mm_start_code and mm.mm_end_code in the image, thus taking advantage
> of the privileged restore code to bypass the mmap_min_addr
> restrictions? 

Well, you can assign some values in image directly (note the crtools
is running with root priveleges and image files have appropriate
owner:group) because image format is opened and well known from
crtools source code or from protobufer files we use to descibe the
entries in image. Thus it's assumed that administrator/root guarantee
that images are not modified after checkpoint (image signing,
checksumming and such). Also note the members being assigned in
this prctl call are not critical but rather used for statistics
output in procfs as far as I remember.

	Cyrill

Re: [PATCH] prctl: fix validation of an address

[Serge E. Hallyn]

Quoting Cyrill Gorcunov (gorcunov@openvz.org):
> > The kernel makes the decision on what is valid via security_mmap_addr().
> > Assuming there are no security fears of an untrusted application
> > tricking some priviledged application to set up these maps the answer is
> > just calling security_mmap_addr() instead of doing if(addr <
> > mmap_min_addr) return -EINVAL;
> 
> If only I've not missed something obvious, the check for security_mmap_addr() here
> instead of poking the mmap_min_addr looks more correct for me. Andrew?

That sounds right to me as well.

-serge

Re: [PATCH] prctl: fix validation of an address

[Andrey Wagin]

>
> The kernel makes the decision on what is valid via security_mmap_addr().
> Assuming there are no security fears of an untrusted application
> tricking some priviledged application to set up these maps the answer is
> just calling security_mmap_addr() instead of doing if(addr <
> mmap_min_addr) return -EINVAL;
>
> I don't know if it is a good idea to allow this interface to ever go
> below mmap_min_addr, but I do know that using (or even thinking about)
> dac_mmap_min_addr is wrong and you should be looking at
> security_mmap_addr() if you look at anything...

This sounds reasonable. Thank you for the comments. I'm going to send
a second version of this patch.

>
> -Eric
>