[PATCH v2 0/2] ima: tar issues

[PATCH v2 0/2] ima: tar issues

[Mimi Zohar]

This patch set addresses a couple of problems with writing security.ima
xattrs from tar.  The first patch prevents file signatures stored in the
security.ima xattr from being replaced when the timestamp is updated.
The second patch identifies empty files created using mknodat as new, so
that the file can subsequently be opened in order to write the file
contents.

Mimi

Mimi Zohar (2):
  ima: fix ima_inode_post_setattr
  ima: add support for creating files using the mknodat syscall

 fs/namei.c                            |  2 ++
 include/linux/ima.h                   |  6 ++++++
 security/integrity/ima/ima_appraise.c |  7 ++++++-
 security/integrity/ima/ima_main.c     | 32 +++++++++++++++++++++++++++++++-
 security/integrity/integrity.h        |  1 +
 5 files changed, 46 insertions(+), 2 deletions(-)

-- 
2.1.0

[PATCH v2 1/2] ima: fix ima_inode_post_setattr

[Mimi Zohar]

Changing file metadata (eg. uid, guid) could result in having to
re-appraise a file's integrity, but does not change the "new file"
status nor the security.ima xattr.  The IMA_PERMIT_DIRECTIO and
IMA_DIGSIG_REQUIRED flags are policy rule specific.  This patch
only resets these flags, not the IMA_NEW_FILE or IMA_DIGSIG flags.

With this patch, changing the file timestamp will not remove the
file signature on new files.

Reported-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Tested-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>
---
 security/integrity/ima/ima_appraise.c | 2 +-
 security/integrity/integrity.h        | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 6b4694a..d2f28a0 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -328,7 +328,7 @@ void ima_inode_post_setattr(struct dentry *dentry)
 	if (iint) {
 		iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
 				 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
-				 IMA_ACTION_FLAGS);
+				 IMA_ACTION_RULE_FLAGS);
 		if (must_appraise)
 			iint->flags |= IMA_APPRAISE;
 	}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index e08935c..90bc57d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -28,6 +28,7 @@
 
 /* iint cache flags */
 #define IMA_ACTION_FLAGS	0xff000000
+#define IMA_ACTION_RULE_FLAGS	0x06000000
 #define IMA_DIGSIG		0x01000000
 #define IMA_DIGSIG_REQUIRED	0x02000000
 #define IMA_PERMIT_DIRECTIO	0x04000000
-- 
2.1.0

[PATCH v2 2/2] ima: add support for creating files using the mknodat syscall

[Mimi Zohar]

Commit 3034a14 "ima: pass 'opened' flag to identify newly created files"
stopped identifying empty files as new files.  However new empty files
can be created using the mknodat syscall.  On systems with IMA-appraisal
enabled, these empty files are not labeled with security.ima extended
attributes properly, preventing them from subsequently being opened in
order to write the file data contents.  This patch defines a new hook
named ima_post_path_mknod() to mark these empty files, created using
mknodat, as new in order to allow the file data contents to be written.

In addition, files with security.ima xattrs containing a file signature
are considered "immutable" and can not be modified.  The file contents
need to be written, before signing the file.  This patch relaxes this
requirement for new files, allowing the file signature to be written
before the file contents.

Changelog:
- defer identifying files with signatures stored as security.ima
  (reported by Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>)

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Al Viro <<viro@zeniv.linux.org.uk>
Tested-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>
---
 fs/namei.c                            |  2 ++
 include/linux/ima.h                   |  6 ++++++
 security/integrity/ima/ima_appraise.c |  5 +++++
 security/integrity/ima/ima_main.c     | 32 +++++++++++++++++++++++++++++++-
 4 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index f624d13..30b5254 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3620,6 +3620,8 @@ retry:
 	switch (mode & S_IFMT) {
 		case 0: case S_IFREG:
 			error = vfs_create(path.dentry->d_inode,dentry,mode,true);
+			if (!error)
+				ima_post_path_mknod(dentry);
 			break;
 		case S_IFCHR: case S_IFBLK:
 			error = vfs_mknod(path.dentry->d_inode,dentry,mode,
diff --git a/include/linux/ima.h b/include/linux/ima.h
index e6516cb..0eb7c2e 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -21,6 +21,7 @@ extern int ima_file_mmap(struct file *file, unsigned long prot);
 extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
 extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 			      enum kernel_read_file_id id);
+extern void ima_post_path_mknod(struct dentry *dentry);
 
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -54,6 +55,11 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
 	return 0;
 }
 
