dmesg_restrict is guarded by #ifdef CONFIG_PRINTK in kernel.h
Its uses need to be guarded as well.
Signed-off-by: Joe Perches <[email protected]>
---
kernel/sysctl.c | 2 +-
security/commoncap.c | 2 ++
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b65bf63..5abfa15 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -702,7 +702,6 @@ static struct ctl_table kern_table[] = {
.extra1 = &zero,
.extra2 = &ten_thousand,
},
-#endif
{
.procname = "dmesg_restrict",
.data = &dmesg_restrict,
@@ -712,6 +711,7 @@ static struct ctl_table kern_table[] = {
.extra1 = &zero,
.extra2 = &one,
},
+#endif
{
.procname = "ngroups_max",
.data = &ngroups_max,
diff --git a/security/commoncap.c b/security/commoncap.c
index 04b80f9..29f2368 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -895,8 +895,10 @@ int cap_syslog(int type, bool from_file)
{
if (type != SYSLOG_ACTION_OPEN && from_file)
return 0;
+#ifdef CONFIG_PRINTK
if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
return -EPERM;
+#endif
if ((type != SYSLOG_ACTION_READ_ALL &&
type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
return -EPERM;
On Sat, Nov 13, 2010 at 9:26 AM, Joe Perches <[email protected]> wrote:
>
> dmesg_restrict is guarded by #ifdef CONFIG_PRINTK in kernel.h
> Its uses need to be guarded as well.
Fair enough, but I think this part:
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 04b80f9..29f2368 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -895,8 +895,10 @@ int cap_syslog(int type, bool from_file)
> ?{
> ? ? ? ?if (type != SYSLOG_ACTION_OPEN && from_file)
> ? ? ? ? ? ? ? ?return 0;
> +#ifdef CONFIG_PRINTK
> ? ? ? ?if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
> ? ? ? ? ? ? ? ?return -EPERM;
> +#endif
> ? ? ? ?if ((type != SYSLOG_ACTION_READ_ALL &&
> ? ? ? ? ? ? type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
> ? ? ? ? ? ? ? ?return -EPERM;
is incredibly ugly. If CONFIG_PRINTK isn't set, the whole function
just becomes pointless, so why guard just that one part of it?
So I would suggest guarding the whole thing, and just returning 0 if
CONFIG_PRINTK isn't set. Or preferably just move the dmesg_restrict
test into do_syslog, and stop playing stupid games with
"security_syslog()", which actually goes away if you disable the you
disable CONFIG_SECURITY.
SECURITY_DMESG_RESTRICT is totally independent of CONFIG_SECURITY, so
doing it in security_syslog() was a bug to begin with.
Or we should make SECURITY_DMESG_RESTRICT _depend_ on CONFIG_SECURITY,
and move it entirely into security/commoncap.c, and not pollute
kernel/printk.c at all with it.
Anyway, suggested replacement patch attached. Comments?
Linus
>
> Anyway, suggested replacement patch attached. Comments?
>
The desired behavior was to allow a reader with CAP_SYS_ADMIN to open
the syslog via /proc/kmsg and continue reading it even after dropping
capabilities, which is why it was placed where it was. I see no problem
with moving it back out to do_syslog, but ideally the same behavior
should be replicated.
-Dan
On Sat, 2010-11-13 at 09:50 -0800, Linus Torvalds wrote:
> On Sat, Nov 13, 2010 at 9:26 AM, Joe Perches <[email protected]> wrote:
> > dmesg_restrict is guarded by #ifdef CONFIG_PRINTK in kernel.h
> > Its uses need to be guarded as well.
> Fair enough, but I think this part:
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index 04b80f9..29f2368 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -895,8 +895,10 @@ int cap_syslog(int type, bool from_file)
> > {
> > if (type != SYSLOG_ACTION_OPEN && from_file)
> > return 0;
> > +#ifdef CONFIG_PRINTK
> > if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
> > return -EPERM;
> > +#endif
> > if ((type != SYSLOG_ACTION_READ_ALL &&
> > type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
> > return -EPERM;
>
> is incredibly ugly. If CONFIG_PRINTK isn't set, the whole function
> just becomes pointless, so why guard just that one part of it?
>
> So I would suggest guarding the whole thing, and just returning 0 if
> CONFIG_PRINTK isn't set. Or preferably just move the dmesg_restrict
> test into do_syslog, and stop playing stupid games with
> "security_syslog()", which actually goes away if you disable the you
> disable CONFIG_SECURITY.
>
> SECURITY_DMESG_RESTRICT is totally independent of CONFIG_SECURITY, so
> doing it in security_syslog() was a bug to begin with.
>
> Or we should make SECURITY_DMESG_RESTRICT _depend_ on CONFIG_SECURITY,
> and move it entirely into security/commoncap.c, and not pollute
> kernel/printk.c at all with it.
>
> Anyway, suggested replacement patch attached. Comments?
>
> Linus
Maybe something like this?
Make CONFIG_SECURITY_DMESG_RESTRICT depend on CONFIG_SECURITY
Remove dependency on CONFIG_PRINTK
Uncompiled/untested
---
include/linux/kernel.h | 5 ++++-
kernel/sysctl.c | 2 ++
security/Kconfig | 1 +
security/commoncap.c | 2 ++
4 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fc3da9e..b9595e8 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -130,6 +130,10 @@ extern const char linux_proc_banner[];
extern int console_printk[];
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
+extern int dmesg_restrict;
+#endif
+
#define console_loglevel (console_printk[0])
#define default_message_loglevel (console_printk[1])
#define minimum_console_loglevel (console_printk[2])
@@ -293,7 +297,6 @@ extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
unsigned int interval_msec);
extern int printk_delay_msec;
-extern int dmesg_restrict;
/*
* Print a one-time message (analogous to WARN_ONCE() et al):
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b65bf63..5d7eaab 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -703,6 +703,7 @@ static struct ctl_table kern_table[] = {
.extra2 = &ten_thousand,
},
#endif
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
{
.procname = "dmesg_restrict",
.data = &dmesg_restrict,
@@ -712,6 +713,7 @@ static struct ctl_table kern_table[] = {
.extra1 = &zero,
.extra2 = &one,
},
+#endif
{
.procname = "ngroups_max",
.data = &ngroups_max,
diff --git a/security/Kconfig b/security/Kconfig
index e80da95..c6583d6 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -41,6 +41,7 @@ config KEYS_DEBUG_PROC_KEYS
config SECURITY_DMESG_RESTRICT
bool "Restrict unprivileged access to the kernel syslog"
+ depends on SECURITY
default n
help
This enforces restrictions on unprivileged users reading the kernel
diff --git a/security/commoncap.c b/security/commoncap.c
index 04b80f9..37759b2 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -895,8 +895,10 @@ int cap_syslog(int type, bool from_file)
{
if (type != SYSLOG_ACTION_OPEN && from_file)
return 0;
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
return -EPERM;
+#endif
if ((type != SYSLOG_ACTION_READ_ALL &&
type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
return -EPERM;
Maybe something like this is better?
Remove dmesg_restrict from kernel.h printk.c
Move dmesg_restrict to security/commoncap.c
Add extern int dmesg_restrict to kernel/sysctl.c
Use CONFIG_SECURITY_DMESG_RESTRICT as variable guard
uncompiled/untested.
---
include/linux/kernel.h | 1 -
kernel/printk.c | 6 ------
kernel/sysctl.c | 6 ++++++
security/Kconfig | 1 +
security/commoncap.c | 8 ++++++++
5 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fc3da9e..b526947 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -293,7 +293,6 @@ extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
unsigned int interval_msec);
extern int printk_delay_msec;
-extern int dmesg_restrict;
/*
* Print a one-time message (analogous to WARN_ONCE() et al):
diff --git a/kernel/printk.c b/kernel/printk.c
index 38e7d58..b2ebaee 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -261,12 +261,6 @@ static inline void boot_delay_msec(void)
}
#endif
-#ifdef CONFIG_SECURITY_DMESG_RESTRICT
-int dmesg_restrict = 1;
-#else
-int dmesg_restrict;
-#endif
-
int do_syslog(int type, char __user *buf, int len, bool from_file)
{
unsigned i, j, limit, count;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b65bf63..f8ba761 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -106,6 +106,10 @@ extern int sysctl_nr_trim_pages;
#ifdef CONFIG_BLOCK
extern int blk_iopoll_enabled;
#endif
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
+extern int dmesg_restrict;
+#endif
+
/* Constants used for minimum and maximum */
#ifdef CONFIG_LOCKUP_DETECTOR
@@ -703,6 +707,7 @@ static struct ctl_table kern_table[] = {
.extra2 = &ten_thousand,
},
#endif
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
{
.procname = "dmesg_restrict",
.data = &dmesg_restrict,
@@ -712,6 +717,7 @@ static struct ctl_table kern_table[] = {
.extra1 = &zero,
.extra2 = &one,
},
+#endif
{
.procname = "ngroups_max",
.data = &ngroups_max,
diff --git a/security/Kconfig b/security/Kconfig
index e80da95..c6583d6 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -41,6 +41,7 @@ config KEYS_DEBUG_PROC_KEYS
config SECURITY_DMESG_RESTRICT
bool "Restrict unprivileged access to the kernel syslog"
+ depends on SECURITY
default n
help
This enforces restrictions on unprivileged users reading the kernel
diff --git a/security/commoncap.c b/security/commoncap.c
index 04b80f9..08066df 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -883,6 +883,12 @@ error:
return error;
}
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
+int dmesg_restrict = 1;
+#else
+int dmesg_restrict;
+#endif
+
/**
* cap_syslog - Determine whether syslog function is permitted
* @type: Function requested
@@ -895,8 +901,10 @@ int cap_syslog(int type, bool from_file)
{
if (type != SYSLOG_ACTION_OPEN && from_file)
return 0;
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
return -EPERM;
+#endif
if ((type != SYSLOG_ACTION_READ_ALL &&
type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
return -EPERM;
On Sat, Nov 13, 2010 at 10:25 AM, Dan Rosenberg
<[email protected]> wrote:
>
>>
>> Anyway, suggested replacement patch attached. Comments?
>>
>
> The desired behavior was to allow a reader with CAP_SYS_ADMIN to open
> the syslog via /proc/kmsg and continue reading it even after dropping
> capabilities, which is why it was placed where it was. ?I see no problem
> with moving it back out to do_syslog, but ideally the same behavior
> should be replicated.
Hmm. No wonder I missed that. The security interface is totally
idiotic. If the intention is for /proc/kmsg security checks to be done
at open time, then dammit, that logic should _not_ be inside some
random security policy.
So I missed the intention, because the code is written in such an odd
way. Those security hooks were obviously done as a
"search-and-replace" kind of thing, rather than trying to make sense.
I suspect "from_file" should never be passed to the security hook,
since the only point would be exactly that "do security checks of
/proc/kmsg at open time" - which I think is better done totally
independent of the security model - otherwise the security models just
inevitably just always do fundamentally different things.
Security people should be the ones to know that the way to do security
is to make it obvious, instead of having totally crazy interfaces for
hooks that make no sense. "Not making sense" is how obvious patches
then miss the point of the check.
So what happens now is that the capability-based logic thinks the
rules are about "open time", while the _other_ security rules seem to
think it's about read time (_and_ open time - they just ignore the
whole from_file).
So which one is right? Making it a case of "random security models can
implement totally random semantics" is just stupid.
So my suspicion is that the intent was to just do the check at open
time, and the confusing interface just means that selinux and others
didn't even realize what the whole intent of that "from_file" thing
was. Why not just fix that. How does this (UNTESTED!) patch look?
I've added James Morris to the recipients list. Comments?
(The diffstat says that this adds more lines than it removes, but that
is misleading: it is due to actually commenting the rule that checks
are done open-time for /proc/kmsg)
Linus
On Sat, Nov 13, 2010 at 11:51 AM, Joe Perches <[email protected]> wrote:
>
> Maybe something like this?
>
> Make CONFIG_SECURITY_DMESG_RESTRICT depend on CONFIG_SECURITY
> Remove dependency on CONFIG_PRINTK
No, this is wrong:
> +#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> +extern int dmesg_restrict;
> +#endif
CONFIG_SECURITY_DMESG_RESTRICT is supposed to be about the initial
_value_ of dmesg_restrict, not about whether it exists or not. If you
don't have CONFIG_SECURITY, you still end up defaulting to the common
capability model, and it would still want that dmesg_restrict thing.
But what can make sense is to move "dmesg_restrict" into
security/commoncap.c, and just make it about capabilities. Of course,
that then means that if you use some other security model that just
doesn't care about capabilities at all, they'll never care about
dmesg_restrict either. So that, to me, smells of really bad interface
design.
We had this exact problem with the whole "mmap_min_addr" thing. People
_thought_ of it as generic, but because it was actually tested by the
security logic, if you ended up enabling SELinux the test actually
went away entirely (or maybe it was the other way around). So with
certain security models, the whole thing was bypassed, and the
security module actually became an _IN_security module.
That's why I don't think we should do things like this inside the
security models themselves. People just get really confused about what
the real semantics are.
If something should be generic (and by all accounts, that's the
intention of 'dmesg_restrict', the same way it was for
'mmap_min_addr'). Which is why I'd change the whole idiotic
security_syslog() model itself as per the patch I just sent out.
Linus
On Sat, Nov 13, 2010 at 09:50:22AM -0800, Linus Torvalds wrote:
> So I would suggest guarding the whole thing, and just returning 0 if
> CONFIG_PRINTK isn't set. Or preferably just move the dmesg_restrict
> test into do_syslog, and stop playing stupid games with
> "security_syslog()", which actually goes away if you disable the you
> disable CONFIG_SECURITY.
>
> SECURITY_DMESG_RESTRICT is totally independent of CONFIG_SECURITY, so
> doing it in security_syslog() was a bug to begin with.
>
> Or we should make SECURITY_DMESG_RESTRICT _depend_ on CONFIG_SECURITY,
> and move it entirely into security/commoncap.c, and not pollute
> kernel/printk.c at all with it.
It seems saner to put it in commoncaps to me, but either way, it must happen
after the from_file and OPEN test or it will break /proc/kmesg readers who
open and drop privs (as done with sysklogd, etc).
> + if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
>...
> {
> if (type != SYSLOG_ACTION_OPEN && from_file)
> return 0;
> - if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
> - return -EPERM;
i.e.
if (dmesg_restrict && (!from_file || type == SYSLOG_ACTION_OPEN) &&
!capable(CAP_SYS_ADMIN))
-Kees
--
Kees Cook
Ubuntu Security Team
On Sat, Nov 13, 2010 at 12:22:15PM -0800, Linus Torvalds wrote:
> Hmm. No wonder I missed that. The security interface is totally
> idiotic. If the intention is for /proc/kmsg security checks to be done
> at open time, then dammit, that logic should _not_ be inside some
> random security policy.
I think the real problem is that this interface exists as both a syscall
and a /proc file (sysklogd things use the /proc file). Dropping the
from_file means that security policy cannot revalidate the policy
(sometimes it might want to block the read, i.e. passing the open fd to
another process that is not privileged). But since nothing is actually
using from_file yet, I guess it's not a big deal.
And note that I'm not defending any specific part of it; I'm just trying
to point out what not be possible in the future if we drop from_file
like this.
-Kees
--
Kees Cook
Ubuntu Security Team
* Linus Torvalds <[email protected]> wrote:
> (The diffstat says that this adds more lines than it removes, but that
> is misleading: it is due to actually commenting the rule that checks
> are done open-time for /proc/kmsg)
>
> Linus
>
> include/linux/security.h | 11 +++++------
> kernel/printk.c | 15 ++++++++++++---
> kernel/sysctl.c | 2 +-
> security/commoncap.c | 7 +------
> security/security.c | 4 ++--
> security/selinux/hooks.c | 4 ++--
> security/smack/smack_lsm.c | 4 ++--
> 7 files changed, 25 insertions(+), 22 deletions(-)
I gave it some testing in -tip and your patch also solves the !EMBEDDED build bug
(and it also still restricts unprivileged dmesg output when desired), so:
Acked-by: Ingo Molnar <[email protected]>
Tested-by: Ingo Molnar <[email protected]>
Thanks,
Ingo
On Sat, 13 Nov 2010, Linus Torvalds wrote:
[Adding the LSM list]
> CONFIG_SECURITY_DMESG_RESTRICT is supposed to be about the initial
> _value_ of dmesg_restrict, not about whether it exists or not. If you
> don't have CONFIG_SECURITY, you still end up defaulting to the common
> capability model, and it would still want that dmesg_restrict thing.
>
> But what can make sense is to move "dmesg_restrict" into
> security/commoncap.c, and just make it about capabilities. Of course,
> that then means that if you use some other security model that just
> doesn't care about capabilities at all, they'll never care about
> dmesg_restrict either. So that, to me, smells of really bad interface
> design.
Yes, it should not be possible for an LSM to reduce the default security
-- an interface which allows this breaks the security model.
> We had this exact problem with the whole "mmap_min_addr" thing. People
> _thought_ of it as generic, but because it was actually tested by the
> security logic, if you ended up enabling SELinux the test actually
> went away entirely (or maybe it was the other way around). So with
> certain security models, the whole thing was bypassed, and the
> security module actually became an _IN_security module.
>
> That's why I don't think we should do things like this inside the
> security models themselves. People just get really confused about what
> the real semantics are.
>
> If something should be generic (and by all accounts, that's the
> intention of 'dmesg_restrict', the same way it was for
> 'mmap_min_addr'). Which is why I'd change the whole idiotic
> security_syslog() model itself as per the patch I just sent out.
Looks like the right approach to me.
Kees, does this patch work for you?
I want to ensure that LSMs which implement security_syslog can't end up
with a less secure system than the default, regardless of whether they
call cap_syslog or not.
- James
--
James Morris
<[email protected]>
On Sat, Nov 13, 2010 at 3:31 PM, Linus Torvalds
<[email protected]> wrote:
> We had this exact problem with the whole "mmap_min_addr" thing. People
> _thought_ of it as generic, but because it was actually tested by the
> security logic, if you ended up enabling SELinux the test actually
> went away entirely (or maybe it was the other way around). So with
> certain security models, the whole thing was bypassed, and the
> security module actually became an _IN_security module.
Your recollection is wrong, although your conclusions of the
ramifications are right. Either SELinux or capabilities checked
mmap_min_addr, depending on which was 'primary.' Just as they are
different modules they checked for different things. Only doing
SELinux checks was stronger for some situations, and only doing
capability checks was stronger in some ways (and the reverse was
obviously true). Today you get the best of both worlds since we
really have 2 different mmap_min_addr values...
In any case the result of that is that LSMs (ok 'I') need to be more
careful making sure they interact properly with the generic
capabilities hooks.
From: James Morris
> I want to ensure that LSMs which implement security_syslog can't end up
> with a less secure system than the default, regardless of whether they
> call cap_syslog or not.
Which really means that this is total crap. If you don't call
cap_syslog() you broke it. That's all there is to it. Calling the
capability code is always required. full stop.
I think this patch is broken though. SELinux and SMACK don't care
about from_file and want to check every time no matter what. Your
patch breaks that and only will call the LSM on occasion. It's only
capabilities that likes those semantics. I think the entire contents
of the cap_syslog hook should be moved up and that hook just dropped
entirely.
I'll code up what I'm thinking in a minute.....
-Eric
On Mon, Nov 15, 2010 at 12:04 PM, Eric Paris <[email protected]> wrote:
> On Sat, Nov 13, 2010 at 3:31 PM, Linus Torvalds
> <[email protected]> wrote:
>> We had this exact problem with the whole "mmap_min_addr" thing. People
>> _thought_ of it as generic, but because it was actually tested by the
>> security logic, if you ended up enabling SELinux the test actually
>> went away entirely (or maybe it was the other way around). So with
>> certain security models, the whole thing was bypassed, and the
>> security module actually became an _IN_security module.
>
> Your recollection is wrong, although your conclusions of the
> ramifications are right. ?Either SELinux or capabilities checked
> mmap_min_addr, depending on which was 'primary.' ?Just as they are
> different modules they checked for different things. ?Only doing
> SELinux checks was stronger for some situations, and only doing
> capability checks was stronger in some ways (and the reverse was
> obviously true). ?Today you get the best of both worlds since we
> really have 2 different mmap_min_addr values...
>
> In any case the result of that is that LSMs (ok 'I') need to be more
> careful making sure they interact properly with the generic
> capabilities hooks.
>
> From: James Morris
>> I want to ensure that LSMs which implement security_syslog can't end up
>> with a less secure system than the default, regardless of whether they
>> call cap_syslog or not.
>
> Which really means that this is total crap. ?If you don't call
> cap_syslog() you broke it. ?That's all there is to it. ?Calling the
> capability code is always required. ?full stop.
>
> I think this patch is broken though. ?SELinux and SMACK don't care
> about from_file and want to check every time no matter what. ?Your
> patch breaks that and only will call the LSM on occasion. ?It's only
> capabilities that likes those semantics. ?I think the entire contents
> of the cap_syslog hook should be moved up and that hook just dropped
> entirely.
>
> I'll code up what I'm thinking in a minute.....
>
> -Eric
I'm sure somebody somewhere hates it, but I was thinking something
like the attached.
include/linux/security.h | 9 ++++-----
kernel/printk.c | 11 ++++++++++-
security/capability.c | 5 +++++
security/commoncap.c | 21 ---------------------
security/security.c | 4 ++--
security/selinux/hooks.c | 6 +-----
security/smack/smack_lsm.c | 8 ++------
7 files changed, 24 insertions(+), 40 deletions(-)
(Personally I think that most of the hooks in commoncap.c code should
be moved out of security/ altogether and we should completely do away
with our current ghetto inter LSM calls. But that's just me)
-Eric
On Mon, Nov 15, 2010 at 12:41 PM, Linus Torvalds
<[email protected]> wrote:
> If the old rule should have been that you _have_
> to call cap_syslog(), then just eviscerating that entirely and putting
> it in the generic code is definitely the right thing.
That is the rule for ALL of the hooks in commoncap.c. The one time I
tried to do something else *cough*mmap_min_addr*cough* I screwed it
up. I'll put a note in my todo list about looking into lifting all of
commoncap.c into the callers.
-Eric
On Mon, Nov 15, 2010 at 9:34 AM, Eric Paris <[email protected]> wrote:
>
> I'm sure somebody somewhere hates it, but I was thinking something
> like the attached.
I certainly like it. If the old rule should have been that you _have_
to call cap_syslog(), then just eviscerating that entirely and putting
it in the generic code is definitely the right thing.
Anyway, I wanted to do -rc2 yesterday, but I don't really want this
kernel build problem to remain (even if it's not a very relevant
config for most people).
So if people can quickly agree on this, I'll take it and do -rc2 later
today, otherwise I'll do Joe's trivial patch as a stop-gap measure
pending approval for cleanup/fixing the security interfaces.
Linus
On Mon, Nov 15, 2010 at 9:45 AM, Eric Paris <[email protected]> wrote:
>
> That is the rule for ALL of the hooks in commoncap.c. ?The one time I
> tried to do something else *cough*mmap_min_addr*cough* I screwed it
> up. ?I'll put a note in my todo list about looking into lifting all of
> commoncap.c into the callers.
Into "security/security.c" itself? That would work, except it doesn't
work exactly in a situation like this where the whole interface was
polluted by the commoncap version simply having fundamentally
different semantics (ie the whole "no security check at read time,
only at open time"). Passing the whole "from_file" thing around was
just ugly.
And while passing the commoncap cases down into the callers of the
"security_xyz()" interface itself makes sense in this case, I don't
think it makes sense in general. With 'security_syslog()' there really
was just one very specific call-site. Other security wrappers have
many more (eg "security_vm_enough_memory()") call sites, and moving
the cap_xyz() code to those callsites would be totally wrong
duplication.
Linus
On Mon, 15 Nov 2010, Eric Paris wrote:
> On Mon, Nov 15, 2010 at 12:41 PM, Linus Torvalds
> <[email protected]> wrote:
> > If the old rule should have been that you _have_
> > to call cap_syslog(), then just eviscerating that entirely and putting
> > it in the generic code is definitely the right thing.
>
> That is the rule for ALL of the hooks in commoncap.c. The one time I
> tried to do something else *cough*mmap_min_addr*cough* I screwed it
> up. I'll put a note in my todo list about looking into lifting all of
> commoncap.c into the callers.
If it's a requirement of the API that all of the cap calls are made
first, then build it into the API, so developers can't make a mistake.
e.g. have the LSM API do the secondary stacking of caps behind the scenes.
I had thought that the idea was that some LSM may want to not implement
capabilities at all, on which case, it should still not be possible for
the API to weaken the default security with or without caps. In any case,
mixing generic logic with capabilities logic seems to be a fundamental
issue, and one which we should avoid, and remove where it may exist (I did
audit the hooks after the mmap_min_addr thing, but it's worth checking
again).
- James
--
James Morris
<[email protected]>
On Mon, 15 Nov 2010, Linus Torvalds wrote:
> On Mon, Nov 15, 2010 at 9:34 AM, Eric Paris <[email protected]> wrote:
> >
> > I'm sure somebody somewhere hates it, but I was thinking something
> > like the attached.
>
> I certainly like it. If the old rule should have been that you _have_
> to call cap_syslog(), then just eviscerating that entirely and putting
> it in the generic code is definitely the right thing.
>
> Anyway, I wanted to do -rc2 yesterday, but I don't really want this
> kernel build problem to remain (even if it's not a very relevant
> config for most people).
>
> So if people can quickly agree on this, I'll take it and do -rc2 later
> today, otherwise I'll do Joe's trivial patch as a stop-gap measure
> pending approval for cleanup/fixing the security interfaces.
Looks good to me:
Acked-by: James Morris <[email protected]>
(I'd really like to see new security features get fully reviewed on the
LSM list and bake in -next for a while in the future, no matter how
obviously correct they seem).
--
James Morris
<[email protected]>
On Mon, Nov 15, 2010 at 2:16 PM, James Morris <[email protected]> wrote:
>
> Acked-by: James Morris <[email protected]>
Ok, now we need sign-off and changelog from Eric... Ping?
> (I'd really like to see new security features get fully reviewed on the
> LSM list and bake in -next for a while in the future, no matter how
> obviously correct they seem).
Hey, I can still just do the trivial "just fix the build" part without
actually cleaning up the security layer for -rc2. Just let me know
which one you'd prefer.
Linus
On Tue, 2010-11-16 at 09:13 +1100, James Morris wrote:
> On Mon, 15 Nov 2010, Eric Paris wrote:
>
> > On Mon, Nov 15, 2010 at 12:41 PM, Linus Torvalds
> > <[email protected]> wrote:
> > > If the old rule should have been that you _have_
> > > to call cap_syslog(), then just eviscerating that entirely and putting
> > > it in the generic code is definitely the right thing.
> >
> > That is the rule for ALL of the hooks in commoncap.c. The one time I
> > tried to do something else *cough*mmap_min_addr*cough* I screwed it
> > up. I'll put a note in my todo list about looking into lifting all of
> > commoncap.c into the callers.
>
> If it's a requirement of the API that all of the cap calls are made
> first, then build it into the API, so developers can't make a mistake.
> e.g. have the LSM API do the secondary stacking of caps behind the scenes.
At this point it's a defacto requirement since noone is doing anything
like that. My mmap_min_addr screw up is, to the best of my knowledge,
the only time anyone has intentionally not called the caps code...
And I sorta like the idea of moving the cap_* calls directly into
security_*. Great, another item on the todo list. Lift as many cap
calls into the caller as is reasonable (I don't think there are
many/any) and if not possible lift them directory into security_*. If
someone else really wants to make a system truely without capabilities
lets look at there solution then....
> I had thought that the idea was that some LSM may want to not implement
> capabilities at all, on which case, it should still not be possible for
> the API to weaken the default security with or without caps.
Not sure how that's possible. I mean, I guess it's possible if the
fabled LSM reimplements the cap call, but I'm not sure how you can
remove a restrictive only security check without 'weakening' the system
in some way.
On Mon, 15 Nov 2010, Linus Torvalds wrote:
> On Mon, Nov 15, 2010 at 2:16 PM, James Morris <[email protected]> wrote:
> >
> > Acked-by: James Morris <[email protected]>
>
> Ok, now we need sign-off and changelog from Eric... Ping?
>
> > (I'd really like to see new security features get fully reviewed on the
> > LSM list and bake in -next for a while in the future, no matter how
> > obviously correct they seem).
>
> Hey, I can still just do the trivial "just fix the build" part without
> actually cleaning up the security layer for -rc2. Just let me know
> which one you'd prefer.
I'd prefer to get the right code in (Eric's).
- James
--
James Morris
<[email protected]>
On Mon, 15 Nov 2010, Eric Paris wrote:
> Not sure how that's possible. I mean, I guess it's possible if the
> fabled LSM reimplements the cap call, but I'm not sure how you can
> remove a restrictive only security check without 'weakening' the system
> in some way.
If generic security logic is mixed into a capability call, then not
implementing the cap call also loses the generic security logic.
e.g. dmesg_restrict should always be verified, regardless of whether
cap_security() is called or not.
If cap_syslog() should always be called, then it should not be possible
not not call it :-)
- James
--
James Morris
<[email protected]>
On Tue, 2010-11-16 at 09:58 +1100, James Morris wrote:
> On Mon, 15 Nov 2010, Eric Paris wrote:
>
> > Not sure how that's possible. I mean, I guess it's possible if the
> > fabled LSM reimplements the cap call, but I'm not sure how you can
> > remove a restrictive only security check without 'weakening' the system
> > in some way.
>
> If generic security logic is mixed into a capability call, then not
> implementing the cap call also loses the generic security logic.
I guess it comes down to what you define 'generic security logic.'
We've come to expect that capabilities are an indispensable mechanism
for control object access. The prevalence of if (!capable(***))
throughout the kernel proves that fact. I think that sometimes open
coding how we expect to use capabilities and sometimes hiding it behind
an LSM hook is just bad news. I'd prefer all open coding, but that
might not be the best in all situations. Hopefully I'll get a chance to
try to clean that up a little.
In any case, right now I need to go write a patch description since I
just compile tested it a couple of ways....
-Eric