| Message ID | 20220921125804.59490-3-guozihua@huawei.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ima: Handle -ESTALE returned by ima_filter_rule_match() | expand |
Hi Scott, On Wed, 2022-09-21 at 20:58 +0800, GUO Zihua wrote: > } > - if (!rc) > - return false; > + > + if (rc == -ESTALE && !rule_reinitialized) { Ok, this limits allocating ima_lsm_copy_rule() to the first -ESTALE, > + lsm_rule = ima_lsm_copy_rule(rule); > + if (lsm_rule) { > + rule_reinitialized = true; > + goto retry; but "retry" is also limited to the first -ESTALE. > + } > + } > + if (!rc) { > + result = false; > + goto out; > + } > } > - return true; > + result = true; > + > +out: > + if (rule_reinitialized) { > + for (i = 0; i < MAX_LSM_RULES; i++) > + ima_filter_rule_free(lsm_rule->lsm[i].rule); > + kfree(lsm_rule); > + } > + return result; > }
On 2022/9/22 19:09, Mimi Zohar wrote: > Hi Scott, > > On Wed, 2022-09-21 at 20:58 +0800, GUO Zihua wrote: >> } >> - if (!rc) >> - return false; >> + >> + if (rc == -ESTALE && !rule_reinitialized) { > > Ok, this limits allocating ima_lsm_copy_rule() to the first -ESTALE, > >> + lsm_rule = ima_lsm_copy_rule(rule); >> + if (lsm_rule) { >> + rule_reinitialized = true; >> + goto retry; > > but "retry" is also limited to the first -ESTALE. Technically we would only need one retry. This loop is looping on all the lsm members of one rule, and ima_lsm_copy_rule would update all the lsm members of this rule. The "lsm member" here refers to LSM defined properties like obj_user, obj_role etc. These members are of AND relation, meaning all lsm members together would form one LSM rule. As of the scenario you mentioned, I think it should be really rare. Spending to much time and code on this might not worth it. > >> + } >> + } >> + if (!rc) { >> + result = false; >> + goto out; >> + } >> } >> - return true; >> + result = true; >> + >> +out: >> + if (rule_reinitialized) { >> + for (i = 0; i < MAX_LSM_RULES; i++) >> + ima_filter_rule_free(lsm_rule->lsm[i].rule); >> + kfree(lsm_rule); >> + } >> + return result; >> } >
On Fri, 2022-09-23 at 12:01 +0800, Guozihua (Scott) wrote: > On 2022/9/22 19:09, Mimi Zohar wrote: > > Hi Scott, > > > > On Wed, 2022-09-21 at 20:58 +0800, GUO Zihua wrote: > >> } > >> - if (!rc) > >> - return false; > >> + > >> + if (rc == -ESTALE && !rule_reinitialized) { > > > > Ok, this limits allocating ima_lsm_copy_rule() to the first -ESTALE, > > > >> + lsm_rule = ima_lsm_copy_rule(rule); > >> + if (lsm_rule) { > >> + rule_reinitialized = true; > >> + goto retry; > > > > but "retry" is also limited to the first -ESTALE. > > Technically we would only need one retry. This loop is looping on all > the lsm members of one rule, and ima_lsm_copy_rule would update all the > lsm members of this rule. The "lsm member" here refers to LSM defined > properties like obj_user, obj_role etc. These members are of AND > relation, meaning all lsm members together would form one LSM rule. > > As of the scenario you mentioned, I think it should be really rare. > Spending to much time and code on this might not worth it. > > > >> + } > >> + } Either there can be multiple LSM fields and the memory is allocated once and freed once at the end, as you suggested, or the memory should be freed here and rule_reinitialized reset, minimizing the code change. > >> + if (!rc) { > >> + result = false; > >> + goto out; > >> + } > >> } > >> - return true; > >> + result = true; > >> + > >> +out: > >> + if (rule_reinitialized) { > >> + for (i = 0; i < MAX_LSM_RULES; i++) > >> + ima_filter_rule_free(lsm_rule->lsm[i].rule); > >> + kfree(lsm_rule); > >> + } > >> + return result; > >> }
On 2022/9/23 19:19, Mimi Zohar wrote: > On Fri, 2022-09-23 at 12:01 +0800, Guozihua (Scott) wrote: >> On 2022/9/22 19:09, Mimi Zohar wrote: >>> Hi Scott, >>> >>> On Wed, 2022-09-21 at 20:58 +0800, GUO Zihua wrote: >>>> } >>>> - if (!rc) >>>> - return false; >>>> + >>>> + if (rc == -ESTALE && !rule_reinitialized) { >>> >>> Ok, this limits allocating ima_lsm_copy_rule() to the first -ESTALE, >>> >>>> + lsm_rule = ima_lsm_copy_rule(rule); >>>> + if (lsm_rule) { >>>> + rule_reinitialized = true; >>>> + goto retry; >>> >>> but "retry" is also limited to the first -ESTALE. >> >> Technically we would only need one retry. This loop is looping on all >> the lsm members of one rule, and ima_lsm_copy_rule would update all the >> lsm members of this rule. The "lsm member" here refers to LSM defined >> properties like obj_user, obj_role etc. These members are of AND >> relation, meaning all lsm members together would form one LSM rule. >> >> As of the scenario you mentioned, I think it should be really rare. >> Spending to much time and code on this might not worth it. >>> >>>> + } >>>> + } > > Either there can be multiple LSM fields and the memory is allocated > once and freed once at the end, as you suggested, or the memory should > be freed here and rule_reinitialized reset, minimizing the code change. I might have overlooked something, but if I understands the code correctly, we would be copying the same rule over and over again till the loop ends in that case. ima_lsm_update_rule() would replace the rule node on the rule list without updating the rule in place. Although synchronize_rcu() should prevent a UAF, the rule in ima_match_rules() would not be updated. Meaning SELinux would always return -ESTALE before we copy and retry as we keep passing in the outdated rule entry. > >>>> + if (!rc) { >>>> + result = false; >>>> + goto out; >>>> + } >>>> } >>>> - return true; >>>> + result = true; >>>> + >>>> +out: >>>> + if (rule_reinitialized) { >>>> + for (i = 0; i < MAX_LSM_RULES; i++) >>>> + ima_filter_rule_free(lsm_rule->lsm[i].rule); >>>> + kfree(lsm_rule); >>>> + } >>>> + return result; >>>> } >
On Sat, 2022-09-24 at 14:05 +0800, Guozihua (Scott) wrote: > I might have overlooked something, but if I understands the code > correctly, we would be copying the same rule over and over again till > the loop ends in that case. ima_lsm_update_rule() would replace the rule > node on the rule list without updating the rule in place. Although > synchronize_rcu() should prevent a UAF, the rule in ima_match_rules() > would not be updated. Meaning SELinux would always return -ESTALE before > we copy and retry as we keep passing in the outdated rule entry. After reviewing this patch set again, the code looks fine. The commit message is still a bit off, but I've pushed the patch set out to next- integrity-testing, waiting for some Reviewed-by/Tested-by tags.
On Wed, 2022-09-28 at 10:11 -0400, Mimi Zohar wrote: > On Sat, 2022-09-24 at 14:05 +0800, Guozihua (Scott) wrote: > > > I might have overlooked something, but if I understands the code > > correctly, we would be copying the same rule over and over again > > till > > the loop ends in that case. ima_lsm_update_rule() would replace the > > rule > > node on the rule list without updating the rule in place. Although > > synchronize_rcu() should prevent a UAF, the rule in > > ima_match_rules() > > would not be updated. Meaning SELinux would always return -ESTALE > > before > > we copy and retry as we keep passing in the outdated rule entry. > > After reviewing this patch set again, the code looks fine. The > commit > message is still a bit off, but I've pushed the patch set out to > next- > integrity-testing, waiting for some Reviewed-by/Tested-by tags. The patches look ok for me. For both: Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com> Roberto
On 2022/9/28 22:11, Mimi Zohar wrote: > > After reviewing this patch set again, the code looks fine. The commit > message is still a bit off, but I've pushed the patch set out to next- > integrity-testing, waiting for some Reviewed-by/Tested-by tags. > Hi Mimi, How's this patch going? I see Roberto is replying with a Reviewed-by.
On Tue, 2022-10-18 at 16:43 +0800, Guozihua (Scott) wrote: > On 2022/9/28 22:11, Mimi Zohar wrote: > > > > After reviewing this patch set again, the code looks fine. The commit > > message is still a bit off, but I've pushed the patch set out to next- > > integrity-testing, waiting for some Reviewed-by/Tested-by tags. > > > > Hi Mimi, > > How's this patch going? I see Roberto is replying with a Reviewed-by. I'd really like to see a "Tested-by" tag as well. Are you able to force the scenario?
On 2022/10/19 9:07, Mimi Zohar wrote: > On Tue, 2022-10-18 at 16:43 +0800, Guozihua (Scott) wrote: >> On 2022/9/28 22:11, Mimi Zohar wrote: >>> >>> After reviewing this patch set again, the code looks fine. The commit >>> message is still a bit off, but I've pushed the patch set out to next- >>> integrity-testing, waiting for some Reviewed-by/Tested-by tags. >>> >> >> Hi Mimi, >> >> How's this patch going? I see Roberto is replying with a Reviewed-by. > > I'd really like to see a "Tested-by" tag as well. > > Are you able to force the scenario? > It's a race condition which could be hard to reproduce easily and in a stable manner. I'll give it a try.
On 2022/10/19 15:17, Guozihua (Scott) wrote: > On 2022/10/19 9:07, Mimi Zohar wrote: >> On Tue, 2022-10-18 at 16:43 +0800, Guozihua (Scott) wrote: >>> On 2022/9/28 22:11, Mimi Zohar wrote: >>>> >>>> After reviewing this patch set again, the code looks fine. The commit >>>> message is still a bit off, but I've pushed the patch set out to next- >>>> integrity-testing, waiting for some Reviewed-by/Tested-by tags. >>>> >>> >>> Hi Mimi, >>> >>> How's this patch going? I see Roberto is replying with a Reviewed-by. >> >> I'd really like to see a "Tested-by" tag as well. >> >> Are you able to force the scenario? >> > > It's a race condition which could be hard to reproduce easily and in a > stable manner. I'll give it a try. Hi Mimi, I managed to re-produce this issue with the help of the following two scripts: read_tmp_measurement.sh: > #!/bin/bash > > while true > do > cat /root/tmp.txt > /dev/null > measurement=`cat /sys/kernel/security/ima/ascii_runtime_measurements | grep "tmp\.txt" | wc -l` > if [ "${measurement}" == "1" ]; then > echo "measurement found" > exit 1 > fi > done test.sh: > #!/bin/bash > > echo "measure obj_user=system_u obj_role=object_r obj_type=unlabeled_t" > /sys/kernel/security/ima/policy > > cat /root/tmp2.txt > measurement=`cat /sys/kernel/security/ima/ascii_runtime_measurements | grep "tmp2\.txt" | wc -l` > [ "$measurement" == "1" ] && echo "measurement for tmp2 found" > > cat /root/tmp.txt > measurement=`cat /sys/kernel/security/ima/ascii_runtime_measurements | grep "tmp\.txt" | wc -l` > [ "$measurement" == "1" ] && echo "measurement for tmp found, preparation failed!" && exit 1 > > ./read_tmp_measurement.sh & > pid=$! > > cd /usr/share/selinux/default > semodule -i clock.pp.bz2 > semodule -r clock > > kill ${pid} I created two files tmp.txt and tmp2.txt, assign them with type user_home_t and unlabeled_t respectively and then run test.sh. On a multi-core environment, I managed to reproduce this issue pretty easily and tested that once the solution is merged, the issue stops happening.
Hi Scott, On Fri, 2022-10-28 at 16:36 +0800, Guozihua (Scott) wrote: > > I managed to re-produce this issue with the help of the following two > scripts: > > read_tmp_measurement.sh: > > #!/bin/bash > > > > while true > > do > > cat /root/tmp.txt > /dev/null > > measurement=`cat /sys/kernel/security/ima/ascii_runtime_measurements | grep "tmp\.txt" | wc -l` > > if [ "${measurement}" == "1" ]; then > > echo "measurement found" > > exit 1 > > fi > > done > > test.sh: > > #!/bin/bash > > > > echo "measure obj_user=system_u obj_role=object_r obj_type=unlabeled_t" > /sys/kernel/security/ima/policy > > > > cat /root/tmp2.txt > > measurement=`cat /sys/kernel/security/ima/ascii_runtime_measurements | grep "tmp2\.txt" | wc -l` > > [ "$measurement" == "1" ] && echo "measurement for tmp2 found" > > > > cat /root/tmp.txt > > measurement=`cat /sys/kernel/security/ima/ascii_runtime_measurements | grep "tmp\.txt" | wc -l` > > [ "$measurement" == "1" ] && echo "measurement for tmp found, preparation failed!" && exit 1 > > > > ./read_tmp_measurement.sh & > > pid=$! > > > > cd /usr/share/selinux/default > > semodule -i clock.pp.bz2 > > semodule -r clock > > > > kill ${pid} Are you loading/unloading any selinux policy or specifically clock? If specifically clock, what is special about it? > I created two files tmp.txt and tmp2.txt, assign them with type > user_home_t and unlabeled_t respectively and then run test.sh. > On a multi-core environment, I managed to reproduce this issue pretty > easily and tested that once the solution is merged, the issue stops > happening. As I only see an IMA measurement policy rule being loaded for "unlabeled_t" and not "user_home_t", should I assume that an IMA measurement rule already exists for "user_home_t"? thanks, Mimi
Hi Mimi, On 2022/11/2 6:15, Mimi Zohar wrote: > Hi Scott, > > On Fri, 2022-10-28 at 16:36 +0800, Guozihua (Scott) wrote: >> >> I managed to re-produce this issue with the help of the following two >> scripts: >> >> read_tmp_measurement.sh: >>> #!/bin/bash >>> >>> while true >>> do >>> cat /root/tmp.txt > /dev/null >>> measurement=`cat /sys/kernel/security/ima/ascii_runtime_measurements | grep "tmp\.txt" | wc -l` >>> if [ "${measurement}" == "1" ]; then >>> echo "measurement found" >>> exit 1 >>> fi >>> done >> >> test.sh: >>> #!/bin/bash >>> >>> echo "measure obj_user=system_u obj_role=object_r obj_type=unlabeled_t" > /sys/kernel/security/ima/policy >>> >>> cat /root/tmp2.txt >>> measurement=`cat /sys/kernel/security/ima/ascii_runtime_measurements | grep "tmp2\.txt" | wc -l` >>> [ "$measurement" == "1" ] && echo "measurement for tmp2 found" >>> >>> cat /root/tmp.txt >>> measurement=`cat /sys/kernel/security/ima/ascii_runtime_measurements | grep "tmp\.txt" | wc -l` >>> [ "$measurement" == "1" ] && echo "measurement for tmp found, preparation failed!" && exit 1 >>> >>> ./read_tmp_measurement.sh & >>> pid=$! >>> >>> cd /usr/share/selinux/default >>> semodule -i clock.pp.bz2 >>> semodule -r clock >>> >>> kill ${pid} > > Are you loading/unloading any selinux policy or specifically clock? If > specifically clock, what is special about it? No there are nothing special about clock. Any selinux policy should do,. > >> I created two files tmp.txt and tmp2.txt, assign them with type >> user_home_t and unlabeled_t respectively and then run test.sh. >> On a multi-core environment, I managed to reproduce this issue pretty >> easily and tested that once the solution is merged, the issue stops >> happening. > > As I only see an IMA measurement policy rule being loaded for > "unlabeled_t" and not "user_home_t", should I assume that an IMA > measurement rule already exists for "user_home_t"? There wasn't a rule for user_home_t. These scripts demonstrate that during a selinux policy reload, IMA would measure files that is not in the range of it's LSM based rules. Which is the issue I am trying to fix. In this test, we only have one rule for measuring files of type unlabeled_t. However, during selinux policy reload, file of user_home_t is also measured. > > thanks, > > Mimi >
On Wed, 2022-11-02 at 09:42 +0800, Guozihua (Scott) wrote: > > As I only see an IMA measurement policy rule being loaded for > > "unlabeled_t" and not "user_home_t", should I assume that an IMA > > measurement rule already exists for "user_home_t"? > > There wasn't a rule for user_home_t. These scripts demonstrate that > during a selinux policy reload, IMA would measure files that is not in > the range of it's LSM based rules. Which is the issue I am trying to fix. > > In this test, we only have one rule for measuring files of type > unlabeled_t. However, during selinux policy reload, file of user_home_t > is also measured. Thanks, Scott. After tweaking the scripts for my system, I was able to reproduce the bug. This patch set is now queued in next-integrity.
On 2022/11/3 21:15, Mimi Zohar wrote: > On Wed, 2022-11-02 at 09:42 +0800, Guozihua (Scott) wrote: >>> As I only see an IMA measurement policy rule being loaded for >>> "unlabeled_t" and not "user_home_t", should I assume that an IMA >>> measurement rule already exists for "user_home_t"? >> >> There wasn't a rule for user_home_t. These scripts demonstrate that >> during a selinux policy reload, IMA would measure files that is not in >> the range of it's LSM based rules. Which is the issue I am trying to fix. >> >> In this test, we only have one rule for measuring files of type >> unlabeled_t. However, during selinux policy reload, file of user_home_t >> is also measured. > > Thanks, Scott. After tweaking the scripts for my system, I was able to > reproduce the bug. This patch set is now queued in next-integrity. > Hi Mimi, Any chance these patches would be in 6.1?
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 8040215c0252..2edff7f58c25 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -545,6 +545,9 @@ static bool ima_match_rules(struct ima_rule_entry *rule, const char *func_data) { int i; + bool result = false; + struct ima_rule_entry *lsm_rule = rule; + bool rule_reinitialized = false; if ((rule->flags & IMA_FUNC) && (rule->func != func && func != POST_SETATTR)) @@ -606,35 +609,55 @@ static bool ima_match_rules(struct ima_rule_entry *rule, int rc = 0; u32 osid; - if (!rule->lsm[i].rule) { - if (!rule->lsm[i].args_p) + if (!lsm_rule->lsm[i].rule) { + if (!lsm_rule->lsm[i].args_p) continue; else return false; } + +retry: switch (i) { case LSM_OBJ_USER: case LSM_OBJ_ROLE: case LSM_OBJ_TYPE: security_inode_getsecid(inode, &osid); - rc = ima_filter_rule_match(osid, rule->lsm[i].type, + rc = ima_filter_rule_match(osid, lsm_rule->lsm[i].type, Audit_equal, - rule->lsm[i].rule); + lsm_rule->lsm[i].rule); break; case LSM_SUBJ_USER: case LSM_SUBJ_ROLE: case LSM_SUBJ_TYPE: - rc = ima_filter_rule_match(secid, rule->lsm[i].type, + rc = ima_filter_rule_match(secid, lsm_rule->lsm[i].type, Audit_equal, - rule->lsm[i].rule); + lsm_rule->lsm[i].rule); break; default: break; } - if (!rc) - return false; + + if (rc == -ESTALE && !rule_reinitialized) { + lsm_rule = ima_lsm_copy_rule(rule); + if (lsm_rule) { + rule_reinitialized = true; + goto retry; + } + } + if (!rc) { + result = false; + goto out; + } } - return true; + result = true; + +out: + if (rule_reinitialized) { + for (i = 0; i < MAX_LSM_RULES; i++) + ima_filter_rule_free(lsm_rule->lsm[i].rule); + kfree(lsm_rule); + } + return result; } /*
IMA relies on the blocking LSM policy notifier callback to update the LSM based IMA policy rules. When SELinux update its policies, IMA would be notified and starts updating all its lsm rules one-by-one. During this time, -ESTALE would be returned by ima_filter_rule_match() if it is called with a LSM rule that has not yet been updated. In ima_match_rules(), -ESTALE is not handled, and the LSM rule is considered a match, causing extra files to be measured by IMA. Fix it by re-initializing a temporary rule if -ESTALE is returned by ima_filter_rule_match(). The origin rule in the rule list would be updated by the LSM policy notifier callback. Fixes: b16942455193 ("ima: use the lsm policy update notifier") Signed-off-by: GUO Zihua <guozihua@huawei.com> --- security/integrity/ima/ima_policy.c | 41 ++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 9 deletions(-)