* [PATCH v2 1/4] ima: added policy support for 'security.ima' type
2013-01-22 22:07 [PATCH v2 0/4] ima: enforcing appraise type Mimi Zohar
@ 2013-01-22 22:07 ` Mimi Zohar
2013-01-30 21:53 ` Vivek Goyal
2013-01-22 22:07 ` [PATCH v2 2/4] ima: increase iint flag size Mimi Zohar
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2013-01-22 22:07 UTC (permalink / raw)
To: linux-security-module
Cc: Dmitry Kasatkin, James Morris, linux-kernel, Mimi Zohar
From: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
The 'security.ima' extended attribute may contain either the file data's
hash or a digital signature. This patch adds support for requiring a
specific extended attribute type. It extends the IMA policy with a new
keyword 'appraise_type=imasig'. (Default is hash.)
Changelog v2:
- Fixed Documentation/ABI/testing/ima_policy option syntax
Changelog v1:
- Differentiate between 'required' vs. 'actual' extended attribute
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
Documentation/ABI/testing/ima_policy | 4 +++-
security/integrity/ima/ima_appraise.c | 5 +++++
security/integrity/ima/ima_main.c | 1 +
security/integrity/ima/ima_policy.c | 18 +++++++++++++++++-
security/integrity/integrity.h | 2 ++
5 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 6a0fc80..de16de3 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -18,10 +18,11 @@ Description:
rule format: action [condition ...]
action: measure | dont_measure | appraise | dont_appraise | audit
- condition:= base | lsm
+ condition:= base | lsm [option]
base: [[func=] [mask=] [fsmagic=] [uid=] [fowner]]
lsm: [[subj_user=] [subj_role=] [subj_type=]
[obj_user=] [obj_role=] [obj_type=]]
+ option: [[appraise_type=]]
base: func:= [BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK]
mask:= [MAY_READ] [MAY_WRITE] [MAY_APPEND] [MAY_EXEC]
@@ -29,6 +30,7 @@ Description:
uid:= decimal value
fowner:=decimal value
lsm: are LSM specific
+ option: appraise_type:= [imasig]
default policy:
# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index fa675c9..8004332 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -102,6 +102,11 @@ int ima_appraise_measurement(struct integrity_iint_cache *iint,
switch (xattr_value->type) {
case IMA_XATTR_DIGEST:
+ if (iint->flags & IMA_DIGSIG_REQUIRED) {
+ cause = "IMA signature required";
+ status = INTEGRITY_FAIL;
+ break;
+ }
rc = memcmp(xattr_value->digest, iint->ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc) {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index cd00ba3..3cdd787 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -169,6 +169,7 @@ static int process_measurement(struct file *file, const char *filename,
* (IMA_MEASURE, IMA_MEASURED, IMA_APPRAISE, IMA_APPRAISED,
* IMA_AUDIT, IMA_AUDITED) */
iint->flags |= action;
+ action &= IMA_DO_MASK;
action &= ~((iint->flags & IMA_DONE_MASK) >> 1);
/* Nothing to do, just return existing appraised status */
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 9519453..1a2543a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -245,6 +245,8 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
if (!ima_match_rules(entry, inode, func, mask))
continue;
+ action |= entry->flags & IMA_ACTION_FLAGS;
+
action |= entry->action & IMA_DO_MASK;
if (entry->action & IMA_DO_MASK)
actmask &= ~(entry->action | entry->action << 1);
@@ -318,7 +320,8 @@ enum {
Opt_audit,
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_uid, Opt_fowner
+ Opt_func, Opt_mask, Opt_fsmagic, Opt_uid, Opt_fowner,
+ Opt_appraise_type
};
static match_table_t policy_tokens = {
@@ -338,6 +341,7 @@ static match_table_t policy_tokens = {
{Opt_fsmagic, "fsmagic=%s"},
{Opt_uid, "uid=%s"},
{Opt_fowner, "fowner=%s"},
+ {Opt_appraise_type, "appraise_type=%s"},
{Opt_err, NULL}
};
@@ -560,6 +564,18 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
LSM_SUBJ_TYPE,
AUDIT_SUBJ_TYPE);
break;
+ case Opt_appraise_type:
+ if (entry->action != APPRAISE) {
+ result = -EINVAL;
+ break;
+ }
+
+ ima_log_string(ab, "appraise_type", args[0].from);
+ if ((strcmp(args[0].from, "imasig")) == 0)
+ entry->flags |= IMA_DIGSIG_REQUIRED;
+ else
+ result = -EINVAL;
+ break;
case Opt_err:
ima_log_string(ab, "UNKNOWN", p);
result = -EINVAL;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 0a298de..9334691b 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -26,7 +26,9 @@
#define IMA_AUDITED 0x0080
/* iint cache flags */
+#define IMA_ACTION_FLAGS 0xff00
#define IMA_DIGSIG 0x0100
+#define IMA_DIGSIG_REQUIRED 0x0200
#define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT)
#define IMA_DONE_MASK (IMA_MEASURED | IMA_APPRAISED | IMA_AUDITED \
--
1.8.1.rc3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/4] ima: added policy support for 'security.ima' type
2013-01-22 22:07 ` [PATCH v2 1/4] ima: added policy support for 'security.ima' type Mimi Zohar
@ 2013-01-30 21:53 ` Vivek Goyal
2013-01-30 22:42 ` Mimi Zohar
0 siblings, 1 reply; 10+ messages in thread
From: Vivek Goyal @ 2013-01-30 21:53 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-security-module, Dmitry Kasatkin, James Morris,
linux-kernel
On Tue, Jan 22, 2013 at 05:07:31PM -0500, Mimi Zohar wrote:
[..]
> /* iint cache flags */
> +#define IMA_ACTION_FLAGS 0xff00
> #define IMA_DIGSIG 0x0100
> +#define IMA_DIGSIG_REQUIRED 0x0200
Hi Mimi,
IMA_DIGSIG_REQUIRED state does not really have to be stored in iint I guess.
This is needed only if we go on to appraise and then we want to make
sure digital signatures are present. Once appraisal is done, IMG_DIGSIG
will be set and saved in iint->flags but looks like we don't require
IMA_DIGSIG_REQUIRED to be saved.
If yes, will it make sense to not save it in iint. There are still 8
bits left unused. We can mark these 8 bits for temporary flags like
IMA_DIGSIG_REQUIRED. If that works, I can use same space for defining
additional temporary flags like IMA_DIGSIG_SIGNED_ONLY to handle the
case of appraise_type=imasig,signed_only. That flag also does not have
to be persistent in iint.
Here is a quick patch compile tested only.
Thanks
Vivek
---
security/integrity/ima/ima.h | 5 +++--
security/integrity/ima/ima_appraise.c | 5 +++--
security/integrity/ima/ima_main.c | 8 ++++++--
security/integrity/ima/ima_policy.c | 2 +-
security/integrity/integrity.h | 5 ++++-
5 files changed, 17 insertions(+), 8 deletions(-)
Index: linux-integrity/security/integrity/integrity.h
===================================================================
--- linux-integrity.orig/security/integrity/integrity.h 2013-01-30 16:22:04.000000000 -0500
+++ linux-integrity/security/integrity/integrity.h 2013-01-30 16:25:03.736892034 -0500
@@ -28,7 +28,10 @@
/* iint cache flags */
#define IMA_ACTION_FLAGS 0xff000000
#define IMA_DIGSIG 0x01000000
-#define IMA_DIGSIG_REQUIRED 0x02000000
+
+/* Temp flags not stored in iint */
+#define IMA_TEMP_FLAGS 0x00ff0000
+#define IMA_DIGSIG_REQUIRED 0x00010000
#define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
IMA_APPRAISE_SUBMASK)
Index: linux-integrity/security/integrity/ima/ima_main.c
===================================================================
--- linux-integrity.orig/security/integrity/ima/ima_main.c 2013-01-29 17:13:05.000000000 -0500
+++ linux-integrity/security/integrity/ima/ima_main.c 2013-01-30 16:39:09.414918012 -0500
@@ -146,7 +146,7 @@ static int process_measurement(struct fi
struct integrity_iint_cache *iint;
char *pathbuf = NULL;
const char *pathname = NULL;
- int rc = -ENOMEM, action, must_appraise, _func;
+ int rc = -ENOMEM, action, must_appraise, _func, temp_flags;
if (!ima_initialized || !S_ISREG(inode->i_mode))
return 0;
@@ -174,6 +174,9 @@ static int process_measurement(struct fi
* (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
* IMA_AUDIT, IMA_AUDITED)
*/
+ /* Temp flags don't have to be stored in iint */
+ temp_flags = action & IMA_TEMP_FLAGS;
+ action &= ~IMA_TEMP_FLAGS;
iint->flags |= action;
action &= IMA_DO_MASK;
action &= ~((iint->flags & IMA_DONE_MASK) >> 1);
@@ -198,7 +201,8 @@ static int process_measurement(struct fi
if (action & IMA_MEASURE)
ima_store_measurement(iint, file, pathname);
if (action & IMA_APPRAISE_SUBMASK)
- rc = ima_appraise_measurement(_func, iint, file, pathname);
+ rc = ima_appraise_measurement(_func, iint, file, pathname,
+ temp_flags);
if (action & IMA_AUDIT)
ima_audit_measurement(iint, pathname);
kfree(pathbuf);
Index: linux-integrity/security/integrity/ima/ima_appraise.c
===================================================================
--- linux-integrity.orig/security/integrity/ima/ima_appraise.c 2013-01-30 16:22:04.000000000 -0500
+++ linux-integrity/security/integrity/ima/ima_appraise.c 2013-01-30 16:37:11.667914395 -0500
@@ -116,7 +116,8 @@ static void ima_cache_flags(struct integ
* Return 0 on success, error code otherwise
*/
int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
- struct file *file, const unsigned char *filename)
+ struct file *file, const unsigned char *filename,
+ int flags)
{
struct dentry *dentry = file->f_dentry;
struct inode *inode = dentry->d_inode;
@@ -154,7 +155,7 @@ int ima_appraise_measurement(int func, s
}
switch (xattr_value->type) {
case IMA_XATTR_DIGEST:
- if (iint->flags & IMA_DIGSIG_REQUIRED) {
+ if (flags & IMA_DIGSIG_REQUIRED) {
cause = "IMA signature required";
status = INTEGRITY_FAIL;
break;
Index: linux-integrity/security/integrity/ima/ima.h
===================================================================
--- linux-integrity.orig/security/integrity/ima/ima.h 2013-01-30 10:46:32.000000000 -0500
+++ linux-integrity/security/integrity/ima/ima.h 2013-01-30 16:38:40.800917133 -0500
@@ -143,7 +143,7 @@ void ima_delete_rules(void);
#ifdef CONFIG_IMA_APPRAISE
int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
- struct file *file, const unsigned char *filename);
+ struct file *file, const unsigned char *filename, int flags);
int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func);
void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
@@ -153,7 +153,8 @@ enum integrity_status ima_get_cache_stat
static inline int ima_appraise_measurement(int func,
struct integrity_iint_cache *iint,
struct file *file,
- const unsigned char *filename)
+ const unsigned char *filename,
+ int flags)
{
return INTEGRITY_UNKNOWN;
}
Index: linux-integrity/security/integrity/ima/ima_policy.c
===================================================================
--- linux-integrity.orig/security/integrity/ima/ima_policy.c 2013-01-30 16:41:39.000000000 -0500
+++ linux-integrity/security/integrity/ima/ima_policy.c 2013-01-30 16:41:56.151923133 -0500
@@ -267,7 +267,7 @@ int ima_match_policy(struct inode *inode
if (!ima_match_rules(entry, inode, func, mask))
continue;
- action |= entry->flags & IMA_ACTION_FLAGS;
+ action |= entry->flags & IMA_TEMP_FLAGS;
action |= entry->action & IMA_DO_MASK;
if (entry->action & IMA_APPRAISE)
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/4] ima: added policy support for 'security.ima' type
2013-01-30 21:53 ` Vivek Goyal
@ 2013-01-30 22:42 ` Mimi Zohar
2013-01-31 18:41 ` Vivek Goyal
0 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2013-01-30 22:42 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-security-module, Dmitry Kasatkin, James Morris,
linux-kernel
On Wed, 2013-01-30 at 16:53 -0500, Vivek Goyal wrote:
> On Tue, Jan 22, 2013 at 05:07:31PM -0500, Mimi Zohar wrote:
>
> [..]
> > /* iint cache flags */
> > +#define IMA_ACTION_FLAGS 0xff00
> > #define IMA_DIGSIG 0x0100
> > +#define IMA_DIGSIG_REQUIRED 0x0200
>
> Hi Mimi,
>
> IMA_DIGSIG_REQUIRED state does not really have to be stored in iint I guess.
> This is needed only if we go on to appraise and then we want to make
> sure digital signatures are present. Once appraisal is done, IMG_DIGSIG
> will be set and saved in iint->flags but looks like we don't require
> IMA_DIGSIG_REQUIRED to be saved.
>
> If yes, will it make sense to not save it in iint. There are still 8
> bits left unused. We can mark these 8 bits for temporary flags like
> IMA_DIGSIG_REQUIRED. If that works, I can use same space for defining
> additional temporary flags like IMA_DIGSIG_SIGNED_ONLY to handle the
> case of appraise_type=imasig,signed_only. That flag also does not have
> to be persistent in iint.
Interesting idea. My only concern would be that we're not leaving room
for additional hooks (eg. firmware).
thanks,
Mimi
> Here is a quick patch compile tested only.
>
> Thanks
> Vivek
>
> ---
> security/integrity/ima/ima.h | 5 +++--
> security/integrity/ima/ima_appraise.c | 5 +++--
> security/integrity/ima/ima_main.c | 8 ++++++--
> security/integrity/ima/ima_policy.c | 2 +-
> security/integrity/integrity.h | 5 ++++-
> 5 files changed, 17 insertions(+), 8 deletions(-)
>
> Index: linux-integrity/security/integrity/integrity.h
> ===================================================================
> --- linux-integrity.orig/security/integrity/integrity.h 2013-01-30 16:22:04.000000000 -0500
> +++ linux-integrity/security/integrity/integrity.h 2013-01-30 16:25:03.736892034 -0500
> @@ -28,7 +28,10 @@
> /* iint cache flags */
> #define IMA_ACTION_FLAGS 0xff000000
> #define IMA_DIGSIG 0x01000000
> -#define IMA_DIGSIG_REQUIRED 0x02000000
> +
> +/* Temp flags not stored in iint */
> +#define IMA_TEMP_FLAGS 0x00ff0000
> +#define IMA_DIGSIG_REQUIRED 0x00010000
>
> #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
> IMA_APPRAISE_SUBMASK)
> Index: linux-integrity/security/integrity/ima/ima_main.c
> ===================================================================
> --- linux-integrity.orig/security/integrity/ima/ima_main.c 2013-01-29 17:13:05.000000000 -0500
> +++ linux-integrity/security/integrity/ima/ima_main.c 2013-01-30 16:39:09.414918012 -0500
> @@ -146,7 +146,7 @@ static int process_measurement(struct fi
> struct integrity_iint_cache *iint;
> char *pathbuf = NULL;
> const char *pathname = NULL;
> - int rc = -ENOMEM, action, must_appraise, _func;
> + int rc = -ENOMEM, action, must_appraise, _func, temp_flags;
>
> if (!ima_initialized || !S_ISREG(inode->i_mode))
> return 0;
> @@ -174,6 +174,9 @@ static int process_measurement(struct fi
> * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
> * IMA_AUDIT, IMA_AUDITED)
> */
> + /* Temp flags don't have to be stored in iint */
> + temp_flags = action & IMA_TEMP_FLAGS;
> + action &= ~IMA_TEMP_FLAGS;
> iint->flags |= action;
> action &= IMA_DO_MASK;
> action &= ~((iint->flags & IMA_DONE_MASK) >> 1);
> @@ -198,7 +201,8 @@ static int process_measurement(struct fi
> if (action & IMA_MEASURE)
> ima_store_measurement(iint, file, pathname);
> if (action & IMA_APPRAISE_SUBMASK)
> - rc = ima_appraise_measurement(_func, iint, file, pathname);
> + rc = ima_appraise_measurement(_func, iint, file, pathname,
> + temp_flags);
> if (action & IMA_AUDIT)
> ima_audit_measurement(iint, pathname);
> kfree(pathbuf);
> Index: linux-integrity/security/integrity/ima/ima_appraise.c
> ===================================================================
> --- linux-integrity.orig/security/integrity/ima/ima_appraise.c 2013-01-30 16:22:04.000000000 -0500
> +++ linux-integrity/security/integrity/ima/ima_appraise.c 2013-01-30 16:37:11.667914395 -0500
> @@ -116,7 +116,8 @@ static void ima_cache_flags(struct integ
> * Return 0 on success, error code otherwise
> */
> int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> - struct file *file, const unsigned char *filename)
> + struct file *file, const unsigned char *filename,
> + int flags)
> {
> struct dentry *dentry = file->f_dentry;
> struct inode *inode = dentry->d_inode;
> @@ -154,7 +155,7 @@ int ima_appraise_measurement(int func, s
> }
> switch (xattr_value->type) {
> case IMA_XATTR_DIGEST:
> - if (iint->flags & IMA_DIGSIG_REQUIRED) {
> + if (flags & IMA_DIGSIG_REQUIRED) {
> cause = "IMA signature required";
> status = INTEGRITY_FAIL;
> break;
> Index: linux-integrity/security/integrity/ima/ima.h
> ===================================================================
> --- linux-integrity.orig/security/integrity/ima/ima.h 2013-01-30 10:46:32.000000000 -0500
> +++ linux-integrity/security/integrity/ima/ima.h 2013-01-30 16:38:40.800917133 -0500
> @@ -143,7 +143,7 @@ void ima_delete_rules(void);
>
> #ifdef CONFIG_IMA_APPRAISE
> int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> - struct file *file, const unsigned char *filename);
> + struct file *file, const unsigned char *filename, int flags);
> int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func);
> void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
> enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
> @@ -153,7 +153,8 @@ enum integrity_status ima_get_cache_stat
> static inline int ima_appraise_measurement(int func,
> struct integrity_iint_cache *iint,
> struct file *file,
> - const unsigned char *filename)
> + const unsigned char *filename,
> + int flags)
> {
> return INTEGRITY_UNKNOWN;
> }
> Index: linux-integrity/security/integrity/ima/ima_policy.c
> ===================================================================
> --- linux-integrity.orig/security/integrity/ima/ima_policy.c 2013-01-30 16:41:39.000000000 -0500
> +++ linux-integrity/security/integrity/ima/ima_policy.c 2013-01-30 16:41:56.151923133 -0500
> @@ -267,7 +267,7 @@ int ima_match_policy(struct inode *inode
> if (!ima_match_rules(entry, inode, func, mask))
> continue;
>
> - action |= entry->flags & IMA_ACTION_FLAGS;
> + action |= entry->flags & IMA_TEMP_FLAGS;
>
> action |= entry->action & IMA_DO_MASK;
> if (entry->action & IMA_APPRAISE)
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/4] ima: added policy support for 'security.ima' type
2013-01-30 22:42 ` Mimi Zohar
@ 2013-01-31 18:41 ` Vivek Goyal
2013-01-31 19:25 ` Mimi Zohar
0 siblings, 1 reply; 10+ messages in thread
From: Vivek Goyal @ 2013-01-31 18:41 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-security-module, Dmitry Kasatkin, James Morris,
linux-kernel
On Wed, Jan 30, 2013 at 05:42:39PM -0500, Mimi Zohar wrote:
> On Wed, 2013-01-30 at 16:53 -0500, Vivek Goyal wrote:
> > On Tue, Jan 22, 2013 at 05:07:31PM -0500, Mimi Zohar wrote:
> >
> > [..]
> > > /* iint cache flags */
> > > +#define IMA_ACTION_FLAGS 0xff00
> > > #define IMA_DIGSIG 0x0100
> > > +#define IMA_DIGSIG_REQUIRED 0x0200
> >
> > Hi Mimi,
> >
> > IMA_DIGSIG_REQUIRED state does not really have to be stored in iint I guess.
> > This is needed only if we go on to appraise and then we want to make
> > sure digital signatures are present. Once appraisal is done, IMG_DIGSIG
> > will be set and saved in iint->flags but looks like we don't require
> > IMA_DIGSIG_REQUIRED to be saved.
> >
> > If yes, will it make sense to not save it in iint. There are still 8
> > bits left unused. We can mark these 8 bits for temporary flags like
> > IMA_DIGSIG_REQUIRED. If that works, I can use same space for defining
> > additional temporary flags like IMA_DIGSIG_SIGNED_ONLY to handle the
> > case of appraise_type=imasig,signed_only. That flag also does not have
> > to be persistent in iint.
>
> Interesting idea. My only concern would be that we're not leaving room
> for additional hooks (eg. firmware).
Ok. May be we can employ another 32bit variable and put both of them in
a structure to expand it, when firmware hooks come along.
Or I can do that right now, if you like it that way.
I need to move out non-persistent flags from iint flags, otherwise it
can happen that conflicting flag might be set from different hooks.
For example, say hypothetically there are too hooks.
appraise fowner=0 func=mmap appraise_type=imasig
appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_signed_only
Now this will lead to setting of both IMA_DISGIG_REQUIRED and
IMA_DIGSIG_SIGNED_ONLY in iint->flags and that does not mean much in
the context of iint.
So by keeping them temporary, these flags become kind of hook specific
and then two appraise hook don't conflict with each other.
Thanks
Vivek
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/4] ima: added policy support for 'security.ima' type
2013-01-31 18:41 ` Vivek Goyal
@ 2013-01-31 19:25 ` Mimi Zohar
2013-01-31 19:37 ` Vivek Goyal
0 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2013-01-31 19:25 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-security-module, Dmitry Kasatkin, James Morris,
linux-kernel
On Thu, 2013-01-31 at 13:41 -0500, Vivek Goyal wrote:
> On Wed, Jan 30, 2013 at 05:42:39PM -0500, Mimi Zohar wrote:
> > On Wed, 2013-01-30 at 16:53 -0500, Vivek Goyal wrote:
> > > On Tue, Jan 22, 2013 at 05:07:31PM -0500, Mimi Zohar wrote:
> > >
> > > [..]
> > > > /* iint cache flags */
> > > > +#define IMA_ACTION_FLAGS 0xff00
> > > > #define IMA_DIGSIG 0x0100
> > > > +#define IMA_DIGSIG_REQUIRED 0x0200
> > >
> > > Hi Mimi,
> > >
> > > IMA_DIGSIG_REQUIRED state does not really have to be stored in iint I guess.
> > > This is needed only if we go on to appraise and then we want to make
> > > sure digital signatures are present. Once appraisal is done, IMG_DIGSIG
> > > will be set and saved in iint->flags but looks like we don't require
> > > IMA_DIGSIG_REQUIRED to be saved.
> > >
> > > If yes, will it make sense to not save it in iint. There are still 8
> > > bits left unused. We can mark these 8 bits for temporary flags like
> > > IMA_DIGSIG_REQUIRED. If that works, I can use same space for defining
> > > additional temporary flags like IMA_DIGSIG_SIGNED_ONLY to handle the
> > > case of appraise_type=imasig,signed_only. That flag also does not have
> > > to be persistent in iint.
> >
> > Interesting idea. My only concern would be that we're not leaving room
> > for additional hooks (eg. firmware).
>
> Ok. May be we can employ another 32bit variable and put both of them in
> a structure to expand it, when firmware hooks come along.
>
> Or I can do that right now, if you like it that way.
>
> I need to move out non-persistent flags from iint flags, otherwise it
> can happen that conflicting flag might be set from different hooks.
>
> For example, say hypothetically there are too hooks.
>
> appraise fowner=0 func=mmap appraise_type=imasig
> appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_signed_only
>
> Now this will lead to setting of both IMA_DISGIG_REQUIRED and
> IMA_DIGSIG_SIGNED_ONLY in iint->flags and that does not mean much in
> the context of iint.
>
> So by keeping them temporary, these flags become kind of hook specific
> and then two appraise hook don't conflict with each other.
With the following patches, which James pulled earlier this week, each
hook can have a different appraise status.
5a73fcf ima: differentiate appraise status only for hook specific rules
d79d72e ima: per hook cache integrity appraisal status
f578c08 ima: increase iint flag size
0e5a247 ima: added policy support for 'security.ima' type
thanks,
Mimi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/4] ima: added policy support for 'security.ima' type
2013-01-31 19:25 ` Mimi Zohar
@ 2013-01-31 19:37 ` Vivek Goyal
0 siblings, 0 replies; 10+ messages in thread
From: Vivek Goyal @ 2013-01-31 19:37 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-security-module, Dmitry Kasatkin, James Morris,
linux-kernel
On Thu, Jan 31, 2013 at 02:25:11PM -0500, Mimi Zohar wrote:
[..]
> With the following patches, which James pulled earlier this week, each
> hook can have a different appraise status.
>
> 5a73fcf ima: differentiate appraise status only for hook specific rules
> d79d72e ima: per hook cache integrity appraisal status
> f578c08 ima: increase iint flag size
> 0e5a247 ima: added policy support for 'security.ima' type
Yes I understand that. I am not worried about appraise status of each hook.
I am only concerned about IMA_DIGSIG_REQUIRED flag which does not belong to
iint->flags.
As now I am trying to extend appraise_type to be able to appraise only
signed executables and I need one more extra flag to keep track of that
state.
If I just use one bit from IMA_ACTION_FLAGS (like IMA_DIGSIG_REQUIRED),
say IMA_DIGSIG_SIGNED_ONLY, then we can potentially run into the
situation where two different appraise hooks set these two flags
separately causing some confusion.
To have written following patch which just uses one bit from
IMA_ACTION_FLAGS. But I am not too happy with it. That's why I wanted
to move temporary flags out of IMA_ACTION_FLAGS.
Thanks
Vivek
ima: Extend appraise policy to appraise only signed files
Currently appraise policy appraises every file. Allow to configure rules
such that one can specify that appraise only signed files and unsinged
ones just pass integrity test always.
This patch etends appraise type and one can specify
apprasie_type=imasig,signed_only to appraise only signed files.
I think there is one issue here. That is new flag IMA_DIGSIG_SIGNED_ONLY
is not hook specific. So if mmap hook says appraise_type=imasig
and bprm_check hook says appraise_type=ima_sig,signed_only, I think
mmap hook settings will be overidded. As both IMA_DIGSIG_REQUIRED and
IMA_DIGSIG_SIGNED_ONLY flags will be set (one from mmap hook and
other from bprm_check hook).
I guess to support it we require hook specific flags.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
security/integrity/ima/ima_appraise.c | 32 ++++++++++++++++++++++++++++----
security/integrity/ima/ima_policy.c | 2 ++
security/integrity/integrity.h | 2 ++
3 files changed, 32 insertions(+), 4 deletions(-)
Index: linux-integrity/security/integrity/integrity.h
===================================================================
--- linux-integrity.orig/security/integrity/integrity.h 2013-01-30 10:46:32.793412454 -0500
+++ linux-integrity/security/integrity/integrity.h 2013-01-30 10:46:33.553412477 -0500
@@ -29,6 +29,8 @@
#define IMA_ACTION_FLAGS 0xff000000
#define IMA_DIGSIG 0x01000000
#define IMA_DIGSIG_REQUIRED 0x02000000
+/* Appraise digital signature only, if present */
+#define IMA_DIGSIG_SIGNED_ONLY 0x04000000
#define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
IMA_APPRAISE_SUBMASK)
Index: linux-integrity/security/integrity/ima/ima_policy.c
===================================================================
--- linux-integrity.orig/security/integrity/ima/ima_policy.c 2013-01-30 10:46:32.793412454 -0500
+++ linux-integrity/security/integrity/ima/ima_policy.c 2013-01-30 10:46:33.554412477 -0500
@@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, st
ima_log_string(ab, "appraise_type", args[0].from);
if ((strcmp(args[0].from, "imasig")) == 0)
entry->flags |= IMA_DIGSIG_REQUIRED;
+ else if (!strcmp(args[0].from, "imasig,signed_only"))
+ entry->flags |= IMA_DIGSIG_SIGNED_ONLY;
else
result = -EINVAL;
break;
Index: linux-integrity/security/integrity/ima/ima_appraise.c
===================================================================
--- linux-integrity.orig/security/integrity/ima/ima_appraise.c 2013-01-30 10:46:32.793412454 -0500
+++ linux-integrity/security/integrity/ima/ima_appraise.c 2013-01-30 15:34:54.500286490 -0500
@@ -124,16 +124,27 @@ int ima_appraise_measurement(int func, s
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = "appraise_data";
char *cause = "unknown";
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode->i_op->getxattr)
+ if (!inode->i_op->getxattr) {
+ /* getxattr not supported. file couldn't have been signed */
+ if (iint->flags & IMA_DIGSIG_SIGNED_ONLY)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)&xattr_value,
0, GFP_NOFS);
if (rc <= 0) {
+ if (rc == -EOPNOTSUPP) {
+ /* File system does not have security label support */
+ if (iint->flags & IMA_DIGSIG_SIGNED_ONLY)
+ return INTEGRITY_PASS;
+ return INTEGRITY_UNKNOWN;
+ }
+
if (rc && rc != -ENODATA)
goto out;
@@ -159,6 +170,13 @@ int ima_appraise_measurement(int func, s
status = INTEGRITY_FAIL;
break;
}
+
+ if (iint->flags & IMA_DIGSIG_SIGNED_ONLY) {
+ /* Appraise happens only for digital signatures */
+ status = INTEGRITY_PASS;
+ break;
+ }
+
rc = memcmp(xattr_value->digest, iint->ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc) {
@@ -197,8 +215,14 @@ out:
if (!ima_fix_xattr(dentry, iint))
status = INTEGRITY_PASS;
}
- integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
- op, cause, rc, 0);
+ if (status == INTEGRITY_NOLABEL &&
+ iint->flags & IMA_DIGSIG_SIGNED_ONLY) {
+ status = INTEGRITY_PASS;
+ /* Don't flood audit logs with skipped appraise */
+ audit_info = 1;
+ }
+ integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
+ filename, op, cause, rc, audit_info);
} else {
ima_cache_flags(iint, func);
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/4] ima: increase iint flag size
2013-01-22 22:07 [PATCH v2 0/4] ima: enforcing appraise type Mimi Zohar
2013-01-22 22:07 ` [PATCH v2 1/4] ima: added policy support for 'security.ima' type Mimi Zohar
@ 2013-01-22 22:07 ` Mimi Zohar
2013-01-22 22:07 ` [PATCH v2 3/4] ima: per hook cache integrity appraisal status Mimi Zohar
2013-01-22 22:07 ` [PATCH v2 4/4] ima: differentiate appraise status only for hook specific rules Mimi Zohar
3 siblings, 0 replies; 10+ messages in thread
From: Mimi Zohar @ 2013-01-22 22:07 UTC (permalink / raw)
To: linux-security-module
Cc: Mimi Zohar, James Morris, linux-kernel, Dmitry Kasatkin
In preparation for hook specific appraise status results, increase
the iint flags size.
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
---
security/integrity/integrity.h | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 9334691b..329ad26 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -16,19 +16,19 @@
#include <crypto/sha.h>
/* iint action cache flags */
-#define IMA_MEASURE 0x0001
-#define IMA_MEASURED 0x0002
-#define IMA_APPRAISE 0x0004
-#define IMA_APPRAISED 0x0008
-/*#define IMA_COLLECT 0x0010 do not use this flag */
-#define IMA_COLLECTED 0x0020
-#define IMA_AUDIT 0x0040
-#define IMA_AUDITED 0x0080
+#define IMA_MEASURE 0x00000001
+#define IMA_MEASURED 0x00000002
+#define IMA_APPRAISE 0x00000004
+#define IMA_APPRAISED 0x00000008
+/*#define IMA_COLLECT 0x00000010 do not use this flag */
+#define IMA_COLLECTED 0x00000020
+#define IMA_AUDIT 0x00000040
+#define IMA_AUDITED 0x00000080
/* iint cache flags */
-#define IMA_ACTION_FLAGS 0xff00
-#define IMA_DIGSIG 0x0100
-#define IMA_DIGSIG_REQUIRED 0x0200
+#define IMA_ACTION_FLAGS 0xff000000
+#define IMA_DIGSIG 0x01000000
+#define IMA_DIGSIG_REQUIRED 0x02000000
#define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT)
#define IMA_DONE_MASK (IMA_MEASURED | IMA_APPRAISED | IMA_AUDITED \
@@ -50,7 +50,7 @@ struct integrity_iint_cache {
struct rb_node rb_node; /* rooted in integrity_iint_tree */
struct inode *inode; /* back pointer to inode in question */
u64 version; /* track inode changes */
- unsigned short flags;
+ unsigned long flags;
struct evm_ima_xattr_data ima_xattr;
enum integrity_status ima_status:4;
enum integrity_status evm_status:4;
--
1.8.1.rc3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 3/4] ima: per hook cache integrity appraisal status
2013-01-22 22:07 [PATCH v2 0/4] ima: enforcing appraise type Mimi Zohar
2013-01-22 22:07 ` [PATCH v2 1/4] ima: added policy support for 'security.ima' type Mimi Zohar
2013-01-22 22:07 ` [PATCH v2 2/4] ima: increase iint flag size Mimi Zohar
@ 2013-01-22 22:07 ` Mimi Zohar
2013-01-22 22:07 ` [PATCH v2 4/4] ima: differentiate appraise status only for hook specific rules Mimi Zohar
3 siblings, 0 replies; 10+ messages in thread
From: Mimi Zohar @ 2013-01-22 22:07 UTC (permalink / raw)
To: linux-security-module
Cc: Mimi Zohar, James Morris, linux-kernel, Dmitry Kasatkin
With the new IMA policy 'appraise_type=' option, different hooks
can require different methods for appraising a file's integrity.
For example, the existing 'ima_appraise_tcb' policy defines a
generic rule, requiring all root files to be appraised, without
specfying the appraisal method. A more specific rule could require
all kernel modules, for example, to be signed.
appraise fowner=0 func=MODULE_CHECK appraise_type=imasig
appraise fowner=0
As a result, the integrity appraisal results for the same inode, but
for different hooks, could differ. This patch caches the integrity
appraisal results on a per hook basis.
Changelog v2:
- Rename ima_cache_status() to ima_set_cache_status()
- Rename and move get_appraise_status() to ima_get_cache_status()
Changelog v0:
- include IMA_APPRAISE/APPRAISED_SUBMASK in IMA_DO/DONE_MASK (Dmitry)
- Support independent MODULE_CHECK appraise status.
- fixed IMA_XXXX_APPRAISE/APPRAISED flags
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
---
security/integrity/iint.c | 10 ++++-
security/integrity/ima/ima.h | 13 ++++++-
security/integrity/ima/ima_appraise.c | 71 ++++++++++++++++++++++++++++++-----
security/integrity/ima/ima_main.c | 19 ++++++----
security/integrity/ima/ima_policy.c | 22 +++++++++++
security/integrity/integrity.h | 26 +++++++++++--
6 files changed, 136 insertions(+), 25 deletions(-)
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index d82a5a1..74522db 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -72,7 +72,10 @@ static void iint_free(struct integrity_iint_cache *iint)
{
iint->version = 0;
iint->flags = 0UL;
- iint->ima_status = INTEGRITY_UNKNOWN;
+ iint->ima_file_status = INTEGRITY_UNKNOWN;
+ iint->ima_mmap_status = INTEGRITY_UNKNOWN;
+ iint->ima_bprm_status = INTEGRITY_UNKNOWN;
+ iint->ima_module_status = INTEGRITY_UNKNOWN;
iint->evm_status = INTEGRITY_UNKNOWN;
kmem_cache_free(iint_cache, iint);
}
@@ -149,7 +152,10 @@ static void init_once(void *foo)
memset(iint, 0, sizeof *iint);
iint->version = 0;
iint->flags = 0UL;
- iint->ima_status = INTEGRITY_UNKNOWN;
+ iint->ima_file_status = INTEGRITY_UNKNOWN;
+ iint->ima_mmap_status = INTEGRITY_UNKNOWN;
+ iint->ima_bprm_status = INTEGRITY_UNKNOWN;
+ iint->ima_module_status = INTEGRITY_UNKNOWN;
iint->evm_status = INTEGRITY_UNKNOWN;
}
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 991844d..ab68bed 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -142,13 +142,16 @@ void ima_delete_rules(void);
#define IMA_APPRAISE_FIX 0x02
#ifdef CONFIG_IMA_APPRAISE
-int ima_appraise_measurement(struct integrity_iint_cache *iint,
+int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
struct file *file, const unsigned char *filename);
int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func);
void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
+enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
+ int func);
#else
-static inline int ima_appraise_measurement(struct integrity_iint_cache *iint,
+static inline int ima_appraise_measurement(int func,
+ struct integrity_iint_cache *iint,
struct file *file,
const unsigned char *filename)
{
@@ -165,6 +168,12 @@ static inline void ima_update_xattr(struct integrity_iint_cache *iint,
struct file *file)
{
}
+
+static inline enum integrity_status ima_get_cache_status(struct integrity_iint_cache
+ *iint, int func)
+{
+ return INTEGRITY_UNKNOWN;
+}
#endif
/* LSM based policy rules require audit */
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 8004332..2d4beca 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -51,6 +51,62 @@ static int ima_fix_xattr(struct dentry *dentry,
sizeof(iint->ima_xattr), 0);
}
+/* Return specific func appraised cached result */
+enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
+ int func)
+{
+ switch(func) {
+ case MMAP_CHECK:
+ return iint->ima_mmap_status;
+ case BPRM_CHECK:
+ return iint->ima_bprm_status;
+ case MODULE_CHECK:
+ return iint->ima_module_status;
+ case FILE_CHECK:
+ default:
+ return iint->ima_file_status;
+ }
+}
+
+static void ima_set_cache_status(struct integrity_iint_cache *iint,
+ int func, enum integrity_status status)
+{
+ switch(func) {
+ case MMAP_CHECK:
+ iint->ima_mmap_status = status;
+ break;
+ case BPRM_CHECK:
+ iint->ima_bprm_status = status;
+ break;
+ case MODULE_CHECK:
+ iint->ima_module_status = status;
+ break;
+ case FILE_CHECK:
+ default:
+ iint->ima_file_status = status;
+ break;
+ }
+}
+
+static void ima_cache_flags(struct integrity_iint_cache *iint, int func)
+{
+ switch(func) {
+ case MMAP_CHECK:
+ iint->flags |= (IMA_MMAP_APPRAISED | IMA_APPRAISED);
+ break;
+ case BPRM_CHECK:
+ iint->flags |= (IMA_BPRM_APPRAISED | IMA_APPRAISED);
+ break;
+ case MODULE_CHECK:
+ iint->flags |= (IMA_MODULE_APPRAISED | IMA_APPRAISED);
+ break;
+ case FILE_CHECK:
+ default:
+ iint->flags |= (IMA_FILE_APPRAISED | IMA_APPRAISED);
+ break;
+ }
+}
+
/*
* ima_appraise_measurement - appraise file measurement
*
@@ -59,7 +115,7 @@ static int ima_fix_xattr(struct dentry *dentry,
*
* Return 0 on success, error code otherwise
*/
-int ima_appraise_measurement(struct integrity_iint_cache *iint,
+int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
struct file *file, const unsigned char *filename)
{
struct dentry *dentry = file->f_dentry;
@@ -75,9 +131,6 @@ int ima_appraise_measurement(struct integrity_iint_cache *iint,
if (!inode->i_op->getxattr)
return INTEGRITY_UNKNOWN;
- if (iint->flags & IMA_APPRAISED)
- return iint->ima_status;
-
rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)&xattr_value,
0, GFP_NOFS);
if (rc <= 0) {
@@ -99,7 +152,6 @@ int ima_appraise_measurement(struct integrity_iint_cache *iint,
cause = "invalid-HMAC";
goto out;
}
-
switch (xattr_value->type) {
case IMA_XATTR_DIGEST:
if (iint->flags & IMA_DIGSIG_REQUIRED) {
@@ -148,9 +200,9 @@ out:
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
op, cause, rc, 0);
} else {
- iint->flags |= IMA_APPRAISED;
+ ima_cache_flags(iint, func);
}
- iint->ima_status = status;
+ ima_set_cache_status(iint, func, status);
kfree(xattr_value);
return status;
}
@@ -196,10 +248,11 @@ void ima_inode_post_setattr(struct dentry *dentry)
must_appraise = ima_must_appraise(inode, MAY_ACCESS, POST_SETATTR);
iint = integrity_iint_find(inode);
if (iint) {
+ iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
+ IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
+ IMA_ACTION_FLAGS);
if (must_appraise)
iint->flags |= IMA_APPRAISE;
- else
- iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED);
}
if (!must_appraise)
rc = inode->i_op->removexattr(dentry, XATTR_NAME_IMA);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 3cdd787..66b7f40 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -151,8 +151,10 @@ static int process_measurement(struct file *file, const char *filename,
if (!ima_initialized || !S_ISREG(inode->i_mode))
return 0;
- /* Determine if in appraise/audit/measurement policy,
- * returns IMA_MEASURE, IMA_APPRAISE, IMA_AUDIT bitmask. */
+ /* Return an IMA_MEASURE, IMA_APPRAISE, IMA_AUDIT action
+ * bitmask based on the appraise/audit/measurement policy.
+ * Included is the appraise submask.
+ */
action = ima_get_action(inode, mask, function);
if (!action)
return 0;
@@ -166,16 +168,17 @@ static int process_measurement(struct file *file, const char *filename,
goto out;
/* Determine if already appraised/measured based on bitmask
- * (IMA_MEASURE, IMA_MEASURED, IMA_APPRAISE, IMA_APPRAISED,
- * IMA_AUDIT, IMA_AUDITED) */
+ * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
+ * IMA_AUDIT, IMA_AUDITED)
+ */
iint->flags |= action;
action &= IMA_DO_MASK;
action &= ~((iint->flags & IMA_DONE_MASK) >> 1);
/* Nothing to do, just return existing appraised status */
if (!action) {
- if (iint->flags & IMA_APPRAISED)
- rc = iint->ima_status;
+ if (must_appraise)
+ rc = ima_get_cache_status(iint, function);
goto out_digsig;
}
@@ -191,8 +194,8 @@ static int process_measurement(struct file *file, const char *filename,
if (action & IMA_MEASURE)
ima_store_measurement(iint, file, pathname);
- if (action & IMA_APPRAISE)
- rc = ima_appraise_measurement(iint, file, pathname);
+ if (action & IMA_APPRAISE_SUBMASK)
+ rc = ima_appraise_measurement(function, iint, file, pathname);
if (action & IMA_AUDIT)
ima_audit_measurement(iint, pathname);
kfree(pathbuf);
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 1a2543a..4d7c0ae 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -218,6 +218,25 @@ retry:
return true;
}
+/*
+ * In addition to knowing that we need to appraise the file in general,
+ * we need to differentiate between calling hooks.
+ */
+static int get_subaction(int func)
+{
+ switch(func) {
+ case MMAP_CHECK:
+ return IMA_MMAP_APPRAISE;
+ case BPRM_CHECK:
+ return IMA_BPRM_APPRAISE;
+ case MODULE_CHECK:
+ return IMA_MODULE_APPRAISE;
+ case FILE_CHECK:
+ default:
+ return IMA_FILE_APPRAISE;
+ }
+}
+
/**
* ima_match_policy - decision based on LSM and other conditions
* @inode: pointer to an inode for which the policy decision is being made
@@ -248,6 +267,9 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
action |= entry->flags & IMA_ACTION_FLAGS;
action |= entry->action & IMA_DO_MASK;
+ if (entry->action & IMA_APPRAISE)
+ action |= get_subaction(func);
+
if (entry->action & IMA_DO_MASK)
actmask &= ~(entry->action | entry->action << 1);
else
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 329ad26..0ae08fc 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -30,9 +30,24 @@
#define IMA_DIGSIG 0x01000000
#define IMA_DIGSIG_REQUIRED 0x02000000
-#define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT)
-#define IMA_DONE_MASK (IMA_MEASURED | IMA_APPRAISED | IMA_AUDITED \
- | IMA_COLLECTED)
+#define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
+ IMA_APPRAISE_SUBMASK)
+#define IMA_DONE_MASK (IMA_MEASURED | IMA_APPRAISED | IMA_AUDITED | \
+ IMA_COLLECTED | IMA_APPRAISED_SUBMASK)
+
+/* iint subaction appraise cache flags */
+#define IMA_FILE_APPRAISE 0x00000100
+#define IMA_FILE_APPRAISED 0x00000200
+#define IMA_MMAP_APPRAISE 0x00000400
+#define IMA_MMAP_APPRAISED 0x00000800
+#define IMA_BPRM_APPRAISE 0x00001000
+#define IMA_BPRM_APPRAISED 0x00002000
+#define IMA_MODULE_APPRAISE 0x00004000
+#define IMA_MODULE_APPRAISED 0x00008000
+#define IMA_APPRAISE_SUBMASK (IMA_FILE_APPRAISE | IMA_MMAP_APPRAISE | \
+ IMA_BPRM_APPRAISE | IMA_MODULE_APPRAISE)
+#define IMA_APPRAISED_SUBMASK (IMA_FILE_APPRAISED | IMA_MMAP_APPRAISED | \
+ IMA_BPRM_APPRAISED | IMA_MODULE_APPRAISED)
enum evm_ima_xattr_type {
IMA_XATTR_DIGEST = 0x01,
@@ -52,7 +67,10 @@ struct integrity_iint_cache {
u64 version; /* track inode changes */
unsigned long flags;
struct evm_ima_xattr_data ima_xattr;
- enum integrity_status ima_status:4;
+ enum integrity_status ima_file_status:4;
+ enum integrity_status ima_mmap_status:4;
+ enum integrity_status ima_bprm_status:4;
+ enum integrity_status ima_module_status:4;
enum integrity_status evm_status:4;
};
--
1.8.1.rc3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 4/4] ima: differentiate appraise status only for hook specific rules
2013-01-22 22:07 [PATCH v2 0/4] ima: enforcing appraise type Mimi Zohar
` (2 preceding siblings ...)
2013-01-22 22:07 ` [PATCH v2 3/4] ima: per hook cache integrity appraisal status Mimi Zohar
@ 2013-01-22 22:07 ` Mimi Zohar
3 siblings, 0 replies; 10+ messages in thread
From: Mimi Zohar @ 2013-01-22 22:07 UTC (permalink / raw)
To: linux-security-module
Cc: Mimi Zohar, James Morris, linux-kernel, Dmitry Kasatkin
Different hooks can require different methods for appraising a
file's integrity. As a result, an integrity appraisal status is
cached on a per hook basis.
Only a hook specific rule, requires the inode to be re-appraised.
This patch eliminates unnecessary appraisals.
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
---
security/integrity/ima/ima_main.c | 9 ++++++---
security/integrity/ima/ima_policy.c | 9 ++++++---
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 66b7f40..3e751a9 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -146,7 +146,7 @@ static int process_measurement(struct file *file, const char *filename,
struct integrity_iint_cache *iint;
char *pathbuf = NULL;
const char *pathname = NULL;
- int rc = -ENOMEM, action, must_appraise;
+ int rc = -ENOMEM, action, must_appraise, _func;
if (!ima_initialized || !S_ISREG(inode->i_mode))
return 0;
@@ -161,6 +161,9 @@ static int process_measurement(struct file *file, const char *filename,
must_appraise = action & IMA_APPRAISE;
+ /* Is the appraise rule hook specific? */
+ _func = (action & IMA_FILE_APPRAISE) ? FILE_CHECK : function;
+
mutex_lock(&inode->i_mutex);
iint = integrity_inode_get(inode);
@@ -178,7 +181,7 @@ static int process_measurement(struct file *file, const char *filename,
/* Nothing to do, just return existing appraised status */
if (!action) {
if (must_appraise)
- rc = ima_get_cache_status(iint, function);
+ rc = ima_get_cache_status(iint, _func);
goto out_digsig;
}
@@ -195,7 +198,7 @@ static int process_measurement(struct file *file, const char *filename,
if (action & IMA_MEASURE)
ima_store_measurement(iint, file, pathname);
if (action & IMA_APPRAISE_SUBMASK)
- rc = ima_appraise_measurement(function, iint, file, pathname);
+ rc = ima_appraise_measurement(_func, iint, file, pathname);
if (action & IMA_AUDIT)
ima_audit_measurement(iint, pathname);
kfree(pathbuf);
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 4d7c0ae..4adcd0f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -220,10 +220,13 @@ retry:
/*
* In addition to knowing that we need to appraise the file in general,
- * we need to differentiate between calling hooks.
+ * we need to differentiate between calling hooks, for hook specific rules.
*/
-static int get_subaction(int func)
+static int get_subaction(struct ima_rule_entry *rule, int func)
{
+ if (!(rule->flags & IMA_FUNC))
+ return IMA_FILE_APPRAISE;
+
switch(func) {
case MMAP_CHECK:
return IMA_MMAP_APPRAISE;
@@ -268,7 +271,7 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
action |= entry->action & IMA_DO_MASK;
if (entry->action & IMA_APPRAISE)
- action |= get_subaction(func);
+ action |= get_subaction(entry, func);
if (entry->action & IMA_DO_MASK)
actmask &= ~(entry->action | entry->action << 1);
--
1.8.1.rc3
^ permalink raw reply related [flat|nested] 10+ messages in thread