+static inline void ima_post_path_mknod(struct dentry *dentry)
+{
+	return;
+}
+
 #endif /* CONFIG_IMA */
 
 #ifdef CONFIG_IMA_APPRAISE
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index d2f28a0..1bcbc12 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -275,6 +275,11 @@ out:
 		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
 			if (!ima_fix_xattr(dentry, iint))
 				status = INTEGRITY_PASS;
+		} else if ((inode->i_size == 0) &&
+			   (iint->flags & IMA_NEW_FILE) &&
+			   (xattr_value &&
+			    xattr_value->type == EVM_IMA_XATTR_DIGSIG)) {
+			status = INTEGRITY_PASS;
 		}
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
 				    op, cause, rc, 0);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 391f417..3297270 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -246,7 +246,8 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 		ima_audit_measurement(iint, pathname);
 
 out_digsig:
-	if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG))
+	if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG) &&
+	     !(iint->flags & IMA_NEW_FILE))
 		rc = -EACCES;
 	kfree(xattr_value);
 out_free:
@@ -316,6 +317,35 @@ int ima_file_check(struct file *file, int mask, int opened)
 EXPORT_SYMBOL_GPL(ima_file_check);
 
 /**
+ * ima_post_path_mknod - mark as a new inode
+ * @dentry: newly created dentry
+ *
+ * Mark files created via the mknodat syscall as new, so that the
+ * file data can be written later.
+ */
+void ima_post_path_mknod(struct dentry *dentry)
+{
+	struct integrity_iint_cache *iint;
+	struct inode *inode;
+	int must_appraise;
+
+	if (!dentry || !dentry->d_inode)
+		return;
+
+	inode = dentry->d_inode;
+	if (inode->i_size != 0)
+		return;
+
+	must_appraise = ima_must_appraise(inode, MAY_ACCESS, FILE_CHECK);
+	if (!must_appraise)
+		return;
+
+	iint = integrity_inode_get(inode);
+	if (iint)
+		iint->flags |= IMA_NEW_FILE;
+}
+
+/**
  * ima_read_file - pre-measure/appraise hook decision based on policy
  * @file: pointer to the file to be measured/appraised/audit
  * @read_id: caller identifier
-- 
2.1.0

Re: [PATCH v2 2/2] ima: add support for creating files using the mknodat syscall

[Al Viro]

On Tue, Mar 15, 2016 at 09:43:11AM -0400, Mimi Zohar wrote:
> +void ima_post_path_mknod(struct dentry *dentry)
> +{
> +	struct integrity_iint_cache *iint;
> +	struct inode *inode;
> +	int must_appraise;
> +
> +	if (!dentry || !dentry->d_inode)

In which situations is that condition supposed to be true?

Re: [PATCH v2 2/2] ima: add support for creating files using the mknodat syscall

[Mimi Zohar]

On Tue, 2016-03-15 at 14:36 +0000, Al Viro wrote:
> On Tue, Mar 15, 2016 at 09:43:11AM -0400, Mimi Zohar wrote:
> > +void ima_post_path_mknod(struct dentry *dentry)
> > +{
> > +	struct integrity_iint_cache *iint;
> > +	struct inode *inode;
> > +	int must_appraise;
> > +
> > +	if (!dentry || !dentry->d_inode)
> 
> In which situations is that condition supposed to be true?

Never, removing tests.

Mimi