[PATCH 0/2] turn on force option for FUSE in builtin policies

[PATCH 0/2] turn on force option for FUSE in builtin policies

[Dongsu Park]

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 patchset depends on the patch "ima: define a new policy option
named force" by Mimi. [1]  For details on testing the force option,
please refer to the testing report by Alban. [2]

The first patch is for simply moving FUSE_*SUPER_MAGIC macros to
include/uapi/linux, to be able to use those in other subsystems like
security/integrity/ima.

The second patch is actually to turn on the force option for FUSE fs
in IMA.

[1] https://www.spinics.net/lists/linux-integrity/msg00948.html
[2] https://marc.info/?l=linux-integrity&m=151559360514676&w=2


Dongsu Park (2):
  fs/fuse: move SUPER_MAGIC definitions to linux/magic.h
  ima: turn on force option for FUSE in builtin policies

 fs/fuse/control.c                   | 3 +--
 fs/fuse/inode.c                     | 3 +--
 include/uapi/linux/magic.h          | 3 +++
 security/integrity/ima/ima_policy.c | 2 ++
 4 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.13.6

[PATCH 1/2] fs/fuse: move SUPER_MAGIC definitions to linux/magic.h

[Dongsu Park]

To be able to use FUSE_*SUPER_MAGIC macros in other subsystems like
security/integrity/ima, we need to move the definitions from fs/fuse
to include/uapi/linux/.

The FUSE_*SUPER_MAGIC macros are made available to userspace in the
same way as other filesystems.

Cc: linux-fsdevel@vger.kernel.org
Cc: linux-integrity@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Alban Crequy <alban@kinvolk.io>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Dongsu Park <dongsu@kinvolk.io>
---
 fs/fuse/control.c          | 3 +--
 fs/fuse/inode.c            | 3 +--
 include/uapi/linux/magic.h | 3 +++
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/control.c b/fs/fuse/control.c
index b9ea99c5..9015c15c 100644
--- a/fs/fuse/control.c
+++ b/fs/fuse/control.c
@@ -10,8 +10,7 @@
 
 #include <linux/init.h>
 #include <linux/module.h>
-
-#define FUSE_CTL_SUPER_MAGIC 0x65735543
+#include <linux/magic.h>
 
 /*
  * This is non-NULL when the single instance of the control filesystem
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 8c98edee..57371b77 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -22,6 +22,7 @@
 #include <linux/exportfs.h>
 #include <linux/posix_acl.h>
 #include <linux/pid_namespace.h>
+#include <linux/magic.h>
 
 MODULE_AUTHOR("Miklos Szeredi <miklos@szeredi.hu>");
 MODULE_DESCRIPTION("Filesystem in Userspace");
@@ -49,8 +50,6 @@ MODULE_PARM_DESC(max_user_congthresh,
  "Global limit for the maximum congestion threshold an "
  "unprivileged user can set");
 
-#define FUSE_SUPER_MAGIC 0x65735546
-
 #define FUSE_DEFAULT_BLKSIZE 512
 
 /** Maximum number of outstanding background requests */
diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h
index 1a6fee97..1534e99c 100644
--- a/include/uapi/linux/magic.h
+++ b/include/uapi/linux/magic.h
@@ -90,4 +90,7 @@
 #define BALLOON_KVM_MAGIC	0x13661366
 #define ZSMALLOC_MAGIC		0x58295829
 
+#define FUSE_CTL_SUPER_MAGIC	0x65735543
+#define FUSE_SUPER_MAGIC	0x65735546
+
 #endif /* __LINUX_MAGIC_H__ */
-- 
2.13.6

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

[Dongsu Park]

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:

====

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 = {
-- 
2.13.6

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

[Mimi Zohar]

> 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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4303 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52605 bytes --]

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

[Dongsu Park]

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

Re: [PATCH 0/2] turn on force option for FUSE in builtin policies

[Christoph Hellwig]

