Re: [PATCH v5 3/3] Allows reading back the current IMA policy;

2015-11-10 Thread Petko Manolov 
On 15-11-09 09:30:58, Mimi Zohar wrote:
> On Mon, 2015-11-02 at 00:39 +0200, Petko Manolov wrote:
> 
> > +
> > +#ifdef CONFIG_IMA_READ_POLICY
> > +enum {
> > +   mask_err = -1,
> > +   mask_exec = 1, mask_write, mask_read, mask_append
> > +};
> > +
> > +static match_table_t mask_tokens = {
> > +   {mask_exec, "MAY_EXEC"},
> > +   {mask_write, "MAY_WRITE"},
> > +   {mask_read, "MAY_READ"},
> > +   {mask_append, "MAY_APPEND"},
> > +   {mask_err, NULL}
> > +};
> > +
> > +enum {
> > +   func_err = -1,
> > +   func_file = 1, func_mmap, func_bprm,
> > +   func_module, func_firmware, func_post
> > +};
> > +
> > +static match_table_t func_tokens = {
> > +   {func_file, "FILE_CHECK"},
> > +   {func_mmap, "MMAP_CHECK"},
> > +   {func_bprm, "BPRM_CHECK"},
> > +   {func_module, "MODULE_CHECK"},
> > +   {func_firmware, "FIRMWARE_CHECK"},
> > +   {func_post, "POST_SETATTR"},
> > +   {func_err, NULL}
> > +};
> 
> Why are we using match_table_t?  Why not define an array of strings which 
> corresponds to the function hooks or use the __stringify macro?

Because you used match_table_t in your code.  Having too many style differences 
does not help it's readability.

> static const char *ima_hooks_string[] = {"", "FILE_CHECK", "MMAP_CHECK",
> "BPRM_CHECK", "MODULE_CHECK", "FIRMWARE_CHECK", "POST_SETATTR"};
> 
> In the first case, to display the function hook string would be 
> "ima_hooks_string[func]".  Using __stringify requires the hook name (eg. 
> __stringify(FILE_CHECK)).
> 
> In either case, there would be a lot less code.

It is always speed vs size.  It is going to be more code or more data.  It is a 
matter of taste which one we'll chose.

If we look at ima_policy.c after the original IMA policy read patch we'll end 
up 
with a source that is littered with strings like "fowner=%s", "fowner" and 
"fowner=%d".  I assumed you want this cleaned up, which i did.

Another example, "FILE_CHECK" was used just once prior to introducing of policy 
read.  Now we use it twice.  Do we want to rely on GCC literals optimization 
and 
use the same string multiple times or hand-optimize it?  What do we do with 
almost the same strings like "fowner=%d", "fowner=%s" and "fowner"?

The answer to the above will determine whether we use an array of strings or 
__stringify.  I kind of lean towards the first option but as a maintainer the 
choice is up to you.


Petko
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/3] Allows reading back the current IMA policy;

2015-11-10 Thread Mimi Zohar 
On Tue, 2015-11-10 at 18:01 +0200, Petko Manolov wrote:
> On 15-11-09 09:30:58, Mimi Zohar wrote:
> > On Mon, 2015-11-02 at 00:39 +0200, Petko Manolov wrote:
> > 
> > > +
> > > +#ifdef   CONFIG_IMA_READ_POLICY
> > > +enum {
> > > + mask_err = -1,
> > > + mask_exec = 1, mask_write, mask_read, mask_append
> > > +};
> > > +
> > > +static match_table_t mask_tokens = {
> > > + {mask_exec, "MAY_EXEC"},
> > > + {mask_write, "MAY_WRITE"},
> > > + {mask_read, "MAY_READ"},
> > > + {mask_append, "MAY_APPEND"},
> > > + {mask_err, NULL}
> > > +};
> > > +
> > > +enum {
> > > + func_err = -1,
> > > + func_file = 1, func_mmap, func_bprm,
> > > + func_module, func_firmware, func_post
> > > +};
> > > +
> > > +static match_table_t func_tokens = {
> > > + {func_file, "FILE_CHECK"},
> > > + {func_mmap, "MMAP_CHECK"},
> > > + {func_bprm, "BPRM_CHECK"},
> > > + {func_module, "MODULE_CHECK"},
> > > + {func_firmware, "FIRMWARE_CHECK"},
> > > + {func_post, "POST_SETATTR"},
> > > + {func_err, NULL}
> > > +};
> > 
> > Why are we using match_table_t?  Why not define an array of strings which 
> > corresponds to the function hooks or use the __stringify macro?
> 
> Because you used match_table_t in your code.  Having too many style 
> differences 
> does not help it's readability.

We're using match_token() for parsing the policy, which requires
match_table_t.  

> > static const char *ima_hooks_string[] = {"", "FILE_CHECK", "MMAP_CHECK",
> > "BPRM_CHECK", "MODULE_CHECK", "FIRMWARE_CHECK", "POST_SETATTR"};
> > 
> > In the first case, to display the function hook string would be 
> > "ima_hooks_string[func]".  Using __stringify requires the hook name (eg. 
> > __stringify(FILE_CHECK)).
> > 
> > In either case, there would be a lot less code.
> 
> It is always speed vs size.  It is going to be more code or more data.  It is 
> a 
> matter of taste which one we'll chose.
> 
> If we look at ima_policy.c after the original IMA policy read patch we'll end 
> up 
> with a source that is littered with strings like "fowner=%s", "fowner" and 
> "fowner=%d".  I assumed you want this cleaned up, which i did.
> 
> Another example, "FILE_CHECK" was used just once prior to introducing of 
> policy 
> read.  Now we use it twice.  Do we want to rely on GCC literals optimization 
> and 
> use the same string multiple times or hand-optimize it?  What do we do with 
> almost the same strings like "fowner=%d", "fowner=%s" and "fowner"?
> 
> The answer to the above will determine whether we use an array of strings or 
> __stringify.  I kind of lean towards the first option but as a maintainer the 
> choice is up to you.

>From what I've seen playing with __stringify, it has its own drawbacks.
I would probably use an enumeration of strings arrays and remove the
switch/case statements as much as possible.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/3] Allows reading back the current IMA policy;

2015-11-09 Thread Mimi Zohar 
On Mon, 2015-11-02 at 00:39 +0200, Petko Manolov wrote:

> +
> +#ifdef   CONFIG_IMA_READ_POLICY
> +enum {
> + mask_err = -1,
> + mask_exec = 1, mask_write, mask_read, mask_append
> +};
> +
> +static match_table_t mask_tokens = {
> + {mask_exec, "MAY_EXEC"},
> + {mask_write, "MAY_WRITE"},
> + {mask_read, "MAY_READ"},
> + {mask_append, "MAY_APPEND"},
> + {mask_err, NULL}
> +};
> +
> +enum {
> + func_err = -1,
> + func_file = 1, func_mmap, func_bprm,
> + func_module, func_firmware, func_post
> +};
> +
> +static match_table_t func_tokens = {
> + {func_file, "FILE_CHECK"},
> + {func_mmap, "MMAP_CHECK"},
> + {func_bprm, "BPRM_CHECK"},
> + {func_module, "MODULE_CHECK"},
> + {func_firmware, "FIRMWARE_CHECK"},
> + {func_post, "POST_SETATTR"},
> + {func_err, NULL}
> +};

Why are we using match_table_t?  Why not define an array of strings
which corresponds to the function hooks or use the __stringify macro?

static const char *ima_hooks_string[] = {"", "FILE_CHECK", "MMAP_CHECK",
"BPRM_CHECK", "MODULE_CHECK", "FIRMWARE_CHECK", "POST_SETATTR"};

In the first case, to display the function hook string would be
"ima_hooks_string[func]".  Using __stringify requires the hook name (eg.
__stringify(FILE_CHECK)).

In either case, there would be a lot less code.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 3/3] Allows reading back the current IMA policy;

2015-11-01 Thread Petko Manolov 
When developing or debugging an IMA enabled system it is useful to be
able to read back the current policy.  This code provides that
functionality.

Signed-off-by: Zbigniew Jasinski 
Signed-off-by: Petko Manolov 
---
 security/integrity/ima/Kconfig  |  11 ++
 security/integrity/ima/ima.h|  13 ++-
 security/integrity/ima/ima_fs.c |  29 +-
 security/integrity/ima/ima_policy.c | 201 +++-
 4 files changed, 247 insertions(+), 7 deletions(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index a489340..8391f76 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -118,6 +118,17 @@ config IMA_WRITE_POLICY
 
  If unsure, say N.
 
+config IMA_READ_POLICY
+   bool "Enable reading back the current IMA policy"
+   depends on IMA
+   default n
+   help
+  When developing or debugging an IMA enabled system it is often
+  useful to be able to read the IMA policy.  This options allows
+  for doing so.
+
+  If unsure, say N.
+
 config IMA_APPRAISE
bool "Appraise integrity measurements"
depends on IMA
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e2a60c3..ebc3554 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -166,6 +166,10 @@ void ima_update_policy(void);
 void ima_update_policy_flag(void);
 ssize_t ima_parse_add_rule(char *);
 void ima_delete_rules(void);
+void *ima_policy_start(struct seq_file *m, loff_t *pos);
+void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
+void ima_policy_stop(struct seq_file *m, void *v);
+int ima_policy_show(struct seq_file *m, void *v);
 
 /* Appraise integrity measurements */
 #define IMA_APPRAISE_ENFORCE   0x01
@@ -263,4 +267,11 @@ static inline int ima_init_keyring(const unsigned int id)
return 0;
 }
 #endif /* CONFIG_IMA_TRUSTED_KEYRING */
-#endif
+
+#ifdef CONFIG_IMA_READ_POLICY
+#definePOLICY_FILE_FLAGS   (S_IWUSR | S_IRUSR)
+#else
+#definePOLICY_FILE_FLAGS   S_IWUSR
+#endif /* CONFIG_IMA_WRITE_POLICY */
+
+#endif /* __LINUX_IMA_H */
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index a3cf5c0..eebb985 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -311,14 +311,31 @@ enum ima_fs_flags {
 
 static unsigned long ima_fs_flags;
 
+#ifdef CONFIG_IMA_READ_POLICY
+static const struct seq_operations ima_policy_seqops = {
+   .start = ima_policy_start,
+   .next = ima_policy_next,
+   .stop = ima_policy_stop,
+   .show = ima_policy_show,
+};
+#endif
+
 /*
  * ima_open_policy: sequentialize access to the policy file
  */
 static int ima_open_policy(struct inode *inode, struct file *filp)
 {
-   /* No point in being allowed to open it if you aren't going to write */
-   if (!(filp->f_flags & O_WRONLY))
+   if (!(filp->f_flags & O_WRONLY)) {
+#ifndefCONFIG_IMA_READ_POLICY
return -EACCES;
+#else
+   if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
+   return -EACCES;
+   if (!capable(CAP_SYS_ADMIN))
+   return -EPERM;
+   return seq_open(filp, _policy_seqops);
+#endif
+   }
if (test_and_set_bit(IMA_FS_BUSY, _fs_flags))
return -EBUSY;
return 0;
@@ -335,6 +352,9 @@ static int ima_release_policy(struct inode *inode, struct 
file *file)
 {
const char *cause = valid_policy ? "completed" : "failed";
 
+   if ((file->f_flags & O_ACCMODE) == O_RDONLY)
+   return 0;
+
pr_info("IMA: policy update %s\n", cause);
integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
"policy_update", cause, !valid_policy, 0);
@@ -345,6 +365,7 @@ static int ima_release_policy(struct inode *inode, struct 
file *file)
clear_bit(IMA_FS_BUSY, _fs_flags);
return 0;
}
+
ima_update_policy();
 #ifndefCONFIG_IMA_WRITE_POLICY
securityfs_remove(ima_policy);
@@ -358,6 +379,7 @@ static int ima_release_policy(struct inode *inode, struct 
file *file)
 static const struct file_operations ima_measure_policy_ops = {
.open = ima_open_policy,
.write = ima_write_policy,
+   .read = seq_read,
.release = ima_release_policy,
.llseek = generic_file_llseek,
 };
@@ -395,8 +417,7 @@ int __init ima_fs_init(void)
if (IS_ERR(violations))
goto out;
 
-   ima_policy = securityfs_create_file("policy",
-   S_IWUSR,
+   ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
ima_dir, NULL,
_measure_policy_ops);
if