[PATCH] integrity ima_policy : Select files by suffix

[PATCH] integrity ima_policy : Select files by suffix

[Lev Olshvang]

From: Lev Olshvang <levonshe@gmail.com>
Date: Fri, 27 Mar 2020 20:50:01 +0300
Reply-To:
Subject: [PATCH] integrity ima_policy : Select files by suffix

IMA policy rule allows to select files based on uid, gid, fsuid. etc.
One tremendously useful selector(IMHO) is the file suffix.

I think of systemd service files, configurution files, etc.

But the real goal of the patch is the ability to validate shell scripts.
Shell provides too many different ways to run the script:
input redirrection, pipe, command line parameters.

IMA police rule below will catch any execution of file with sh suffix :

appraise FILE_OPEN suffix=sh

Another interesting byproduct of this feature is that any file might be put under validation.
All that you need is just create hard link to this file and apply immutable attribute to the link.
Example
appraise FILE_OPEN suffix=ini
ln /etc/shadow /etc/shadow.ini
chattr -i /etc/shadow.ini

Signed-off-by: Lev Olshvang <levonshe@gmail.com>
---
 security/integrity/ima/ima_policy.c | 41 ++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 453427048999..e8c7a4016fc6 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -35,6 +35,7 @@
 #define IMA_PCR		0x0100
 #define IMA_FSNAME	0x0200
 #define IMA_KEYRINGS	0x0400
+#define IMA_SUFFIX	0x0800

 #define UNKNOWN		0
 #define MEASURE		0x0001	/* same as IMA_MEASURE */
@@ -45,6 +46,8 @@
 #define HASH		0x0100
 #define DONT_HASH	0x0200

+#define IMA_SUFFIX_LEN 4
+
 #define INVALID_PCR(a) (((a) < 0) || \
 	(a) >= (sizeof_field(struct integrity_iint_cache, measured_pcrs) * 8))

@@ -82,6 +85,8 @@ struct ima_rule_entry {
 	char *fsname;
 	char *keyrings; /* Measure keys added to these keyrings */
 	struct ima_template_desc *template;
+	// In the first byte we encode length, then string itself
+	char suffix[IMA_SUFFIX_LEN+2];
 };

 /*
@@ -419,6 +424,9 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 			    const char *keyring)
 {
 	int i;
+	u8 sfx_len;
+	u16 name_offset;
+	struct dentry *dentry;

 	if ((func == KEXEC_CMDLINE) || (func == KEY_CHECK)) {
 		if ((rule->flags & IMA_FUNC) && (rule->func == func)) {
@@ -443,6 +451,13 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 	if ((rule->flags & IMA_FSNAME)
 	    && strcmp(rule->fsname, inode->i_sb->s_type->name))
 		return false;
+	if (rule->flags & IMA_SUFFIX) {
+		dentry = container_of(inode->i_dentry.first, struct dentry, d_u.d_alias);
+		sfx_len = (u8)rule->suffix[0] - 1;
+		name_offset = (u16) strlen(dentry->d_name.name) - sfx_len;
+		if ((memcmp(rule->suffix + 1, dentry->d_name.name + name_offset, sfx_len) != 0))
+			return false;
+	}
 	if ((rule->flags & IMA_FSUUID) &&
 	    !uuid_equal(&rule->fsuuid, &inode->i_sb->s_uuid))
 		return false;
@@ -822,6 +837,7 @@ enum {
 	Opt_obj_user, Opt_obj_role, Opt_obj_type,
 	Opt_subj_user, Opt_subj_role, Opt_subj_type,
 	Opt_func, Opt_mask, Opt_fsmagic, Opt_fsname,
+	Opt_suffix,
 	Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq,
 	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
 	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
@@ -849,6 +865,7 @@ static const match_table_t policy_tokens = {
 	{Opt_fsmagic, "fsmagic=%s"},
 	{Opt_fsname, "fsname=%s"},
 	{Opt_fsuuid, "fsuuid=%s"},
+	{Opt_suffix, "suffix=%s"},
 	{Opt_uid_eq, "uid=%s"},
 	{Opt_euid_eq, "euid=%s"},
 	{Opt_fowner_eq, "fowner=%s"},
@@ -991,7 +1008,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)

 			if (entry->action != UNKNOWN)
 				result = -EINVAL;
-
 			entry->action = DONT_MEASURE;
 			break;
 		case Opt_appraise:
@@ -1120,6 +1136,21 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			result = 0;
 			entry->flags |= IMA_FSNAME;
 			break;
+		case Opt_suffix:
+			ima_log_string(ab, "suffix", args[0].from);
+			result = strlen(args[0].from);
+			result++; // add '\0'
+			if (result > IMA_SUFFIX_LEN) {
+				result = -E2BIG;
+				break;
+			}
+
+			/* encode suffix len in the first byte */
+			entry->suffix[0] = (u8) result;
+			memcpy(entry->suffix + 1, args[0].from, result);
+			result = 0;
+			entry->flags |= IMA_SUFFIX;
+			break;
 		case Opt_keyrings:
 			ima_log_string(ab, "keyrings", args[0].from);

@@ -1526,6 +1557,10 @@ int ima_policy_show(struct seq_file *m, void *v)
 		seq_printf(m, pt(Opt_fsmagic), tbuf);
 		seq_puts(m, " ");
 	}
+	if (entry->flags & IMA_SUFFIX) {
+		seq_printf(m, "suffix=%s", entry->suffix + 1);
+		seq_puts(m, " ");
+	}

 	if (entry->flags & IMA_FSNAME) {
 		snprintf(tbuf, sizeof(tbuf), "%s", entry->fsname);
@@ -1546,6 +1581,10 @@ int ima_policy_show(struct seq_file *m, void *v)
 		seq_puts(m, " ");
 	}

+	if (entry->flags & IMA_SUFFIX) {
+		seq_printf(m, "suffix=%s", entry->suffix + 1);
+		seq_puts(m, " ");
+	}
 	if (entry->flags & IMA_FSUUID) {
 		seq_printf(m, "fsuuid=%pU", &entry->fsuuid);
 		seq_puts(m, " ");
--
2.17.1

RE: [PATCH] integrity ima_policy : Select files by suffix

[Roberto Sassu]

> -----Original Message-----
> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> owner@vger.kernel.org] On Behalf Of Lev Olshvang
> Sent: Monday, March 30, 2020 2:28 PM
> To: linux-integrity@vger.kernel.org; Mimi Zohar <zohar@us.ibm.com>
> Subject: [PATCH] integrity ima_policy : Select files by suffix
> 
> From: Lev Olshvang <levonshe@gmail.com>
> Date: Fri, 27 Mar 2020 20:50:01 +0300
> Reply-To:
> Subject: [PATCH] integrity ima_policy : Select files by suffix
> 
> IMA policy rule allows to select files based on uid, gid, fsuid. etc.
> One tremendously useful selector(IMHO) is the file suffix.
> 
> I think of systemd service files, configurution files, etc.
> 
> But the real goal of the patch is the ability to validate shell scripts.
> Shell provides too many different ways to run the script:
> input redirrection, pipe, command line parameters.

Given that file name is not protected, I would suggest to look instead at
the execution permission of the file. This information is protected by EVM.

In a second time, we could consider to enforce the policy in the interpreters
that every script must be executable, as suggested here:

https://lkml.org/lkml/2019/4/15/825

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH] integrity ima_policy : Select files by suffix

[Lev R. Oshvang .]

I already answered to Mimi Zohar that applications expect file name in
open() syscall.
So there is no need to protect file name otherwise applications just
stop to work.
Even now when ima hash is not correct application stops to work.
Put aside scripts for a second. A lot of programs are configured in
.ini or .conf files.
The suffix is a very convenient way to provide these files would be measured.

Now I returning to scripts.
It is very hard to enforce IMA checks in interpreters. And thinks
about perl scrips. awk. python scripts. etc
The proposed suffix rule is easy and lightweight.
I once had programmed BRM hook of LSM
I had a very hard time trying to figure out whether shell is opening a
script or data , how to get filename to check its signature.
Sometimes script file does not have shebang or does not have
executable permission.

I hope I convinced you.

On Mon, Mar 30, 2020 at 7:45 PM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > -----Original Message-----
> > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> > owner@vger.kernel.org] On Behalf Of Lev Olshvang
> > Sent: Monday, March 30, 2020 2:28 PM
> > To: linux-integrity@vger.kernel.org; Mimi Zohar <zohar@us.ibm.com>
> > Subject: [PATCH] integrity ima_policy : Select files by suffix
> >
> > From: Lev Olshvang <levonshe@gmail.com>
> > Date: Fri, 27 Mar 2020 20:50:01 +0300
> > Reply-To:
> > Subject: [PATCH] integrity ima_policy : Select files by suffix
> >
> > IMA policy rule allows to select files based on uid, gid, fsuid. etc.
> > One tremendously useful selector(IMHO) is the file suffix.
> >
> > I think of systemd service files, configurution files, etc.
> >
> > But the real goal of the patch is the ability to validate shell scripts.
> > Shell provides too many different ways to run the script:
> > input redirrection, pipe, command line parameters.
>
> Given that file name is not protected, I would suggest to look instead at
> the execution permission of the file. This information is protected by EVM.
>
> In a second time, we could consider to enforce the policy in the interpreters
> that every script must be executable, as suggested here:
>
> https://lkml.org/lkml/2019/4/15/825
>
> Roberto
>
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH] integrity ima_policy : Select files by suffix

[Mimi Zohar]

[The normal conventions for this mailing list is bottom post.]

Lev,

On Mon, 2020-03-30 at 20:21 +0300, Lev R. Oshvang . wrote:
> I already answered to Mimi Zohar that applications expect file name in
> open() syscall.

And I disagreed with you.  Not only can filenames be renamed, as I
mentioned, but they aren't protected, as Roberto said.

> So there is no need to protect file name otherwise applications just
> stop to work.
> Even now when ima hash is not correct application stops to work.
> Put aside scripts for a second. A lot of programs are configured in
> .ini or .conf files.
> The suffix is a very convenient way to provide these files would be measured.
> 
> Now I returning to scripts.
> It is very hard to enforce IMA checks in interpreters. And thinks
> about perl scrips. awk. python scripts. etc
> The proposed suffix rule is easy and lightweight.
> I once had programmed BRM hook of LSM
> I had a very hard time trying to figure out whether shell is opening a
> script or data , how to get filename to check its signature.
> Sometimes script file does not have shebang or does not have
> executable permission.

Only the interpreter knows how the file will be used.

> 
> I hope I convinced you.

There have been a number of attempts to define IMA policy rules based
on pathname, which have not been upstreamed.  Feel free to use your
solution, but it can't be upstreamed as is.
  
Mimi

Re: [PATCH] integrity ima_policy : Select files by suffix

[Lev R. Oshvang .]

On Mon, Mar 30, 2020 at 9:05 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> [The normal conventions for this mailing list is bottom post.]
>
> Lev,
>
> On Mon, 2020-03-30 at 20:21 +0300, Lev R. Oshvang . wrote:
> > I already answered to Mimi Zohar that applications expect file name in
> > open() syscall.
>
> And I disagreed with you.  Not only can filenames be renamed, as I
> mentioned, but they aren't protected, as Roberto said.
>
I politely remind you that the goal of IMA  is to provide the
application the guarantee that the file was not altered when it tries
to use it, whether it is open(), stat(), , execve , etc
BPRM_CHECK, MMAP_CHECK narrows the scope of checks to executable files/libs.

But there are a tons of files in file systems and we can not check them all.
My intention is to put under scrutiny just important configuration
files, systemd service files, some files in etc, ex /etc/shadow.

The result will be good for providing TCB, because we can be sure that
all systemd services passed attestation.
Right now we measure  sshd binary but we do not mesure sshd config.

An attacker may change the configuration file of iptables, sshd.
sudoers to make attack successful or leave a backdoor.
He can not achieve the attack goal if he changes a file name. The
application/service will just not start.
TOCTOU is not relevant. Attacker works from userspace but open()/IMA
happens in the kernel. So there is no way to avoid IMA check.

> > So there is no need to protect file name otherwise applications just
> > stop to work.
> > Even now when ima hash is not correct application stops to work.
> > Put aside scripts for a second. A lot of programs are configured in
> > .ini or .conf files.
> > The suffix is a very convenient way to provide these files would be measured.
> >
> > Now I returning to scripts.
> > It is very hard to enforce IMA checks in interpreters. And thinks
> > about perl scrips. awk. python scripts. etc
> > The proposed suffix rule is easy and lightweight.
> > I once had programmed BRM hook of LSM
> > I had a very hard time trying to figure out whether shell is opening a
> > script or data , how to get filename to check its signature.
> > Sometimes script file does not have shebang or does not have
> > executable permission.
>
> Only the interpreter knows how the file will be used.
>
Not true. It is a common practice to use file suffix to indicate how a
file is going to be used
Coding styles required from programmers to use suffixes.
So let's be on a common-sense side and agree that if some system as a
coding style use .sh for shell scripts name
we can use it in IMA policy rule.
> >
> > I hope I convinced you.
>
> There have been a number of attempts to define IMA policy rules based
> on pathname, which have not been upstreamed.  Feel free to use your
> solution, but it can't be upstreamed as is.
>
> Mimi
>
Hi Mimi,
I am completely newbie in such discussion so please forgive that I may
violate style again because I responded in the middle of your reply
Another argument that is responsible sysadmin may use the immutable
attribute to prevent name change,

Regards,
Lev