On Thu, Jan 11, 2018 at 08:51:48PM +0100, Dongsu Park wrote:
> 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.

The same is true for any distributed file system.  Checking for magic
numbers is always the wrong thing.  You'll need flags for specific
behavior in struct file_system_type instead.

Re: [PATCH 0/2] turn on force option for FUSE in builtin policies

[Mimi Zohar]

On Mon, 2018-01-15 at 06:48 -0800, Christoph Hellwig wrote:
> On Thu, Jan 11, 2018 at 08:51:48PM +0100, Dongsu Park wrote:
> > 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.
> 
> The same is true for any distributed file system.  Checking for magic
> numbers is always the wrong thing.  You'll need flags for specific
> behavior in struct file_system_type instead.

For XFS, which considers fsmagic numbers private to the filesystem,
*always* using the fsmagic number is wrong.  As to whether this is
true for other filesystems is unclear.  IMA policies have been defined
in terms of fsmagic numbers for a long time.  fsmagic numbers were
moved from the filesystems to magic.h for this purpose.  Someone would
have complained earlier if it is always wrong.
 
I just posted a patch titled "ima: define new policy condition based
on the filesystem name" to allow policies to be defined in terms of
the i_sb->s_type->name.

Mimi

Re: [PATCH 0/2] turn on force option for FUSE in builtin policies

[Christoph Hellwig]

On Mon, Jan 15, 2018 at 11:32:41AM -0500, Mimi Zohar wrote:
> For XFS, which considers fsmagic numbers private to the filesystem,
> *always* using the fsmagic number is wrong.  As to whether this is
> true for other filesystems is unclear.  IMA policies have been defined
> in terms of fsmagic numbers for a long time.  fsmagic numbers were
> moved from the filesystems to magic.h for this purpose.  Someone would
> have complained earlier if it is always wrong.
>  
> I just posted a patch titled "ima: define new policy condition based
> on the filesystem name" to allow policies to be defined in terms of
> the i_sb->s_type->name.

ima has no business looking at either the name _or_ the magic number.

Re: [PATCH 0/2] turn on force option for FUSE in builtin policies

[Mimi Zohar]

On Mon, 2018-01-15 at 09:18 -0800, Christoph Hellwig wrote:
> On Mon, Jan 15, 2018 at 11:32:41AM -0500, Mimi Zohar wrote:
> > For XFS, which considers fsmagic numbers private to the filesystem,
> > *always* using the fsmagic number is wrong.  As to whether this is
> > true for other filesystems is unclear.  IMA policies have been defined
> > in terms of fsmagic numbers for a long time.  fsmagic numbers were
> > moved from the filesystems to magic.h for this purpose.  Someone would
> > have complained earlier if it is always wrong.
> >  
> > I just posted a patch titled "ima: define new policy condition based
> > on the filesystem name" to allow policies to be defined in terms of
> > the i_sb->s_type->name.
> 
> ima has no business looking at either the name _or_ the magic number.

There are a couple of reasons to define policies in terms of the
filesystem name or magic numbers.

One example is pseudo filesystems (eg. pseudo filesystems - sysfs,
securitys, cgroups, selinuxfs, etc).  These should never be measured
or appraised.

The current example is fuse and remote file systems.  These should
always be re-evaluated and not rely on cached file info.

If not based on IMA policy, what do you propose?  Define new SB_ flags
 to indicate IMA disabled/enabled (eg. SB_IMA) and nocaching (eg.
SB_IMA_NOCACHE)?

Mimi

[PATCH] ima,fuse: introduce new fs flag FS_NO_IMA_CACHE

[Alban Crequy]

From: Alban Crequy <alban@kinvolk.io>

This patch forces files to be re-measured, re-appraised and re-audited
on file systems with the feature flag FS_NO_IMA_CACHE. In that way,
cached integrity results won't be used.

For now, only FUSE filesystems use this flag. This is because the
userspace FUSE process can change the underlying files at any times.

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

