* [PATCH 2/3] security: introducing security_request_module
2009-08-13 13:44 [PATCH 1/3] Networking: use CAP_NET_ADMIN when deciding to call request_module Eric Paris
@ 2009-08-13 13:44 ` Eric Paris
2009-08-13 14:03 ` Serge E. Hallyn
2009-08-13 18:40 ` Serge E. Hallyn
2009-08-13 13:45 ` [PATCH 3/3] SELinux: add selinux_kernel_module_request Eric Paris
` (3 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Eric Paris @ 2009-08-13 13:44 UTC (permalink / raw)
To: linux-kernel, selinux, netdev, linux-security-module
Cc: sds, davem, shemminger, kees, morgan, casey, dwalsh
Calling request_module() will trigger a userspace upcall which will load a
new module into the kernel. This can be a dangerous event if the process
able to trigger request_module() is able to control either the modprobe
binary or the module binary. This patch adds a new security hook to
request_module() which can be used by an LSM to control a processes ability
to call request_module().
Signed-off-by: Eric Paris <eparis@redhat.com>
---
include/linux/security.h | 10 ++++++++++
kernel/kmod.c | 4 ++++
security/capability.c | 6 ++++++
security/security.c | 5 +++++
4 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index d5f6578..34c5465 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -678,6 +678,9 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
* @inode points to the inode to use as a reference.
* The current task must be the one that nominated @inode.
* Return 0 if successful.
+ * @kernel_module_request:
+ * Ability to trigger the kernel to automatically upcall to userspace for
+ * userspace to load a kernel module with the given name.
* @task_setuid:
* Check permission before setting one or more of the user identity
* attributes of the current process. The @flags parameter indicates
@@ -1500,6 +1503,7 @@ struct security_operations {
void (*cred_commit)(struct cred *new, const struct cred *old);
int (*kernel_act_as)(struct cred *new, u32 secid);
int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
+ int (*kernel_module_request)(void);
int (*task_setuid) (uid_t id0, uid_t id1, uid_t id2, int flags);
int (*task_fix_setuid) (struct cred *new, const struct cred *old,
int flags);
@@ -1755,6 +1759,7 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
void security_commit_creds(struct cred *new, const struct cred *old);
int security_kernel_act_as(struct cred *new, u32 secid);
int security_kernel_create_files_as(struct cred *new, struct inode *inode);
+int security_kernel_module_request(void);
int security_task_setuid(uid_t id0, uid_t id1, uid_t id2, int flags);
int security_task_fix_setuid(struct cred *new, const struct cred *old,
int flags);
@@ -2306,6 +2311,11 @@ static inline int security_kernel_create_files_as(struct cred *cred,
return 0;
}
+static inline int security_kernel_module_request(void)
+{
+ return 0;
+}
+
static inline int security_task_setuid(uid_t id0, uid_t id1, uid_t id2,
int flags)
{
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 385c31a..5a7ae57 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -78,6 +78,10 @@ int __request_module(bool wait, const char *fmt, ...)
#define MAX_KMOD_CONCURRENT 50 /* Completely arbitrary value - KAO */
static int kmod_loop_msg;
+ ret = security_kernel_module_request();
+ if (ret)
+ return ret;
+
va_start(args, fmt);
ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
va_end(args);
diff --git a/security/capability.c b/security/capability.c
index 4f23f4f..06400cf 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -396,6 +396,11 @@ static int cap_kernel_create_files_as(struct cred *new, struct inode *inode)
return 0;
}
+static int cap_kernel_module_request(void)
+{
+ return 0;
+}
+
static int cap_task_setuid(uid_t id0, uid_t id1, uid_t id2, int flags)
{
return 0;
@@ -961,6 +966,7 @@ void security_fixup_ops(struct security_operations *ops)
set_to_cap_if_null(ops, cred_commit);
set_to_cap_if_null(ops, kernel_act_as);
set_to_cap_if_null(ops, kernel_create_files_as);
+ set_to_cap_if_null(ops, kernel_module_request);
set_to_cap_if_null(ops, task_setuid);
set_to_cap_if_null(ops, task_fix_setuid);
set_to_cap_if_null(ops, task_setgid);
diff --git a/security/security.c b/security/security.c
index b98c684..f88eaf6 100644
--- a/security/security.c
+++ b/security/security.c
@@ -709,6 +709,11 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
return security_ops->kernel_create_files_as(new, inode);
}
+int security_kernel_module_request(void)
+{
+ return security_ops->kernel_module_request();
+}
+
int security_task_setuid(uid_t id0, uid_t id1, uid_t id2, int flags)
{
return security_ops->task_setuid(id0, id1, id2, flags);
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/3] security: introducing security_request_module
2009-08-13 13:44 ` [PATCH 2/3] security: introducing security_request_module Eric Paris
@ 2009-08-13 14:03 ` Serge E. Hallyn
2009-08-13 15:28 ` Eric Paris
2009-08-13 18:40 ` Serge E. Hallyn
1 sibling, 1 reply; 16+ messages in thread
From: Serge E. Hallyn @ 2009-08-13 14:03 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netdev, linux-security-module, sds, davem,
shemminger, kees, morgan, casey, dwalsh
Quoting Eric Paris (eparis@redhat.com):
> Calling request_module() will trigger a userspace upcall which will load a
> new module into the kernel. This can be a dangerous event if the process
> able to trigger request_module() is able to control either the modprobe
> binary or the module binary. This patch adds a new security hook to
> request_module() which can be used by an LSM to control a processes ability
> to call request_module().
Is there a specific case in which you'd want to deny this ability
from a real task?
-serge
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] security: introducing security_request_module
2009-08-13 14:03 ` Serge E. Hallyn
@ 2009-08-13 15:28 ` Eric Paris
2009-08-13 17:54 ` Serge E. Hallyn
0 siblings, 1 reply; 16+ messages in thread
From: Eric Paris @ 2009-08-13 15:28 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: linux-kernel, selinux, netdev, linux-security-module, sds, davem,
shemminger, kees, morgan, casey, dwalsh
On Thu, 2009-08-13 at 09:03 -0500, Serge E. Hallyn wrote:
> Quoting Eric Paris (eparis@redhat.com):
> > Calling request_module() will trigger a userspace upcall which will load a
> > new module into the kernel. This can be a dangerous event if the process
> > able to trigger request_module() is able to control either the modprobe
> > binary or the module binary. This patch adds a new security hook to
> > request_module() which can be used by an LSM to control a processes ability
> > to call request_module().
>
> Is there a specific case in which you'd want to deny this ability
> from a real task?
qemu and any network facing daemon are all programs I don't want to be
able to even ask the kernel to load a module. Clearly you are right,
that the best protection is done by controlling access to modprobe and
the modules on disk (which we are working to fix vs what happened in the
xen fb exploit I showed earlier) but stopping it from the other
direction is, I feel, a useful defense in depth.
If they can't get modprobe called, they can't take over the system
directly, even if they did change a module or change modprobe. I agree
it's not strong security as if they can change modprobe or modules they
might be able to just wait until something else calls modprobe (next
reboot maybe?) to take over the system. But I'd find it very
interesting to know that a high threat target tried to do anything which
attempted to load a module....
-Eric
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] security: introducing security_request_module
2009-08-13 15:28 ` Eric Paris
@ 2009-08-13 17:54 ` Serge E. Hallyn
2009-08-13 18:19 ` Eric Paris
0 siblings, 1 reply; 16+ messages in thread
From: Serge E. Hallyn @ 2009-08-13 17:54 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netdev, linux-security-module, sds, davem,
shemminger, kees, morgan, casey, dwalsh
Quoting Eric Paris (eparis@redhat.com):
> On Thu, 2009-08-13 at 09:03 -0500, Serge E. Hallyn wrote:
> > Quoting Eric Paris (eparis@redhat.com):
> > > Calling request_module() will trigger a userspace upcall which will load a
> > > new module into the kernel. This can be a dangerous event if the process
> > > able to trigger request_module() is able to control either the modprobe
> > > binary or the module binary. This patch adds a new security hook to
> > > request_module() which can be used by an LSM to control a processes ability
> > > to call request_module().
> >
> > Is there a specific case in which you'd want to deny this ability
> > from a real task?
>
> qemu and any network facing daemon are all programs I don't want to be
> able to even ask the kernel to load a module. Clearly you are right,
... What if the network facing daemon might want to use a kernel crypto
module? What if qemu needs the tun module loaded?
> that the best protection is done by controlling access to modprobe and
> the modules on disk (which we are working to fix vs what happened in the
> xen fb exploit I showed earlier) but stopping it from the other
> direction is, I feel, a useful defense in depth.
>
> If they can't get modprobe called, they can't take over the system
> directly, even if they did change a module or change modprobe. I agree
> it's not strong security as if they can change modprobe or modules they
> might be able to just wait until something else calls modprobe (next
> reboot maybe?) to take over the system. But I'd find it very
> interesting to know that a high threat target tried to do anything which
> attempted to load a module....
>
> -Eric
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] security: introducing security_request_module
2009-08-13 17:54 ` Serge E. Hallyn
@ 2009-08-13 18:19 ` Eric Paris
2009-08-13 18:31 ` Serge E. Hallyn
0 siblings, 1 reply; 16+ messages in thread
From: Eric Paris @ 2009-08-13 18:19 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: linux-kernel, selinux, netdev, linux-security-module, sds, davem,
shemminger, kees, morgan, casey, dwalsh
On Thu, 2009-08-13 at 12:54 -0500, Serge E. Hallyn wrote:
> Quoting Eric Paris (eparis@redhat.com):
> > On Thu, 2009-08-13 at 09:03 -0500, Serge E. Hallyn wrote:
> > > Quoting Eric Paris (eparis@redhat.com):
> > > > Calling request_module() will trigger a userspace upcall which will load a
> > > > new module into the kernel. This can be a dangerous event if the process
> > > > able to trigger request_module() is able to control either the modprobe
> > > > binary or the module binary. This patch adds a new security hook to
> > > > request_module() which can be used by an LSM to control a processes ability
> > > > to call request_module().
> > >
> > > Is there a specific case in which you'd want to deny this ability
> > > from a real task?
> >
> > qemu and any network facing daemon are all programs I don't want to be
> > able to even ask the kernel to load a module. Clearly you are right,
>
> ... What if the network facing daemon might want to use a kernel crypto
> module? What if qemu needs the tun module loaded?
Loading code into the kernel is a dangerous operation. We should find
the places where high risk processes are doing this and either choose to
accept the security risk or make sure they are loaded before the
dangerous code is run, aka libvirt knows if the guest needs the tun
device and it should be allowed to trigger it's loading. It's shouldn't
be the guest doing the triggering.
It was also pointed out to me that stopping processes from being able to
trigger module loads could be useful to keep a subverted process from
loading an uncommon or unloaded module with a vulnerability which they
could then use to exploit the kernel. It's all about hardening.
It won't be a simple task for SELinux policy to do something useful with
this hook as they need to identify all of the legitimate callers of
request_module(), but then again, that should be a lot easier than 90%
of what SELinux already has to do, and until SELinux policy decides to
make use of the new permission things will 'just work'
-Eric
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] security: introducing security_request_module
2009-08-13 18:19 ` Eric Paris
@ 2009-08-13 18:31 ` Serge E. Hallyn
0 siblings, 0 replies; 16+ messages in thread
From: Serge E. Hallyn @ 2009-08-13 18:31 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netdev, linux-security-module, sds, davem,
shemminger, kees, morgan, casey, dwalsh
Quoting Eric Paris (eparis@redhat.com):
> On Thu, 2009-08-13 at 12:54 -0500, Serge E. Hallyn wrote:
> > Quoting Eric Paris (eparis@redhat.com):
> > > On Thu, 2009-08-13 at 09:03 -0500, Serge E. Hallyn wrote:
> > > > Quoting Eric Paris (eparis@redhat.com):
> > > > > Calling request_module() will trigger a userspace upcall which will load a
> > > > > new module into the kernel. This can be a dangerous event if the process
> > > > > able to trigger request_module() is able to control either the modprobe
> > > > > binary or the module binary. This patch adds a new security hook to
> > > > > request_module() which can be used by an LSM to control a processes ability
> > > > > to call request_module().
> > > >
> > > > Is there a specific case in which you'd want to deny this ability
> > > > from a real task?
> > >
> > > qemu and any network facing daemon are all programs I don't want to be
> > > able to even ask the kernel to load a module. Clearly you are right,
> >
> > ... What if the network facing daemon might want to use a kernel crypto
> > module? What if qemu needs the tun module loaded?
>
> Loading code into the kernel is a dangerous operation. We should find
> the places where high risk processes are doing this and either choose to
> accept the security risk or make sure they are loaded before the
> dangerous code is run, aka libvirt knows if the guest needs the tun
> device and it should be allowed to trigger it's loading. It's shouldn't
> be the guest doing the triggering.
Well, I continue to be dubious, but the hook does no harm so in case
it sounded otherwise, I don't object.
Mind you the right thing to do would be i.e. in the qemu case to have
a more privileged helper load the needed modules and then fire off a
qemu unable to cause module loads. If userspace is willing to jump
through that rather minimal hoop then this could be useful I suppose.
thanks,
-serge
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] security: introducing security_request_module
2009-08-13 13:44 ` [PATCH 2/3] security: introducing security_request_module Eric Paris
2009-08-13 14:03 ` Serge E. Hallyn
@ 2009-08-13 18:40 ` Serge E. Hallyn
1 sibling, 0 replies; 16+ messages in thread
From: Serge E. Hallyn @ 2009-08-13 18:40 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netdev, linux-security-module, sds, davem,
shemminger, kees, morgan, casey, dwalsh
Quoting Eric Paris (eparis@redhat.com):
> Calling request_module() will trigger a userspace upcall which will load a
> new module into the kernel. This can be a dangerous event if the process
> able to trigger request_module() is able to control either the modprobe
> binary or the module binary. This patch adds a new security hook to
> request_module() which can be used by an LSM to control a processes ability
> to call request_module().
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
Looks fine code-wise.
Just to match the rest of the documentation, the description in
security.h should probably mention 'Return 0 if allowed'.
Acked-by: Serge Hallyn <serue@us.ibm.com>
> ---
>
> include/linux/security.h | 10 ++++++++++
> kernel/kmod.c | 4 ++++
> security/capability.c | 6 ++++++
> security/security.c | 5 +++++
> 4 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d5f6578..34c5465 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -678,6 +678,9 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
> * @inode points to the inode to use as a reference.
> * The current task must be the one that nominated @inode.
> * Return 0 if successful.
> + * @kernel_module_request:
> + * Ability to trigger the kernel to automatically upcall to userspace for
> + * userspace to load a kernel module with the given name.
> * @task_setuid:
> * Check permission before setting one or more of the user identity
> * attributes of the current process. The @flags parameter indicates
> @@ -1500,6 +1503,7 @@ struct security_operations {
> void (*cred_commit)(struct cred *new, const struct cred *old);
> int (*kernel_act_as)(struct cred *new, u32 secid);
> int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
> + int (*kernel_module_request)(void);
> int (*task_setuid) (uid_t id0, uid_t id1, uid_t id2, int flags);
> int (*task_fix_setuid) (struct cred *new, const struct cred *old,
> int flags);
> @@ -1755,6 +1759,7 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
> void security_commit_creds(struct cred *new, const struct cred *old);
> int security_kernel_act_as(struct cred *new, u32 secid);
> int security_kernel_create_files_as(struct cred *new, struct inode *inode);
> +int security_kernel_module_request(void);
> int security_task_setuid(uid_t id0, uid_t id1, uid_t id2, int flags);
> int security_task_fix_setuid(struct cred *new, const struct cred *old,
> int flags);
> @@ -2306,6 +2311,11 @@ static inline int security_kernel_create_files_as(struct cred *cred,
> return 0;
> }
>
> +static inline int security_kernel_module_request(void)
> +{
> + return 0;
> +}
> +
> static inline int security_task_setuid(uid_t id0, uid_t id1, uid_t id2,
> int flags)
> {
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 385c31a..5a7ae57 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -78,6 +78,10 @@ int __request_module(bool wait, const char *fmt, ...)
> #define MAX_KMOD_CONCURRENT 50 /* Completely arbitrary value - KAO */
> static int kmod_loop_msg;
>
> + ret = security_kernel_module_request();
> + if (ret)
> + return ret;
> +
> va_start(args, fmt);
> ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
> va_end(args);
> diff --git a/security/capability.c b/security/capability.c
> index 4f23f4f..06400cf 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -396,6 +396,11 @@ static int cap_kernel_create_files_as(struct cred *new, struct inode *inode)
> return 0;
> }
>
> +static int cap_kernel_module_request(void)
> +{
> + return 0;
> +}
> +
> static int cap_task_setuid(uid_t id0, uid_t id1, uid_t id2, int flags)
> {
> return 0;
> @@ -961,6 +966,7 @@ void security_fixup_ops(struct security_operations *ops)
> set_to_cap_if_null(ops, cred_commit);
> set_to_cap_if_null(ops, kernel_act_as);
> set_to_cap_if_null(ops, kernel_create_files_as);
> + set_to_cap_if_null(ops, kernel_module_request);
> set_to_cap_if_null(ops, task_setuid);
> set_to_cap_if_null(ops, task_fix_setuid);
> set_to_cap_if_null(ops, task_setgid);
> diff --git a/security/security.c b/security/security.c
> index b98c684..f88eaf6 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -709,6 +709,11 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
> return security_ops->kernel_create_files_as(new, inode);
> }
>
> +int security_kernel_module_request(void)
> +{
> + return security_ops->kernel_module_request();
> +}
> +
> int security_task_setuid(uid_t id0, uid_t id1, uid_t id2, int flags)
> {
> return security_ops->task_setuid(id0, id1, id2, flags);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] SELinux: add selinux_kernel_module_request
2009-08-13 13:44 [PATCH 1/3] Networking: use CAP_NET_ADMIN when deciding to call request_module Eric Paris
2009-08-13 13:44 ` [PATCH 2/3] security: introducing security_request_module Eric Paris
@ 2009-08-13 13:45 ` Eric Paris
2009-08-13 18:43 ` Serge E. Hallyn
2009-08-13 14:01 ` [PATCH 1/3] Networking: use CAP_NET_ADMIN when deciding to call request_module Serge E. Hallyn
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Eric Paris @ 2009-08-13 13:45 UTC (permalink / raw)
To: linux-kernel, selinux, netdev, linux-security-module
Cc: sds, davem, shemminger, kees, morgan, casey, dwalsh
This patch adds a new selinux hook so SELinux can arbitrate if a given
process should be allowed to trigger a request for the kernel to try to
load a module. This is a different operation than a process trying to load
a module itself, which is already protected by CAP_SYS_MODULE.
Signed-off-by: Eric Paris <eparis@redhat.com>
---
security/selinux/hooks.c | 6 ++++++
security/selinux/include/av_perm_to_string.h | 1 +
security/selinux/include/av_permissions.h | 1 +
3 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 38afca9..b0d72f1 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3296,6 +3296,11 @@ static int selinux_kernel_create_files_as(struct cred *new, struct inode *inode)
return 0;
}
+static int selinux_kernel_module_request(void)
+{
+ return task_has_system(current, SYSTEM__MODULE_REQUEST);
+}
+
static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
{
return current_has_perm(p, PROCESS__SETPGID);
@@ -5457,6 +5462,7 @@ static struct security_operations selinux_ops = {
.cred_prepare = selinux_cred_prepare,
.kernel_act_as = selinux_kernel_act_as,
.kernel_create_files_as = selinux_kernel_create_files_as,
+ .kernel_module_request = selinux_kernel_module_request,
.task_setpgid = selinux_task_setpgid,
.task_getpgid = selinux_task_getpgid,
.task_getsid = selinux_task_getsid,
diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h
index 31df1d7..2b683ad 100644
--- a/security/selinux/include/av_perm_to_string.h
+++ b/security/selinux/include/av_perm_to_string.h
@@ -107,6 +107,7 @@
S_(SECCLASS_SYSTEM, SYSTEM__SYSLOG_READ, "syslog_read")
S_(SECCLASS_SYSTEM, SYSTEM__SYSLOG_MOD, "syslog_mod")
S_(SECCLASS_SYSTEM, SYSTEM__SYSLOG_CONSOLE, "syslog_console")
+ S_(SECCLASS_SYSTEM, SYSTEM__MODULE_REQUEST, "module_request")
S_(SECCLASS_CAPABILITY, CAPABILITY__CHOWN, "chown")
S_(SECCLASS_CAPABILITY, CAPABILITY__DAC_OVERRIDE, "dac_override")
S_(SECCLASS_CAPABILITY, CAPABILITY__DAC_READ_SEARCH, "dac_read_search")
diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
index 0b41ad5..0546d61 100644
--- a/security/selinux/include/av_permissions.h
+++ b/security/selinux/include/av_permissions.h
@@ -530,6 +530,7 @@
#define SYSTEM__SYSLOG_READ 0x00000002UL
#define SYSTEM__SYSLOG_MOD 0x00000004UL
#define SYSTEM__SYSLOG_CONSOLE 0x00000008UL
+#define SYSTEM__MODULE_REQUEST 0x00000010UL
#define CAPABILITY__CHOWN 0x00000001UL
#define CAPABILITY__DAC_OVERRIDE 0x00000002UL
#define CAPABILITY__DAC_READ_SEARCH 0x00000004UL
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 3/3] SELinux: add selinux_kernel_module_request
2009-08-13 13:45 ` [PATCH 3/3] SELinux: add selinux_kernel_module_request Eric Paris
@ 2009-08-13 18:43 ` Serge E. Hallyn
0 siblings, 0 replies; 16+ messages in thread
From: Serge E. Hallyn @ 2009-08-13 18:43 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netdev, linux-security-module, sds, davem,
shemminger, kees, morgan, casey, dwalsh
Quoting Eric Paris (eparis@redhat.com):
> This patch adds a new selinux hook so SELinux can arbitrate if a given
> process should be allowed to trigger a request for the kernel to try to
> load a module. This is a different operation than a process trying to load
> a module itself, which is already protected by CAP_SYS_MODULE.
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
> ---
>
> security/selinux/hooks.c | 6 ++++++
> security/selinux/include/av_perm_to_string.h | 1 +
> security/selinux/include/av_permissions.h | 1 +
> 3 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 38afca9..b0d72f1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3296,6 +3296,11 @@ static int selinux_kernel_create_files_as(struct cred *new, struct inode *inode)
> return 0;
> }
>
> +static int selinux_kernel_module_request(void)
> +{
> + return task_has_system(current, SYSTEM__MODULE_REQUEST);
> +}
> +
> static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
> {
> return current_has_perm(p, PROCESS__SETPGID);
> @@ -5457,6 +5462,7 @@ static struct security_operations selinux_ops = {
> .cred_prepare = selinux_cred_prepare,
> .kernel_act_as = selinux_kernel_act_as,
> .kernel_create_files_as = selinux_kernel_create_files_as,
> + .kernel_module_request = selinux_kernel_module_request,
> .task_setpgid = selinux_task_setpgid,
> .task_getpgid = selinux_task_getpgid,
> .task_getsid = selinux_task_getsid,
> diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h
> index 31df1d7..2b683ad 100644
> --- a/security/selinux/include/av_perm_to_string.h
> +++ b/security/selinux/include/av_perm_to_string.h
> @@ -107,6 +107,7 @@
> S_(SECCLASS_SYSTEM, SYSTEM__SYSLOG_READ, "syslog_read")
> S_(SECCLASS_SYSTEM, SYSTEM__SYSLOG_MOD, "syslog_mod")
> S_(SECCLASS_SYSTEM, SYSTEM__SYSLOG_CONSOLE, "syslog_console")
> + S_(SECCLASS_SYSTEM, SYSTEM__MODULE_REQUEST, "module_request")
> S_(SECCLASS_CAPABILITY, CAPABILITY__CHOWN, "chown")
> S_(SECCLASS_CAPABILITY, CAPABILITY__DAC_OVERRIDE, "dac_override")
> S_(SECCLASS_CAPABILITY, CAPABILITY__DAC_READ_SEARCH, "dac_read_search")
> diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
> index 0b41ad5..0546d61 100644
> --- a/security/selinux/include/av_permissions.h
> +++ b/security/selinux/include/av_permissions.h
> @@ -530,6 +530,7 @@
> #define SYSTEM__SYSLOG_READ 0x00000002UL
> #define SYSTEM__SYSLOG_MOD 0x00000004UL
> #define SYSTEM__SYSLOG_CONSOLE 0x00000008UL
> +#define SYSTEM__MODULE_REQUEST 0x00000010UL
> #define CAPABILITY__CHOWN 0x00000001UL
> #define CAPABILITY__DAC_OVERRIDE 0x00000002UL
> #define CAPABILITY__DAC_READ_SEARCH 0x00000004UL
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] Networking: use CAP_NET_ADMIN when deciding to call request_module
2009-08-13 13:44 [PATCH 1/3] Networking: use CAP_NET_ADMIN when deciding to call request_module Eric Paris
2009-08-13 13:44 ` [PATCH 2/3] security: introducing security_request_module Eric Paris
2009-08-13 13:45 ` [PATCH 3/3] SELinux: add selinux_kernel_module_request Eric Paris
@ 2009-08-13 14:01 ` Serge E. Hallyn
2009-08-13 18:45 ` Paul Moore
2009-08-14 1:56 ` James Morris
4 siblings, 0 replies; 16+ messages in thread
From: Serge E. Hallyn @ 2009-08-13 14:01 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netdev, linux-security-module, sds, davem,
shemminger, kees, morgan, casey, dwalsh
Quoting Eric Paris (eparis@redhat.com):
> The networking code checks CAP_SYS_MODULE before using request_module() to
> try to load a kernel module. While this seems reasonable it's actually
> weakening system security since we have to allow CAP_SYS_MODULE for things
> like /sbin/ip and bluetoothd which need to be able to trigger module loads.
> CAP_SYS_MODULE actually grants those binaries the ability to directly load
> any code into the kernel. We should instead be protecting modprobe and the
> modules on disk, rather than granting random programs the ability to load code
> directly into the kernel. Instead we are going to gate those networking checks
> on CAP_NET_ADMIN which still limits them to root but which does not grant
> those processes the ability to load arbitrary code into the kernel.
Right, so we want to check that the caller has the rights to perform the
action which (in the end) requires the module load, not the module load
itself. CAP_SYS_MODULE should be reserved for callers which specify a
module (full pathname) to load.
> Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
> ---
>
> drivers/staging/comedi/comedi_fops.c | 8 ++++----
> net/core/dev.c | 2 +-
> net/ipv4/tcp_cong.c | 4 ++--
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
> index 42e4bc4..f54bb9b 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -1772,12 +1772,12 @@ static int comedi_open(struct inode *inode, struct file *file)
> mutex_lock(&dev->mutex);
> if (dev->attached)
> goto ok;
> - if (!capable(CAP_SYS_MODULE) && dev->in_request_module) {
> + if (!capable(CAP_NET_ADMIN) && dev->in_request_module) {
> DPRINTK("in request module\n");
> mutex_unlock(&dev->mutex);
> return -ENODEV;
> }
> - if (capable(CAP_SYS_MODULE) && dev->in_request_module)
> + if (capable(CAP_NET_ADMIN) && dev->in_request_module)
> goto ok;
>
> dev->in_request_module = 1;
> @@ -1790,8 +1790,8 @@ static int comedi_open(struct inode *inode, struct file *file)
>
> dev->in_request_module = 0;
>
> - if (!dev->attached && !capable(CAP_SYS_MODULE)) {
> - DPRINTK("not attached and not CAP_SYS_MODULE\n");
> + if (!dev->attached && !capable(CAP_NET_ADMIN)) {
> + DPRINTK("not attached and not CAP_NET_ADMIN\n");
> mutex_unlock(&dev->mutex);
> return -ENODEV;
> }
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 09fb03f..2604db9 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1031,7 +1031,7 @@ void dev_load(struct net *net, const char *name)
> dev = __dev_get_by_name(net, name);
> read_unlock(&dev_base_lock);
>
> - if (!dev && capable(CAP_SYS_MODULE))
> + if (!dev && capable(CAP_NET_ADMIN))
> request_module("%s", name);
> }
>
> diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
> index e92beb9..6428b34 100644
> --- a/net/ipv4/tcp_cong.c
> +++ b/net/ipv4/tcp_cong.c
> @@ -116,7 +116,7 @@ int tcp_set_default_congestion_control(const char *name)
> spin_lock(&tcp_cong_list_lock);
> ca = tcp_ca_find(name);
> #ifdef CONFIG_MODULES
> - if (!ca && capable(CAP_SYS_MODULE)) {
> + if (!ca && capable(CAP_NET_ADMIN)) {
> spin_unlock(&tcp_cong_list_lock);
>
> request_module("tcp_%s", name);
> @@ -246,7 +246,7 @@ int tcp_set_congestion_control(struct sock *sk, const char *name)
>
> #ifdef CONFIG_MODULES
> /* not found attempt to autoload module */
> - if (!ca && capable(CAP_SYS_MODULE)) {
> + if (!ca && capable(CAP_NET_ADMIN)) {
> rcu_read_unlock();
> request_module("tcp_%s", name);
> rcu_read_lock();
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/3] Networking: use CAP_NET_ADMIN when deciding to call request_module
2009-08-13 13:44 [PATCH 1/3] Networking: use CAP_NET_ADMIN when deciding to call request_module Eric Paris
` (2 preceding siblings ...)
2009-08-13 14:01 ` [PATCH 1/3] Networking: use CAP_NET_ADMIN when deciding to call request_module Serge E. Hallyn
@ 2009-08-13 18:45 ` Paul Moore
2009-08-13 22:55 ` James Morris
2009-08-13 23:14 ` David Miller
2009-08-14 1:56 ` James Morris
4 siblings, 2 replies; 16+ messages in thread
From: Paul Moore @ 2009-08-13 18:45 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netdev, linux-security-module, sds, davem,
shemminger, kees, morgan, casey, dwalsh
On Thursday 13 August 2009 09:44:51 am Eric Paris wrote:
> The networking code checks CAP_SYS_MODULE before using request_module() to
> try to load a kernel module. While this seems reasonable it's actually
> weakening system security since we have to allow CAP_SYS_MODULE for things
> like /sbin/ip and bluetoothd which need to be able to trigger module loads.
> CAP_SYS_MODULE actually grants those binaries the ability to directly load
> any code into the kernel. We should instead be protecting modprobe and the
> modules on disk, rather than granting random programs the ability to load
> code directly into the kernel. Instead we are going to gate those
> networking checks on CAP_NET_ADMIN which still limits them to root but
> which does not grant those processes the ability to load arbitrary code
> into the kernel.
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
Sounds and looks reasonable to me.
Acked-by: Paul Moore <paul.moore@hp.com>
> ---
>
> drivers/staging/comedi/comedi_fops.c | 8 ++++----
> net/core/dev.c | 2 +-
> net/ipv4/tcp_cong.c | 4 ++--
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c
> b/drivers/staging/comedi/comedi_fops.c index 42e4bc4..f54bb9b 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -1772,12 +1772,12 @@ static int comedi_open(struct inode *inode, struct
> file *file) mutex_lock(&dev->mutex);
> if (dev->attached)
> goto ok;
> - if (!capable(CAP_SYS_MODULE) && dev->in_request_module) {
> + if (!capable(CAP_NET_ADMIN) && dev->in_request_module) {
> DPRINTK("in request module\n");
> mutex_unlock(&dev->mutex);
> return -ENODEV;
> }
> - if (capable(CAP_SYS_MODULE) && dev->in_request_module)
> + if (capable(CAP_NET_ADMIN) && dev->in_request_module)
> goto ok;
>
> dev->in_request_module = 1;
> @@ -1790,8 +1790,8 @@ static int comedi_open(struct inode *inode, struct
> file *file)
>
> dev->in_request_module = 0;
>
> - if (!dev->attached && !capable(CAP_SYS_MODULE)) {
> - DPRINTK("not attached and not CAP_SYS_MODULE\n");
> + if (!dev->attached && !capable(CAP_NET_ADMIN)) {
> + DPRINTK("not attached and not CAP_NET_ADMIN\n");
> mutex_unlock(&dev->mutex);
> return -ENODEV;
> }
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 09fb03f..2604db9 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1031,7 +1031,7 @@ void dev_load(struct net *net, const char *name)
> dev = __dev_get_by_name(net, name);
> read_unlock(&dev_base_lock);
>
> - if (!dev && capable(CAP_SYS_MODULE))
> + if (!dev && capable(CAP_NET_ADMIN))
> request_module("%s", name);
> }
>
> diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
> index e92beb9..6428b34 100644
> --- a/net/ipv4/tcp_cong.c
> +++ b/net/ipv4/tcp_cong.c
> @@ -116,7 +116,7 @@ int tcp_set_default_congestion_control(const char
> *name) spin_lock(&tcp_cong_list_lock);
> ca = tcp_ca_find(name);
> #ifdef CONFIG_MODULES
> - if (!ca && capable(CAP_SYS_MODULE)) {
> + if (!ca && capable(CAP_NET_ADMIN)) {
> spin_unlock(&tcp_cong_list_lock);
>
> request_module("tcp_%s", name);
> @@ -246,7 +246,7 @@ int tcp_set_congestion_control(struct sock *sk, const
> char *name)
>
> #ifdef CONFIG_MODULES
> /* not found attempt to autoload module */
> - if (!ca && capable(CAP_SYS_MODULE)) {
> + if (!ca && capable(CAP_NET_ADMIN)) {
> rcu_read_unlock();
> request_module("tcp_%s", name);
> rcu_read_lock();
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in the body of a message to
> majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
paul moore
linux @ hp
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/3] Networking: use CAP_NET_ADMIN when deciding to call request_module
2009-08-13 18:45 ` Paul Moore
@ 2009-08-13 22:55 ` James Morris
2009-08-13 23:14 ` David Miller
1 sibling, 0 replies; 16+ messages in thread
From: James Morris @ 2009-08-13 22:55 UTC (permalink / raw)
To: David S. Miller
Cc: Eric Paris, linux-kernel, selinux, netdev, linux-security-module,
Stephen Smalley, shemminger, kees, morgan, Casey Schaufler,
Daniel J Walsh, Herbert Xu
Any nacks/acks from netdev folk?
It looks like the right thing to me.
On Thu, 13 Aug 2009, Paul Moore wrote:
> On Thursday 13 August 2009 09:44:51 am Eric Paris wrote:
> > The networking code checks CAP_SYS_MODULE before using request_module() to
> > try to load a kernel module. While this seems reasonable it's actually
> > weakening system security since we have to allow CAP_SYS_MODULE for things
> > like /sbin/ip and bluetoothd which need to be able to trigger module loads.
> > CAP_SYS_MODULE actually grants those binaries the ability to directly load
> > any code into the kernel. We should instead be protecting modprobe and the
> > modules on disk, rather than granting random programs the ability to load
> > code directly into the kernel. Instead we are going to gate those
> > networking checks on CAP_NET_ADMIN which still limits them to root but
> > which does not grant those processes the ability to load arbitrary code
> > into the kernel.
> >
> > Signed-off-by: Eric Paris <eparis@redhat.com>
>
> Sounds and looks reasonable to me.
>
> Acked-by: Paul Moore <paul.moore@hp.com>
>
> > ---
> >
> > drivers/staging/comedi/comedi_fops.c | 8 ++++----
> > net/core/dev.c | 2 +-
> > net/ipv4/tcp_cong.c | 4 ++--
> > 3 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/comedi_fops.c
> > b/drivers/staging/comedi/comedi_fops.c index 42e4bc4..f54bb9b 100644
> > --- a/drivers/staging/comedi/comedi_fops.c
> > +++ b/drivers/staging/comedi/comedi_fops.c
> > @@ -1772,12 +1772,12 @@ static int comedi_open(struct inode *inode, struct
> > file *file) mutex_lock(&dev->mutex);
> > if (dev->attached)
> > goto ok;
> > - if (!capable(CAP_SYS_MODULE) && dev->in_request_module) {
> > + if (!capable(CAP_NET_ADMIN) && dev->in_request_module) {
> > DPRINTK("in request module\n");
> > mutex_unlock(&dev->mutex);
> > return -ENODEV;
> > }
> > - if (capable(CAP_SYS_MODULE) && dev->in_request_module)
> > + if (capable(CAP_NET_ADMIN) && dev->in_request_module)
> > goto ok;
> >
> > dev->in_request_module = 1;
> > @@ -1790,8 +1790,8 @@ static int comedi_open(struct inode *inode, struct
> > file *file)
> >
> > dev->in_request_module = 0;
> >
> > - if (!dev->attached && !capable(CAP_SYS_MODULE)) {
> > - DPRINTK("not attached and not CAP_SYS_MODULE\n");
> > + if (!dev->attached && !capable(CAP_NET_ADMIN)) {
> > + DPRINTK("not attached and not CAP_NET_ADMIN\n");
> > mutex_unlock(&dev->mutex);
> > return -ENODEV;
> > }
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 09fb03f..2604db9 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -1031,7 +1031,7 @@ void dev_load(struct net *net, const char *name)
> > dev = __dev_get_by_name(net, name);
> > read_unlock(&dev_base_lock);
> >
> > - if (!dev && capable(CAP_SYS_MODULE))
> > + if (!dev && capable(CAP_NET_ADMIN))
> > request_module("%s", name);
> > }
> >
> > diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
> > index e92beb9..6428b34 100644
> > --- a/net/ipv4/tcp_cong.c
> > +++ b/net/ipv4/tcp_cong.c
> > @@ -116,7 +116,7 @@ int tcp_set_default_congestion_control(const char
> > *name) spin_lock(&tcp_cong_list_lock);
> > ca = tcp_ca_find(name);
> > #ifdef CONFIG_MODULES
> > - if (!ca && capable(CAP_SYS_MODULE)) {
> > + if (!ca && capable(CAP_NET_ADMIN)) {
> > spin_unlock(&tcp_cong_list_lock);
> >
> > request_module("tcp_%s", name);
> > @@ -246,7 +246,7 @@ int tcp_set_congestion_control(struct sock *sk, const
> > char *name)
> >
> > #ifdef CONFIG_MODULES
> > /* not found attempt to autoload module */
> > - if (!ca && capable(CAP_SYS_MODULE)) {
> > + if (!ca && capable(CAP_NET_ADMIN)) {
> > rcu_read_unlock();
> > request_module("tcp_%s", name);
> > rcu_read_lock();
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-security-module" in the body of a message to
> > majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> paul moore
> linux @ hp
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/3] Networking: use CAP_NET_ADMIN when deciding to call request_module
2009-08-13 18:45 ` Paul Moore
2009-08-13 22:55 ` James Morris
@ 2009-08-13 23:14 ` David Miller
1 sibling, 0 replies; 16+ messages in thread
From: David Miller @ 2009-08-13 23:14 UTC (permalink / raw)
To: paul.moore
Cc: eparis, linux-kernel, selinux, netdev, linux-security-module, sds,
shemminger, kees, morgan, casey, dwalsh
From: Paul Moore <paul.moore@hp.com>
Date: Thu, 13 Aug 2009 14:45:37 -0400
> On Thursday 13 August 2009 09:44:51 am Eric Paris wrote:
>> The networking code checks CAP_SYS_MODULE before using request_module() to
>> try to load a kernel module. While this seems reasonable it's actually
>> weakening system security since we have to allow CAP_SYS_MODULE for things
>> like /sbin/ip and bluetoothd which need to be able to trigger module loads.
>> CAP_SYS_MODULE actually grants those binaries the ability to directly load
>> any code into the kernel. We should instead be protecting modprobe and the
>> modules on disk, rather than granting random programs the ability to load
>> code directly into the kernel. Instead we are going to gate those
>> networking checks on CAP_NET_ADMIN which still limits them to root but
>> which does not grant those processes the ability to load arbitrary code
>> into the kernel.
>>
>> Signed-off-by: Eric Paris <eparis@redhat.com>
>
> Sounds and looks reasonable to me.
>
> Acked-by: Paul Moore <paul.moore@hp.com>
Looks fine to me:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] Networking: use CAP_NET_ADMIN when deciding to call request_module
2009-08-13 13:44 [PATCH 1/3] Networking: use CAP_NET_ADMIN when deciding to call request_module Eric Paris
` (3 preceding siblings ...)
2009-08-13 18:45 ` Paul Moore
@ 2009-08-14 1:56 ` James Morris
4 siblings, 0 replies; 16+ messages in thread
From: James Morris @ 2009-08-14 1:56 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netdev, linux-security-module, sds, davem,
shemminger, kees, morgan, casey, dwalsh
On Thu, 13 Aug 2009, Eric Paris wrote:
> The networking code checks CAP_SYS_MODULE before using request_module() to
I applied all three patches to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next
and fixed up the documentation per Serge's suggestion.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 16+ messages in thread