2010-04-21 14:00:42

by Kyle McMartin

[permalink] [raw]
Subject: [PATCH] trivial: make mmap_min_addr perms 600

Chuck points out that mmap_min_addr is 644...

CAP_SYS_RAWIO will deny users read/write to the file, let's let them see
that this is intended.

Signed-off-by: Kyle McMartin <[email protected]>
---
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 8686b0f..5868481 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1209,7 +1209,7 @@ static struct ctl_table vm_table[] = {
.procname = "mmap_min_addr",
.data = &dac_mmap_min_addr,
.maxlen = sizeof(unsigned long),
- .mode = 0644,
+ .mode = 0600,
.proc_handler = mmap_min_addr_handler,
},
#endif


2010-04-21 14:04:33

by Eric Paris

[permalink] [raw]
Subject: Re: [PATCH] trivial: make mmap_min_addr perms 600

On Wed, 2010-04-21 at 10:00 -0400, Kyle McMartin wrote:
> Chuck points out that mmap_min_addr is 644...
>
> CAP_SYS_RAWIO will deny users read/write to the file, let's let them see
> that this is intended.
>
> Signed-off-by: Kyle McMartin <[email protected]>

I'm fine with it. RAWIO was only really added to block writes as I
recall, but I don't see a good reason normal users need to see this and
blocking them with rwx perms first is a good idea.

Acked-by: Eric Paris <[email protected]>

James, do you want to pick up and push towards linus?

-Eric
> ---
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 8686b0f..5868481 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1209,7 +1209,7 @@ static struct ctl_table vm_table[] = {
> .procname = "mmap_min_addr",
> .data = &dac_mmap_min_addr,
> .maxlen = sizeof(unsigned long),
> - .mode = 0644,
> + .mode = 0600,
> .proc_handler = mmap_min_addr_handler,
> },
> #endif

2010-04-21 18:38:01

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] trivial: make mmap_min_addr perms 600

On Wed, Apr 21, 2010 at 10:00:33AM -0400, Kyle McMartin wrote:
> Chuck points out that mmap_min_addr is 644...
> CAP_SYS_RAWIO will deny users read/write to the file, let's let them see
> that this is intended.

Hmm. Denying read is actually an unintended side-effect of the
CAP_SYS_RAWIO check.

The value is useful information for an admin to see without being root
("is my system vulnerable to kernel NULL pointer attacks?") and its
setting is trivially easy for an attacker to determine by calling mmap()
in PAGE_SIZE increments starting at 0.

I would opt for greater transparency for the admin, and leave it 644 and
instead more carefully check CAP_SYS_RAWIO.

Signed-off-by: Kees Cook <[email protected]>
---
security/min_addr.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/security/min_addr.c b/security/min_addr.c
index e86f297..f728728 100644
--- a/security/min_addr.c
+++ b/security/min_addr.c
@@ -33,7 +33,7 @@ int mmap_min_addr_handler(struct ctl_table *table, int write,
{
int ret;

- if (!capable(CAP_SYS_RAWIO))
+ if (write && !capable(CAP_SYS_RAWIO))
return -EPERM;

ret = proc_doulongvec_minmax(table, write, buffer, lenp, ppos);
--
1.7.0.4


--
Kees Cook
Ubuntu Security Team

2010-04-22 15:20:57

by Kyle McMartin

[permalink] [raw]
Subject: Re: [PATCH] trivial: make mmap_min_addr perms 600

On Wed, Apr 21, 2010 at 11:29:11AM -0700, Kees Cook wrote:
> I would opt for greater transparency for the admin, and leave it 644 and
> instead more carefully check CAP_SYS_RAWIO.
>
> Signed-off-by: Kees Cook <[email protected]>

fine with me.