security_ops was declared as a global variable, so other drivers or
kernel code can easily change its value, like:
extern struct security_operations *security_ops;
security_ops = NULL;
then insmod this driver immediately, it will get an oops. Other evil
drivers can aslo fake this variable as extern.
Signed-off-by: wzt <[email protected]>
---
security/security.c | 25 ++++++++++++++++++++++++-
security/selinux/hooks.c | 18 ++++++------------
2 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/security/security.c b/security/security.c
index 24e060b..781117d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -26,7 +26,12 @@ static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
extern struct security_operations default_security_ops;
extern void security_fixup_ops(struct security_operations *ops);
-struct security_operations *security_ops; /* Initialized to NULL */
+static struct security_operations *security_ops; /* Initialized
to NULL */
+/*
+ * Minimal support for a secondary security module,
+ * just to allow the use of the capability module.
+ */
+static struct security_operations *secondary_ops;
static inline int verify(struct security_operations *ops)
{
@@ -63,6 +68,24 @@ int __init security_init(void)
return 0;
}
+void reset_secondary_ops(void)
+{
+ secondary_ops = security_ops;
+ if (!secondary_ops)
+ panic("SELinux: No initial security operations\n");
+}
+
+void reset_security_ops(void)
+{
+ /* Reset security_ops to the secondary module, dummy or capability. */
+ security_ops = secondary_ops;
+}
/* Save user chosen LSM */
static int __init choose_lsm(char *str)
{
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9a2ee84..9e8607e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -92,7 +92,9 @@
#define NUM_SEL_MNT_OPTS 5
extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
-extern struct security_operations *security_ops;
+extern void reset_secondary_ops(void);
+extern void reset_security_ops(void);
/* SECMARK reference count */
atomic_t selinux_secmark_refcount = ATOMIC_INIT(0);
@@ -126,12 +128,6 @@ int selinux_enabled = 1;
#endif
-/*
- * Minimal support for a secondary security module,
- * just to allow the use of the capability module.
- */
-static struct security_operations *secondary_ops;
-
/* Lists of inode and superblock security structures initialized
before the policy was loaded. */
static LIST_HEAD(superblock_security_head);
@@ -5672,9 +5668,8 @@ static __init int selinux_init(void)
0, SLAB_PANIC, NULL);
avc_init();
- secondary_ops = security_ops;
- if (!secondary_ops)
- panic("SELinux: No initial security operations\n");
+ reset_secondary_ops();
+
if (register_security(&selinux_ops))
panic("SELinux: Unable to register with kernel.\n");
@@ -5835,8 +5830,7 @@ int selinux_disable(void)
selinux_disabled = 1;
selinux_enabled = 0;
- /* Reset security_ops to the secondary module, dummy or capability. */
- security_ops = secondary_ops;
+ reset_security_ops();
/* Try to destroy the avc node cache */
avc_disable();
--
1.6.6.1
On Sun, Feb 07, 2010 at 07:24:44PM +0800, wzt wzt wrote:
> security_ops was declared as a global variable, so other drivers or
> kernel code can easily change its value, like:
>
> extern struct security_operations *security_ops;
> security_ops = NULL;
>
> then insmod this driver immediately, it will get an oops. Other evil
> drivers can aslo fake this variable as extern.
Evil drivers can do lots of things, if you can load a kernel module on
the system, all bets are off. Just making this a private variable does
not mean much.
What external module are you trying to keep from using this variable?
thanks,
greg k-h
On Sun, 2010-02-07 at 19:24 +0800, wzt wzt wrote:
> security_ops was declared as a global variable, so other drivers or
> kernel code can easily change its value, like:
>
> extern struct security_operations *security_ops;
> security_ops = NULL;
>
> then insmod this driver immediately, it will get an oops. Other evil
> drivers can aslo fake this variable as extern.
I'd support a patch along these lines (but with the changes below) for a
different reason: at present, SELinux directly manipulates security_ops
for the purpose of runtime disable support, whereas that ought to be
handled by the security framework.
>
> Signed-off-by: wzt <[email protected]>
> ---
> security/security.c | 25 ++++++++++++++++++++++++-
> security/selinux/hooks.c | 18 ++++++------------
> 2 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 24e060b..781117d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -26,7 +26,12 @@ static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
> extern struct security_operations default_security_ops;
> extern void security_fixup_ops(struct security_operations *ops);
>
> -struct security_operations *security_ops; /* Initialized to NULL */
> +static struct security_operations *security_ops; /* Initialized
> to NULL */
> +/*
> + * Minimal support for a secondary security module,
> + * just to allow the use of the capability module.
> + */
The comment is no longer accurate - secondary_ops was originally used by
SELinux to call the "secondary" security module (capability or dummy),
but that was replaced by direct calls to capability and the only
remaining use is to save and restore the original security ops pointer
value if SELinux is disabled by early userspace based
on /etc/selinux/config. Further, if we support this directly in the
security framework, then we can just use &default_security_ops for this
purpose since that is now available. So I don't believe we need
secondary_ops at all.
> +static struct security_operations *secondary_ops;
We don't need the above variable at all.
> static inline int verify(struct security_operations *ops)
> {
> @@ -63,6 +68,24 @@ int __init security_init(void)
> return 0;
> }
>
> +void reset_secondary_ops(void)
> +{
> + secondary_ops = security_ops;
> + if (!secondary_ops)
> + panic("SELinux: No initial security operations\n");
> +}
We don't need the above function at all.
> +
> +void reset_security_ops(void)
> +{
> + /* Reset security_ops to the secondary module, dummy or capability. */
The dummy module was removed so this can only be capability.
> + security_ops = secondary_ops;
This can just be:
security_ops = &default_security_ops;
> +}
>
> /* Save user chosen LSM */
> static int __init choose_lsm(char *str)
> {
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9a2ee84..9e8607e 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -92,7 +92,9 @@
> #define NUM_SEL_MNT_OPTS 5
>
> extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
> -extern struct security_operations *security_ops;
> +extern void reset_secondary_ops(void);
> +extern void reset_security_ops(void);
The extern declaration for reset_security_ops() would properly go in
include/linux/security.h for general use by security modules.
> /* SECMARK reference count */
> atomic_t selinux_secmark_refcount = ATOMIC_INIT(0);
> @@ -126,12 +128,6 @@ int selinux_enabled = 1;
> #endif
>
>
> -/*
> - * Minimal support for a secondary security module,
> - * just to allow the use of the capability module.
> - */
> -static struct security_operations *secondary_ops;
> -
> /* Lists of inode and superblock security structures initialized
> before the policy was loaded. */
> static LIST_HEAD(superblock_security_head);
> @@ -5672,9 +5668,8 @@ static __init int selinux_init(void)
> 0, SLAB_PANIC, NULL);
> avc_init();
>
> - secondary_ops = security_ops;
> - if (!secondary_ops)
> - panic("SELinux: No initial security operations\n");
> + reset_secondary_ops();
> +
We don't need to save this value as it is available via
&default_security_ops and there is now only one possible value (dummy
module killed).
> if (register_security(&selinux_ops))
> panic("SELinux: Unable to register with kernel.\n");
>
> @@ -5835,8 +5830,7 @@ int selinux_disable(void)
> selinux_disabled = 1;
> selinux_enabled = 0;
>
> - /* Reset security_ops to the secondary module, dummy or capability. */
> - security_ops = secondary_ops;
> + reset_security_ops();
So this is the only change needed here.
>
> /* Try to destroy the avc node cache */
> avc_disable();
--
Stephen Smalley
National Security Agency
I rewrite the patch, thx.
---
include/linux/security.h | 2 ++
security/security.c | 7 ++++++-
security/selinux/hooks.c | 14 ++------------
3 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 2c627d3..3a15b57 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -95,6 +95,8 @@ struct seq_file;
extern int cap_netlink_send(struct sock *sk, struct sk_buff *skb);
extern int cap_netlink_recv(struct sk_buff *skb, int cap);
+extern void reset_security_ops(void);
+
#ifdef CONFIG_MMU
extern unsigned long mmap_min_addr;
extern unsigned long dac_mmap_min_addr;
diff --git a/security/security.c b/security/security.c
index 122b748..3e4c4bc 100644
--- a/security/security.c
+++ b/security/security.c
@@ -26,7 +26,7 @@ static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
extern struct security_operations default_security_ops;
extern void security_fixup_ops(struct security_operations *ops);
-struct security_operations *security_ops; /* Initialized to NULL */
+static struct security_operations *security_ops; /* Initialized
to NULL */
static inline int verify(struct security_operations *ops)
{
@@ -63,6 +63,11 @@ int __init security_init(void)
return 0;
}
+void reset_security_ops(void)
+{
+ security_ops = &default_security_ops;
+}
+
/* Save user chosen LSM */
static int __init choose_lsm(char *str)
{
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9a2ee84..e9599fd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -93,6 +93,7 @@
extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
extern struct security_operations *security_ops;
+extern struct security_operations default_security_ops;
/* SECMARK reference count */
atomic_t selinux_secmark_refcount = ATOMIC_INIT(0);
@@ -125,13 +126,6 @@ __setup("selinux=", selinux_enabled_setup);
int selinux_enabled = 1;
#endif
-
-/*
- * Minimal support for a secondary security module,
- * just to allow the use of the capability module.
- */
-static struct security_operations *secondary_ops;
-
/* Lists of inode and superblock security structures initialized
before the policy was loaded. */
static LIST_HEAD(superblock_security_head);
@@ -5672,9 +5666,6 @@ static __init int selinux_init(void)
0, SLAB_PANIC, NULL);
avc_init();
- secondary_ops = security_ops;
- if (!secondary_ops)
- panic("SELinux: No initial security operations\n");
if (register_security(&selinux_ops))
panic("SELinux: Unable to register with kernel.\n");
@@ -5835,8 +5826,7 @@ int selinux_disable(void)
selinux_disabled = 1;
selinux_enabled = 0;
- /* Reset security_ops to the secondary module, dummy or capability. */
- security_ops = secondary_ops;
+ reset_security_ops();
/* Try to destroy the avc node cache */
avc_disable();
--
1.6.5.3
On Tue, Feb 16, 2010 at 10:57 PM, Stephen Smalley <[email protected]> wrote:
> On Sun, 2010-02-07 at 19:24 +0800, wzt wzt wrote:
>> security_ops was declared as a global variable, so other drivers or
>> kernel code can easily change its value, like:
>>
>> extern struct security_operations *security_ops;
>> security_ops = NULL;
>>
>> then insmod this driver immediately, it will get an oops. Other evil
>> drivers can aslo fake this variable as extern.
>
> I'd support a patch along these lines (but with the changes below) for a
> different reason: at present, SELinux directly manipulates security_ops
> for the purpose of runtime disable support, whereas that ought to be
> handled by the security framework.
>
>>
>> Signed-off-by: wzt <[email protected]>
>> ---
>> security/security.c | 25 ++++++++++++++++++++++++-
>> security/selinux/hooks.c | 18 ++++++------------
>> 2 files changed, 30 insertions(+), 13 deletions(-)
>>
>> diff --git a/security/security.c b/security/security.c
>> index 24e060b..781117d 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -26,7 +26,12 @@ static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
>> extern struct security_operations default_security_ops;
>> extern void security_fixup_ops(struct security_operations *ops);
>>
>> -struct security_operations *security_ops; /* Initialized to NULL */
>> +static struct security_operations *security_ops; /* Initialized
>> to NULL */
>> +/*
>> + * Minimal support for a secondary security module,
>> + * just to allow the use of the capability module.
>> + */
>
> The comment is no longer accurate - secondary_ops was originally used by
> SELinux to call the "secondary" security module (capability or dummy),
> but that was replaced by direct calls to capability and the only
> remaining use is to save and restore the original security ops pointer
> value if SELinux is disabled by early userspace based
> on /etc/selinux/config. Further, if we support this directly in the
> security framework, then we can just use &default_security_ops for this
> purpose since that is now available. So I don't believe we need
> secondary_ops at all.
>
>> +static struct security_operations *secondary_ops;
>
> We don't need the above variable at all.
>
>> static inline int verify(struct security_operations *ops)
>> {
>> @@ -63,6 +68,24 @@ int __init security_init(void)
>> return 0;
>> }
>>
>> +void reset_secondary_ops(void)
>> +{
>> + secondary_ops = security_ops;
>> + if (!secondary_ops)
>> + panic("SELinux: No initial security operations\n");
>> +}
>
> We don't need the above function at all.
>
>> +
>> +void reset_security_ops(void)
>> +{
>> + /* Reset security_ops to the secondary module, dummy or capability. */
>
> The dummy module was removed so this can only be capability.
>
>> + security_ops = secondary_ops;
>
> This can just be:
> security_ops = &default_security_ops;
>
>> +}
>>
>> /* Save user chosen LSM */
>> static int __init choose_lsm(char *str)
>> {
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 9a2ee84..9e8607e 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -92,7 +92,9 @@
>> #define NUM_SEL_MNT_OPTS 5
>>
>> extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
>> -extern struct security_operations *security_ops;
>> +extern void reset_secondary_ops(void);
>> +extern void reset_security_ops(void);
>
> The extern declaration for reset_security_ops() would properly go in
> include/linux/security.h for general use by security modules.
>
>> /* SECMARK reference count */
>> atomic_t selinux_secmark_refcount = ATOMIC_INIT(0);
>> @@ -126,12 +128,6 @@ int selinux_enabled = 1;
>> #endif
>>
>>
>> -/*
>> - * Minimal support for a secondary security module,
>> - * just to allow the use of the capability module.
>> - */
>> -static struct security_operations *secondary_ops;
>> -
>> /* Lists of inode and superblock security structures initialized
>> before the policy was loaded. */
>> static LIST_HEAD(superblock_security_head);
>> @@ -5672,9 +5668,8 @@ static __init int selinux_init(void)
>> 0, SLAB_PANIC, NULL);
>> avc_init();
>>
>> - secondary_ops = security_ops;
>> - if (!secondary_ops)
>> - panic("SELinux: No initial security operations\n");
>> + reset_secondary_ops();
>> +
>
> We don't need to save this value as it is available via
> &default_security_ops and there is now only one possible value (dummy
> module killed).
>
>> if (register_security(&selinux_ops))
>> panic("SELinux: Unable to register with kernel.\n");
>>
>> @@ -5835,8 +5830,7 @@ int selinux_disable(void)
>> selinux_disabled = 1;
>> selinux_enabled = 0;
>>
>> - /* Reset security_ops to the secondary module, dummy or capability. */
>> - security_ops = secondary_ops;
>> + reset_security_ops();
>
> So this is the only change needed here.
>
>>
>> /* Try to destroy the avc node cache */
>> avc_disable();
> --
> Stephen Smalley
> National Security Agency
>
>
On Sun, Feb 7, 2010 at 1:24 PM, wzt wzt <[email protected]> wrote:
> security_ops was declared as a global variable, so other drivers or
> kernel code can easily change its value, like:
With your patch they still can.
> extern struct security_operations *security_ops;
> security_ops = NULL;
>
> then insmod this driver immediately, it will get an oops. ?Other evil
> drivers can aslo fake this variable as extern.
Maybe, but The attackers will use a complicated way to find the
security_ops address, it's a barrier to attackers. LSM is security
framework, we don't want the attackers can easily to break it. Just
like the sys_call_table variable in kernel 2.4.x(global and
writeable), evil drivers can extern the variable, then replace the
Sys_X functions.
On Fri, Feb 19, 2010 at 7:44 PM, Alexey Dobriyan <[email protected]> wrote:
> On Sun, Feb 7, 2010 at 1:24 PM, wzt wzt <[email protected]> wrote:
>> security_ops was declared as a global variable, so other drivers or
>> kernel code can easily change its value, like:
>
> With your patch they still can.
>
>> extern struct security_operations *security_ops;
>> security_ops = NULL;
>>
>> then insmod this driver immediately, it will get an oops. Other evil
>> drivers can aslo fake this variable as extern.
>
On Fri, Feb 19, 2010 at 1:57 PM, wzt wzt <[email protected]> wrote:
> Maybe, but The attackers will use a complicated way to find the
> security_ops address, it's a barrier to attackers.
It's not a barrier, it's garbage. Once you know the adress security_ops
ended up at, you simply write to it.
> LSM is security framework, ?we don't want the attackers can easily
> to break it.
LSM doesn't protect kernel from kernel.
> Just like the sys_call_table variable in kernel 2.4.x(global and
> writeable), evil drivers can extern the variable, ?then replace the
> Sys_X functions.
Not that easily, but they still can.
> It's not a barrier, it's garbage. Once you know the adress security_ops
> ended up at, you simply write to it.
How to find the security_ops address if the variable is static? Would
you please make an example?
> Not that easily, but they still can.
That's why i suggest to make the variable to static, if you had wrote
a rootkit, you will find that in kernel 2.4.x, there are many many
rootkits, but in kernel 2.6.x, rootkit became fewer. Not all the
kernel driver writers can master this method to find the variable's
address.
The patch also delete the secondary_ops variable.
On Fri, Feb 19, 2010 at 8:02 PM, Alexey Dobriyan <[email protected]> wrote:
> On Fri, Feb 19, 2010 at 1:57 PM, wzt wzt <[email protected]> wrote:
>> Maybe, but The attackers will use a complicated way to find the
>> security_ops address, it's a barrier to attackers.
>
> It's not a barrier, it's garbage. Once you know the adress security_ops
> ended up at, you simply write to it.
>
>> LSM is security framework, we don't want the attackers can easily
>> to break it.
>
> LSM doesn't protect kernel from kernel.
>
>> Just like the sys_call_table variable in kernel 2.4.x(global and
>> writeable), evil drivers can extern the variable, then replace the
>> Sys_X functions.
>
> Not that easily, but they still can.
>
On Fri, Feb 19, 2010 at 2:23 PM, wzt wzt <[email protected]> wrote:
>> It's not a barrier, it's garbage. Once you know the adress security_ops
>> ended up at, you simply write to it.
>
> How to find the security_ops address if the variable is static? Would
> you please make an example?
See /proc/kallsyms .
>> Not that easily, but they still can.
> That's why i suggest to make the variable to static, if you had wrote
> a rootkit, you will find that in kernel 2.4.x, there are many many
> rootkits, but in kernel 2.6.x, rootkit became fewer. Not all the
> kernel driver writers can master this method to find the variable's
> address.
Please.
> The patch also delete the secondary_ops variable.
On Fre, 2010-02-19 at 20:23 +0800, wzt wzt wrote:
> > It's not a barrier, it's garbage. Once you know the adress security_ops
> > ended up at, you simply write to it.
>
> How to find the security_ops address if the variable is static? Would
> you please make an example?
- Find the accessor function (which is surely non-static).
- Find the address where writes the parameter.
Other must decide, if it's more secure (as in "obscure") with the
accessor funtion or not.
Bernd
--
Bernd Petrovitsch Email : [email protected]
LUGA : http://www.luga.at
security_ops is not static, so you can find the address with kallsysm,
but you can try secondary_ops:
static struct security_operations *secondary_ops = NULL;
cat /proc/kallsysm|grep secondary_ops
On Fri, Feb 19, 2010 at 8:27 PM, Alexey Dobriyan <[email protected]> wrote:
> On Fri, Feb 19, 2010 at 2:23 PM, wzt wzt <[email protected]> wrote:
>>> It's not a barrier, it's garbage. Once you know the adress security_ops
>>> ended up at, you simply write to it.
>>
>> How to find the security_ops address if the variable is static? Would
>> you please make an example?
>
> See /proc/kallsyms .
>
>>> Not that easily, but they still can.
>> That's why i suggest to make the variable to static, if you had wrote
>> a rootkit, you will find that in kernel 2.4.x, there are many many
>> rootkits, but in kernel 2.6.x, rootkit became fewer. Not all the
>> kernel driver writers can master this method to find the variable's
>> address.
>
> Please.
>
>> The patch also delete the secondary_ops variable.
>
On Fri, 2010-02-19 at 19:40 +0800, wzt wzt wrote:
> I rewrite the patch, thx.
Patch description needs to be descriptive, e.g.:
"Enhance the security framework to support resetting the active security
module. This eliminates the need for direct use of the security_ops
variable outside of security.c, so make security_ops static."
Subject line could be more descriptive too, and likely just use
security: as the prefix.
You also need a Signed-off-by line, with a real name.
Also, see the comments below.
> ---
> include/linux/security.h | 2 ++
> security/security.c | 7 ++++++-
> security/selinux/hooks.c | 14 ++------------
> 3 files changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 122b748..3e4c4bc 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -26,7 +26,7 @@ static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
> extern struct security_operations default_security_ops;
> extern void security_fixup_ops(struct security_operations *ops);
>
> -struct security_operations *security_ops; /* Initialized to NULL */
> +static struct security_operations *security_ops; /* Initialized
> to NULL */
You can drop the /* Initialized to NULL */ comment since it is now
static.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9a2ee84..e9599fd 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -93,6 +93,7 @@
>
> extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
> extern struct security_operations *security_ops;
> +extern struct security_operations default_security_ops;
We don't need this extern declaration.
The next obvious cleanup would be to make default_security_ops static.
That will require some code reorganization though - it is presently
defined in capability.c and manipulated by security.c.
--
Stephen Smalley
National Security Agency