All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] ima: enforcing appraise type
@ 2013-01-22 22:07 Mimi Zohar
  2013-01-22 22:07 ` [PATCH v2 1/4] ima: added policy support for 'security.ima' type Mimi Zohar
                   ` (3 more replies)
  0 siblings, 4 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

The IMA-appraisal extension added local integrity validation and
enforcement of the measurement against a "good" value stored as an
extended attribute 'security.ima'.  The initial methods for validating
'security.ima' were hash and digital signature based.

Missing is the ability to specify the validation method required. This
patch set addresses this omission, by defining a new policy option
'appraise_type='.

As a result of this change, appraisal rules can now be written on a
per hook basis, resulting in different appraisal status results.  The
'ima_appraise_tcb' policy appraises all files owned by root.  A new
hook specific rule could specify that all kernel modules, for example,
require a digital signature.

This patchset defines the 'appraise_type' option and caches the
appraisal result on a per hook basis.

Changelog v2:
- Minor cleanup as described in patch descriptions

Mimi

Dmitry Kasatkin (1):
  ima: added policy support for 'security.ima' type

Mimi Zohar (3):
  ima: increase iint flag size
  ima: per hook cache integrity appraisal status
  ima: differentiate appraise status only for hook specific rules

 Documentation/ABI/testing/ima_policy  |  4 +-
 security/integrity/iint.c             | 10 ++++-
 security/integrity/ima/ima.h          | 13 +++++-
 security/integrity/ima/ima_appraise.c | 76 ++++++++++++++++++++++++++++++-----
 security/integrity/ima/ima_main.c     | 25 +++++++-----
 security/integrity/ima/ima_policy.c   | 43 +++++++++++++++++++-
 security/integrity/integrity.h        | 48 +++++++++++++++-------
 7 files changed, 181 insertions(+), 38 deletions(-)

-- 
1.8.1.rc3


^ permalink raw reply	[flat|nested] 10+ messages in thread
* [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
* [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
* 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
end of thread, other threads:[~2013-01-31 19:37 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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-30 21:53   ` Vivek Goyal
2013-01-30 22:42     ` Mimi Zohar
2013-01-31 18:41       ` Vivek Goyal
2013-01-31 19:25         ` Mimi Zohar
2013-01-31 19:37           ` Vivek Goyal
2013-01-22 22:07 ` [PATCH v2 2/4] ima: increase iint flag size 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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.