| Message ID | 20200717222819.26198-5-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LSM: Measure security module state | expand |
Hi Lakshmi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on integrity/next-integrity]
[cannot apply to pcmoore-selinux/next security/next-testing linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Lakshmi-Ramasubramanian/LSM-Measure-security-module-state/20200718-063111
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
security/selinux/measure.c: In function 'selinux_measure_state':
security/selinux/measure.c:132:11: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
132 | if (curr >= 0 && curr < selinux_state_string_len)
| ^~
>> security/selinux/measure.c:148:2: error: implicit declaration of function 'vfree'; did you mean 'kvfree'? [-Werror=implicit-function-declaration]
148 | vfree(policy);
| ^~~~~
| kvfree
cc1: some warnings being treated as errors
vim +148 security/selinux/measure.c
94
95 void selinux_measure_state(struct selinux_state *selinux_state)
96 {
97 void *policy = NULL;
98 void *policy_hash = NULL;
99 size_t curr, buflen;
100 int i, policy_hash_len, rc = 0;
101
102 if (!selinux_initialized(selinux_state)) {
103 pr_warn("%s: SELinux not yet initialized.\n", __func__);
104 return;
105 }
106
107 if (!selinux_state_string) {
108 pr_warn("%s: Buffer for state not allocated.\n", __func__);
109 return;
110 }
111
112 curr = snprintf(selinux_state_string, selinux_state_string_len,
113 str_format, "enabled",
114 !selinux_disabled(selinux_state));
115 curr += snprintf((selinux_state_string + curr),
116 (selinux_state_string_len - curr),
117 str_format, "enforcing",
118 enforcing_enabled(selinux_state));
119 curr += snprintf((selinux_state_string + curr),
120 (selinux_state_string_len - curr),
121 str_format, "checkreqprot",
122 selinux_checkreqprot(selinux_state));
123
124 for (i = 3; i < selinux_state_count; i++) {
125 curr += snprintf((selinux_state_string + curr),
126 (selinux_state_string_len - curr),
127 str_format,
128 selinux_policycap_names[i - 3],
129 selinux_state->policycap[i - 3]);
130 }
131
> 132 if (curr >= 0 && curr < selinux_state_string_len)
133 ima_lsm_state("selinux-state", selinux_state_string, curr);
134 else {
135 rc = -EINVAL;
136 goto out;
137 }
138
139 rc = security_read_policy_kernel(selinux_state, &policy, &buflen);
140 if (!rc)
141 rc = selinux_hash_policy("sha256", policy, buflen,
142 &policy_hash, &policy_hash_len);
143 if (!rc)
144 ima_lsm_state("selinux-policy-hash", policy_hash,
145 policy_hash_len);
146
147 out:
> 148 vfree(policy);
149 kfree(policy_hash);
150 }
151
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Lakshmi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on integrity/next-integrity]
[cannot apply to pcmoore-selinux/next security/next-testing linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Lakshmi-Ramasubramanian/LSM-Measure-security-module-state/20200718-063111
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
security/selinux/measure.c: In function 'selinux_hash_policy':
>> security/selinux/measure.c:57:8: error: implicit declaration of function 'crypto_alloc_shash' [-Werror=implicit-function-declaration]
57 | tfm = crypto_alloc_shash(hash_alg_name, 0, 0);
| ^~~~~~~~~~~~~~~~~~
>> security/selinux/measure.c:57:6: warning: assignment to 'struct crypto_shash *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
57 | tfm = crypto_alloc_shash(hash_alg_name, 0, 0);
| ^
>> security/selinux/measure.c:61:14: error: implicit declaration of function 'crypto_shash_descsize' [-Werror=implicit-function-declaration]
61 | desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
| ^~~~~~~~~~~~~~~~~~~~~
>> security/selinux/measure.c:61:50: error: dereferencing pointer to incomplete type 'struct shash_desc'
61 | desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
| ^~~~~
>> security/selinux/measure.c:62:16: error: implicit declaration of function 'crypto_shash_digestsize' [-Werror=implicit-function-declaration]
62 | digest_size = crypto_shash_digestsize(tfm);
| ^~~~~~~~~~~~~~~~~~~~~~~
>> security/selinux/measure.c:78:8: error: implicit declaration of function 'crypto_shash_digest' [-Werror=implicit-function-declaration]
78 | ret = crypto_shash_digest(desc, policy, policy_len, digest);
| ^~~~~~~~~~~~~~~~~~~
>> security/selinux/measure.c:90:2: error: implicit declaration of function 'crypto_free_shash' [-Werror=implicit-function-declaration]
90 | crypto_free_shash(tfm);
| ^~~~~~~~~~~~~~~~~
security/selinux/measure.c: In function 'selinux_measure_state':
security/selinux/measure.c:132:11: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
132 | if (curr >= 0 && curr < selinux_state_string_len)
| ^~
security/selinux/measure.c:148:2: error: implicit declaration of function 'vfree'; did you mean 'kvfree'? [-Werror=implicit-function-declaration]
148 | vfree(policy);
| ^~~~~
| kvfree
cc1: some warnings being treated as errors
vim +/crypto_alloc_shash +57 security/selinux/measure.c
45
46 static int selinux_hash_policy(const char *hash_alg_name,
47 void *policy, size_t policy_len,
48 void **policy_hash, int *policy_hash_len)
49 {
50 struct crypto_shash *tfm;
51 struct shash_desc *desc = NULL;
52 void *digest = NULL;
53 int desc_size;
54 int digest_size;
55 int ret = 0;
56
> 57 tfm = crypto_alloc_shash(hash_alg_name, 0, 0);
58 if (IS_ERR(tfm))
59 return PTR_ERR(tfm);
60
> 61 desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> 62 digest_size = crypto_shash_digestsize(tfm);
63
64 digest = kmalloc(digest_size, GFP_KERNEL);
65 if (!digest) {
66 ret = -ENOMEM;
67 goto error;
68 }
69
70 desc = kzalloc(desc_size, GFP_KERNEL);
71 if (!desc) {
72 ret = -ENOMEM;
73 goto error;
74 }
75
76 desc->tfm = tfm;
77
> 78 ret = crypto_shash_digest(desc, policy, policy_len, digest);
79 if (ret < 0)
80 goto error;
81
82 *policy_hash_len = digest_size;
83 *policy_hash = digest;
84 digest = NULL;
85
86 error:
87 kfree(desc);
88 kfree(digest);
89
> 90 crypto_free_shash(tfm);
91
92 return ret;
93 }
94
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Lakshmi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on integrity/next-integrity]
[cannot apply to pcmoore-selinux/next security/next-testing linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Lakshmi-Ramasubramanian/LSM-Measure-security-module-state/20200718-063111
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: x86_64-randconfig-s022-20200717 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-49-g707c5017-dirty
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> security/selinux/ss/services.c:3737:5: sparse: sparse: symbol 'security_read_selinux_policy' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 7/17/20 8:14 PM, kernel test robot wrote: > Hi Lakshmi, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on integrity/next-integrity] > [cannot apply to pcmoore-selinux/next security/next-testing linus/master v5.8-rc5 next-20200717] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] Thank you for catching this. I did not see these failures with the compiler and make options I'd used. Am able to reproduce the errors with the instructions you'd provided. Will post the updated patches shortly. thanks, -lakshmi
On Fri, Jul 17, 2020 at 6:28 PM Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote: > > SELinux configuration and policy are some of the critical data for this > security module that needs to be measured. To enable this measurement > SELinux needs to implement the interface function, > security_measure_data(), that the LSM can call. > > Define the security_measure_data() function in SELinux to measure SELinux > configuration and policy. Call this function to measure SELinux data > when there is a change in the security module's state. > > Sample measurement of SELinux state and hash of the policy: > > 10 e32e...5ac3 ima-buf sha256:86e8...4594 selinux-state 656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574706565723d313b6f70656e7065726d3d313b657874736f636b636c6173733d313b616c776179736e6574776f726b3d303b6367726f75707365636c6162656c3d313b6e6e706e6f737569647472616e736974696f6e3d313b67656e66737365636c6162656c73796d6c696e6b3d303b > 10 f4a7...9408 ima-buf sha256:4941...68fc selinux-policy-hash 8d1d...1834 > > To verify the measurement check the following: > > Execute the following command to extract the measured data > from the IMA log for SELinux configuration (selinux-state). > > cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep -m 1 "selinux-state" | cut -d' ' -f 6 | xxd -r -p > > The output should be the list of key-value pairs. For example, > enabled=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0; > > To verify the measured data with the current SELinux state: > > => enabled should be set to 1 if /sys/fs/selinux folder exists, > 0 otherwise > > For other entries, compare the integer value in the files > => /sys/fs/selinux/enforce > => /sys/fs/selinux/checkreqprot > And, each of the policy capabilities files under > => /sys/fs/selinux/policy_capabilities > > The data for selinux-policy-hash is the SHA256 hash of SELinux policy. > > To verify the measured data with the current SELinux policy run > the following commands and verify the output hash values match. > > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1 > > cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep -m 1 "selinux-policy-hash" | cut -d' ' -f 6 > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> > --- > diff --git a/security/selinux/measure.c b/security/selinux/measure.c > new file mode 100644 > index 000000000000..659011637ae7 > --- /dev/null > +++ b/security/selinux/measure.c > @@ -0,0 +1,155 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Measure SELinux state using IMA subsystem. > + */ > +#include <linux/ima.h> > +#include "security.h" > + > +/* Pre-allocated buffer used for measuring state */ > +static char *selinux_state_string; > +static size_t selinux_state_string_len; > +static char *str_format = "%s=%d;"; > +static int selinux_state_count; > + > +void __init selinux_init_measurement(void) > +{ > + int i; > + > + /* > + * enabled > + * enforcing > + * checkreqport checkreqprot (spelling) What about initialized? Or do you consider that to be implicitly true/1 else we wouldn't be taking a measurement? Only caveat there is that it provides one more means of disabling measurements (at the same time as disabling enforcement) by setting it to false/0 via kernel write flaw. > + * All policy capability flags > + */ > + selinux_state_count = 3 + __POLICYDB_CAPABILITY_MAX; > + > + selinux_state_string_len = snprintf(NULL, 0, str_format, > + "enabled", 0); > + selinux_state_string_len += snprintf(NULL, 0, str_format, > + "enforcing", 0); > + selinux_state_string_len += snprintf(NULL, 0, str_format, > + "checkreqprot", 0); > + for (i = 3; i < selinux_state_count; i++) { > + selinux_state_string_len += > + snprintf(NULL, 0, str_format, > + selinux_policycap_names[i-3], 0); > + } What's the benefit of this pattern versus just making the loop go from 0 to __POLICYDB_CAPABILITY_MAX and using selinux_policycap_names[i]? > +void selinux_measure_state(struct selinux_state *selinux_state) > +{ > + void *policy = NULL; > + void *policy_hash = NULL; > + size_t curr, buflen; > + int i, policy_hash_len, rc = 0; > + > + if (!selinux_initialized(selinux_state)) { > + pr_warn("%s: SELinux not yet initialized.\n", __func__); > + return; > + } We could measure the global state variables before full SELinux initialization (i.e. policy load). Only the policy hash depends on having loaded the policy. > + > + if (!selinux_state_string) { > + pr_warn("%s: Buffer for state not allocated.\n", __func__); > + return; > + } > + > + curr = snprintf(selinux_state_string, selinux_state_string_len, > + str_format, "enabled", > + !selinux_disabled(selinux_state)); > + curr += snprintf((selinux_state_string + curr), > + (selinux_state_string_len - curr), > + str_format, "enforcing", > + enforcing_enabled(selinux_state)); > + curr += snprintf((selinux_state_string + curr), > + (selinux_state_string_len - curr), > + str_format, "checkreqprot", > + selinux_checkreqprot(selinux_state)); > + > + for (i = 3; i < selinux_state_count; i++) { > + curr += snprintf((selinux_state_string + curr), > + (selinux_state_string_len - curr), > + str_format, > + selinux_policycap_names[i - 3], > + selinux_state->policycap[i - 3]); > + } Same question here as for the previous loop; seems cleaner to go from 0 to __POLICYDB_CAPABILITY_MAX and use [i]. What public git tree / branch would you recommend trying to use your patches against? Didn't seem to apply to any of the obvious ones.
On 7/20/20 7:31 AM, Stephen Smalley wrote: >> +void __init selinux_init_measurement(void) >> +{ >> + int i; >> + >> + /* >> + * enabled >> + * enforcing >> + * checkreqport > > checkreqprot (spelling) :( - will fix that. > > What about initialized? Or do you consider that to be implicitly > true/1 else we wouldn't be taking a measurement? Only caveat there is > that it provides one more means of disabling measurements (at the same > time as disabling enforcement) by setting it to false/0 via kernel > write flaw. Yes - I was thinking measuring SELinux state would be meaningful only when initialized is set to true/1. I can include "initialized" as well in the measurement. > >> + * All policy capability flags >> + */ >> + selinux_state_count = 3 + __POLICYDB_CAPABILITY_MAX; >> + >> + selinux_state_string_len = snprintf(NULL, 0, str_format, >> + "enabled", 0); >> + selinux_state_string_len += snprintf(NULL, 0, str_format, >> + "enforcing", 0); >> + selinux_state_string_len += snprintf(NULL, 0, str_format, >> + "checkreqprot", 0); >> + for (i = 3; i < selinux_state_count; i++) { >> + selinux_state_string_len += >> + snprintf(NULL, 0, str_format, >> + selinux_policycap_names[i-3], 0); >> + } > > What's the benefit of this pattern versus just making the loop go from > 0 to __POLICYDB_CAPABILITY_MAX and using selinux_policycap_names[i]? No real benefit - I was just trying to use selinux_state_count. I'll change the loop to go from 0 to POLICY_CAP_MAX > >> +void selinux_measure_state(struct selinux_state *selinux_state) >> +{ >> + void *policy = NULL; >> + void *policy_hash = NULL; >> + size_t curr, buflen; >> + int i, policy_hash_len, rc = 0; >> + >> + if (!selinux_initialized(selinux_state)) { >> + pr_warn("%s: SELinux not yet initialized.\n", __func__); >> + return; >> + } > > We could measure the global state variables before full SELinux > initialization (i.e. policy load). > Only the policy hash depends on having loaded the policy. Thanks for the information. I'll measure the state variables always and measure policy only if "initialized" is true/1. > >> + >> + if (!selinux_state_string) { >> + pr_warn("%s: Buffer for state not allocated.\n", __func__); >> + return; >> + } >> + >> + curr = snprintf(selinux_state_string, selinux_state_string_len, >> + str_format, "enabled", >> + !selinux_disabled(selinux_state)); >> + curr += snprintf((selinux_state_string + curr), >> + (selinux_state_string_len - curr), >> + str_format, "enforcing", >> + enforcing_enabled(selinux_state)); >> + curr += snprintf((selinux_state_string + curr), >> + (selinux_state_string_len - curr), >> + str_format, "checkreqprot", >> + selinux_checkreqprot(selinux_state)); >> + >> + for (i = 3; i < selinux_state_count; i++) { >> + curr += snprintf((selinux_state_string + curr), >> + (selinux_state_string_len - curr), >> + str_format, >> + selinux_policycap_names[i - 3], >> + selinux_state->policycap[i - 3]); >> + } > > Same question here as for the previous loop; seems cleaner to go from > 0 to __POLICYDB_CAPABILITY_MAX and use [i]. Will change it. > > What public git tree / branch would you recommend trying to use your > patches against? Didn't seem to apply to any of the obvious ones. > Please try it on Mimi's next-integrity branch https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/log/?h=next-integrity You can try it on Linus's mainline as well if you apply the following patch first (have mentioned that in the Cover letter as well) https://patchwork.kernel.org/patch/11612989/ Thanks for trying out the changes. Please let me know the defects you find. Just to let you know - I am making the following change (will update in the next patch): => Save the last policy hash and state string in selinux_state struct. => Measure policy and hash only if it has changed since the last measurement. => Also, suffix the IMA event name used with time stamp. For example, 10 e32e...5ac3 ima-buf sha256:86e8...4594 selinux-state-1595257807:874963248 656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574706565723d313b6f70656e7065726d3d313b657874736f636b636c6173733d313b616c776179736e6574776f726b3d303b6367726f75707365636c6162656c3d313b6e6e706e6f737569647472616e736974696f6e3d313b67656e66737365636c6162656c73796d6c696e6b3d303b 10 f4a7...9408 ima-buf sha256:4941...68fc selinux-policy-hash-1595257807:874963248 8d1d...1834 The above will ensure the following sequence will be measured: #1 State A - Measured #2 Change from State A to State B - Measured #3 Change from State B back to State A - Since the measured data is same as in #1, the change will be measured only if the event name is different between #1 and #3 thanks, -lakshmi
On Mon, Jul 20, 2020 at 11:17 AM Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote: > Thanks for trying out the changes. Please let me know the defects you find. > > Just to let you know - I am making the following change (will update in > the next patch): > > => Save the last policy hash and state string in selinux_state struct. > => Measure policy and hash only if it has changed since the last > measurement. > => Also, suffix the IMA event name used with time stamp. For example, > > 10 e32e...5ac3 ima-buf sha256:86e8...4594 > selinux-state-1595257807:874963248 > 656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574706565723d313b6f70656e7065726d3d313b657874736f636b636c6173733d313b616c776179736e6574776f726b3d303b6367726f75707365636c6162656c3d313b6e6e706e6f737569647472616e736974696f6e3d313b67656e66737365636c6162656c73796d6c696e6b3d303b > > 10 f4a7...9408 ima-buf sha256:4941...68fc > selinux-policy-hash-1595257807:874963248 > 8d1d...1834 > > The above will ensure the following sequence will be measured: > #1 State A - Measured > #2 Change from State A to State B - Measured > #3 Change from State B back to State A - Since the measured data is > same as in #1, the change will be measured only if the event name is > different between #1 and #3 Perhaps the timestamp / sequence number should be part of the hashed data instead of the event name? I can see the appraiser wanting to know two things: 1) The current state of the system (e.g. is it enforcing, is the currently loaded policy the expected one?). 2) Has the system ever been in an unexpected state (e.g. was it temporarily switched to permissive or had an unexpected policy loaded?) I applied the patch series on top of the next-integrity branch, added measure func=LSM_STATE to ima-policy, and booted that kernel. I get the following entries in ascii_runtime_measurements, but seemingly missing the final field: 10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24 selinux-state 10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318 selinux-policy-hash Thus, I cannot verify. What am I missing?
On Mon, 2020-07-20 at 13:06 -0400, Stephen Smalley wrote: > > > I applied the patch series on top of the next-integrity branch, added > measure func=LSM_STATE to ima-policy, and booted that kernel. I get > the following entries in ascii_runtime_measurements, but seemingly > missing the final field: > > 10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng > sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24 > selinux-state > 10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng > sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318 > selinux-policy-hash > > Thus, I cannot verify. What am I missing? Missing is "template=ima-buf" on the policy rule. Tyler's patch set just added some support for verifying the policy. Refer to ima_validate_rule(). There are still some things missing. For example, nayna noticed that making sure that asymmetric key support is enabled. Another example is requiring "template=" for any of the buffer measurements. Template names can be defined dynamically, so it will need to support either format: measure func=KEXEC_CMDLINE template=ima-buf measure func=KEXEC_CMDLINE template=d-ng|n-ng|buf Mimi
On 7/20/20 10:06 AM, Stephen Smalley wrote: >> The above will ensure the following sequence will be measured: >> #1 State A - Measured >> #2 Change from State A to State B - Measured >> #3 Change from State B back to State A - Since the measured data is >> same as in #1, the change will be measured only if the event name is >> different between #1 and #3 > > Perhaps the timestamp / sequence number should be part of the hashed > data instead of the event name? If the timestamp/seqno is part of the hashed data, on every call to measure IMA will add a new entry in the IMA log. This would fill up the IMA log - even when there is no change in the measured data. To avoid that I keep the last measurement in SELinux and measure only when there is a change with the timestamp in the event name. > I can see the appraiser wanting to know two things: > 1) The current state of the system (e.g. is it enforcing, is the > currently loaded policy the expected one?). > 2) Has the system ever been in an unexpected state (e.g. was it > temporarily switched to permissive or had an unexpected policy > loaded?) Yes - you are right. The appraiser will have to look at the entire IMA log (and the corresponding TPM PCR data) to know the above. Time t0 => State of the system measured Time tn => State changed and the new state measured Time tm => State changed again and the new state measured. Say, the measurement at "Time tn" was an illegal change, the appraiser would know. > > I applied the patch series on top of the next-integrity branch, added > measure func=LSM_STATE to ima-policy, and booted that kernel. I get > the following entries in ascii_runtime_measurements, but seemingly > missing the final field: > > 10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng > sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24 > selinux-state > 10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng > sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318 > selinux-policy-hash > > Thus, I cannot verify. What am I missing? > Looks like the template used is ima-ng which doesn't include the measured buffer. Please set template to "ima-buf" in the policy. For example, measure func=LSM_STATE template=ima-buf thanks, -lakshmi
On Mon, Jul 20, 2020 at 1:34 PM Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote: > > On 7/20/20 10:06 AM, Stephen Smalley wrote: > > >> The above will ensure the following sequence will be measured: > >> #1 State A - Measured > >> #2 Change from State A to State B - Measured > >> #3 Change from State B back to State A - Since the measured data is > >> same as in #1, the change will be measured only if the event name is > >> different between #1 and #3 > > > > Perhaps the timestamp / sequence number should be part of the hashed > > data instead of the event name? > > If the timestamp/seqno is part of the hashed data, on every call to > measure IMA will add a new entry in the IMA log. This would fill up the > IMA log - even when there is no change in the measured data. > > To avoid that I keep the last measurement in SELinux and measure only > when there is a change with the timestamp in the event name. > > > I can see the appraiser wanting to know two things: > > 1) The current state of the system (e.g. is it enforcing, is the > > currently loaded policy the expected one?). > > 2) Has the system ever been in an unexpected state (e.g. was it > > temporarily switched to permissive or had an unexpected policy > > loaded?) > > Yes - you are right. > The appraiser will have to look at the entire IMA log (and the > corresponding TPM PCR data) to know the above. > > Time t0 => State of the system measured > Time tn => State changed and the new state measured > Time tm => State changed again and the new state measured. > > Say, the measurement at "Time tn" was an illegal change, the appraiser > would know. > > > > > I applied the patch series on top of the next-integrity branch, added > > measure func=LSM_STATE to ima-policy, and booted that kernel. I get > > the following entries in ascii_runtime_measurements, but seemingly > > missing the final field: > > > > 10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng > > sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24 > > selinux-state > > 10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng > > sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318 > > selinux-policy-hash > > > > Thus, I cannot verify. What am I missing? > > > > Looks like the template used is ima-ng which doesn't include the > measured buffer. Please set template to "ima-buf" in the policy. > > For example, > measure func=LSM_STATE template=ima-buf It seems like one shouldn't need to manually specify it if it is the only template that yields a useful result for the LSM_STATE function?
On Mon, Jul 20, 2020 at 1:40 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Mon, Jul 20, 2020 at 1:34 PM Lakshmi Ramasubramanian > <nramas@linux.microsoft.com> wrote: > > > > On 7/20/20 10:06 AM, Stephen Smalley wrote: > > > > >> The above will ensure the following sequence will be measured: > > >> #1 State A - Measured > > >> #2 Change from State A to State B - Measured > > >> #3 Change from State B back to State A - Since the measured data is > > >> same as in #1, the change will be measured only if the event name is > > >> different between #1 and #3 > > > > > > Perhaps the timestamp / sequence number should be part of the hashed > > > data instead of the event name? > > > > If the timestamp/seqno is part of the hashed data, on every call to > > measure IMA will add a new entry in the IMA log. This would fill up the > > IMA log - even when there is no change in the measured data. > > > > To avoid that I keep the last measurement in SELinux and measure only > > when there is a change with the timestamp in the event name. > > > > > I can see the appraiser wanting to know two things: > > > 1) The current state of the system (e.g. is it enforcing, is the > > > currently loaded policy the expected one?). > > > 2) Has the system ever been in an unexpected state (e.g. was it > > > temporarily switched to permissive or had an unexpected policy > > > loaded?) > > > > Yes - you are right. > > The appraiser will have to look at the entire IMA log (and the > > corresponding TPM PCR data) to know the above. > > > > Time t0 => State of the system measured > > Time tn => State changed and the new state measured > > Time tm => State changed again and the new state measured. > > > > Say, the measurement at "Time tn" was an illegal change, the appraiser > > would know. > > > > > > > > I applied the patch series on top of the next-integrity branch, added > > > measure func=LSM_STATE to ima-policy, and booted that kernel. I get > > > the following entries in ascii_runtime_measurements, but seemingly > > > missing the final field: > > > > > > 10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng > > > sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24 > > > selinux-state > > > 10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng > > > sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318 > > > selinux-policy-hash > > > > > > Thus, I cannot verify. What am I missing? > > > > > > > Looks like the template used is ima-ng which doesn't include the > > measured buffer. Please set template to "ima-buf" in the policy. > > > > For example, > > measure func=LSM_STATE template=ima-buf > > It seems like one shouldn't need to manually specify it if it is the > only template that yields a useful result for the LSM_STATE function? Actually, if we used ima-ng template for selinux-policy-hash, then instead of needing to hash the policy first and passing the hash to IMA, we could just pass the policy as the buffer and IMA would take care of the hashing, right? And we only need to use ima-buf for the selinux-state if we want the measurement list to include the string value that was hashed; if we just want to compare against a known-good, it would suffice to use ima-ng for it as well, right?
On 7/20/20 10:49 AM, Stephen Smalley wrote: >>> >>> Looks like the template used is ima-ng which doesn't include the >>> measured buffer. Please set template to "ima-buf" in the policy. >>> >>> For example, >>> measure func=LSM_STATE template=ima-buf >> >> It seems like one shouldn't need to manually specify it if it is the >> only template that yields a useful result for the LSM_STATE function? > > Actually, if we used ima-ng template for selinux-policy-hash, then > instead of needing to hash the policy > first and passing the hash to IMA, we could just pass the policy as > the buffer and IMA would take care of the hashing, right? That is correct. The IMA hook I've added to measure LSM structures is a generic one that can be used by any security module (SM). I feel it would be better to not have policy or state or any such SM specific logic in IMA, but leave that to the individual SM to handle. What do you think? > And we only need to use ima-buf for the selinux-state if we want the > measurement list to include the string value that > was hashed; if we just want to compare against a known-good, it would > suffice to use ima-ng for it as well, right? > -lakshmi
On Mon, Jul 20, 2020 at 2:27 PM Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote: > > On 7/20/20 10:49 AM, Stephen Smalley wrote: > > >>> > >>> Looks like the template used is ima-ng which doesn't include the > >>> measured buffer. Please set template to "ima-buf" in the policy. > >>> > >>> For example, > >>> measure func=LSM_STATE template=ima-buf > >> > >> It seems like one shouldn't need to manually specify it if it is the > >> only template that yields a useful result for the LSM_STATE function? > > > > Actually, if we used ima-ng template for selinux-policy-hash, then > > instead of needing to hash the policy > > first and passing the hash to IMA, we could just pass the policy as > > the buffer and IMA would take care of the hashing, right? > > That is correct. > > The IMA hook I've added to measure LSM structures is a generic one that > can be used by any security module (SM). I feel it would be better to > not have policy or state or any such SM specific logic in IMA, but leave > that to the individual SM to handle. > > What do you think? It is correct to remain security module agnostic. However, I think you can remain LSM-neutral while still avoiding the double hashing of the policy here. Can't you just pass in the policy itself as the buffer and let IMA hash it? Then you can let the policy author decide on the template to be used (ima-buf versus ima-ng). If you want to support the use of different templates for different "kinds" of LSM state (e.g. state versus policy) you could either provide two funcs (LSM_STATE, LSM_POLICY) or otherwise support selection based on some other attribute.
On 7/20/20 11:44 AM, Stephen Smalley wrote: >>> >>> Actually, if we used ima-ng template for selinux-policy-hash, then >>> instead of needing to hash the policy >>> first and passing the hash to IMA, we could just pass the policy as >>> the buffer and IMA would take care of the hashing, right? >> >> That is correct. >> >> The IMA hook I've added to measure LSM structures is a generic one that >> can be used by any security module (SM). I feel it would be better to >> not have policy or state or any such SM specific logic in IMA, but leave >> that to the individual SM to handle. >> >> What do you think? > > It is correct to remain security module agnostic. However, I think > you can remain LSM-neutral while still avoiding the double hashing of > the policy here. Can't you just pass in the policy itself as the > buffer and let IMA hash it? Yes - that is an option. If I do that then, as you have stated below, we'll need to two funcs - one that will only add the hash but not the entire data payload in the IMA log (i.e., "ima-ng") and, the other that handles hashing and including date payload (i.e., "ima-buf"). Then you can let the policy author decide > on the template to be used (ima-buf versus ima-ng). If you want to > support the use of different templates for different "kinds" of LSM > state (e.g. state versus policy) you could either provide two funcs > (LSM_STATE, LSM_POLICY) or otherwise support selection based on some > other attribute. > I can do the above. -lakshmi
diff --git a/security/selinux/Makefile b/security/selinux/Makefile index 4d8e0e8adf0b..83d512116341 100644 --- a/security/selinux/Makefile +++ b/security/selinux/Makefile @@ -16,6 +16,8 @@ selinux-$(CONFIG_NETLABEL) += netlabel.o selinux-$(CONFIG_SECURITY_INFINIBAND) += ibpkey.o +selinux-$(CONFIG_IMA) += measure.o + ccflags-y := -I$(srctree)/security/selinux -I$(srctree)/security/selinux/include $(addprefix $(obj)/,$(selinux-y)): $(obj)/flask.h diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index efa6108b1ce9..cda1d328339f 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -7259,6 +7259,8 @@ static __init int selinux_init(void) fs_validate_description("selinux", selinux_fs_parameters); + selinux_init_measurement(); + return 0; } @@ -7284,6 +7286,7 @@ DEFINE_LSM(selinux) = { .enabled = &selinux_enabled_boot, .blobs = &selinux_blob_sizes, .init = selinux_init, + .security_measure_data = selinux_measure_data, }; #if defined(CONFIG_NETFILTER) @@ -7394,6 +7397,7 @@ int selinux_disable(struct selinux_state *state) } selinux_mark_disabled(state); + selinux_measure_state(state); pr_info("SELinux: Disabled at runtime.\n"); diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index b0e02cfe3ce1..1f41c16a4845 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -222,16 +222,34 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void) return state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS]; } +static inline bool selinux_checkreqprot(const struct selinux_state *state) +{ + return READ_ONCE(state->checkreqprot); +} + int security_mls_enabled(struct selinux_state *state); int security_load_policy(struct selinux_state *state, void *data, size_t len); int security_read_policy(struct selinux_state *state, void **data, size_t *len); +int security_read_policy_kernel(struct selinux_state *state, + void **data, size_t *len); size_t security_policydb_len(struct selinux_state *state); int security_policycap_supported(struct selinux_state *state, unsigned int req_cap); +#ifdef CONFIG_IMA +extern void __init selinux_init_measurement(void); +extern void selinux_measure_data(void); +extern void selinux_measure_state(struct selinux_state *selinux_state); +#else +static inline void __init selinux_init_measurement(void) {} +static inline void selinux_measure_data(void) {} +static inline void selinux_measure_state( + struct selinux_state *selinux_state) {} +#endif + #define SEL_VEC_MAX 32 struct av_decision { u32 allowed; diff --git a/security/selinux/measure.c b/security/selinux/measure.c new file mode 100644 index 000000000000..659011637ae7 --- /dev/null +++ b/security/selinux/measure.c @@ -0,0 +1,155 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Measure SELinux state using IMA subsystem. + */ +#include <linux/ima.h> +#include "security.h" + +/* Pre-allocated buffer used for measuring state */ +static char *selinux_state_string; +static size_t selinux_state_string_len; +static char *str_format = "%s=%d;"; +static int selinux_state_count; + +void __init selinux_init_measurement(void) +{ + int i; + + /* + * enabled + * enforcing + * checkreqport + * All policy capability flags + */ + selinux_state_count = 3 + __POLICYDB_CAPABILITY_MAX; + + selinux_state_string_len = snprintf(NULL, 0, str_format, + "enabled", 0); + selinux_state_string_len += snprintf(NULL, 0, str_format, + "enforcing", 0); + selinux_state_string_len += snprintf(NULL, 0, str_format, + "checkreqprot", 0); + for (i = 3; i < selinux_state_count; i++) { + selinux_state_string_len += + snprintf(NULL, 0, str_format, + selinux_policycap_names[i-3], 0); + } + ++selinux_state_string_len; + + selinux_state_string = kzalloc(selinux_state_string_len, GFP_KERNEL); + if (!selinux_state_string) { + pr_warn("Failed to alloc memory for SELinux measurement\n"); + selinux_state_string_len = 0; + } +} + +static int selinux_hash_policy(const char *hash_alg_name, + void *policy, size_t policy_len, + void **policy_hash, int *policy_hash_len) +{ + struct crypto_shash *tfm; + struct shash_desc *desc = NULL; + void *digest = NULL; + int desc_size; + int digest_size; + int ret = 0; + + tfm = crypto_alloc_shash(hash_alg_name, 0, 0); + if (IS_ERR(tfm)) + return PTR_ERR(tfm); + + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc); + digest_size = crypto_shash_digestsize(tfm); + + digest = kmalloc(digest_size, GFP_KERNEL); + if (!digest) { + ret = -ENOMEM; + goto error; + } + + desc = kzalloc(desc_size, GFP_KERNEL); + if (!desc) { + ret = -ENOMEM; + goto error; + } + + desc->tfm = tfm; + + ret = crypto_shash_digest(desc, policy, policy_len, digest); + if (ret < 0) + goto error; + + *policy_hash_len = digest_size; + *policy_hash = digest; + digest = NULL; + +error: + kfree(desc); + kfree(digest); + + crypto_free_shash(tfm); + + return ret; +} + +void selinux_measure_state(struct selinux_state *selinux_state) +{ + void *policy = NULL; + void *policy_hash = NULL; + size_t curr, buflen; + int i, policy_hash_len, rc = 0; + + if (!selinux_initialized(selinux_state)) { + pr_warn("%s: SELinux not yet initialized.\n", __func__); + return; + } + + if (!selinux_state_string) { + pr_warn("%s: Buffer for state not allocated.\n", __func__); + return; + } + + curr = snprintf(selinux_state_string, selinux_state_string_len, + str_format, "enabled", + !selinux_disabled(selinux_state)); + curr += snprintf((selinux_state_string + curr), + (selinux_state_string_len - curr), + str_format, "enforcing", + enforcing_enabled(selinux_state)); + curr += snprintf((selinux_state_string + curr), + (selinux_state_string_len - curr), + str_format, "checkreqprot", + selinux_checkreqprot(selinux_state)); + + for (i = 3; i < selinux_state_count; i++) { + curr += snprintf((selinux_state_string + curr), + (selinux_state_string_len - curr), + str_format, + selinux_policycap_names[i - 3], + selinux_state->policycap[i - 3]); + } + + if (curr >= 0 && curr < selinux_state_string_len) + ima_lsm_state("selinux-state", selinux_state_string, curr); + else { + rc = -EINVAL; + goto out; + } + + rc = security_read_policy_kernel(selinux_state, &policy, &buflen); + if (!rc) + rc = selinux_hash_policy("sha256", policy, buflen, + &policy_hash, &policy_hash_len); + if (!rc) + ima_lsm_state("selinux-policy-hash", policy_hash, + policy_hash_len); + +out: + vfree(policy); + kfree(policy_hash); +} + +void selinux_measure_data(void) +{ + selinux_measure_state(&selinux_state); +} diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 4781314c2510..b1f70739d709 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -173,6 +173,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf, from_kuid(&init_user_ns, audit_get_loginuid(current)), audit_get_sessionid(current)); enforcing_set(state, new_value); + selinux_measure_state(&selinux_state); if (new_value) avc_ss_reset(state->avc, 0); selnl_notify_setenforce(new_value); diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index ef0afd878bfc..79a6b462f1fe 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -3720,14 +3720,22 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state, } #endif /* CONFIG_NETLABEL */ +static int security_read_policy_len(struct selinux_state *state, size_t *len) +{ + if (!selinux_initialized(state)) + return -EINVAL; + + *len = security_policydb_len(state); + return 0; +} + /** * security_read_policy - read the policy. * @data: binary policy data * @len: length of data in bytes - * */ -int security_read_policy(struct selinux_state *state, - void **data, size_t *len) +int security_read_selinux_policy(struct selinux_state *state, + void **data, size_t *len) { struct policydb *policydb = &state->ss->policydb; int rc; @@ -3736,12 +3744,6 @@ int security_read_policy(struct selinux_state *state, if (!selinux_initialized(state)) return -EINVAL; - *len = security_policydb_len(state); - - *data = vmalloc_user(*len); - if (!*data) - return -ENOMEM; - fp.data = *data; fp.len = *len; @@ -3754,5 +3756,51 @@ int security_read_policy(struct selinux_state *state, *len = (unsigned long)fp.data - (unsigned long)*data; return 0; +} + +/** + * security_read_policy - read the policy. + * @data: binary policy data + * @len: length of data in bytes + * + */ +int security_read_policy(struct selinux_state *state, + void **data, size_t *len) +{ + int rc; + + rc = security_read_policy_len(state, len); + if (rc) + return rc; + + *data = vmalloc_user(*len); + if (!*data) + return -ENOMEM; + + return security_read_selinux_policy(state, data, len); +} + +/** + * security_read_policy_kernel - read the policy. + * @data: binary policy data + * @len: length of data in bytes + * + * Allocates kernel memory for reading SELinux policy. + * This function is for internal use only and should not + * be used for returning data to user space + */ +int security_read_policy_kernel(struct selinux_state *state, + void **data, size_t *len) +{ + int rc; + + rc = security_read_policy_len(state, len); + if (rc) + return rc; + + *data = vmalloc(*len); + if (!*data) + return -ENOMEM; + return security_read_selinux_policy(state, data, len); }
SELinux configuration and policy are some of the critical data for this security module that needs to be measured. To enable this measurement SELinux needs to implement the interface function, security_measure_data(), that the LSM can call. Define the security_measure_data() function in SELinux to measure SELinux configuration and policy. Call this function to measure SELinux data when there is a change in the security module's state. Sample measurement of SELinux state and hash of the policy: 10 e32e...5ac3 ima-buf sha256:86e8...4594 selinux-state 656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574706565723d313b6f70656e7065726d3d313b657874736f636b636c6173733d313b616c776179736e6574776f726b3d303b6367726f75707365636c6162656c3d313b6e6e706e6f737569647472616e736974696f6e3d313b67656e66737365636c6162656c73796d6c696e6b3d303b 10 f4a7...9408 ima-buf sha256:4941...68fc selinux-policy-hash 8d1d...1834 To verify the measurement check the following: Execute the following command to extract the measured data from the IMA log for SELinux configuration (selinux-state). cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep -m 1 "selinux-state" | cut -d' ' -f 6 | xxd -r -p The output should be the list of key-value pairs. For example, enabled=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0; To verify the measured data with the current SELinux state: => enabled should be set to 1 if /sys/fs/selinux folder exists, 0 otherwise For other entries, compare the integer value in the files => /sys/fs/selinux/enforce => /sys/fs/selinux/checkreqprot And, each of the policy capabilities files under => /sys/fs/selinux/policy_capabilities The data for selinux-policy-hash is the SHA256 hash of SELinux policy. To verify the measured data with the current SELinux policy run the following commands and verify the output hash values match. sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1 cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep -m 1 "selinux-policy-hash" | cut -d' ' -f 6 Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> --- security/selinux/Makefile | 2 + security/selinux/hooks.c | 4 + security/selinux/include/security.h | 18 ++++ security/selinux/measure.c | 155 ++++++++++++++++++++++++++++ security/selinux/selinuxfs.c | 1 + security/selinux/ss/services.c | 66 ++++++++++-- 6 files changed, 237 insertions(+), 9 deletions(-) create mode 100644 security/selinux/measure.c