How to test this:

====

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 "alban/fuse-flag-ima-nocache" [4].

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 patch, there are no new measurements, despite the FUSE
driver having served different executables.

With the patch, 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/alban/fuse-flag-ima-nocache
[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>
Cc: Christoph Hellwig <hch@infradead.org>
Tested-by: Dongsu Park <dongsu@kinvolk.io>
Signed-off-by: Alban Crequy <alban@kinvolk.io>
---
 fs/fuse/inode.c                   |  2 +-
 include/linux/fs.h                |  1 +
 security/integrity/ima/ima_main.c | 11 +++++++----
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 8c98edee3628..b511e6469b0a 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1212,7 +1212,7 @@ static void fuse_kill_sb_anon(struct super_block *sb)
 static struct file_system_type fuse_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "fuse",
-	.fs_flags	= FS_HAS_SUBTYPE | FS_USERNS_MOUNT,
+	.fs_flags	= FS_HAS_SUBTYPE | FS_USERNS_MOUNT | FS_NO_IMA_CACHE,
 	.mount		= fuse_mount,
 	.kill_sb	= fuse_kill_sb_anon,
 };
diff --git a/include/linux/fs.h b/include/linux/fs.h
index fce19c491970..88da6908a2b2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2075,6 +2075,7 @@ struct file_system_type {
 #define FS_BINARY_MOUNTDATA	2
 #define FS_HAS_SUBTYPE		4
 #define FS_USERNS_MOUNT		8	/* Can be mounted by userns root */
+#define FS_NO_IMA_CACHE		16	/* Force IMA to re-measure, re-appraise, re-audit files */
 #define FS_RENAME_DOES_D_MOVE	32768	/* FS will handle d_move() during rename() internally. */
 	struct dentry *(*mount) (struct file_system_type *, int,
 		       const char *, void *);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 88af481502f7..e6e45ab15dfc 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/xattr.h>
 #include <linux/ima.h>
+#include <linux/fs.h>
 
 #include "ima.h"
 
@@ -229,13 +230,15 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 				 IMA_ACTION_FLAGS);
 
 	/*
-	 * Reset the measure, appraise and audit cached flags either if
-	 * ima_inode_setxattr was called or based on policy, forcing
-	 * the file to be re-evaluated.
+	 * Reset the measure, appraise and audit cached flags either if:
+	 * - ima_inode_setxattr was called, or
+	 * - based on policy ("force"), or
+	 * - based on filesystem feature flag
+	 * forcing the file to be re-evaluated.
 	 */
 	if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) {
 		iint->flags &= ~IMA_DONE_MASK;
-	} else if (action & IMA_FORCE) {
+	} else if (action & IMA_FORCE || inode->i_sb->s_type->fs_flags & FS_NO_IMA_CACHE) {
 		if (action & IMA_MEASURE) {
 			iint->measured_pcrs = 0;
 			iint->flags &=
-- 
2.13.6

Re: [PATCH] ima,fuse: introduce new fs flag FS_NO_IMA_CACHE

[Alban Crequy]

On Tue, Jan 16, 2018 at 11:41 AM, Alban Crequy <alban.crequy@gmail.com> wrote:
> From: Alban Crequy <alban@kinvolk.io>
>
> This patch forces files to be re-measured, re-appraised and re-audited
> on file systems with the feature flag FS_NO_IMA_CACHE. In that way,
> cached integrity results won't be used.
>
> For now, only FUSE filesystems use this flag. This is because the
> userspace FUSE process can change the underlying files at any times.
>
> This patch is based on the patch "ima: define a new policy option
> named force" by Mimi. [1]
>
> How to test this:
>
> ====
>
> 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 "alban/fuse-flag-ima-nocache" [4].
>
> 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 patch, there are no new measurements, despite the FUSE
> driver having served different executables.
>
> With the patch, 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/alban/fuse-flag-ima-nocache
> [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>
> Cc: Christoph Hellwig <hch@infradead.org>
> Tested-by: Dongsu Park <dongsu@kinvolk.io>
> Signed-off-by: Alban Crequy <alban@kinvolk.io>
> ---
>  fs/fuse/inode.c                   |  2 +-
>  include/linux/fs.h                |  1 +
>  security/integrity/ima/ima_main.c | 11 +++++++----
>  3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 8c98edee3628..b511e6469b0a 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1212,7 +1212,7 @@ static void fuse_kill_sb_anon(struct super_block *sb)
>  static struct file_system_type fuse_fs_type = {
>         .owner          = THIS_MODULE,
>         .name           = "fuse",
> -       .fs_flags       = FS_HAS_SUBTYPE | FS_USERNS_MOUNT,
> +       .fs_flags       = FS_HAS_SUBTYPE | FS_USERNS_MOUNT | FS_NO_IMA_CACHE,

I just realised I should not have submitted this patch based on the
branch I am on because FS_USERNS_MOUNT is not there now, so this patch
does not apply cleanly on next-integrity at the moment. Sorry about
that.

>         .mount          = fuse_mount,
>         .kill_sb        = fuse_kill_sb_anon,
>  };
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index fce19c491970..88da6908a2b2 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2075,6 +2075,7 @@ struct file_system_type {
>  #define FS_BINARY_MOUNTDATA    2
>  #define FS_HAS_SUBTYPE         4
>  #define FS_USERNS_MOUNT                8       /* Can be mounted by userns root */
> +#define FS_NO_IMA_CACHE                16      /* Force IMA to re-measure, re-appraise, re-audit files */
>  #define FS_RENAME_DOES_D_MOVE  32768   /* FS will handle d_move() during rename() internally. */
>         struct dentry *(*mount) (struct file_system_type *, int,
>                        const char *, void *);
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 88af481502f7..e6e45ab15dfc 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -24,6 +24,7 @@
>  #include <linux/slab.h>
>  #include <linux/xattr.h>
>  #include <linux/ima.h>
> +#include <linux/fs.h>
>
>  #include "ima.h"
>
> @@ -229,13 +230,15 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>                                  IMA_ACTION_FLAGS);
>
>         /*
> -        * Reset the measure, appraise and audit cached flags either if
> -        * ima_inode_setxattr was called or based on policy, forcing
> -        * the file to be re-evaluated.
> +        * Reset the measure, appraise and audit cached flags either if:
> +        * - ima_inode_setxattr was called, or
> +        * - based on policy ("force"), or
> +        * - based on filesystem feature flag
> +        * forcing the file to be re-evaluated.
>          */

Now that I think about it, it's also possible to write this patch
without basing it on Mimi's patch "ima: define a new policy option
named force", which is not in next-integrity yet. Should I try that?

>         if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) {
>                 iint->flags &= ~IMA_DONE_MASK;
> -       } else if (action & IMA_FORCE) {
> +       } else if (action & IMA_FORCE || inode->i_sb->s_type->fs_flags & FS_NO_IMA_CACHE) {
>                 if (action & IMA_MEASURE) {
>                         iint->measured_pcrs = 0;
>                         iint->flags &=
> --
> 2.13.6
>

Re: [PATCH] ima,fuse: introduce new fs flag FS_NO_IMA_CACHE

[Mimi Zohar]

> >
> >         /*
> > -        * Reset the measure, appraise and audit cached flags either if
> > -        * ima_inode_setxattr was called or based on policy, forcing
> > -        * the file to be re-evaluated.
> > +        * Reset the measure, appraise and audit cached flags either if:
> > +        * - ima_inode_setxattr was called, or
> > +        * - based on policy ("force"), or
> > +        * - based on filesystem feature flag
> > +        * forcing the file to be re-evaluated.
> >          */
> 
> Now that I think about it, it's also possible to write this patch
> without basing it on Mimi's patch "ima: define a new policy option
> named force", which is not in next-integrity yet. Should I try that?

Yes, thank you.  As you're proposing a new flag, please remember to Cc
the fsdevel mailing list as well.

Mimi

> 
> >         if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) {
> >                 iint->flags &= ~IMA_DONE_MASK;
> > -       } else if (action & IMA_FORCE) {
> > +       } else if (action & IMA_FORCE || inode->i_sb->s_type->fs_flags & FS_NO_IMA_CACHE) {
> >                 if (action & IMA_MEASURE) {
> >                         iint->measured_pcrs = 0;
> >                         iint->flags &=
> > --
> > 2.13.6
> >
> 

Re: [PATCH 0/2] turn on force option for FUSE in builtin policies

[Dongsu Park]

Hi,

On Thu, Jan 11, 2018 at 8:51 PM, Dongsu Park <dongsu@kinvolk.io> wrote:
> 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.

Since yesterday Alban and I have been working on a different approach
that does not depend on IMA rules, nor fsmagic. Please see:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1587390.html

If that's ok, I'm ready to discard this patchset.

Thanks,
Dongsu

> This patchset depends on the patch "ima: define a new policy option
> named force" by Mimi. [1]  For details on testing the force option,
> please refer to the testing report by Alban. [2]
>
> The first patch is for simply moving FUSE_*SUPER_MAGIC macros to
> include/uapi/linux, to be able to use those in other subsystems like
> security/integrity/ima.
>
> The second patch is actually to turn on the force option for FUSE fs
> in IMA.
>
> [1] https://www.spinics.net/lists/linux-integrity/msg00948.html
> [2] https://marc.info/?l=linux-integrity&m=151559360514676&w=2
>
>
> Dongsu Park (2):
>   fs/fuse: move SUPER_MAGIC definitions to linux/magic.h
>   ima: turn on force option for FUSE in builtin policies
>
>  fs/fuse/control.c                   | 3 +--
>  fs/fuse/inode.c                     | 3 +--
>  include/uapi/linux/magic.h          | 3 +++
>  security/integrity/ima/ima_policy.c | 2 ++
>  4 files changed, 7 insertions(+), 4 deletions(-)
>
> --
> 2.13.6
>

Re: [PATCH 0/2] turn on force option for FUSE in builtin policies

[Mimi Zohar]

On Tue, 2018-01-16 at 12:09 +0100, Dongsu Park wrote:
> Hi,
> 
> On Thu, Jan 11, 2018 at 8:51 PM, Dongsu Park <dongsu@kinvolk.io> wrote:
> > 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.
> 
> Since yesterday Alban and I have been working on a different approach
> that does not depend on IMA rules, nor fsmagic. Please see:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1587390.html
> 
> If that's ok, I'm ready to discard this patchset.

You dropped a number of people involved in this discussion and mailing
lists.  Please post the proposed patch inline as an RFC, cc'ing the
same people, those involved in the discussion, and previous mailing
lists, including LSM, integrity, and fsdevel.

thanks,

Mimi

Re: [PATCH 0/2] turn on force option for FUSE in builtin policies

[Dongsu Park]

Hi Mimi,

On Tue, Jan 16, 2018 at 12:23 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2018-01-16 at 12:09 +0100, Dongsu Park wrote:
>> Since yesterday Alban and I have been working on a different approach
>> that does not depend on IMA rules, nor fsmagic. Please see:
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1587390.html
>>
>> If that's ok, I'm ready to discard this patchset.
>
> You dropped a number of people involved in this discussion and mailing
> lists.  Please post the proposed patch inline as an RFC, cc'ing the
> same people, those involved in the discussion, and previous mailing
> lists, including LSM, integrity, and fsdevel.

Sorry about that. Starting from the patchset v2, we will add Cc correctly.
Thank you also for the detailed review.

Dongsu

> thanks,
>
> Mimi
>