diff mbox

[2/2] ima: turn on force option for FUSE in builtin policies

Message ID 7a89ce9a7b8264f83fa5d61e146c01571017cca0.1515682581.git.dongsu@kinvolk.io (mailing list archive)
State New, archived
Headers show

Commit Message

Dongsu Park Jan. 11, 2018, 7:51 p.m. UTC 
In case of FUSE filesystem, cached integrity results in IMA could be
reused, when the userspace FUSE process has changed the
underlying files. To be able to avoid such cases, we need to turn on
the force option in builtin policies, for actions of measure and
appraise. Then integrity values become re-measured and re-appraised.
In that way, cached integrity results won't be used.

This patch depends on the patch "ima: define a new policy option
named force" by Mimi. [1]

How to test the force option written by Alban:

Comments

Mimi Zohar Jan. 12, 2018, 3:33 p.m. UTC | #1 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index fddef8f8..8de40d85 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -127,6 +127,7 @@ static struct ima_rule_entry default_measurement_rules[] __ro_after_init = {
>  	{.action = MEASURE, .func = MODULE_CHECK, .flags = IMA_FUNC},
>  	{.action = MEASURE, .func = FIRMWARE_CHECK, .flags = IMA_FUNC},
>  	{.action = MEASURE, .func = POLICY_CHECK, .flags = IMA_FUNC},
> +	{.action = MEASURE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
>  };

Depending on the ordering of the policies and the number of "actions",
this works.  It also matches all hooks, not only those in the
default_measurement_rule policy.

Although the rules are walked sequentially, there is an optimization
in ima_match_rules(), which ends walking the list early, as soon as
the last "action" rule is matched.  Look at "actmask".

