diff mbox

[v3,1/7] security: rename security_kernel_read_file() hook

Message ID 87po1k2304.fsf@xmission.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric W. Biederman May 24, 2018, 8:49 p.m. UTC 
I already nacked this approach because the two cases don't
share a bit of code.  When I looked closer it was even crazier.

The way ima uses this hook and the post_load hook today is a travesty.

The way the security_kernel_file_read and security_kernel_file_post_read
are called today and are used by ima don't make the least little bit of
sense.

Abusing security_kernel_file_read in the module loader and then abusing
security_kernel_file_post_read in the firmware loader is insane.  The
loadpin lsm could not even figure this out and so it failed to work
because of these shenanighans.

Only implementing kernel_file_read to handle the !file case is pretty
much insane.   There is no way this should be expanded to cover kexec
until the code actually makes sense.  We need a maintainable kernel.

Below is where I suggest you start on sorting out these security hooks.
- Adding a security_kernel_arg to catch when you want to allow/deny the
  use of an argument to a syscall.  What security_kernel_file_read and
  security_kernel_file_post_read have been abused for.

- Removing ima_file_read because it is completely subsumed by the new
  call.

- Please note with adding this new hook there is no code shared between
  the cases, and the lsm code becomes simpler shorter when it can assume
  security_kernel_file_post_read always takes a struct file.  (Even with
  the addition of a new security hook).

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

Comments

Mimi Zohar May 24, 2018, 11:29 p.m. UTC | #1 
On Thu, 2018-05-24 at 15:49 -0500, Eric W. Biederman wrote:
> I already nacked this approach because the two cases don't
> share a bit of code.  When I looked closer it was even crazier.

It hasn't been clear what you meant by "the two cases don't share a
bit of code".  The first attempt called
security_kernel_read_file().  As per your comments, kexec_load doesn't
load a file.  Thinking it was a naming issue the second attempt
defined a wrapper named security_kernel_read_blob() for
security_kernel_read_file().  Still thinking it was a naming issue,
this attempt renamed the security_kernel_read_file() to
security_kernel_read_data().

> 
> The way ima uses this hook and the post_load hook today is a travesty.

Instead of having multiple functions, each a bit different, for
reading a file from the kernel, kernel_read_file() is a generic
implementation with both pre and post security calls.

 I think the pre and post security kernel_read_file() LSM hooks are
quite well thought out.  The security_kernel_read_file is called
before the kernel reads the file.  The security_kernel_post_read_file
is called after the kernel reads the file.

> The way the security_kernel_file_read and security_kernel_file_post_read
> are called today and are used by ima don't make the least little bit of
> sense.
> 
> Abusing security_kernel_file_read in the module loader and then abusing
> security_kernel_file_post_read in the firmware loader is insane.  The
> loadpin lsm could not even figure this out and so it failed to work
> because of these shenanighans.
> 
> Only implementing kernel_file_read to handle the !file case is pretty
> much insane.   There is no way this should be expanded to cover kexec
> until the code actually makes sense.  We need a maintainable kernel.

It wasn't implemented *only* for the IMA !file case, but as a generic
mechanism.  True, IMA is only using the security_kernel_read_file hook
for detecting !file, but the security_kernel_post_read_file hook is
used for verifying the file's integrity.

> Below is where I suggest you start on sorting out these security hooks.
> - Adding a security_kernel_arg to catch when you want to allow/deny the
>   use of an argument to a syscall.  What security_kernel_file_read and
>   security_kernel_file_post_read have been abused for.

Assuming we define a new LSM hook named "security_kernel_arg", would
we also define a new enumeration or could we still use the existing
kernel_read_file_id?

> 
> - Removing ima_file_read because it is completely subsumed by the new
>   call.

The existing IMA function wouldn't be removed, but renamed to whatever
the new LSM hook is named.

> 
> - Please note with adding this new hook there is no code shared between
>   the cases, and the lsm code becomes simpler shorter when it can assume
>   security_kernel_file_post_read always takes a struct file.  (Even with
>   the addition of a new security hook).

We would be defining a new LSM hook, not removing the existing
security_kernel_read_file hook, and only renaming the IMA usage of the
hook.

If defining a new LSM hook named security_kernel_arg makes you happy,
I don't have a problem with implementing it.

James, Casey, are you Ok with this?

Mimi

--
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
Mimi Zohar May 25, 2018, 12:22 p.m. UTC | #2 
On Thu, 2018-05-24 at 15:49 -0500, Eric W. Biederman wrote:

Thank you for the sample code below.  It needs to be broken up into
proper patches, with some changes, but it is a good start.

Mimi 

> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> index 358354148dec..04536ff81bd2 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -294,9 +294,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>  				dev_err(dev, "%s: map pages failed\n",
>  					__func__);
>  			else
> -				rc = security_kernel_post_read_file(NULL,
> -						fw_priv->data, fw_priv->size,
> -						READING_FIRMWARE);
> +				rc = security_kernel_arg(KARG_FIRMWARE);
> 
>  			/*
>  			 * Same logic as fw_load_abort, only the DONE bit
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 0e4647e0eb60..9fb42736ba29 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -11,6 +11,7 @@
>  #define _LINUX_IMA_H
> 
>  #include <linux/fs.h>
> +#include <linux/security.h>
>  #include <linux/kexec.h>
>  struct linux_binprm;
> 
> @@ -19,7 +20,7 @@ extern int ima_bprm_check(struct linux_binprm *bprm);
>  extern int ima_file_check(struct file *file, int mask, int opened);
>  extern void ima_file_free(struct file *file);
>  extern int ima_file_mmap(struct file *file, unsigned long prot);
> -extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
> +extern int ima_kernel_arg(enum kernel_arg_id id);
>  extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
>  			      enum kernel_read_file_id id);
>  extern void ima_post_path_mknod(struct dentry *dentry);
> @@ -49,7 +50,7 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
>  	return 0;
>  }
> 
> -static inline int ima_read_file(struct file *file, enum kernel_read_file_id id)
> +static inline int ima_kernel_arg(enum kernel_arg_id id)
>  {
>  	return 0;
>  }
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 9d0b286f3dba..7f8bc3030784 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -576,6 +576,10 @@
>   *	userspace to load a kernel module with the given name.
>   *	@kmod_name name of the module requested by the kernel
>   *	Return 0 if successful.
> + * @kernel_arg:
> + *	Use a syscall argument
> + *	@id kernel argument identifier
> + *	Return 0 if permission is granted.
>   * @kernel_read_file:
>   *	Read a file specified by userspace.
>   *	@file contains the file structure pointing to the file being read
> @@ -1577,6 +1581,7 @@ union security_list_options {
>  	int (*kernel_act_as)(struct cred *new, u32 secid);
>  	int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
>  	int (*kernel_module_request)(char *kmod_name);
> +	int (*kernel_arg)(enum kernel_arg_id id);
>  	int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id);
>  	int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
>  				     enum kernel_read_file_id id);
> @@ -1866,6 +1871,7 @@ struct security_hook_heads {
>  	struct hlist_head cred_getsecid;
>  	struct hlist_head kernel_act_as;
>  	struct hlist_head kernel_create_files_as;
> +	struct hlist_head kernel_arg;
>  	struct hlist_head kernel_read_file;
>  	struct hlist_head kernel_post_read_file;
>  	struct hlist_head kernel_module_request;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 200920f521a1..6cf1bd87f041 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -159,6 +159,32 @@ extern int mmap_min_addr_handler(struct ctl_table *table, int write,
>  typedef int (*initxattrs) (struct inode *inode,
>  			   const struct xattr *xattr_array, void *fs_data);
> 
> +#define __kernel_arg_id(id) \
> +	id(UNKNOWN, unknown)		\
> +	id(FIRMWARE, firmware)		\
> +	id(MODULE, kernel-module)	\
> +	id(MAX_ID, )
> +
> +#define __karg_enumify(ENUM, dummy) KARG_ ## ENUM,
> +#define __karg_stringify(dummy, str) #str,
> +
> +enum kernel_arg_id {
> +	__kernel_arg_id(__karg_enumify)
> +};
> +
> +static const char * const kernel_arg_str[] = {
> +	__kernel_arg_id(__karg_stringify)
> +};
> +
> +static inline const char *kernel_arg_id_str(enum kernel_arg_id id)
> +{
> +	if ((unsigned)id >= KARG_MAX_ID)
> +		return kernel_arg_str[KARG_UNKNOWN];
> +
> +	return kernel_arg_str[id];
> +}
> +
> +
>  #ifdef CONFIG_SECURITY
> 
>  struct security_mnt_opts {
> @@ -326,6 +352,7 @@ void security_cred_getsecid(const struct cred *c, u32 *secid);
>  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(char *kmod_name);
> +int security_kernel_arg(enum kernel_arg_id id);
>  int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
>  int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
>  				   enum kernel_read_file_id id);
> @@ -923,6 +950,11 @@ static inline int security_kernel_module_request(char *kmod_name)
>  	return 0;
>  }
> 
> +static inline int security_kernel_arg(enum kernel_arg_id id)
> +{
> +	return 0;
> +}
> +
>  static inline int security_kernel_read_file(struct file *file,
>  					    enum kernel_read_file_id id)
>  {
> diff --git a/kernel/module.c b/kernel/module.c
> index ce8066b88178..03a1dd21ad4a 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2879,7 +2879,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
>  	if (info->len < sizeof(*(info->hdr)))
>  		return -ENOEXEC;
> 
> -	err = security_kernel_read_file(NULL, READING_MODULE);
> +	err = security_kernel_arg(KARG_MODULE);
>  	if (err)
>  		return err;
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 74d0bd7e76d7..d51a8ca97238 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -421,32 +421,6 @@ void ima_post_path_mknod(struct dentry *dentry)
>  		iint->flags |= IMA_NEW_FILE;
>  }
> 
> -/**
> - * ima_read_file - pre-measure/appraise hook decision based on policy
> - * @file: pointer to the file to be measured/appraised/audit
> - * @read_id: caller identifier
> - *
> - * Permit reading a file based on policy. The policy rules are written
> - * in terms of the policy identifier.  Appraising the integrity of
> - * a file requires a file descriptor.
> - *
> - * For permission return 0, otherwise return -EACCES.
> - */
> -int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> -{
> -	bool sig_enforce = is_module_sig_enforced();
> -
> -	if (!file && read_id == READING_MODULE) {
> -		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
> -		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> -			pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
> -			return -EACCES;	/* INTEGRITY_UNKNOWN */
> -		}
> -		return 0;	/* We rely on module signature checking */
> -	}
> -	return 0;
> -}
> -
>  static int read_idmap[READING_MAX_ID] = {
>  	[READING_FIRMWARE] = FIRMWARE_CHECK,
>  	[READING_MODULE] = MODULE_CHECK,
> @@ -474,21 +448,7 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
>  	enum ima_hooks func;
>  	u32 secid;
> 
> -	if (!file && read_id == READING_FIRMWARE) {
> -		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> -		    (ima_appraise & IMA_APPRAISE_ENFORCE))
> -			return -EACCES;	/* INTEGRITY_UNKNOWN */
> -		return 0;
> -	}
> -
> -	if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
> -		return 0;
> -
> -	/* permit signed certs */
> -	if (!file && read_id == READING_X509_CERTIFICATE)
> -		return 0;
> -
> -	if (!file || !buf || size == 0) { /* should never happen */
> +	if (!buf || size == 0) { /* should never happen */
>  		if (ima_appraise & IMA_APPRAISE_ENFORCE)
>  			return -EACCES;
>  		return 0;
> @@ -500,6 +460,40 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
>  				   MAY_READ, func, 0);
>  }
> 
> +/**
> + * ima_kernel_arg - pre-measure/appraise hook decision based on policy
> + * @id: caller identifier
> + *
> + * Permit using an argument to a syscall based on policy. The policy
> + * rules are written in terms of the policy identifier.
> + *
> + * For permission return 0, otherwise return -EACCES.
> + */
> +int ima_kernel_arg(enum kernel_arg_id id)
> +{
> +	if (id == KARG_MODULE) {
> +		bool sig_enforce = is_module_sig_enforced();
> +		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
> +		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> +			pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
> +			return -EACCES;	/* INTEGRITY_UNKNOWN */
> +		}
> +		return 0;	/* We rely on module signature checking */
> +	}
> +	else if (id == KARG_FIRMWARE) {
> +		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> +		    (ima_appraise & IMA_APPRAISE_ENFORCE))
> +			return -EACCES;	/* INTEGRITY_UNKNOWN */
> +		return 0;
> +	}
> +	else {
> +		if (ima_appraise & IMA_APPRAISE_ENFORCE)
> +			return -EACCES;
> +		return 0;
> +	}
> +	return 0;
> +}
> +
>  static int __init init_ima(void)
>  {
>  	int error;
> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> index 5fa191252c8f..f5333e5abac9 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -121,23 +121,24 @@ static void loadpin_sb_free_security(struct super_block *mnt_sb)
>  	}
>  }
> 
> -static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
> +static int loadpin_read_data(enum kernel_read_data_id id)
>  {
> -	struct super_block *load_root;
> -	const char *origin = kernel_read_file_id_str(id);
> +	const char *origin = kernel_arg_id_str(id);
> 
>  	/* This handles the older init_module API that has a NULL file. */
> -	if (!file) {
> -		if (!enabled) {
> -			report_load(origin, NULL, "old-api-pinning-ignored");
> -			return 0;
> -		}
> -
> -		report_load(origin, NULL, "old-api-denied");
> -		return -EPERM;
> +	if (!enabled) {
> +		report_load(origin, NULL, "old-api-pinning-ignored");
> +		return 0;
>  	}
> 
> -	load_root = file->f_path.mnt->mnt_sb;
> +	report_load(origin, NULL, "old-api-denied");
> +	return -EPERM;
> +}
> +
> +static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
> +{
> +	struct super_block *load_root;
> +	const char *origin = kernel_read_file_id_str(id);
> 
>  	/* First loaded module/firmware defines the root for all others. */
>  	spin_lock(&pinned_root_spinlock);
> @@ -175,6 +176,7 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
> 
>  static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
> +	LSM_HOOK_INIT(kernel_read_data, loadpin_read_data),
>  	LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
>  };
> 
> diff --git a/security/security.c b/security/security.c
> index 7bc2fde023a7..9b5f43c24ee2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1033,14 +1033,19 @@ int security_kernel_module_request(char *kmod_name)
>  	return call_int_hook(kernel_module_request, 0, kmod_name);
>  }
> 
> -int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
> +int security_kernel_arg(enum kernel_arg_id id)
>  {
>  	int ret;
> 
> -	ret = call_int_hook(kernel_read_file, 0, file, id);
> -	if (ret)
> -		return ret;
> -	return ima_read_file(file, id);
> +	ret = call_int_hook(kernel_arg, 0, id);
> +	if (!ret)
> +		ret = ima_kernel_arg(id);
> +	return ret;
> +}
> +
> +int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
> +{
> +	return call_int_hook(kernel_read_file, 0, file, id);
>  }
>  EXPORT_SYMBOL_GPL(security_kernel_read_file);
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 4cafe6a19167..76843099fed6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4010,6 +4010,15 @@ static int selinux_kernel_module_request(char *kmod_name)
>  			    SYSTEM__MODULE_REQUEST, &ad);
>  }
> 
> +static int selinux_kernel_module_arg(void)
> +{
> +	/* init_module */
> +	u32 sid = current_sid();
> +	return avc_has_perm(&selinux_state,
> +			    sid, sid, SECCLASS_SYSTEM,
> +			    SYSTEM__MODULE_LOAD, NULL);
> +}
> +
>  static int selinux_kernel_module_from_file(struct file *file)
>  {
>  	struct common_audit_data ad;
> @@ -4018,12 +4027,6 @@ static int selinux_kernel_module_from_file(struct file *file)
>  	u32 sid = current_sid();
>  	int rc;
> 
> -	/* init_module */
> -	if (file == NULL)
> -		return avc_has_perm(&selinux_state,
> -				    sid, sid, SECCLASS_SYSTEM,
> -					SYSTEM__MODULE_LOAD, NULL);
> -
>  	/* finit_module */
> 
>  	ad.type = LSM_AUDIT_DATA_FILE;
> @@ -4043,6 +4046,20 @@ static int selinux_kernel_module_from_file(struct file *file)
>  				SYSTEM__MODULE_LOAD, &ad);
>  }
> 
> +static int selinux_kernel_arg(enum kernel_arg_id id)
> +{
> +	int rc = 0;
> +
> +	switch (id) {
> +	case KARG_MODULE:
> +		rc = selinux_kernel_module_arg();
> +		break;
> +	default:
> +		break;
> +	}
> +	return rc;
> +}
> +
>  static int selinux_kernel_read_file(struct file *file,
>  				    enum kernel_read_file_id id)
>  {
> @@ -6938,6 +6955,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
>  	LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
>  	LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
> +	LSM_HOOK_INIT(kernel_arg, selinux_kernel_arg),
>  	LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
>  	LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
>  	LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
> 
> --
> 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
> 

--
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 May 25, 2018, 3:41 p.m. UTC | #3 
On Thu, 24 May 2018, Eric W. Biederman wrote:

> Below is where I suggest you start on sorting out these security hooks.
> - Adding a security_kernel_arg to catch when you want to allow/deny the
>   use of an argument to a syscall.  What security_kernel_file_read and
>   security_kernel_file_post_read have been abused for.

NAK. This abstraction is too semantically weak.

LSM hooks need to map to stronger semantics so we can reason about what 
the hook and the policy is supposed to be mediating.
Eric W. Biederman May 25, 2018, 7:51 p.m. UTC | #4 
James Morris <jmorris@namei.org> writes:

> On Thu, 24 May 2018, Eric W. Biederman wrote:
>
>> Below is where I suggest you start on sorting out these security hooks.
>> - Adding a security_kernel_arg to catch when you want to allow/deny the
>>   use of an argument to a syscall.  What security_kernel_file_read and
>>   security_kernel_file_post_read have been abused for.
>
> NAK. This abstraction is too semantically weak.
>
> LSM hooks need to map to stronger semantics so we can reason about what 
> the hook and the policy is supposed to be mediating.

I will take that as an extremely weak nack as all I did was expose the
existing code and what the code is currently doing.  I don't see how you
can NAK what is already being merged and used.

I will be happy to see a better proposal.

The best I can see is to take each and every syscall that my patch
is calling syscall_kernel_arg and make it it's own hook without an
enumeration.  I did not see any real duplication between the cases in my
enumeration so I don't think that will be a problem.  Maybe a bit of a
challenge for loadpin but otherwise not.

Thank you in this for understanding why I am having problems with the
current hook.

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
James Morris May 29, 2018, 8:32 p.m. UTC | #5 
On Fri, 25 May 2018, Eric W. Biederman wrote:

> James Morris <jmorris@namei.org> writes:
> 
> > On Thu, 24 May 2018, Eric W. Biederman wrote:
> >
> >> Below is where I suggest you start on sorting out these security hooks.
> >> - Adding a security_kernel_arg to catch when you want to allow/deny the
> >>   use of an argument to a syscall.  What security_kernel_file_read and
> >>   security_kernel_file_post_read have been abused for.
> >
> > NAK. This abstraction is too semantically weak.
> >
> > LSM hooks need to map to stronger semantics so we can reason about what 
> > the hook and the policy is supposed to be mediating.
> 
> I will take that as an extremely weak nack as all I did was expose the
> existing code and what the code is currently doing.  I don't see how you
> can NAK what is already being merged and used.

It's a strong NAK.

LSM is a logical API, it provides an abstraction layer for security 
policies to mediate kernel security behaviors.

Adding an argument to a syscall is not a security behavior.

Loading a firmware file is.

=
Eric W. Biederman May 29, 2018, 9:10 p.m. UTC | #6 
James Morris <jmorris@namei.org> writes:

> On Fri, 25 May 2018, Eric W. Biederman wrote:
>
>> James Morris <jmorris@namei.org> writes:
>> 
>> > On Thu, 24 May 2018, Eric W. Biederman wrote:
>> >
>> >> Below is where I suggest you start on sorting out these security hooks.
>> >> - Adding a security_kernel_arg to catch when you want to allow/deny the
>> >>   use of an argument to a syscall.  What security_kernel_file_read and
>> >>   security_kernel_file_post_read have been abused for.
>> >
>> > NAK. This abstraction is too semantically weak.
>> >
>> > LSM hooks need to map to stronger semantics so we can reason about what 
>> > the hook and the policy is supposed to be mediating.
>> 
>> I will take that as an extremely weak nack as all I did was expose the
>> existing code and what the code is currently doing.  I don't see how you
>> can NAK what is already being merged and used.
>
> It's a strong NAK.

We are either not understading each other or you have just strong NAK'd
part of the existing LSM api.  Not my proposal.

> LSM is a logical API, it provides an abstraction layer for security 
> policies to mediate kernel security behaviors.

The way it deals with firmware blobs and module loading is not logical.
It is some random pass a NULL pointer into some other security hook.

> Adding an argument to a syscall is not a security behavior.
>
> Loading a firmware file is.

It is a firmware blob not a file.  Perhaps the blob is stored as a file
on-disk, perhaps it is not.

The similar case with kexec never stores all of the data in a file.

Why module_init (which does not take a file) is calling a file based lsm
hook is also bizarre.


Perhaps that means all 3 of these cases should have their own void
security hooks.  Perhaps it means something else.  I just know the name
on the security hook, how it is getting called, and how it is getting
used simply do not agree.

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
diff mbox

Patch

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 358354148dec..04536ff81bd2 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -294,9 +294,7 @@  static ssize_t firmware_loading_store(struct device *dev,
 				dev_err(dev, "%s: map pages failed\n",
 					__func__);
 			else
-				rc = security_kernel_post_read_file(NULL,
-						fw_priv->data, fw_priv->size,
-						READING_FIRMWARE);
+				rc = security_kernel_arg(KARG_FIRMWARE);
 
 			/*
 			 * Same logic as fw_load_abort, only the DONE bit
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 0e4647e0eb60..9fb42736ba29 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -11,6 +11,7 @@ 
 #define _LINUX_IMA_H
 
 #include <linux/fs.h>
+#include <linux/security.h>
 #include <linux/kexec.h>
 struct linux_binprm;
 
@@ -19,7 +20,7 @@  extern int ima_bprm_check(struct linux_binprm *bprm);
 extern int ima_file_check(struct file *file, int mask, int opened);
 extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
-extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
+extern int ima_kernel_arg(enum kernel_arg_id id);
 extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 			      enum kernel_read_file_id id);
 extern void ima_post_path_mknod(struct dentry *dentry);
@@ -49,7 +50,7 @@  static inline int ima_file_mmap(struct file *file, unsigned long prot)
 	return 0;
 }
 
-static inline int ima_read_file(struct file *file, enum kernel_read_file_id id)
+static inline int ima_kernel_arg(enum kernel_arg_id id)
 {
 	return 0;
 }
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 9d0b286f3dba..7f8bc3030784 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -576,6 +576,10 @@ 
  *	userspace to load a kernel module with the given name.
  *	@kmod_name name of the module requested by the kernel
  *	Return 0 if successful.
+ * @kernel_arg:
+ *	Use a syscall argument
+ *	@id kernel argument identifier
+ *	Return 0 if permission is granted.
  * @kernel_read_file:
  *	Read a file specified by userspace.
  *	@file contains the file structure pointing to the file being read
@@ -1577,6 +1581,7 @@  union security_list_options {
 	int (*kernel_act_as)(struct cred *new, u32 secid);
 	int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
 	int (*kernel_module_request)(char *kmod_name);
+	int (*kernel_arg)(enum kernel_arg_id id);
 	int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id);
 	int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
 				     enum kernel_read_file_id id);
@@ -1866,6 +1871,7 @@  struct security_hook_heads {
 	struct hlist_head cred_getsecid;
 	struct hlist_head kernel_act_as;
 	struct hlist_head kernel_create_files_as;
+	struct hlist_head kernel_arg;
 	struct hlist_head kernel_read_file;
 	struct hlist_head kernel_post_read_file;
 	struct hlist_head kernel_module_request;
diff --git a/include/linux/security.h b/include/linux/security.h
index 200920f521a1..6cf1bd87f041 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -159,6 +159,32 @@  extern int mmap_min_addr_handler(struct ctl_table *table, int write,
 typedef int (*initxattrs) (struct inode *inode,
 			   const struct xattr *xattr_array, void *fs_data);
 
+#define __kernel_arg_id(id) \
+	id(UNKNOWN, unknown)		\
+	id(FIRMWARE, firmware)		\
+	id(MODULE, kernel-module)	\
+	id(MAX_ID, )
+
+#define __karg_enumify(ENUM, dummy) KARG_ ## ENUM,
+#define __karg_stringify(dummy, str) #str,
+
+enum kernel_arg_id {
+	__kernel_arg_id(__karg_enumify)
+};
+
+static const char * const kernel_arg_str[] = {
+	__kernel_arg_id(__karg_stringify)
+};
+
+static inline const char *kernel_arg_id_str(enum kernel_arg_id id)
+{
+	if ((unsigned)id >= KARG_MAX_ID)
+		return kernel_arg_str[KARG_UNKNOWN];
+
+	return kernel_arg_str[id];
+}
+
+
 #ifdef CONFIG_SECURITY
 
 struct security_mnt_opts {
@@ -326,6 +352,7 @@  void security_cred_getsecid(const struct cred *c, u32 *secid);
 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(char *kmod_name);
+int security_kernel_arg(enum kernel_arg_id id);
 int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
 int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
 				   enum kernel_read_file_id id);
@@ -923,6 +950,11 @@  static inline int security_kernel_module_request(char *kmod_name)
 	return 0;
 }
 
+static inline int security_kernel_arg(enum kernel_arg_id id)
+{
+	return 0;
+}
+
 static inline int security_kernel_read_file(struct file *file,
 					    enum kernel_read_file_id id)
 {
diff --git a/kernel/module.c b/kernel/module.c
index ce8066b88178..03a1dd21ad4a 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2879,7 +2879,7 @@  static int copy_module_from_user(const void __user *umod, unsigned long len,
 	if (info->len < sizeof(*(info->hdr)))
 		return -ENOEXEC;
 
-	err = security_kernel_read_file(NULL, READING_MODULE);
+	err = security_kernel_arg(KARG_MODULE);
 	if (err)
 		return err;
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 74d0bd7e76d7..d51a8ca97238 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -421,32 +421,6 @@  void ima_post_path_mknod(struct dentry *dentry)
 		iint->flags |= IMA_NEW_FILE;
 }
 
-/**
- * ima_read_file - pre-measure/appraise hook decision based on policy
- * @file: pointer to the file to be measured/appraised/audit
- * @read_id: caller identifier
- *
- * Permit reading a file based on policy. The policy rules are written
- * in terms of the policy identifier.  Appraising the integrity of
- * a file requires a file descriptor.
- *
- * For permission return 0, otherwise return -EACCES.
- */
-int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
-{
-	bool sig_enforce = is_module_sig_enforced();
-
-	if (!file && read_id == READING_MODULE) {
-		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
-		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
-			pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
-			return -EACCES;	/* INTEGRITY_UNKNOWN */
-		}
-		return 0;	/* We rely on module signature checking */
-	}
-	return 0;
-}
-
 static int read_idmap[READING_MAX_ID] = {
 	[READING_FIRMWARE] = FIRMWARE_CHECK,
 	[READING_MODULE] = MODULE_CHECK,
@@ -474,21 +448,7 @@  int ima_post_read_file(struct file *file, void *buf, loff_t size,
 	enum ima_hooks func;
 	u32 secid;
 
-	if (!file && read_id == READING_FIRMWARE) {
-		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
-		    (ima_appraise & IMA_APPRAISE_ENFORCE))
-			return -EACCES;	/* INTEGRITY_UNKNOWN */
-		return 0;
-	}
-
-	if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
-		return 0;
-
-	/* permit signed certs */
-	if (!file && read_id == READING_X509_CERTIFICATE)
-		return 0;
-
-	if (!file || !buf || size == 0) { /* should never happen */
+	if (!buf || size == 0) { /* should never happen */
 		if (ima_appraise & IMA_APPRAISE_ENFORCE)
 			return -EACCES;
 		return 0;
@@ -500,6 +460,40 @@  int ima_post_read_file(struct file *file, void *buf, loff_t size,
 				   MAY_READ, func, 0);
 }
 
+/**
+ * ima_kernel_arg - pre-measure/appraise hook decision based on policy
+ * @id: caller identifier
+ *
+ * Permit using an argument to a syscall based on policy. The policy
+ * rules are written in terms of the policy identifier.
+ *
+ * For permission return 0, otherwise return -EACCES.
+ */
+int ima_kernel_arg(enum kernel_arg_id id)
+{
+	if (id == KARG_MODULE) {
+		bool sig_enforce = is_module_sig_enforced();
+		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
+		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+			pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
+			return -EACCES;	/* INTEGRITY_UNKNOWN */
+		}
+		return 0;	/* We rely on module signature checking */
+	}
+	else if (id == KARG_FIRMWARE) {
+		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+		    (ima_appraise & IMA_APPRAISE_ENFORCE))
+			return -EACCES;	/* INTEGRITY_UNKNOWN */
+		return 0;
+	}
+	else {
+		if (ima_appraise & IMA_APPRAISE_ENFORCE)
+			return -EACCES;
+		return 0;
+	}
+	return 0;
+}
+
 static int __init init_ima(void)
 {
 	int error;
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 5fa191252c8f..f5333e5abac9 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -121,23 +121,24 @@  static void loadpin_sb_free_security(struct super_block *mnt_sb)
 	}
 }
 
