[PATCH 0/2] support to read and tune appraise mode in runtime

[PATCH 0/2] support to read and tune appraise mode in runtime

[Tianjia Zhang]

Support the read and write operations of ima_appraise by adding a
securifyfs file 'appraise_mode'.

In order to tune appraise mode in runtime, writing a PKCS#7 signature
corresponding the signed content is required. The content should be off,
enforce, log or fix. Given a simple way to archive this:

$ echo -n off > mode
$ openssl smime -sign -nocerts -noattr -binary \
    -in mode -inkey <system_trusted_key> \
    -signer <cert> -outform der -out mode.p7s
$ sudo cat mode.p7s \
    > /sys/kernel/security/ima/appraise_mode

Note that the signing key must be a trust key located in
system trusted keyring. So even the root privilege cannot
simply disable the enforcement.

Tianjia Zhang (2):
  ima: support to read appraise mode
  ima: support to tune appraise mode in runtime

 security/integrity/ima/ima_fs.c | 134 +++++++++++++++++++++++++++++++-
 1 file changed, 133 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH 1/2] ima: support to read appraise mode

[Tianjia Zhang]

Support to read appraise mode in runtime through securityfs file
'integrity/ima/appraise_mode'.

Signed-off-by: luanshi <zhangliguang@linux.alibaba.com>
Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
---
 security/integrity/ima/ima_fs.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index a71e822a6e92..65384f6ac0d9 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -360,6 +360,7 @@ static struct dentry *ascii_runtime_measurements;
 static struct dentry *runtime_measurements_count;
 static struct dentry *violations;
 static struct dentry *ima_policy;