>  static struct ima_rule_entry default_appraise_rules[] __ro_after_init = {
> @@ -154,6 +155,7 @@ static struct ima_rule_entry default_appraise_rules[] __ro_after_init = {
>  	{.action = APPRAISE, .fowner = GLOBAL_ROOT_UID, .fowner_op = &uid_eq,
>  	 .flags = IMA_FOWNER | IMA_DIGSIG_REQUIRED},
>  #endif
> +	{.action = APPRAISE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
>  };
> 
>  static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {

This rule applies to the secure-boot rules as well.  (How likely is it
to load a kernel module, kexec from a fuse filsystem?)  However, after
replacing the builtin policies with a custom policy, the custom policy
might not contain the requirement to re-appraise fuse files.

With the "lockdown" patches, booting with secure-boot enabled, and
after loading a custom policy, the "secure-boot" rules are still
enabled.  But now if the custom policy does not require fuse files to
be re-appraised, the secure boot rules will not require fuse files to
be re-appraised either.

This patch is simple and straight forward.  It would be nice if it
worked in all cases.  Unfortunately, there are a number of situations,
as described, that will not work.

Mimi
kernel test robot Jan. 14, 2018, 7:09 p.m. UTC | #2 
Hi Dongsu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.15-rc7 next-20180112]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dongsu-Park/turn-on-force-option-for-FUSE-in-builtin-policies/20180115-015830
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0
reproduce:
        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
        make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

>> security/integrity/ima/ima_policy.c:130:74: error: 'IMA_FORCE' undeclared here (not in a function); did you mean 'IMA_FUNC'?
     {.action = MEASURE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
                                                                             ^~~~~~~~~
                                                                             IMA_FUNC
   security/integrity/ima/ima_policy.c:158:73: error: invalid operands to binary | (have 'int' and 'struct ima_rule_entry *')
     {.action = APPRAISE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
                                                                            ^
   security/integrity/ima/ima_policy.c:29:21: warning: initialization makes integer from pointer without a cast [-Wint-conversion]
    #define IMA_FSMAGIC 0x0004
                        ^
   security/integrity/ima/ima_policy.c:158:61: note: in expansion of macro 'IMA_FSMAGIC'
     {.action = APPRAISE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
                                                                ^~~~~~~~~~~
   security/integrity/ima/ima_policy.c:29:21: note: (near initialization for 'default_appraise_rules[14].flags')
    #define IMA_FSMAGIC 0x0004
                        ^
   security/integrity/ima/ima_policy.c:158:61: note: in expansion of macro 'IMA_FSMAGIC'
     {.action = APPRAISE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
                                                                ^~~~~~~~~~~
   security/integrity/ima/ima_policy.c:29:21: error: initializer element is not constant
    #define IMA_FSMAGIC 0x0004
                        ^
   security/integrity/ima/ima_policy.c:158:61: note: in expansion of macro 'IMA_FSMAGIC'
     {.action = APPRAISE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
                                                                ^~~~~~~~~~~
   security/integrity/ima/ima_policy.c:29:21: note: (near initialization for 'default_appraise_rules[14].flags')
    #define IMA_FSMAGIC 0x0004
                        ^
   security/integrity/ima/ima_policy.c:158:61: note: in expansion of macro 'IMA_FSMAGIC'
     {.action = APPRAISE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
                                                                ^~~~~~~~~~~

vim +130 security/integrity/ima/ima_policy.c

   115	
   116	static struct ima_rule_entry default_measurement_rules[] __ro_after_init = {
   117		{.action = MEASURE, .func = MMAP_CHECK, .mask = MAY_EXEC,
   118		 .flags = IMA_FUNC | IMA_MASK},
   119		{.action = MEASURE, .func = BPRM_CHECK, .mask = MAY_EXEC,
   120		 .flags = IMA_FUNC | IMA_MASK},
   121		{.action = MEASURE, .func = FILE_CHECK, .mask = MAY_READ,
   122		 .uid = GLOBAL_ROOT_UID, .uid_op = &uid_eq,
   123		 .flags = IMA_FUNC | IMA_INMASK | IMA_EUID},
   124		{.action = MEASURE, .func = FILE_CHECK, .mask = MAY_READ,
   125		 .uid = GLOBAL_ROOT_UID, .uid_op = &uid_eq,
   126		 .flags = IMA_FUNC | IMA_INMASK | IMA_UID},
   127		{.action = MEASURE, .func = MODULE_CHECK, .flags = IMA_FUNC},
   128		{.action = MEASURE, .func = FIRMWARE_CHECK, .flags = IMA_FUNC},
   129		{.action = MEASURE, .func = POLICY_CHECK, .flags = IMA_FUNC},
 > 130		{.action = MEASURE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
   131	};
   132	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Dongsu Park Jan. 16, 2018, 10:47 a.m. UTC | #3 
Hi,

On Sun, Jan 14, 2018 at 8:09 PM, kbuild test robot <lkp@intel.com> wrote:
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.15-rc7 next-20180112]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

As already mentioned in the commit message, this patch depends on
patches that are not yet in the mainline, or not even in next-integrity.
So please make it excluded from kbuild.

Thanks,
Dongsu

> url:    https://github.com/0day-ci/linux/commits/Dongsu-Park/turn-on-force-option-for-FUSE-in-builtin-policies/20180115-015830
> config: xtensa-allmodconfig (attached as .config)
> compiler: xtensa-linux-gcc (GCC) 7.2.0
> reproduce:
>         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
>         make.cross ARCH=xtensa
>
> All errors (new ones prefixed by >>):
>
>>> security/integrity/ima/ima_policy.c:130:74: error: 'IMA_FORCE' undeclared here (not in a function); did you mean 'IMA_FUNC'?
>      {.action = MEASURE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
>                                                                              ^~~~~~~~~
>                                                                              IMA_FUNC
>    security/integrity/ima/ima_policy.c:158:73: error: invalid operands to binary | (have 'int' and 'struct ima_rule_entry *')
>      {.action = APPRAISE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
>                                                                             ^
>    security/integrity/ima/ima_policy.c:29:21: warning: initialization makes integer from pointer without a cast [-Wint-conversion]
>     #define IMA_FSMAGIC 0x0004
>                         ^
>    security/integrity/ima/ima_policy.c:158:61: note: in expansion of macro 'IMA_FSMAGIC'
>      {.action = APPRAISE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
>                                                                 ^~~~~~~~~~~
>    security/integrity/ima/ima_policy.c:29:21: note: (near initialization for 'default_appraise_rules[14].flags')
>     #define IMA_FSMAGIC 0x0004
>                         ^
>    security/integrity/ima/ima_policy.c:158:61: note: in expansion of macro 'IMA_FSMAGIC'
>      {.action = APPRAISE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
>                                                                 ^~~~~~~~~~~
>    security/integrity/ima/ima_policy.c:29:21: error: initializer element is not constant
>     #define IMA_FSMAGIC 0x0004
>                         ^
>    security/integrity/ima/ima_policy.c:158:61: note: in expansion of macro 'IMA_FSMAGIC'
>      {.action = APPRAISE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
>                                                                 ^~~~~~~~~~~
>    security/integrity/ima/ima_policy.c:29:21: note: (near initialization for 'default_appraise_rules[14].flags')
>     #define IMA_FSMAGIC 0x0004
>                         ^
>    security/integrity/ima/ima_policy.c:158:61: note: in expansion of macro 'IMA_FSMAGIC'
>      {.action = APPRAISE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
>                                                                 ^~~~~~~~~~~
>
> vim +130 security/integrity/ima/ima_policy.c
>
>    115
>    116  static struct ima_rule_entry default_measurement_rules[] __ro_after_init = {
>    117          {.action = MEASURE, .func = MMAP_CHECK, .mask = MAY_EXEC,
>    118           .flags = IMA_FUNC | IMA_MASK},
>    119          {.action = MEASURE, .func = BPRM_CHECK, .mask = MAY_EXEC,
>    120           .flags = IMA_FUNC | IMA_MASK},
>    121          {.action = MEASURE, .func = FILE_CHECK, .mask = MAY_READ,
>    122           .uid = GLOBAL_ROOT_UID, .uid_op = &uid_eq,
>    123           .flags = IMA_FUNC | IMA_INMASK | IMA_EUID},
>    124          {.action = MEASURE, .func = FILE_CHECK, .mask = MAY_READ,
>    125           .uid = GLOBAL_ROOT_UID, .uid_op = &uid_eq,
>    126           .flags = IMA_FUNC | IMA_INMASK | IMA_UID},
>    127          {.action = MEASURE, .func = MODULE_CHECK, .flags = IMA_FUNC},
>    128          {.action = MEASURE, .func = FIRMWARE_CHECK, .flags = IMA_FUNC},
>    129          {.action = MEASURE, .func = POLICY_CHECK, .flags = IMA_FUNC},
>  > 130          {.action = MEASURE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
>    131  };
>    132
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

====

The test I did was using a patched version of the memfs FUSE driver
[2][3] and two very simple "hello-world" programs [5] (prog1 prints
"hello world: 1" and prog2 prints "hello world: 2").

I copy prog1 and prog2 in the fuse-memfs mount point, execute them and
check the sha1 hash in
"/sys/kernel/security/ima/ascii_runtime_measurements".

My patch on the memfs FUSE driver added a backdoor command to serve
prog1 when the kernel asks for prog2 or vice-versa. In this way, I can
exec prog1 and get it to print "hello world: 2" without ever replacing
the file via the VFS, so the kernel is not aware of the change.

The test was done using the branch "dongsu/fuse-userns-v5-2" [4],
including both this new force option and Sascha's patch ("ima: Use
i_version only when filesystem supports it").


Step by step test procedure:

1. Mount the memfs FUSE using [3]:
rm -f  /tmp/memfs-switch* ; memfs -L DEBUG  /mnt/memfs

2. Copy prog1 and prog2 using [5]
cp prog1 /mnt/memfs/prog1
cp prog2 /mnt/memfs/prog2

3. Lookup the files and let the FUSE driver to keep the handles open:
dd if=/mnt/memfs/prog1 bs=1 | (read -n 1 x ; sleep 3600 ) &
dd if=/mnt/memfs/prog2 bs=1 | (read -n 1 x ; sleep 3600 ) &

4. Check the 2 programs work correctly:
$ /mnt/memfs/prog1
hello world: 1
$ /mnt/memfs/prog2
hello world: 2

5. Check the measurements for prog1 and prog2:
$ sudo cat /sys/kernel/security/ima/ascii_runtime_measurements|grep
/mnt/memfs/prog
10 7ac5aed52061cb09120e977c6d04ee5c7b11c371 ima-ng
sha1:ac14c9268cd2811f7a5adea17b27d84f50e1122c /mnt/memfs/prog1
10 9acc17a9a32aec4a676b8f6558e17a3d6c9a78e6 ima-ng
sha1:799cb5d1e06d5c37ae7a76ba25ecd1bd01476383 /mnt/memfs/prog2

6. Use the backdoor command in my patched memfs to redirect file
operations on file handle 3 to file handle 2:
rm -f  /tmp/memfs-switch* ; touch /tmp/memfs-switch-3-2

7. Check how the FUSE driver serves different content for the files:
$ /mnt/memfs/prog1
hello world: 2
$ /mnt/memfs/prog2
hello world: 2

8. Check the measurements:
sudo cat /sys/kernel/security/ima/ascii_runtime_measurements|grep
/mnt/memfs/prog

Without the patches, on a vanilla kernel, there are no new
measurements, despite the FUSE driver having served different
executables.

However, with the "force" option enabled, I can see additional
measurements for prog1 and prog2 with the hashes reversed when the
FUSE driver served the alternative content.

====

[1] https://www.spinics.net/lists/linux-integrity/msg00948.html
[2] https://github.com/bbengfort/memfs
[3] https://github.com/kinvolk/memfs/commits/alban/switch-files
[4] https://github.com/kinvolk/linux/commits/dongsu/fuse-userns-v5-2
[5] https://github.com/kinvolk/fuse-userns-patches/commit/cf1f5750cab0

Cc: linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Seth Forshee <seth.forshee@canonical.com>
Tested-by: Alban Crequy <alban@kinvolk.io>
Signed-off-by: Dongsu Park <dongsu@kinvolk.io>
---
 security/integrity/ima/ima_policy.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index fddef8f8..8de40d85 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -127,6 +127,7 @@  static struct ima_rule_entry default_measurement_rules[] __ro_after_init = {
 	{.action = MEASURE, .func = MODULE_CHECK, .flags = IMA_FUNC},
 	{.action = MEASURE, .func = FIRMWARE_CHECK, .flags = IMA_FUNC},
 	{.action = MEASURE, .func = POLICY_CHECK, .flags = IMA_FUNC},
+	{.action = MEASURE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
 };
 
 static struct ima_rule_entry default_appraise_rules[] __ro_after_init = {
@@ -154,6 +155,7 @@  static struct ima_rule_entry default_appraise_rules[] __ro_after_init = {
 	{.action = APPRAISE, .fowner = GLOBAL_ROOT_UID, .fowner_op = &uid_eq,
 	 .flags = IMA_FOWNER | IMA_DIGSIG_REQUIRED},
 #endif
+	{.action = APPRAISE, .fsmagic = FUSE_SUPER_MAGIC, .flags = IMA_FSMAGIC | IMA_FORCE},
 };
 
 static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {