| Message ID | 20210720092404.120172-2-simon.thoby@viveris.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IMA: restrict the accepted digest algorithms | expand |
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;
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
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
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;
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(+)