+static struct dentry *appraise_mode;
 
 enum ima_fs_flags {
 	IMA_FS_BUSY,
@@ -447,6 +448,29 @@ static const struct file_operations ima_measure_policy_ops = {
 	.llseek = generic_file_llseek,
 };
 
+static ssize_t ima_appraise_mode_read(struct file *filp,
+					char __user *buf,
+					size_t count, loff_t *ppos)
+{
+	const char *mode;
+
+	if (ima_appraise & IMA_APPRAISE_ENFORCE)
+		mode = "enforce";
+	else if (ima_appraise & IMA_APPRAISE_FIX)
+		mode = "fix";
+	else if (ima_appraise & IMA_APPRAISE_LOG)
+		mode = "log";
+	else
+		mode = "off";
+
+	return simple_read_from_buffer(buf, count, ppos, mode, strlen(mode));
+}
+
+static const struct file_operations ima_appraise_mode_ops = {
+	.read = ima_appraise_mode_read,
+	.llseek = generic_file_llseek,
+};
+
 int __init ima_fs_init(void)
 {
 	ima_dir = securityfs_create_dir("ima", integrity_dir);
@@ -491,14 +515,20 @@ int __init ima_fs_init(void)
 	if (IS_ERR(ima_policy))
 		goto out;
 
+	appraise_mode =
+	    securityfs_create_file("appraise_mode", S_IRUSR | S_IRGRP,
+				   ima_dir, NULL, &ima_appraise_mode_ops);
+	if (IS_ERR(appraise_mode))
+		goto out;
+
 	return 0;
 out:
+	securityfs_remove(ima_policy);
 	securityfs_remove(violations);
 	securityfs_remove(runtime_measurements_count);
 	securityfs_remove(ascii_runtime_measurements);
 	securityfs_remove(binary_runtime_measurements);
 	securityfs_remove(ima_symlink);
 	securityfs_remove(ima_dir);
-	securityfs_remove(ima_policy);
 	return -1;
 }
-- 
2.17.1

[PATCH 2/2] ima: support to tune appraise mode in runtime

[Tianjia Zhang]

In order to tune appraise mode in runtime, writing a PKCS#7 signature
corresponding the signed content is required. The content should be off,
enforce, log or fix. Given a simple way to archive this:

$ echo -n off > mode
$ openssl smime -sign -nocerts -noattr -binary \
    -in mode -inkey <system_trusted_key> \
    -signer <cert> -outform der -out mode.p7s
$ sudo cat mode.p7s \
    > /sys/kernel/security/ima/appraise_mode

Note that the signing key must be a trust key located in
system trusted keyring. So even the root privilege cannot
simply disable the enforcement.

Signed-off-by: luanshi <zhangliguang@linux.alibaba.com>
Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
---
 security/integrity/ima/ima_fs.c | 102 ++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 65384f6ac0d9..c21ca145de0f 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -20,11 +20,15 @@
 #include <linux/rcupdate.h>
 #include <linux/parser.h>
 #include <linux/vmalloc.h>
+#include <linux/verification.h>
 
 #include "ima.h"
 
 static DEFINE_MUTEX(ima_write_mutex);
 
+/* maximum length of token allowed for signed appraise mode */
+#define APPRAISE_MAX_TOKEN_SIZE (512 * 1024)
+
 bool ima_canonical_fmt;
 static int __init default_canonical_fmt_setup(char *str)
 {
@@ -466,8 +470,106 @@ static ssize_t ima_appraise_mode_read(struct file *filp,
 	return simple_read_from_buffer(buf, count, ppos, mode, strlen(mode));
 }
 
+static int check_signature_info(char *buf, size_t count)
+{
+	u8 *p;
+
+	/*
+	 * In order to tune the appraise mode, a PKCS#7 signature is
+	 * supplied.
+	 *
+	 * Assuming ASN.1 encoding supplied, the minimal length would be
+	 * 4-byte header plus at least 256-byte payload.
+	 */
+	if (count < 260)
+		return -EINVAL;
+
+	p = (u8 *)buf;
+
+	/* The primitive type must be a sequence */
+	if (p[0] != 0x30 || p[1] != 0x82)
+		return -EINVAL;
+
+	/* Match up the length of the supplied buffer */
+	if (be16_to_cpup((__be16 *)(p + 2)) != count - 4)
+		return -EINVAL;
+
+	return 0;
+}
+
+/* Verify the supplied PKCS#7 signature. The signed content may be off,
+ * enforce, log, fix.
+ */
+static int repopulate_ima_appraise_mode(void *pkcs7, size_t pkcs7_len)
+{
+	static char *appraise_mode_strings[] = { "off", "enforce", "fix", "log" };
+	static int appraise_modes[] = {
+		0,
+		IMA_APPRAISE_ENFORCE,
+		IMA_APPRAISE_FIX,
+		IMA_APPRAISE_LOG,
+	};
+	int index, ret = -1;
+	const char *s;
+	int size = ARRAY_SIZE(appraise_mode_strings);
+
+	for (index = 0; index < size; index++) {
+		s = appraise_mode_strings[index];
+		ret = verify_pkcs7_signature(s, strlen(s), pkcs7, pkcs7_len,
+					    NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
+					    NULL, NULL);
+		if (!ret)
+			break;
+	}
+
+	if (index == size)
+		goto out;
+
+	ima_appraise = appraise_modes[index];
+
+out:
+	return ret;
+}
+
+static ssize_t ima_appraise_mode_write(struct file *filp,
+					const char __user *ubuf,
+					size_t count, loff_t *ppos)
+{
+	char *buf;
+	ssize_t ret;
+
+	if (*ppos > 1)
+		return -EFBIG;
+
+	if (count > APPRAISE_MAX_TOKEN_SIZE)
+		return -EFBIG;
+
+	buf = kmalloc(count, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = simple_write_to_buffer(buf, count, ppos, ubuf, count);
+	if (ret <= 0)
+		goto out;
+
+	ret = check_signature_info(buf, count);
+	if (ret)
+		goto out;
+
+	ret = repopulate_ima_appraise_mode(buf, count);
+	if (ret)
+		goto out;
+
+	ret = count;
+
+out:
+	kfree(buf);
+	return ret;
+}
+
 static const struct file_operations ima_appraise_mode_ops = {
 	.read = ima_appraise_mode_read,
+	.write = ima_appraise_mode_write,
 	.llseek = generic_file_llseek,
 };
 
-- 
2.17.1

Re: [PATCH 2/2] ima: support to tune appraise mode in runtime

[kbuild test robot]

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

Hi Tianjia,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on integrity/next-integrity]
[also build test ERROR on v5.6 next-20200408]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Tianjia-Zhang/support-to-read-and-tune-appraise-mode-in-runtime/20200409-114057
base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: c6x-randconfig-a001-20200409 (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.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
        GCC_VERSION=9.3.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   security/integrity/ima/ima_fs.c: In function 'repopulate_ima_appraise_mode':
>> security/integrity/ima/ima_fs.c:518:9: error: implicit declaration of function 'verify_pkcs7_signature' [-Werror=implicit-function-declaration]
     518 |   ret = verify_pkcs7_signature(s, strlen(s), pkcs7, pkcs7_len,
         |         ^~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/verify_pkcs7_signature +518 security/integrity/ima/ima_fs.c

   499	
   500	/* Verify the supplied PKCS#7 signature. The signed content may be off,
   501	 * enforce, log, fix.
   502	 */
   503	static int repopulate_ima_appraise_mode(void *pkcs7, size_t pkcs7_len)
   504	{
   505		static char *appraise_mode_strings[] = { "off", "enforce", "fix", "log" };
   506		static int appraise_modes[] = {
   507			0,
   508			IMA_APPRAISE_ENFORCE,
   509			IMA_APPRAISE_FIX,
   510			IMA_APPRAISE_LOG,
   511		};
   512		int index, ret = -1;
   513		const char *s;
   514		int size = ARRAY_SIZE(appraise_mode_strings);
   515	
   516		for (index = 0; index < size; index++) {
   517			s = appraise_mode_strings[index];
 > 518			ret = verify_pkcs7_signature(s, strlen(s), pkcs7, pkcs7_len,
   519						    NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
   520						    NULL, NULL);
   521			if (!ret)
   522				break;
   523		}
   524	
   525		if (index == size)
   526			goto out;
   527	
   528		ima_appraise = appraise_modes[index];
   529	
   530	out:
   531		return ret;
   532	}
   533	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

Re: [PATCH 0/2] support to read and tune appraise mode in runtime

[Mimi Zohar]

On Thu, 2020-04-09 at 11:39 +0800, Tianjia Zhang wrote:
> Support the read and write operations of ima_appraise by adding a
> securifyfs file 'appraise_mode'.
> 
> In order to tune appraise mode in runtime, writing a PKCS#7 signature
> corresponding the signed content is required. The content should be off,
> enforce, log or fix. Given a simple way to archive this:
> 
> $ echo -n off > mode
> $ openssl smime -sign -nocerts -noattr -binary \
>     -in mode -inkey <system_trusted_key> \
>     -signer <cert> -outform der -out mode.p7s
> $ sudo cat mode.p7s \
>     > /sys/kernel/security/ima/appraise_mode
> 
> Note that the signing key must be a trust key located in
> system trusted keyring. So even the root privilege cannot
> simply disable the enforcement.

There are major problems with disabling IMA appraisal.  This patch set
proposes disabling IMA appraisal without even providing the motivation
for such support.

A lot of effort went into preventing custom IMA policies from
disabling appraising the kexec or kernel module signatures.  In
addition, the "lockdown" patch set was upstreamed permitting IMA
signature verification.  This patch set would break both of these
features.

IMA relies on its own keyring for verifying file signatures, not the
builtin or secondary trusted kernel keyrings.

Two methods already exist - xattr and appended signatures - for
verifying file signatures.  This patch set assumes creating and
signing a file, which is then written to a securityfs file.  Like for
loading a custom IMA policy, instead of cat'ing the file, write the
pathname to the securityfs file.

If you must define a new IMA method for verifying file signatures,
then it needs to be generic and added to ima_appraise_measurement().
 (Refer to the new IMA appended signature support.)

Mimi

> 
> Tianjia Zhang (2):
>   ima: support to read appraise mode
>   ima: support to tune appraise mode in runtime
> 
>  security/integrity/ima/ima_fs.c | 134 +++++++++++++++++++++++++++++++-
>  1 file changed, 133 insertions(+), 1 deletion(-)
> 

Re: [PATCH 0/2] support to read and tune appraise mode in runtime

[Tianjia Zhang]

On 2020/4/14 5:55, Mimi Zohar wrote:
> On Thu, 2020-04-09 at 11:39 +0800, Tianjia Zhang wrote:
>> Support the read and write operations of ima_appraise by adding a
>> securifyfs file 'appraise_mode'.
>>
>> In order to tune appraise mode in runtime, writing a PKCS#7 signature
>> corresponding the signed content is required. The content should be off,
>> enforce, log or fix. Given a simple way to archive this:
>>
>> $ echo -n off > mode
>> $ openssl smime -sign -nocerts -noattr -binary \
>>      -in mode -inkey <system_trusted_key> \
>>      -signer <cert> -outform der -out mode.p7s
>> $ sudo cat mode.p7s \
>>      > /sys/kernel/security/ima/appraise_mode
>>
>> Note that the signing key must be a trust key located in
>> system trusted keyring. So even the root privilege cannot
>> simply disable the enforcement.
> 
> There are major problems with disabling IMA appraisal.  This patch set
> proposes disabling IMA appraisal without even providing the motivation
> for such support.
> 
> A lot of effort went into preventing custom IMA policies from
> disabling appraising the kexec or kernel module signatures.  In
> addition, the "lockdown" patch set was upstreamed permitting IMA
> signature verification.  This patch set would break both of these
> features.
> 
> IMA relies on its own keyring for verifying file signatures, not the
> builtin or secondary trusted kernel keyrings.
> 
> Two methods already exist - xattr and appended signatures - for
> verifying file signatures.  This patch set assumes creating and
> signing a file, which is then written to a securityfs file.  Like for
> loading a custom IMA policy, instead of cat'ing the file, write the
> pathname to the securityfs file.
> 
> If you must define a new IMA method for verifying file signatures,
> then it needs to be generic and added to ima_appraise_measurement().
>   (Refer to the new IMA appended signature support.)
> 
> Mimi
> 
>>
>> Tianjia Zhang (2):
>>    ima: support to read appraise mode
>>    ima: support to tune appraise mode in runtime
>>
>>   security/integrity/ima/ima_fs.c | 134 +++++++++++++++++++++++++++++++-
>>   1 file changed, 133 insertions(+), 1 deletion(-)
>>

Thanks for your suggestion, the way to close the appraise mode here is 
indeed a bit rude, I will reconsider again according to your suggestions.

In addition, [PATCH 1/2] ima: support to read appraise mode, by the way, 
see if this patch is acceptable.

Thanks and best,
Tianjia

Re: [PATCH 0/2] support to read and tune appraise mode in runtime

[Mimi Zohar]

On Tue, 2020-04-14 at 11:36 +0800, Tianjia Zhang wrote:
> 
> On 2020/4/14 5:55, Mimi Zohar wrote:
> > On Thu, 2020-04-09 at 11:39 +0800, Tianjia Zhang wrote:
> >> Support the read and write operations of ima_appraise by adding a
> >> securifyfs file 'appraise_mode'.
> >>
> >> In order to tune appraise mode in runtime, writing a PKCS#7 signature
> >> corresponding the signed content is required. The content should be off,
> >> enforce, log or fix. Given a simple way to archive this:
> >>
> >> $ echo -n off > mode
> >> $ openssl smime -sign -nocerts -noattr -binary \
> >>      -in mode -inkey <system_trusted_key> \
> >>      -signer <cert> -outform der -out mode.p7s
> >> $ sudo cat mode.p7s \
> >>      > /sys/kernel/security/ima/appraise_mode
> >>
> >> Note that the signing key must be a trust key located in
> >> system trusted keyring. So even the root privilege cannot
> >> simply disable the enforcement.
> > 
> > There are major problems with disabling IMA appraisal.  This patch set
> > proposes disabling IMA appraisal without even providing the motivation
> > for such support.
> > 
> > A lot of effort went into preventing custom IMA policies from
> > disabling appraising the kexec or kernel module signatures.  In
> > addition, the "lockdown" patch set was upstreamed permitting IMA
> > signature verification.  This patch set would break both of these
> > features.
> > 
> > IMA relies on its own keyring for verifying file signatures, not the
> > builtin or secondary trusted kernel keyrings.
> > 
> > Two methods already exist - xattr and appended signatures - for
> > verifying file signatures.  This patch set assumes creating and
> > signing a file, which is then written to a securityfs file.  Like for
> > loading a custom IMA policy, instead of cat'ing the file, write the
> > pathname to the securityfs file.
> > 
> > If you must define a new IMA method for verifying file signatures,
> > then it needs to be generic and added to ima_appraise_measurement().
> >   (Refer to the new IMA appended signature support.)
> > 
> > Mimi
> > 
> >>
> >> Tianjia Zhang (2):
> >>    ima: support to read appraise mode
> >>    ima: support to tune appraise mode in runtime
> >>
> >>   security/integrity/ima/ima_fs.c | 134 +++++++++++++++++++++++++++++++-
> >>   1 file changed, 133 insertions(+), 1 deletion(-)
> >>
> 
> Thanks for your suggestion, the way to close the appraise mode here is 
> indeed a bit rude, I will reconsider again according to your suggestions.
> 
> In addition, [PATCH 1/2] ima: support to read appraise mode, by the way, 
> see if this patch is acceptable.

My comments were not meant as suggestions, but as an explanation as to
how IMA works.  More details follow.

IMA is based on policy.  That decision was made a long time ago.  It
allowed distros to configure IMA, allowing customers to experiment
with it.  You have one opportunity to totally change the boot time
policy rules, by loading a custom policy.  After that, rules may only
be added.

There is no valid reason for "turning off" the policy once it has been
enabled.  It breaks existing expectations.

Mimi

Re: [PATCH 0/2] support to read and tune appraise mode in runtime

[Tianjia Zhang]

On 2020/4/14 21:41, Mimi Zohar wrote:
> On Tue, 2020-04-14 at 11:36 +0800, Tianjia Zhang wrote:
>>
>> On 2020/4/14 5:55, Mimi Zohar wrote:
>>> On Thu, 2020-04-09 at 11:39 +0800, Tianjia Zhang wrote:
>>>> Support the read and write operations of ima_appraise by adding a
>>>> securifyfs file 'appraise_mode'.
>>>>
>>>> In order to tune appraise mode in runtime, writing a PKCS#7 signature
>>>> corresponding the signed content is required. The content should be off,
>>>> enforce, log or fix. Given a simple way to archive this:
>>>>
>>>> $ echo -n off > mode
>>>> $ openssl smime -sign -nocerts -noattr -binary \
>>>>       -in mode -inkey <system_trusted_key> \
>>>>       -signer <cert> -outform der -out mode.p7s
>>>> $ sudo cat mode.p7s \
>>>>       > /sys/kernel/security/ima/appraise_mode
>>>>
>>>> Note that the signing key must be a trust key located in
>>>> system trusted keyring. So even the root privilege cannot
>>>> simply disable the enforcement.
>>>
>>> There are major problems with disabling IMA appraisal.  This patch set
>>> proposes disabling IMA appraisal without even providing the motivation
>>> for such support.
>>>
>>> A lot of effort went into preventing custom IMA policies from
>>> disabling appraising the kexec or kernel module signatures.  In
>>> addition, the "lockdown" patch set was upstreamed permitting IMA
>>> signature verification.  This patch set would break both of these
>>> features.
>>>
>>> IMA relies on its own keyring for verifying file signatures, not the
>>> builtin or secondary trusted kernel keyrings.
>>>
>>> Two methods already exist - xattr and appended signatures - for
>>> verifying file signatures.  This patch set assumes creating and
>>> signing a file, which is then written to a securityfs file.  Like for
>>> loading a custom IMA policy, instead of cat'ing the file, write the
>>> pathname to the securityfs file.
>>>
>>> If you must define a new IMA method for verifying file signatures,
>>> then it needs to be generic and added to ima_appraise_measurement().
>>>    (Refer to the new IMA appended signature support.)
>>>
>>> Mimi
>>>
>>>>
>>>> Tianjia Zhang (2):
>>>>     ima: support to read appraise mode
>>>>     ima: support to tune appraise mode in runtime
>>>>
>>>>    security/integrity/ima/ima_fs.c | 134 +++++++++++++++++++++++++++++++-
>>>>    1 file changed, 133 insertions(+), 1 deletion(-)
>>>>
>>
>> Thanks for your suggestion, the way to close the appraise mode here is
>> indeed a bit rude, I will reconsider again according to your suggestions.
>>
>> In addition, [PATCH 1/2] ima: support to read appraise mode, by the way,
>> see if this patch is acceptable.
> 
> My comments were not meant as suggestions, but as an explanation as to
> how IMA works.  More details follow.
> 
> IMA is based on policy.  That decision was made a long time ago.  It
> allowed distros to configure IMA, allowing customers to experiment
> with it.  You have one opportunity to totally change the boot time
> policy rules, by loading a custom policy.  After that, rules may only
> be added.
> 
> There is no valid reason for "turning off" the policy once it has been
> enabled.  It breaks existing expectations.
> 
> Mimi
> 

Thank you very much for your explanation. I'm sorry I may not have 
stated clearly. I didn't have to change the working mode of IMA. I was 
convinced by you to give up the idea of "turn off" the appraise. 
However, it should be possible to support appraise mode reading. Right?