Re: [PATCH 2/3] security: introducing security_request_module

Re: [PATCH 2/3] security: introducing security_request_module

[Daniel J Walsh]

On 08/12/2009 11:35 PM, Eric Paris wrote:
> 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);
> 
Every domain that I know of that currently causes this sys_module has net_admin privs, so this will allow us to run a tighter policy.

Re: [PATCH 2/3] security: introducing security_request_module

[Serge E. Hallyn]

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

Re: [PATCH 2/3] security: introducing security_request_module

[Serge E. Hallyn]

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

Re: [PATCH 2/3] security: introducing security_request_module

[Eric Paris]

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

Re: [PATCH 2/3] security: introducing security_request_module

[Serge E. Hallyn]

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

Re: [PATCH 2/3] security: introducing security_request_module

[Eric Paris]

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

Re: [PATCH 2/3] security: introducing security_request_module

[Serge E. Hallyn]

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

[PATCH 2/3] security: introducing security_request_module

[Eric Paris]

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);