| Message ID | 20241213222014.1580991-7-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | LTP tests: load predefined policy, enhancements | expand |
Hi Petr, On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote: > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > .../integrity/ima/datafiles/ima_violations/violations.policy | 1 + > 1 file changed, 1 insertion(+) > create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > new file mode 100644 > index 0000000000..5734c7617f > --- /dev/null > +++ b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > @@ -0,0 +1 @@ > +func=FILE_CHECK "[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh" contains two rules to measure files opened by root on file open. measure func=FILE_CHECK mask=^MAY_READ euid=0 measure func=FILE_CHECK mask=^MAY_READ uid=0 If the 'tcb' or equivalent policy is loaded, there is no need to load another policy rule. Thanks, Mimi
> Hi Petr, > On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote: > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > .../integrity/ima/datafiles/ima_violations/violations.policy | 1 + > > 1 file changed, 1 insertion(+) > > create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > new file mode 100644 > > index 0000000000..5734c7617f > > --- /dev/null > > +++ b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > @@ -0,0 +1 @@ > > +func=FILE_CHECK > "[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh" > contains two rules to measure files opened by root on file open. > measure func=FILE_CHECK mask=^MAY_READ euid=0 > measure func=FILE_CHECK mask=^MAY_READ uid=0 My bad of course "func=FILE_CHECK" is not enough. Thanks for providing a correct example policy (required part of 'tcb' policy). > If the 'tcb' or equivalent policy is loaded, there is no need to load another > policy rule. Yes, I'll fix the next commit to avoid loading example policy when ima_policy=tcb. Kind regards, Petr > Thanks, > Mimi
Hi Mimi, > Hi Petr, > On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote: > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > .../integrity/ima/datafiles/ima_violations/violations.policy | 1 + > > 1 file changed, 1 insertion(+) > > create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > new file mode 100644 > > index 0000000000..5734c7617f > > --- /dev/null > > +++ b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > @@ -0,0 +1 @@ > > +func=FILE_CHECK > "[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh" > contains two rules to measure files opened by root on file open. > measure func=FILE_CHECK mask=^MAY_READ euid=0 > measure func=FILE_CHECK mask=^MAY_READ uid=0 > If the 'tcb' or equivalent policy is loaded, there is no need to load another > policy rule. I guess I'll move check for builtin policy loaded via kernel command line parameter also to ima_setup.sh to avoid loading example policy when there is a required builtin policy loaded. I also wonder what is a common approach - don't try to load custom example policy when there is builtin policy loaded? My goal was to allow more broad IMA testing based on different setup: * running tests with ima_policy=tcb builtin policy (current approach). Many tests will be skipped due missing required policy content. * running tests without any builtin policy + load a custom policy + reboot via LTP_IMA_LOAD_POLICY=1 (this patchset), but this should be probably be done only if required (or even none) builtin policy is loaded. * Ideally not require CONFIG_IMA_READ_POLICY=y as some distros does not have it (but then it is hard to detect whether failures are real bugs or just false positives due not having a proper policy). Maybe convert TBROK/TFAIL to TCONF if policy content is required but cannot be read due CONFIG_IMA_READ_POLICY (and custom policy with proper content was not loaded). But you may have an idea what is more useful (brings more test coverage). Kind regards, Petr > Thanks, > Mimi
On Tue, 2024-12-31 at 13:23 +0100, Petr Vorel wrote: > Hi Mimi, > > > Hi Petr, > > > On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote: > > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > --- > > > .../integrity/ima/datafiles/ima_violations/violations.policy | 1 + > > > 1 file changed, 1 insertion(+) > > > create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > > > diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > > new file mode 100644 > > > index 0000000000..5734c7617f > > > --- /dev/null > > > +++ b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > > @@ -0,0 +1 @@ > > > +func=FILE_CHECK > > > "[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh" > > contains two rules to measure files opened by root on file open. > > > measure func=FILE_CHECK mask=^MAY_READ euid=0 > > measure func=FILE_CHECK mask=^MAY_READ uid=0 > > > If the 'tcb' or equivalent policy is loaded, there is no need to load another > > policy rule. > > I guess I'll move check for builtin policy loaded via kernel command line > parameter also to ima_setup.sh to avoid loading example policy when there is a > required builtin policy loaded. > Between the builtin and arch specific policies, most of the rules are already defined. The exception is measuring the boot command line. Perhaps we should update the arch specific policy to include it with the other kexec rules. The arch specific policy may include the rule that requires the IMA policy to be signed. > I also wonder what is a common approach - don't > try to load custom example policy when there is builtin policy loaded? How about first checking if the rule exists when there is a builtin or equivalent custom policy loaded, before loading the example test policy? > > My goal was to allow more broad IMA testing based on different setup: Very good. > > * running tests with ima_policy=tcb builtin policy (current approach). Many > tests will be skipped due missing required policy content. Ok. Remember even with "ima_policy=tcb" specified on the boot command line, the results will differ depending on whether the arch specific policy is loaded. > * running tests without any builtin policy + load a custom policy + reboot via > LTP_IMA_LOAD_POLICY=1 (this patchset), but this should be probably be done only > if required (or even none) builtin policy is loaded. Good. The first patch introduces the equivalent custom policy to "ima_policy=tcb". By "load a custom policy" are you referring to this policy or a specific policy test rule? > * Ideally not require CONFIG_IMA_READ_POLICY=y as some distros does not have it > (but then it is hard to detect whether failures are real bugs or just false > positives due not having a proper policy). Maybe convert TBROK/TFAIL to TCONF if > policy content is required but cannot be read due CONFIG_IMA_READ_POLICY (and > custom policy with proper content was not loaded). Probably the latter option of converting from TBROK/TFAIL to TCONF is preferable. Why fail a test without knowing it will fail. > But you may have an idea what is more useful (brings more test coverage). There are two main problems: - Not being able to read the policy. - Only being able to load a signed policy. I think between your above ordering and a new test to see if the policy needs to be signed, it's the best we can do for now. As mentioned in my 2/8 response, a new package containing pre-defined custom policies that are signed by the distro would resolve the latter problem. Thanks, Mimi
> On Tue, 2024-12-31 at 13:23 +0100, Petr Vorel wrote: > > Hi Mimi, > > > Hi Petr, > > > On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote: > > > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > > --- > > > > .../integrity/ima/datafiles/ima_violations/violations.policy | 1 + > > > > 1 file changed, 1 insertion(+) > > > > create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > > > diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > > > new file mode 100644 > > > > index 0000000000..5734c7617f > > > > --- /dev/null > > > > +++ b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy > > > > @@ -0,0 +1 @@ > > > > +func=FILE_CHECK > > > "[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh" > > > contains two rules to measure files opened by root on file open. > > > measure func=FILE_CHECK mask=^MAY_READ euid=0 > > > measure func=FILE_CHECK mask=^MAY_READ uid=0 > > > If the 'tcb' or equivalent policy is loaded, there is no need to load another > > > policy rule. > > I guess I'll move check for builtin policy loaded via kernel command line > > parameter also to ima_setup.sh to avoid loading example policy when there is a > > required builtin policy loaded. > Between the builtin and arch specific policies, most of the rules are already > defined. The exception is measuring the boot command line. Perhaps we should > update the arch specific policy to include it with the other kexec rules. > The arch specific policy may include the rule that requires the IMA policy to be > signed. > > I also wonder what is a common approach - don't > > try to load custom example policy when there is builtin policy loaded? > How about first checking if the rule exists when there is a builtin or > equivalent custom policy loaded, before loading the example test policy? > > My goal was to allow more broad IMA testing based on different setup: > Very good. > > * running tests with ima_policy=tcb builtin policy (current approach). Many > > tests will be skipped due missing required policy content. > Ok. Remember even with "ima_policy=tcb" specified on the boot command line, the > results will differ depending on whether the arch specific policy is loaded. > > * running tests without any builtin policy + load a custom policy + reboot via > > LTP_IMA_LOAD_POLICY=1 (this patchset), but this should be probably be done only > > if required (or even none) builtin policy is loaded. > Good. The first patch introduces the equivalent custom policy to > "ima_policy=tcb". By "load a custom policy" are you referring to this policy or > a specific policy test rule? I refer to this policy. Maybe better would be "policy content required by the test" or "test example policy". My point is to allow testing without forcing ima_policy=tcb setup (some tooling might not allow easily to add kernel cmdline parameters). Also, mixing test example policy with ima_policy=tcb may result a different measurements, right? If the above assumption is correct I would like to have testing *with* ima_policy=tcb without loading any test example policy and *without* ima_policy=tcb but loading test example policy via LTP_IMA_LOAD_POLICY=1. > > * Ideally not require CONFIG_IMA_READ_POLICY=y as some distros does not have it > > (but then it is hard to detect whether failures are real bugs or just false > > positives due not having a proper policy). Maybe convert TBROK/TFAIL to TCONF if I'm sorry, I was wrong here, I meant to ask: convert TFAIL to either TBROK or TCONF, e.g. my patch [1]. > > policy content is required but cannot be read due CONFIG_IMA_READ_POLICY (and > > custom policy with proper content was not loaded). > Probably the latter option of converting from TBROK/TFAIL to TCONF is > preferable. Why fail a test without knowing it will fail. Because on distros without CONFIG_IMA_READ_POLICY=y we never get notified about the failure (maybe kernel is broken when it fails but nobody notices TCONF). But although there is a slight difference between TFAIL and TBROK [2], I agree that TCONF is probably the best (nobody wants to deal with false positives), which is handled in my patch [1]. But instead of this I'll try for all tests which require to have certain policy content (currently all but ima_conditionals.sh): if LTP_IMA_LOAD_POLICY=1 set try to load example policy even policy content cannot be checked (TCONF when policy fails to be loaded or if LTP_IMA_LOAD_POLICY not set). Kind regards, Petr > > But you may have an idea what is more useful (brings more test coverage). > There are two main problems: > - Not being able to read the policy. > - Only being able to load a signed policy. > I think between your above ordering and a new test to see if the policy needs to > be signed, it's the best we can do for now. > As mentioned in my 2/8 response, a new package containing pre-defined custom > policies that are signed by the distro would resolve the latter problem. > Thanks, > Mimi [1] https://patchwork.ozlabs.org/project/ltp/patch/20241213222014.1580991-9-pvorel@suse.cz/ [2] https://linux-test-project.readthedocs.io/en/latest/developers/api_c_tests.html#tst-res-flags-constants
On Fri, 2025-01-03 at 20:02 +0100, Petr Vorel wrote: > > On Tue, 2024-12-31 at 13:23 +0100, Petr Vorel wrote: > > > Hi Mimi, > > > > > Hi Petr, > > > > > On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote: > > > > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > > > --- > > > > > .../integrity/ima/datafiles/ima_violations/violations.policy | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > create mode 100644 > > > > > testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations > > > > > .policy > > > > > > diff --git > > > > > a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violatio > > > > > ns.policy > > > > > b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violatio > > > > > ns.policy > > > > > new file mode 100644 > > > > > index 0000000000..5734c7617f > > > > > --- /dev/null > > > > > +++ > > > > > b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violatio > > > > > ns.policy > > > > > @@ -0,0 +1 @@ > > > > > +func=FILE_CHECK > > > > > "[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh" > > > > contains two rules to measure files opened by root on file open. > > > > > measure func=FILE_CHECK mask=^MAY_READ euid=0 > > > > measure func=FILE_CHECK mask=^MAY_READ uid=0 > > > > > If the 'tcb' or equivalent policy is loaded, there is no need to load another > > > > policy rule. > > > > I guess I'll move check for builtin policy loaded via kernel command line > > > parameter also to ima_setup.sh to avoid loading example policy when there is a > > > required builtin policy loaded. > > > > Between the builtin and arch specific policies, most of the rules are already > > defined. The exception is measuring the boot command line. Perhaps we should > > update the arch specific policy to include it with the other kexec rules. > > > The arch specific policy may include the rule that requires the IMA policy to be > > signed. > > > > I also wonder what is a common approach - don't > > > try to load custom example policy when there is builtin policy loaded? > > > How about first checking if the rule exists when there is a builtin or > > equivalent custom policy loaded, before loading the example test policy? > > > > > My goal was to allow more broad IMA testing based on different setup: > > > Very good. > > > > * running tests with ima_policy=tcb builtin policy (current approach). Many > > > tests will be skipped due missing required policy content. > > > Ok. Remember even with "ima_policy=tcb" specified on the boot command line, the > > results will differ depending on whether the arch specific policy is loaded. > > > > * running tests without any builtin policy + load a custom policy + reboot via > > > LTP_IMA_LOAD_POLICY=1 (this patchset), but this should be probably be done only > > > if required (or even none) builtin policy is loaded. > > > Good. The first patch introduces the equivalent custom policy to > > "ima_policy=tcb". By "load a custom policy" are you referring to this policy or > > a specific policy test rule? > > I refer to this policy. Maybe better would be "policy content required by the test" > or "test example policy". > > My point is to allow testing without forcing ima_policy=tcb setup (some tooling > might not allow easily to add kernel cmdline parameters). Also, mixing test > example policy with ima_policy=tcb may result a different measurements, right? Only if the file matches multiple, different measurement rules. The ordering of the policy rules impacts the measurement and might even prevent the measurement. > > If the above assumption is correct I would like to have testing *with* > ima_policy=tcb without loading any test example policy I assume the purpose is to simplify testing. However, - Not all of the policy rules needed by the tests are included in the builtin "tcb" measurement policy. Without loading test specific example policy rules, the testing would be incomplete. - There's no guarantee that the builtin "tcb" measurement policy has not been replaced with a custom policy. > and *without* > ima_policy=tcb but loading test example policy via LTP_IMA_LOAD_POLICY=1. Ok, but this assumes the ability of loading an unsigned policy. > > > > * Ideally not require CONFIG_IMA_READ_POLICY=y as some distros does not have it > > > (but then it is hard to detect whether failures are real bugs or just false > > > positives due not having a proper policy). Maybe convert TBROK/TFAIL to TCONF > > > if > > I'm sorry, I was wrong here, I meant to ask: convert TFAIL to either TBROK or > TCONF, > e.g. my patch [1]. > > > > policy content is required but cannot be read due CONFIG_IMA_READ_POLICY (and > > > custom policy with proper content was not loaded). > > > Probably the latter option of converting from TBROK/TFAIL to TCONF is > > preferable. Why fail a test without knowing it will fail. > > Because on distros without CONFIG_IMA_READ_POLICY=y we never get notified about > the failure (maybe kernel is broken when it fails but nobody notices TCONF). > But although there is a slight difference between TFAIL and TBROK [2], I agree > that TCONF is probably the best (nobody wants to deal with false positives), > which is handled in my patch [1]. > > But instead of this I'll try for all tests which require to have certain policy > content (currently all but ima_conditionals.sh): if LTP_IMA_LOAD_POLICY=1 set > try to load example policy even policy content cannot be checked (TCONF when > policy fails to be loaded or if LTP_IMA_LOAD_POLICY not set). Sounds good. Mimi
diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy new file mode 100644 index 0000000000..5734c7617f --- /dev/null +++ b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy @@ -0,0 +1 @@ +func=FILE_CHECK
Suggested-by: Mimi Zohar <zohar@linux.ibm.com> Signed-off-by: Petr Vorel <pvorel@suse.cz> --- .../integrity/ima/datafiles/ima_violations/violations.policy | 1 + 1 file changed, 1 insertion(+) create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy