[PATCH v2 0/3] IMA: restrict the accepted digest algorithms

[PATCH v2 0/3] IMA: restrict the accepted digest algorithms

[THOBY Simon]

IMA protects files by storing a hash (or a signature thereof) of their
content in the security.ima xattr. While the security.ima xattr itself
is protected by EVM with either a HMAC or a digital signature, no
mechanism is currently in place to ensure that the security.ima xattr
was generated with a strong digest algorithm, as was outlined in
https://lore.kernel.org/linux-integrity/10dde047d76b447f32ca91356599be679b8a76e5.camel@linux.ibm.com/t/#m0f8127c6982ef94aa42f5cc13ea83b9f9000917e

One important point is safeguarding users from mislabelling their
files when using userland utilities to update their files, as this
is the kind of behavior one can observe with evmctl (`evmctl ima_hash`
defaults to sha1). Another group that may be interested is those
that have deployed IMA years ago, possibly using algorithms that
was then deemed sufficiently collision-resistant, but that proved
to be weak with the passage of time (note that this could also
happen in the future with algorithms considered safe today).
This patch provides a migration path of sorts for these users.

This patch series gives users the ability to restrict the algorithms
accepted by their system, both when writing/updating xattrs, and
when appraising files, while retaining a permissive behavior by default
to preserve backward compatibility.

To provide these features, alter the behavior of setxattr to
only accept hashes built in the kernel, instead of any hash listed
in the kernel (complete list crypto/hash_info.c). In addition, if
secure boot is enabled, assume the system settings must be enforced
(as IMA already does for the ima_appraise boot parameter) and only
accept writes that uses the same digest the kernel uses (the
'ima_hash' kernel parameter).
In addition, users may opt-in to whitelisting the accepted hash
algorithms with the new "appraise_hash" IMA policy option.
By default IMA will keep accepting any hash algorithm, but specifying
that option will make appraisal of files hash with another algorithm
fail.


Even when using this option to restrict accepted hashes, migrations
to a new algorithm are still possible. Suppose your policy states you
must migrate from 'old_algo' (e.g. sha1) to 'new_algo' (e.g. one of
sha256/384/512). You can upgrade without relaxing the hash requirements:
alter your policy rules from 'appraise_hash=old_algo' to
'appraise_hash=old_algo,new_algo', update the "ima_hash" parameter to
'new_algo', reboot, relabel all your files with 'new_algo', alter your
policy_rule from 'appraise_hash=old_algo,new_algo' to
'appraise_hash=new_algo', reboot again and you're done.
Agreed, it's quite a lot of churn - I don't know if this can be reduced -
but this is technically doable.


Note that the secure boot restriction is not backward compatible, so
maybe this cannot be merged as-is. Yet this only applies to new
hash/signatures performed from userspace, and does not impact the
appraisal of existing files, so this will probably not break systems on
upgrade either.

This series is based on the following repo/branch:
 repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
 branch: master
 commit 2734d6c1b1a089fb593ef6a23d4b70903526fe0c ("Linux 5.14-rc2")

Changelog since v1:
- Remove the two boot parameters
- filter out hash algorithms nto compiled in the kernel
  on xattr writes
- add a special case when secure boot is enabled: only the
  ima_hash algorithm is accepted on userland writes
- add a policy option to opt-in to restricting digest algorithms
  at a per-rule granularity


Simon Thoby (3):
  IMA: block writes of the security.ima xattr with weak hash algorithms
  IMA: add policy support for restricting the accepted hash algorithms
  IMA: honor the appraise_hash policy option

 Documentation/ABI/testing/ima_policy  |  6 +-
 security/integrity/ima/ima.h          |  6 +-
 security/integrity/ima/ima_api.c      |  6 +-
 security/integrity/ima/ima_appraise.c | 59 +++++++++++++++++-
 security/integrity/ima/ima_main.c     | 22 ++++++-
 security/integrity/ima/ima_policy.c   | 90 +++++++++++++++++++++++++--
 6 files changed, 173 insertions(+), 16 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] IMA: block writes of the security.ima xattr with weak hash algorithms

[THOBY Simon]

By default, writes of the extended attributes security.ima will be
forbidden if the xattr value uses a hash algorithm not compiled in the
kernel. Disabling weak hashes when building the kernel will thus prevent
their use in IMA (these hashes will not only be blocked for setxattr,
but they won't be allowed for measurement/appraisal either as the kernel
will obviously not be able to measure files hashed with them). Note
however that CONFIG_IMA depends on CONFIG_CRYPTO_MD5 and
CONFIG_CRYPTO_SHA1, so this limits the security benefits of this
measure.
To bypass that limitation, if secure boot is enabled on the system,
the allowed algorithms are further restricted: only writes performed
with the algorithm specified in the ima_hash parameter (defined at
build-time with CONFIG_IMA_DEFAULT_HASH or overwritten with a boot
cmdline option) are allowed.

Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
---
 security/integrity/ima/ima_appraise.c | 54 +++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ef9dcfce45d4..e9a24acf25c6 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -575,6 +575,54 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
 		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
 }
 
+/**
+ * ima_setxattr_validate_hash_alg
+ *
+ * Called when the user tries to write the security.ima xattr.
+ * The xattr value maps to the hash algorithm hash_alg, and this function
+ * returns whether this setxattr should be allowed, emitting an audit
+ * message if necessary.
+ */
+int ima_setxattr_validate_hash_alg(struct dentry *dentry,
+				   const void *xattr_value,
+				   size_t xattr_value_len)
+{
+	int res = -EACCES;
+	char *path = NULL, *pathbuf = NULL;
+	enum hash_algo hash_alg =
+		ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
+				  xattr_value_len);
+
+	/**
+	 * When secure boot is enabled, only accept the IMA xattr if
+	 * hashed with the same algorithm as defined in the ima_hash
+	 * parameter (just like the 'ima_appraise' cmdline parameter
+	 * is ignored if secure boot is enabled)
+	 */
+	if (arch_ima_get_secureboot() && hash_alg != ima_hash_algo)
+		goto out_warn;
+
+	/* disallow xattr writes with algorithms not built in the kernel */
+	if (hash_alg != ima_hash_algo
+	    && !crypto_has_alg(hash_algo_name[hash_alg], 0, 0))
+		goto out_warn;
+
+	return 0;
+
+out_warn:
+	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
+	/* no memory available ? no file path for you */
+	if (pathbuf)
+		path = dentry_path(dentry, pathbuf, PATH_MAX);
+
+	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
+		path, "collect_data", "forbidden-hash-algorithm", res, 0);
+
+	kfree(pathbuf);
+
+	return res;
+}
+
 int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		       const void *xattr_value, size_t xattr_value_len)
 {
@@ -592,6 +640,12 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		digsig = (xvalue->type == EVM_XATTR_PORTABLE_DIGSIG);
 	}
 	if (result == 1 || evm_revalidate_status(xattr_name)) {
+		/* the user-supplied xattr must use an allowed hash algo */
+		int rc = ima_setxattr_validate_hash_alg(dentry, xattr_value,
+							xattr_value_len);
+		if (rc != 0)
+			return rc;
+
 		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
 		if (result == 1)
 			result = 0;
-- 
2.31.1

Re: [PATCH v2 1/3] IMA: block writes of the security.ima xattr with weak hash algorithms

[Mimi Zohar]

Hi Simon,

On Tue, 2021-07-20 at 09:25 +0000, THOBY Simon wrote:
> By default, writes of the extended attributes security.ima will be
> forbidden if the xattr value uses a hash algorithm not compiled in the
> kernel. Disabling weak hashes when building the kernel will thus prevent
> their use in IMA (these hashes will not only be blocked for setxattr,
> but they won't be allowed for measurement/appraisal either as the kernel
> will obviously not be able to measure files hashed with them). Note
> however that CONFIG_IMA depends on CONFIG_CRYPTO_MD5 and
> CONFIG_CRYPTO_SHA1, so this limits the security benefits of this
> measure.
> To bypass that limitation, if secure boot is enabled on the system,
> the allowed algorithms are further restricted: only writes performed
> with the algorithm specified in the ima_hash parameter (defined at
> build-time with CONFIG_IMA_DEFAULT_HASH or overwritten with a boot
> cmdline option) are allowed.

Although the intention of this patch is to prevent writing file
signatures based on weak hash algorithms, there are two logical
changes.  Each should be a separate patch.  For example, one patch
would only allow writing security.ima signatures based on configured
hash algorithms, while the other patch would limit writing security.ima
signatures based on the IMA default hash algorithm.

Basing the decision on whether to limit the security.ima signature to
the IMA default hash algorithm based on the secure boot flag, seems
rather arbitrary.   Instead perhaps base it on whether the IMA policy
contains any new policy rule "appraise_hash" options.  A policy without
the new "appraise_hash" option would permit both writing and verifying
signatures based on any configured hash algorithm.   A policy
containing "appraise_hash", would both limit the hash algorithms
writing the security.ima signatures and verifying them.

A new builtin policy could be defined based on the new "appraise_hash"
option or simply a flag (e.g. ima_policy=).


> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> ---
>  security/integrity/ima/ima_appraise.c | 54 +++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index ef9dcfce45d4..e9a24acf25c6 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -575,6 +575,54 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
>  		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
>  }
>  
> +/**
> + * ima_setxattr_validate_hash_alg
> + *
> + * Called when the user tries to write the security.ima xattr.
> + * The xattr value maps to the hash algorithm hash_alg, and this function
> + * returns whether this setxattr should be allowed, emitting an audit
> + * message if necessary.
> + */
> +int ima_setxattr_validate_hash_alg(struct dentry *dentry,
> +				   const void *xattr_value,
> +				   size_t xattr_value_len)
> +{
> +	int res = -EACCES;
> +	char *path = NULL, *pathbuf = NULL;
> +	enum hash_algo hash_alg =
> +		ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
> +				  xattr_value_len);
> +
> +	/**

"/**" is used to indicate the start of kernel-doc comments.  Please use
the normal "/*"
comment here.

> +	 * When secure boot is enabled, only accept the IMA xattr if
> +	 * hashed with the same algorithm as defined in the ima_hash
> +	 * parameter (just like the 'ima_appraise' cmdline parameter
> +	 * is ignored if secure boot is enabled)
> +	 */
> +	if (arch_ima_get_secureboot() && hash_alg != ima_hash_algo)
> +		goto out_warn;
> +
> +	/* disallow xattr writes with algorithms not built in the kernel */
> +	if (hash_alg != ima_hash_algo
> +	    && !crypto_has_alg(hash_algo_name[hash_alg], 0, 0))
> +		goto out_warn;
> +
> +	return 0;
> +
> +out_warn:
> +	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> +	/* no memory available ? no file path for you */
> +	if (pathbuf)
> +		path = dentry_path(dentry, pathbuf, PATH_MAX);
> +
> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
> +		path, "collect_data", "forbidden-hash-algorithm", res, 0);