-static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
+static int loadpin_read_data(enum kernel_read_data_id id)
 {
-	struct super_block *load_root;
-	const char *origin = kernel_read_file_id_str(id);
+	const char *origin = kernel_arg_id_str(id);
 
 	/* This handles the older init_module API that has a NULL file. */
-	if (!file) {
-		if (!enabled) {
-			report_load(origin, NULL, "old-api-pinning-ignored");
-			return 0;
-		}
-
-		report_load(origin, NULL, "old-api-denied");
-		return -EPERM;
+	if (!enabled) {
+		report_load(origin, NULL, "old-api-pinning-ignored");
+		return 0;
 	}
 
-	load_root = file->f_path.mnt->mnt_sb;
+	report_load(origin, NULL, "old-api-denied");
+	return -EPERM;
+}
+
+static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
+{
+	struct super_block *load_root;
+	const char *origin = kernel_read_file_id_str(id);
 
 	/* First loaded module/firmware defines the root for all others. */
 	spin_lock(&pinned_root_spinlock);
@@ -175,6 +176,7 @@  static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
 
 static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
+	LSM_HOOK_INIT(kernel_read_data, loadpin_read_data),
 	LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
 };
 
diff --git a/security/security.c b/security/security.c
index 7bc2fde023a7..9b5f43c24ee2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1033,14 +1033,19 @@  int security_kernel_module_request(char *kmod_name)
 	return call_int_hook(kernel_module_request, 0, kmod_name);
 }
 
-int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
+int security_kernel_arg(enum kernel_arg_id id)
 {
 	int ret;
 
-	ret = call_int_hook(kernel_read_file, 0, file, id);
-	if (ret)
-		return ret;
-	return ima_read_file(file, id);
+	ret = call_int_hook(kernel_arg, 0, id);
+	if (!ret)
+		ret = ima_kernel_arg(id);
+	return ret;
+}
+
+int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
+{
+	return call_int_hook(kernel_read_file, 0, file, id);
 }
 EXPORT_SYMBOL_GPL(security_kernel_read_file);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4cafe6a19167..76843099fed6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4010,6 +4010,15 @@  static int selinux_kernel_module_request(char *kmod_name)
 			    SYSTEM__MODULE_REQUEST, &ad);
 }
 
+static int selinux_kernel_module_arg(void)
+{
+	/* init_module */
+	u32 sid = current_sid();
+	return avc_has_perm(&selinux_state,
+			    sid, sid, SECCLASS_SYSTEM,
+			    SYSTEM__MODULE_LOAD, NULL);
+}
+
 static int selinux_kernel_module_from_file(struct file *file)
 {
 	struct common_audit_data ad;
@@ -4018,12 +4027,6 @@  static int selinux_kernel_module_from_file(struct file *file)
 	u32 sid = current_sid();
 	int rc;
 
-	/* init_module */
-	if (file == NULL)
-		return avc_has_perm(&selinux_state,
-				    sid, sid, SECCLASS_SYSTEM,
-					SYSTEM__MODULE_LOAD, NULL);
-
 	/* finit_module */
 
 	ad.type = LSM_AUDIT_DATA_FILE;
@@ -4043,6 +4046,20 @@  static int selinux_kernel_module_from_file(struct file *file)
 				SYSTEM__MODULE_LOAD, &ad);
 }
 
+static int selinux_kernel_arg(enum kernel_arg_id id)
+{
+	int rc = 0;
+
+	switch (id) {
+	case KARG_MODULE:
+		rc = selinux_kernel_module_arg();
+		break;
+	default:
+		break;
+	}
+	return rc;
+}
+
 static int selinux_kernel_read_file(struct file *file,
 				    enum kernel_read_file_id id)
 {
@@ -6938,6 +6955,7 @@  static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
 	LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
 	LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
+	LSM_HOOK_INIT(kernel_arg, selinux_kernel_arg),
 	LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
 	LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
 	LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),