linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mimi Zohar <zohar@linux.ibm.com>
To: THOBY Simon <Simon.THOBY@viveris.fr>,
	"dmitry.kasatkin@gmail.com" <dmitry.kasatkin@gmail.com>,
	"linux-integrity@vger.kernel.org"
	<linux-integrity@vger.kernel.org>,
	BARVAUX Didier <Didier.BARVAUX@viveris.fr>
Subject: Re: [PATCH v3 3/4] IMA: add a policy option to restrict xattr hash algorithms on appraisal
Date: Tue, 27 Jul 2021 12:24:16 -0400	[thread overview]
Message-ID: <bd785a9d0c029cec34514d306e110bdef945c13e.camel@linux.ibm.com> (raw)
In-Reply-To: <20210727102307.552052-4-simon.thoby@viveris.fr>

Hi Simon,

On Tue, 2021-07-27 at 10:23 +0000, THOBY Simon wrote:
> This patch defines a new IMA policy rule option "appraise_hash=",
> that restricts the hash algorithms accepted for the extended attribute
> security.ima when appraising.

"Define ..."

> When a policy rule uses the 'appraise_hash' option, appraisal of a
> file referenced by that rule will now fail if the digest algorithm
> employed to hash the file was not one of those explicitly listed
> in the option. In its absence, any hash algorithm compiled in the
> kernel will be accepted.
> 
> For example, on a system where SELinux is properly deployed, the rule
>   appraise func=BPRM_CHECK obj_type=iptables_exec_t appraise_hash=sha256,sha384
> will block the execution of iptables if the xattr security.ima of its
> executables were not hashed with either sha256 or sha384.
> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> ---
>  Documentation/ABI/testing/ima_policy |  6 ++-
>  security/integrity/ima/ima_policy.c  | 72 ++++++++++++++++++++++++++--
>  2 files changed, 74 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index 070779e8d836..365e4c91719e 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -27,7 +27,7 @@ Description:
>  			lsm:	[[subj_user=] [subj_role=] [subj_type=]
>  				 [obj_user=] [obj_role=] [obj_type=]]
>  			option:	[[appraise_type=]] [template=] [permit_directio]
> -				[appraise_flag=] [keyrings=]
> +				[appraise_flag=] [keyrings=] [appraise_hash=]

Nit:
Probably nicer to keep the "appraise_" options together.  How about
placing it after "appraise_flag", instead of at the end.

>  		  base:
>  			func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
>  			        [FIRMWARE_CHECK]
> @@ -55,6 +55,10 @@ Description:
>  			label:= [selinux]|[kernel_info]|[data_label]
>  			data_label:= a unique string used for grouping and limiting critical data.
>  			For example, "selinux" to measure critical data for SELinux.
> +			appraise_hash:= comma-separated list of hash algorithms
> +			For example, "sha256,sha512" to only accept to appraise
> +			files where the security.ima xattr was hashed with one
> +			of these two algorithms.
>  
>  		  default policy:
>  			# PROC_SUPER_MAGIC
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 344b5b0dc1a1..a7f110cbbff0 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -92,6 +92,7 @@ struct ima_rule_entry {
>  	struct ima_template_desc *template;
>  };
>  
> +

Is this extra blank line intentional?

>  /*
>   * sanity check in case the kernels gains more hash algorithms that can
>   * fit in an unsigned int
> @@ -962,7 +963,7 @@ enum {
>  	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
>  	Opt_appraise_type, Opt_appraise_flag,
>  	Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
> -	Opt_label, Opt_err
> +	Opt_label, Opt_appraise_hash, Opt_err

Nit: move Opt_appraise_hash after Opt_appraise_type.
>  };
>  
>  static const match_table_t policy_tokens = {
> @@ -1000,6 +1001,7 @@ static const match_table_t policy_tokens = {
>  	{Opt_template, "template=%s"},
>  	{Opt_keyrings, "keyrings=%s"},
>  	{Opt_label, "label=%s"},
> +	{Opt_appraise_hash, "appraise_hash=%s"},

ditto

>  	{Opt_err, NULL}
>  };
>  
> @@ -1125,7 +1127,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>  				     IMA_UID | IMA_FOWNER | IMA_FSUUID |
>  				     IMA_INMASK | IMA_EUID | IMA_PCR |
>  				     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
> -				     IMA_PERMIT_DIRECTIO))
> +				     IMA_PERMIT_DIRECTIO | IMA_VALIDATE_HASH))
>  			return false;
>  
>  		break;
> @@ -1137,7 +1139,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>  				     IMA_INMASK | IMA_EUID | IMA_PCR |
>  				     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
>  				     IMA_PERMIT_DIRECTIO | IMA_MODSIG_ALLOWED |
> -				     IMA_CHECK_BLACKLIST))
> +				     IMA_CHECK_BLACKLIST | IMA_VALIDATE_HASH))
>  			return false;
>  
>  		break;
> @@ -1187,6 +1189,27 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>  	return true;
>  }
>  
> +static unsigned int ima_parse_appraise_hash(char *arg)
> +{
> +	unsigned int res = 0;
> +	char *token;
> +
> +	while ((token = strsep(&arg, ",")) != NULL) {
> +		int idx = match_string(hash_algo_name, HASH_ALGO__LAST, token);

Move the variable definition to the beginning of the function.
> +
> +		if (idx < 0) {
> +			pr_err("unknown hash algorithm \"%s\", ignoring",
> +			       token);
> +			continue;
> +		}
> +
> +		/* Add the hash algorithm to the 'allowed' bitfield */
> +		res |= (1U << idx);
> +	}
> +
> +	return res;
> +}
> +
>  static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  {
>  	struct audit_buffer *ab;
> @@ -1204,6 +1227,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  	entry->uid_op = &uid_eq;
>  	entry->fowner_op = &uid_eq;
>  	entry->action = UNKNOWN;
> +	entry->allowed_hashes = 0;
>  	while ((p = strsep(&rule, " \t")) != NULL) {
>  		substring_t args[MAX_OPT_ARGS];
>  		int token;
> @@ -1556,6 +1580,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  						 &(template_desc->fields),
>  						 &(template_desc->num_fields));
>  			entry->template = template_desc;
> +			break;
> +		case Opt_appraise_hash:

ditto

"appraise_hash=" should be limited to appraise rules.  Please update
ima_validate_rule().

thanks,

Mimi

> +			ima_log_string(ab, "appraise_hash", args[0].from);
> +
> +			if (entry->allowed_hashes) {
> +				result = -EINVAL;
> +				break;
> +			}
> +
> +			entry->allowed_hashes = ima_parse_appraise_hash(args[0].from);
> +			if (!entry->allowed_hashes) {
> +				result = -EINVAL;
> +				break;
> +			}
> +
> +			entry->flags |= IMA_VALIDATE_HASH;
> +
>  			break;
>  		case Opt_err:
>  			ima_log_string(ab, "UNKNOWN", p);
> 
<trim>


  reply	other threads:[~2021-07-27 16:24 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-27 10:23 [PATCH v3 0/4] IMA: restrict the accepted digest algorithms for the security.ima xattr THOBY Simon
2021-07-27 10:23 ` [PATCH v3 1/4] IMA: block writes of the security.ima xattr with unsupported algorithms THOBY Simon
2021-07-27 14:04   ` Mimi Zohar
2021-07-27 10:23 ` [PATCH v3 2/4] IMA: add support to restrict the hash algorithms used for file appraisal THOBY Simon
2021-07-27 15:29   ` Mimi Zohar
2021-07-27 10:23 ` [PATCH v3 3/4] IMA: add a policy option to restrict xattr hash algorithms on appraisal THOBY Simon
2021-07-27 16:24   ` Mimi Zohar [this message]
2021-07-27 10:23 ` [PATCH v3 4/4] IMA: introduce a new policy option func=SETXATTR_CHECK THOBY Simon
2021-07-27 11:24   ` THOBY Simon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bd785a9d0c029cec34514d306e110bdef945c13e.camel@linux.ibm.com \
    --to=zohar@linux.ibm.com \
    --cc=Didier.BARVAUX@viveris.fr \
    --cc=Simon.THOBY@viveris.fr \
    --cc=dmitry.kasatkin@gmail.com \
    --cc=linux-integrity@vger.kernel.org \
    --subject='Re: [PATCH v3 3/4] IMA: add a policy option to restrict xattr hash algorithms on appraisal' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).