This function is writing security xattrs, not collecting/calculating
the file hash.  Please update the audit message.  Instead of
"forbidden", perhaps use something a little less dramatic, like
"unsupported" or even "denied".

thanks,

Mimi

> +
> +	kfree(pathbuf);
> +
> +	return res;
> +}
> +
>  int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>  		       const void *xattr_value, size_t xattr_value_len)
>  {
> @@ -592,6 +640,12 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>  		digsig = (xvalue->type == EVM_XATTR_PORTABLE_DIGSIG);
>  	}
>  	if (result == 1 || evm_revalidate_status(xattr_name)) {
> +		/* the user-supplied xattr must use an allowed hash algo */
> +		int rc = ima_setxattr_validate_hash_alg(dentry, xattr_value,
> +							xattr_value_len);
> +		if (rc != 0)
> +			return rc;
> +
>  		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
>  		if (result == 1)
>  			result = 0;

Re: [PATCH v2 1/3] IMA: block writes of the security.ima xattr with weak hash algorithms

[THOBY Simon]

Hello Mimi,

On 7/23/21 1:46 PM, Mimi Zohar wrote:
> Hi Simon,
> 
> On Tue, 2021-07-20 at 09:25 +0000, THOBY Simon wrote:
>> By default, writes of the extended attributes security.ima will be
>> forbidden if the xattr value uses a hash algorithm not compiled in the
>> kernel. Disabling weak hashes when building the kernel will thus prevent
>> their use in IMA (these hashes will not only be blocked for setxattr,
>> but they won't be allowed for measurement/appraisal either as the kernel
>> will obviously not be able to measure files hashed with them). Note
>> however that CONFIG_IMA depends on CONFIG_CRYPTO_MD5 and
>> CONFIG_CRYPTO_SHA1, so this limits the security benefits of this
>> measure.
>> To bypass that limitation, if secure boot is enabled on the system,
>> the allowed algorithms are further restricted: only writes performed
>> with the algorithm specified in the ima_hash parameter (defined at
>> build-time with CONFIG_IMA_DEFAULT_HASH or overwritten with a boot
>> cmdline option) are allowed.
> 
> Although the intention of this patch is to prevent writing file
> signatures based on weak hash algorithms, there are two logical
> changes.  Each should be a separate patch.  For example, one patch
> would only allow writing security.ima signatures based on configured
> hash algorithms, while the other patch would limit writing security.ima
> signatures based on the IMA default hash algorithm.
> 

Will do, this will be fixed in the next iteration of this patchset.

> Basing the decision on whether to limit the security.ima signature to
> the IMA default hash algorithm based on the secure boot flag, seems
> rather arbitrary.   Instead perhaps base it on whether the IMA policy
> contains any new policy rule "appraise_hash" options.  A policy without
> the new "appraise_hash" option would permit both writing and verifying
> signatures based on any configured hash algorithm.   A policy
> containing "appraise_hash", would both limit the hash algorithms
> writing the security.ima signatures and verifying them.
> 
> A new builtin policy could be defined based on the new "appraise_hash"
> option or simply a flag (e.g. ima_policy=).

I have started to take a look at what I might do in that regard. I think your
idea to filter writes with the ima policy is definitely better than my secure
boot "hack". However I still wonder the form this might take to be correct.

IMHO we cannot simply consider whether there is one rule in the policy that uses the
'appraise_hash' option, and apply that hash algorithm policy everywhere: we do not
want to constrain files that rule doesn't impact.
e.g. if a rule constrains every file owned by root to be valid only if the IMA
signature was generated with sha256, another user shouldn't be constrained by that
rule. Consider this policy:
appraise func=MODULE_CHECK appraise_hash=sha256
appraise func=BPRM_CHECK fowner=0

Here we do not want to constrain xattr writes to arbitrary files because we want
more insurances on the the kernel modules.
This would be a behavior hard to understand for users, and probably lead to
unexpected system breakage if someone were to upgrade their ima policy and change the
'appraise_hash' value, because it would apply to files that the user didn't expect
to be impacted.

For this reason, I believe there must be a way for the setxattr hook to determine if a
file should be affected by the hash policy or not.

At first I thought about using 'ima_get_action' in the 'ima_inode_setxattr' hook
to extract the rule that matches the file, verify if there is a list of allowed
hash algorithms in that rule and apply the hash restriction to the xattr being
written.
But then I hit a significant setback: as I understand it, IMA cannot
detect if a given rule apply to a file *outside* of trying to executing that rule.
Let me explain what I mean with an example. Let us suppose we have the following
ima policy:
appraise func=BPRM_CHECK euid=0 appraise_hash=sha1,sha256 # (1)
appraise func=FILE_MMAP fowner=0 mask=MAY_EXEC appraise_hash=sha256,sha512 # (3)
appraise func=FILE_MMAP euid=0 mask=MAY_EXEC appraise_hash=sha384  # (3)

(I agree that such a diversity of hashes is quite implausible on a single system
in practice, but I also think it best to try to think of degenerate usecases
before implementing that feature, as users will tend to rely on them)

When a user try to update the ima hash (or ima signature) of a file, how can we
know the hash algorithms that the user can use ? How do we know if the users uses 
a rule or another, and thus the algorithm that should apply ?
There is no one-to-one mapping between files and rules in IMA (I understand that is not
at all the philosophy of IMA), so the answer is "We cannot".
Worse, two rules could both apply to the same file (e.g. he could both mmap the dynamic
loader and run it directly, so rules (1) and (2) would both apply.
Except they do not use the same appraise_hash parameter!
So the step "extract the rule that matches a file" is not possible, and I need to get
back to the drawing board.

Technically, we could try every possible combination of mask/func to determine which
would apply to the file whose xattr is being updated, but that would be absolutely
terrible performance wise, and it would still have bad semantics:
- either we would choose the first rule that match, and in that case the order of the
 policy (and the order of our exhaustive search) would impact the resulting algorithms 
 allowed;
- or we could consider the intersection of hash algorithms allowed in each rule
 (it might be null) or their union (it might be overly broad and we might choose
 an algorithm not part of the intersection, thus the will will not be usable in
 some situations).

In short, I believe both situations would be a nightmare, for user experience,
performance, maintainability and probable the sanity of maintainers/code reviewers.

I think one possible way of getting out of this conundrum would be to extend the ima
policy further by adding a new value for the 'func' policy option (something like
WRITE_XATTR_HASH maybe ?). In that mode, the 'mask' option would have no effect, the
appraise_hash parameter would be mandatory, and any file matching this policy would
have the corresponding 'appraise_hash' policy enforced.
This might give policy rules of the following sort:
appraise func=BPRM_CHECK euid=0 appraise_hash=sha1,sha256
appraise func=FILE_MMAP fowner=0 mask=MAY_EXEC appraise_hash=sha256,sha512
appraise func=FILE_MMAP euid=0 mask=MAY_EXEC appraise_hash=sha384
appraise func=WRITE_XATTR_HASH fowner=0 obj_type=bin_t appraise_hash=sha256

The first three rules would just impact executions/mmap()s, and the last one
would restrict xattr writes.

I agree that would add quite a bit of complexity (and a performance hit to check
if a IMA policy matches) to the setxattr hook, that I don't see yet another way out
of this issue.

Please let me know what you think, I certainly would prefer it if someone came up
with a much simpler option that I could then implement.


[snip]

>> +
>> +	/**
> 
> "/**" is used to indicate the start of kernel-doc comments.  Please use
> the normal "/*"
> comment here.
> 

Noted, thanks.

[snip]>> +		path = dentry_path(dentry, pathbuf, PATH_MAX);
>> +
>> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
>> +		path, "collect_data", "forbidden-hash-algorithm", res, 0);
> 
> This function is writing security xattrs, not collecting/calculating
> the file hash.  Please update the audit message.  Instead of
> "forbidden", perhaps use something a little less dramatic, like
> "unsupported" or even "denied".
> 

Noted.

Thanks,
I hope my explanations weren't too confused,
Simon

Re: [PATCH v2 1/3] IMA: block writes of the security.ima xattr with weak hash algorithms

[Mimi Zohar]

Hi Simon,

On Mon, 2021-07-26 at 09:49 +0000, THOBY Simon wrote:

<snip>
 
> > A new builtin policy could be defined based on the new "appraise_hash"
> > option or simply a flag (e.g. ima_policy=).
> 
> I have started to take a look at what I might do in that regard. I think your
> idea to filter writes with the ima policy is definitely better than my secure
> boot "hack". However I still wonder the form this might take to be correct.
> 
> IMHO we cannot simply consider whether there is one rule in the policy that uses the
> 'appraise_hash' option, and apply that hash algorithm policy everywhere: we do not
> want to constrain files that rule doesn't impact.
> e.g. if a rule constrains every file owned by root to be valid only if the IMA
> signature was generated with sha256, another user shouldn't be constrained by that
> rule. Consider this policy:
> appraise func=MODULE_CHECK appraise_hash=sha256
> appraise func=BPRM_CHECK fowner=0
> 
> Here we do not want to constrain xattr writes to arbitrary files because we want
> more insurances on the the kernel modules.
> This would be a behavior hard to understand for users, and probably lead to
> unexpected system breakage if someone were to upgrade their ima policy and change the
> 'appraise_hash' value, because it would apply to files that the user didn't expect
> to be impacted.
> 
> For this reason, I believe there must be a way for the setxattr hook to determine if a
> file should be affected by the hash policy or not.
> 
> At first I thought about using 'ima_get_action' in the 'ima_inode_setxattr' hook
> to extract the rule that matches the file, verify if there is a list of allowed
> hash algorithms in that rule and apply the hash restriction to the xattr being
> written.
> But then I hit a significant setback: as I understand it, IMA cannot
> detect if a given rule apply to a file *outside* of trying to executing that rule.
> Let me explain what I mean with an example. Let us suppose we have the following
> ima policy:
> appraise func=BPRM_CHECK euid=0 appraise_hash=sha1,sha256 # (1)
> appraise func=FILE_MMAP fowner=0 mask=MAY_EXEC appraise_hash=sha256,sha512 # (3)
> appraise func=FILE_MMAP euid=0 mask=MAY_EXEC appraise_hash=sha384  # (3)
> 
> (I agree that such a diversity of hashes is quite implausible on a single system
> in practice, but I also think it best to try to think of degenerate usecases
> before implementing that feature, as users will tend to rely on them)
> 
> When a user try to update the ima hash (or ima signature) of a file, how can we
> know the hash algorithms that the user can use ? How do we know if the users uses 
> a rule or another, and thus the algorithm that should apply ?
> There is no one-to-one mapping between files and rules in IMA (I understand that is not
> at all the philosophy of IMA), so the answer is "We cannot".
> Worse, two rules could both apply to the same file (e.g. he could both mmap the dynamic
> loader and run it directly, so rules (1) and (2) would both apply.
> Except they do not use the same appraise_hash parameter!
> So the step "extract the rule that matches a file" is not possible, and I need to get
> back to the drawing board.
> 
> Technically, we could try every possible combination of mask/func to determine which
> would apply to the file whose xattr is being updated, but that would be absolutely
> terrible performance wise, and it would still have bad semantics:
> - either we would choose the first rule that match, and in that case the order of the
>  policy (and the order of our exhaustive search) would impact the resulting algorithms 
>  allowed;
> - or we could consider the intersection of hash algorithms allowed in each rule
>  (it might be null) or their union (it might be overly broad and we might choose
>  an algorithm not part of the intersection, thus the will will not be usable in
>  some situations).
> 
> In short, I believe both situations would be a nightmare, for user experience,
> performance, maintainability and probable the sanity of maintainers/code reviewers.

Agreed.

> 
> I think one possible way of getting out of this conundrum would be to extend the ima
> policy further by adding a new value for the 'func' policy option (something like
> WRITE_XATTR_HASH maybe ?). In that mode, the 'mask' option would have no effect, the
> appraise_hash parameter would be mandatory, and any file matching this policy would
> have the corresponding 'appraise_hash' policy enforced.
> This might give policy rules of the following sort:
> appraise func=BPRM_CHECK euid=0 appraise_hash=sha1,sha256
> appraise func=FILE_MMAP fowner=0 mask=MAY_EXEC appraise_hash=sha256,sha512
> appraise func=FILE_MMAP euid=0 mask=MAY_EXEC appraise_hash=sha384
> appraise func=WRITE_XATTR_HASH fowner=0 obj_type=bin_t appraise_hash=sha256
> 
> The first three rules would just impact executions/mmap()s, and the last one
> would restrict xattr writes.
> 
> I agree that would add quite a bit of complexity (and a performance hit to check
> if a IMA policy matches) to the setxattr hook, that I don't see yet another way out
> of this issue.
> 
> Please let me know what you think, I certainly would prefer it if someone came up
> with a much simpler option that I could then implement.

Implementing complete setxattr policy rules, including LSM labels,
would be the safest, but as you said, it would impact performance. 
Most systems could have a simpler rule to limit the hash algorithm(s).

For example,
appraise func=SETXATTR_CHECK appraise_hash=sha256
appraise func=SETXATTR_CHECK appraise_hash=sha256,sha384,sha512

Without a SETXATTR_CHECK rule, the default would be to limit it to the
configured crypto algorithms.

(The LSM hook is named security_inode_setxattr.)

thanks,

Mimi

[PATCH v2 2/3] IMA: add policy support for restricting the accepted hash algorithms

[THOBY Simon]

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.

This patch is *not* self-contained, as it plugs in the support for
parsing the parameter and showing it to the user, but it doesn't enforce
the hashes yet, this will come in a subsequent patch.

Here is an example of a valid rule that enforces the use of sha256 or
sha512 when appraising iptables binaries:
  appraise func=BPRM_CHECK obj_type=iptables_exec_t appraise_type=imasig appraise_hash=sha256,sha512

This do not apply only to IMA in hash mode, it also works with digital
signatures, in which case it requires the hash (which was then signed
by a trusted private key) to have been generated with one of the
whitelisted algorithms.

Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
---
 Documentation/ABI/testing/ima_policy |  6 +-
 security/integrity/ima/ima.h         |  2 +-
 security/integrity/ima/ima_policy.c  | 82 +++++++++++++++++++++++++++-
 3 files changed, 85 insertions(+), 5 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=]
 		  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.h b/security/integrity/ima/ima.h
index f0e448ed1f9f..049748e3fe9b 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -47,7 +47,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
 extern int ima_policy_flag;
 
 /* set during initialization */
-extern int ima_hash_algo;
+extern int ima_hash_algo __ro_after_init;
 extern int ima_sha1_idx __ro_after_init;
 extern int ima_hash_algo_idx __ro_after_init;
 extern int ima_extra_slots __ro_after_init;
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index fd5d46e511f1..1b6c00baa397 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -35,6 +35,7 @@
 #define IMA_FSNAME	0x0200
 #define IMA_KEYRINGS	0x0400
 #define IMA_LABEL	0x0800
+#define IMA_VALIDATE_HASH	0x1000
 
 #define UNKNOWN		0
 #define MEASURE		0x0001	/* same as IMA_MEASURE */
@@ -79,6 +80,7 @@ struct ima_rule_entry {
 	bool (*uid_op)(kuid_t, kuid_t);    /* Handlers for operators       */
 	bool (*fowner_op)(kuid_t, kuid_t); /* uid_eq(), uid_gt(), uid_lt() */
 	int pcr;
+	unsigned int allowed_hashes;
 	struct {
 		void *rule;	/* LSM file metadata specific */
 		char *args_p;	/* audit value */
@@ -90,6 +92,15 @@ struct ima_rule_entry {
 	struct ima_template_desc *template;
 };
 
+
+/**
+ * sanity check in case the kernels gains more hash algorithms that can
+ * fit in un unsigned int
+ */
+static_assert(
+	8 * sizeof(unsigned int) >= HASH_ALGO__LAST,
+	"The bitfields allowed_hashes in ruleset are too small to contain all the supported hash algorithms, consider changing its type");
+
 /*
  * Without LSM specific knowledge, the default policy can only be
  * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner
@@ -948,7 +959,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
 };
 
 static const match_table_t policy_tokens = {
@@ -986,6 +997,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"},
 	{Opt_err, NULL}
 };
 
@@ -1111,7 +1123,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;
@@ -1123,7 +1135,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;
@@ -1173,6 +1185,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);
+
+		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;
@@ -1190,6 +1223,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;
@@ -1542,6 +1576,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:
+			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);
@@ -1700,6 +1751,25 @@ static void ima_show_rule_opt_list(struct seq_file *m,
 		seq_printf(m, "%s%s", i ? "|" : "", opt_list->items[i]);
 }
 
+static void ima_policy_show_appraise_hash(struct seq_file *m,
+					  unsigned int allowed_hashes)
+{
+	int idx;
+	bool first = true;
+
+	for (idx = 0; idx < HASH_ALGO__LAST; idx++) {
+		if (!(allowed_hashes & (1U << idx)))
+			continue;
+
+		if (!first)
+			seq_puts(m, ",");
+		first = false;
+
+		seq_puts(m, hash_algo_name[idx]);
+	}
+
+}
+
 int ima_policy_show(struct seq_file *m, void *v)
 {
 	struct ima_rule_entry *entry = v;
@@ -1811,6 +1881,12 @@ int ima_policy_show(struct seq_file *m, void *v)
 		seq_puts(m, " ");
 	}
 
+	if (entry->flags & IMA_VALIDATE_HASH) {
+		seq_puts(m, "appraise_hash=");
+		ima_policy_show_appraise_hash(m, entry->allowed_hashes);
+		seq_puts(m, " ");
+	}
+
 	for (i = 0; i < MAX_LSM_RULES; i++) {
 		if (entry->lsm[i].rule) {
 			switch (i) {
-- 
2.31.1

Re: [PATCH v2 2/3] IMA: add policy support for restricting the accepted hash algorithms

[Mimi Zohar]

Hi Simon,

On Tue, 2021-07-20 at 09:25 +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.
> This patch is *not* self-contained, as it plugs in the support for
> parsing the parameter and showing it to the user, but it doesn't enforce
> the hashes yet, this will come in a subsequent patch.

Right, in order for the patch set to be bisect safe, the order of
patches 2 & 3 should be reversed.  First implement the support, then
add the policy rule support.  Otherwise the policy rules could be
processed, but not enforced.

thanks,

Mimi

> 
> Here is an example of a valid rule that enforces the use of sha256 or
> sha512 when appraising iptables binaries:
>   appraise func=BPRM_CHECK obj_type=iptables_exec_t appraise_type=imasig appraise_hash=sha256,sha512
> 
> This do not apply only to IMA in hash mode, it also works with digital
> signatures, in which case it requires the hash (which was then signed
> by a trusted private key) to have been generated with one of the
> whitelisted algorithms.
> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>

[PATCH v2 3/3] IMA: honor the appraise_hash policy option

[THOBY Simon]

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>
---
 security/integrity/ima/ima.h          |  4 ++--
 security/integrity/ima/ima_api.c      |  6 ++++--
 security/integrity/ima/ima_appraise.c |  5 +++--
 security/integrity/ima/ima_main.c     | 22 +++++++++++++++++++---
 security/integrity/ima/ima_policy.c   |  8 ++++++--
 5 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 049748e3fe9b..7ef1b214d358 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -254,7 +254,7 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
 		   const struct cred *cred, u32 secid, int mask,
 		   enum ima_hooks func, int *pcr,
 		   struct ima_template_desc **template_desc,
-		   const char *func_data);
+		   const char *func_data, unsigned int *allowed_hashes);
 int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
 int ima_collect_measurement(struct integrity_iint_cache *iint,
 			    struct file *file, void *buf, loff_t size,
@@ -285,7 +285,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
 		     const struct cred *cred, u32 secid, enum ima_hooks func,
 		     int mask, int flags, int *pcr,
 		     struct ima_template_desc **template_desc,
-		     const char *func_data);
+		     const char *func_data, unsigned int *allowed_hashes);
 void ima_init_policy(void);
 void ima_update_policy(void);
 void ima_update_policy_flag(void);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index d8e321cc6936..c91c2c402498 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -172,6 +172,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  * @pcr: pointer filled in if matched measure policy sets pcr=
  * @template_desc: pointer filled in if matched measure policy sets template=
  * @func_data: func specific data, may be NULL
+ * @allowed_hashes: whitelist of hash algorithms allowed for the IMA xattr
  *
  * The policy is defined in terms of keypairs:
  *		subj=, obj=, type=, func=, mask=, fsmagic=
@@ -188,14 +189,15 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
 		   const struct cred *cred, u32 secid, int mask,
 		   enum ima_hooks func, int *pcr,
 		   struct ima_template_desc **template_desc,
-		   const char *func_data)
+		   const char *func_data, unsigned int *allowed_hashes)
 {
 	int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
 
 	flags &= ima_policy_flag;
 
 	return ima_match_policy(mnt_userns, inode, cred, secid, func, mask,
-				flags, pcr, template_desc, func_data);
+				flags, pcr, template_desc, func_data,
+				allowed_hashes);
 }
 
 /*
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index e9a24acf25c6..b0856c4bf515 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -77,8 +77,9 @@ int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode,
 		return 0;
 
 	security_task_getsecid_subj(current, &secid);
-	return ima_match_policy(mnt_userns, inode, current_cred(), secid, func,
-				mask, IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL);
+	return ima_match_policy(mnt_userns, inode, current_cred(), secid,
+				func, mask, IMA_APPRAISE | IMA_HASH, NULL,
+				NULL, NULL, NULL);
 }
 
 static int ima_fix_xattr(struct dentry *dentry,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 287b90509006..d7289164807e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -210,6 +210,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	int xattr_len = 0;
 	bool violation_check;
 	enum hash_algo hash_algo;
+	unsigned int appraisal_allowed_hashes = 0;
 
 	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
 		return 0;
@@ -219,7 +220,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	 * Included is the appraise submask.
 	 */
 	action = ima_get_action(file_mnt_user_ns(file), inode, cred, secid,
-				mask, func, &pcr, &template_desc, NULL);
+				mask, func, &pcr, &template_desc, NULL,
+				&appraisal_allowed_hashes);
 	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
 			   (ima_policy_flag & IMA_MEASURE));
 	if (!action && !violation_check)
@@ -327,6 +329,20 @@ static int process_measurement(struct file *file, const struct cred *cred,
 
 	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
 
+	/* Ensure that the digest was generated using an allowed algorithm */
+	if (appraisal_allowed_hashes &&
+	    !(appraisal_allowed_hashes & (1U << hash_algo))) {
+		rc = -EACCES;
+
+		if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
+			pathname = ima_d_path(&file->f_path, &pathbuf, filename);
+
+		integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file),
+			pathname, "collect_data", "forbidden-hash-algorithm", rc, 0);
+
+		goto out_locked;
+	}
+
 	rc = ima_collect_measurement(iint, file, buf, size, hash_algo, modsig);
 	if (rc != 0 && rc != -EBADF && rc != -EINVAL)
 		goto out_locked;
@@ -433,7 +449,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
 	inode = file_inode(vma->vm_file);
 	action = ima_get_action(file_mnt_user_ns(vma->vm_file), inode,
 				current_cred(), secid, MAY_EXEC, MMAP_CHECK,
-				&pcr, &template, NULL);
+				&pcr, &template, NULL, NULL);
 
 	/* Is the mmap'ed file in policy? */
 	if (!(action & (IMA_MEASURE | IMA_APPRAISE_SUBMASK)))
@@ -882,7 +898,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
 		security_task_getsecid_subj(current, &secid);
 		action = ima_get_action(mnt_userns, inode, current_cred(),
 					secid, 0, func, &pcr, &template,
-					func_data);
+					func_data, NULL);
 		if (!(action & IMA_MEASURE))
 			return;
 	}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 1b6c00baa397..260b3b0520cc 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -657,6 +657,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
  * @pcr: set the pcr to extend
  * @template_desc: the template that should be used for this rule
  * @func_data: func specific data, may be NULL
+ * @allowed_hashes: whitelist of hash algorithms allowed for the IMA xattr
  *
  * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
  * conditions.
@@ -669,7 +670,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
 		     const struct cred *cred, u32 secid, enum ima_hooks func,
 		     int mask, int flags, int *pcr,
 		     struct ima_template_desc **template_desc,
-		     const char *func_data)
+		     const char *func_data, unsigned int *allowed_hashes)
 {
 	struct ima_rule_entry *entry;
 	int action = 0, actmask = flags | (flags << 1);
@@ -695,8 +696,11 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
 			action &= ~IMA_HASH;
 			if (ima_fail_unverifiable_sigs)
 				action |= IMA_FAIL_UNVERIFIABLE_SIGS;
-		}
 
+			if (allowed_hashes &&
+			    entry->flags & IMA_VALIDATE_HASH)
+				*allowed_hashes = entry->allowed_hashes;
+		}
 
 		if (entry->action & IMA_DO_MASK)
 			actmask &= ~(entry->action | entry->action << 1);
-- 
2.31.1