All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V3 1/2] IMA: Add support for IMA validation after credentials are committed
@ 2017-10-11 19:12 Matthew Garrett
  2017-10-11 19:12 ` [PATCH V3 2/2] Add a bprm_interpreted security hook and call into IMA from it Matthew Garrett
  0 siblings, 1 reply; 4+ messages in thread
From: Matthew Garrett @ 2017-10-11 19:12 UTC (permalink / raw)
  To: linux-integrity; +Cc: zohar, james.l.morris, keescook, oleg, Matthew Garrett

It may be desirable to perform appraisal after credentials are
committed, for instance in the case where validation is only required if
the binary has transitioned into a privileged security context. Add an
additional call into IMA in the committed_credentials security hook and
abort execution if it fails. This will only be called at
install_exec_creds() time, which means it will only be run on binary
interpreters rather than on the scripts or files handled by binfmt_misc.
The following patch will rectify this.

Signed-off-by: Matthew Garrett <mjg59@google.com>
---
 Documentation/ABI/testing/ima_policy  |  2 +-
 arch/x86/ia32/ia32_aout.c             |  4 +++-
 fs/binfmt_aout.c                      |  4 +++-
 fs/binfmt_elf.c                       |  5 ++++-
 fs/binfmt_elf_fdpic.c                 |  5 ++++-
 fs/binfmt_flat.c                      |  4 +++-
 fs/exec.c                             |  8 ++++++--
 include/linux/binfmts.h               |  2 +-
 include/linux/ima.h                   |  6 ++++++
 include/linux/security.h              |  5 +++--
 security/integrity/iint.c             |  1 +
 security/integrity/ima/ima.h          |  1 +
 security/integrity/ima/ima_api.c      |  2 +-
 security/integrity/ima/ima_appraise.c |  8 ++++++++
 security/integrity/ima/ima_main.c     | 24 +++++++++++++++++++++++-
 security/integrity/ima/ima_policy.c   |  4 ++++
 security/integrity/integrity.h        |  9 +++++++--
 security/security.c                   |  3 ++-
 18 files changed, 81 insertions(+), 16 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index e76432b9954d..5dc9eed035fb 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -25,7 +25,7 @@ Description:
 				 [obj_user=] [obj_role=] [obj_type=]]
 			option:	[[appraise_type=]] [permit_directio]
 
-		base: 	func:= [BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK]
+		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
index 8e02b30cf08e..0dba3ed8459f 100644
--- a/arch/x86/ia32/ia32_aout.c
+++ b/arch/x86/ia32/ia32_aout.c
@@ -312,7 +312,9 @@ static int load_aout_binary(struct linux_binprm *bprm)
 	if (retval < 0)
 		return retval;
 
-	install_exec_creds(bprm);
+	retval = install_exec_creds(bprm);
+	if (retval)
+		return retval;
 
 	if (N_MAGIC(ex) == OMAGIC) {
 		unsigned long text_addr, map_size;
diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index ce1824f47ba6..02c7c4320fc8 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -256,7 +256,9 @@ static int load_aout_binary(struct linux_binprm * bprm)
 	if (retval < 0)
 		return retval;
 
-	install_exec_creds(bprm);
+	retval = install_exec_creds(bprm);
+	if (retval)
+		return retval;
 
 	if (N_MAGIC(ex) == OMAGIC) {
 		unsigned long text_addr, map_size;
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 73b01e474fdc..a6883a855a14 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -866,7 +866,10 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		current->flags |= PF_RANDOMIZE;
 
 	setup_new_exec(bprm);
-	install_exec_creds(bprm);
+
+	retval = install_exec_creds(bprm);
+	if (retval)
+		goto out_free_dentry;
 
 	/* Do this so that we can load the interpreter, if need be.  We will
 	   change some of these later */
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index e70c039ac190..d89830095e19 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -433,7 +433,10 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
 	current->mm->start_stack = current->mm->start_brk + stack_size;
 #endif
 
-	install_exec_creds(bprm);
+	retval = install_exec_creds(bprm);
+	if (retval)
+		goto error;
+
 	if (create_elf_fdpic_tables(bprm, current->mm,
 				    &exec_params, &interp_params) < 0)
 		goto error;
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index 475d083f8088..5f59c53ba97f 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -948,7 +948,9 @@ static int load_flat_binary(struct linux_binprm *bprm)
 		}
 	}
 
-	install_exec_creds(bprm);
+	retval = install_exec_creds(bprm);
+	if (retval)
+		return retval;
 
 	set_binfmt(&flat_format);
 
diff --git a/fs/exec.c b/fs/exec.c
index 5470d3c1892a..c7e9f84abd36 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1425,8 +1425,10 @@ EXPORT_SYMBOL(bprm_change_interp);
 /*
  * install the new credentials for this executable
  */
-void install_exec_creds(struct linux_binprm *bprm)
+int install_exec_creds(struct linux_binprm *bprm)
 {
+	int ret = 0;
+
 	security_bprm_committing_creds(bprm);
 
 	commit_creds(bprm->cred);
@@ -1445,8 +1447,10 @@ void install_exec_creds(struct linux_binprm *bprm)
 	 * ptrace_attach() from altering our determination of the task's
 	 * credentials; any time after this it may be unlocked.
 	 */
-	security_bprm_committed_creds(bprm);
+	ret = security_bprm_committed_creds(bprm);
 	mutex_unlock(&current->signal->cred_guard_mutex);
+
+	return ret;
 }
 EXPORT_SYMBOL(install_exec_creds);
 
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 18d05b5491f3..725d5582589f 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -135,7 +135,7 @@ extern int bprm_change_interp(const char *interp, struct linux_binprm *bprm);
 extern int copy_strings_kernel(int argc, const char *const *argv,
 			       struct linux_binprm *bprm);
 extern int prepare_bprm_creds(struct linux_binprm *bprm);
-extern void install_exec_creds(struct linux_binprm *bprm);
+extern int install_exec_creds(struct linux_binprm *bprm);
 extern void set_binfmt(struct linux_binfmt *new);
 extern ssize_t read_code(struct file *, unsigned long, loff_t, size_t);
 
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 0e4647e0eb60..f9a64f94b0d3 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -16,6 +16,7 @@ struct linux_binprm;
 
 #ifdef CONFIG_IMA
 extern int ima_bprm_check(struct linux_binprm *bprm);
+extern int ima_creds_check(struct linux_binprm *bprm);
 extern int ima_file_check(struct file *file, int mask, int opened);
 extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
@@ -34,6 +35,11 @@ static inline int ima_bprm_check(struct linux_binprm *bprm)
 	return 0;
 }
 
+static inline int ima_creds_check(struct linux_binprm *bprm)
+{
+	return 0;
+}
+
 static inline int ima_file_check(struct file *file, int mask, int opened)
 {
 	return 0;
diff --git a/include/linux/security.h b/include/linux/security.h
index ce6265960d6c..ad970a4f19f6 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -232,7 +232,7 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages);
 int security_bprm_set_creds(struct linux_binprm *bprm);
 int security_bprm_check(struct linux_binprm *bprm);
 void security_bprm_committing_creds(struct linux_binprm *bprm);
-void security_bprm_committed_creds(struct linux_binprm *bprm);
+int security_bprm_committed_creds(struct linux_binprm *bprm);
 int security_sb_alloc(struct super_block *sb);
 void security_sb_free(struct super_block *sb);
 int security_sb_copy_data(char *orig, char *copy);
@@ -536,8 +536,9 @@ static inline void security_bprm_committing_creds(struct linux_binprm *bprm)
 {
 }
 
-static inline void security_bprm_committed_creds(struct linux_binprm *bprm)
+static inline int security_bprm_committed_creds(struct linux_binprm *bprm)
 {
+	return 0;
 }
 
 static inline int security_sb_alloc(struct super_block *sb)
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 6fc888ca468e..ad30094a58b4 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -78,6 +78,7 @@ static void iint_free(struct integrity_iint_cache *iint)
 	iint->ima_mmap_status = INTEGRITY_UNKNOWN;
 	iint->ima_bprm_status = INTEGRITY_UNKNOWN;
 	iint->ima_read_status = INTEGRITY_UNKNOWN;
+	iint->ima_creds_status = INTEGRITY_UNKNOWN;
 	iint->evm_status = INTEGRITY_UNKNOWN;
 	iint->measured_pcrs = 0;
 	kmem_cache_free(iint_cache, iint);
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d52b487ad259..547ea832bb1b 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -177,6 +177,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 	hook(FILE_CHECK)		\
 	hook(MMAP_CHECK)		\
 	hook(BPRM_CHECK)		\
+	hook(CREDS_CHECK)		\
 	hook(POST_SETATTR)		\
 	hook(MODULE_CHECK)		\
 	hook(FIRMWARE_CHECK)		\
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c2edba8de35e..0c19bb423570 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -165,7 +165,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  * The policy is defined in terms of keypairs:
  *		subj=, obj=, type=, func=, mask=, fsmagic=
  *	subj,obj, and type: are LSM specific.
- *	func: FILE_CHECK | BPRM_CHECK | MMAP_CHECK | MODULE_CHECK
+ *	func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 809ba70fbbbf..edb82e722a0d 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -86,6 +86,8 @@ enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
 		return iint->ima_mmap_status;
 	case BPRM_CHECK:
 		return iint->ima_bprm_status;
+	case CREDS_CHECK:
+		return iint->ima_creds_status;
 	case FILE_CHECK:
 	case POST_SETATTR:
 		return iint->ima_file_status;
@@ -106,6 +108,9 @@ static void ima_set_cache_status(struct integrity_iint_cache *iint,
 	case BPRM_CHECK:
 		iint->ima_bprm_status = status;
 		break;
+	case CREDS_CHECK:
+		iint->ima_creds_status = status;
+		break;
 	case FILE_CHECK:
 	case POST_SETATTR:
 		iint->ima_file_status = status;
@@ -127,6 +132,9 @@ static void ima_cache_flags(struct integrity_iint_cache *iint,
 	case BPRM_CHECK:
 		iint->flags |= (IMA_BPRM_APPRAISED | IMA_APPRAISED);
 		break;
+	case CREDS_CHECK:
+		iint->flags |= (IMA_CREDS_APPRAISED | IMA_APPRAISED);
+		break;
 	case FILE_CHECK:
 	case POST_SETATTR:
 		iint->flags |= (IMA_FILE_APPRAISED | IMA_APPRAISED);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2aebb7984437..5be8307a1dd1 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -14,7 +14,7 @@
  *
  * File: ima_main.c
  *	implements the IMA hooks: ima_bprm_check, ima_file_mmap,
- *	and ima_file_check.
+ *	ima_creds_check and ima_file_check.
  */
 #include <linux/module.h>
 #include <linux/file.h>
@@ -306,6 +306,28 @@ int ima_bprm_check(struct linux_binprm *bprm)
 				   BPRM_CHECK, 0);
 }
 
+/**
+ * ima_creds_check - based on policy, collect/store measurement.
+ * @bprm: contains the linux_binprm structure
+ *
+ * The OS protects against an executable file, already open for write,
+ * from being executed in deny_write_access() and an executable file,
+ * already open for execute, from being modified in get_write_access().
+ * So we can be certain that what we verify and measure here is actually
+ * what is being executed.
+ *
+ * This is identical to ima_bprm_check, except called after child credentials
+ * have been committed.
+ *
+ * On success return 0.  On integrity appraisal error, assuming the file
+ * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
+ */
+int ima_creds_check(struct linux_binprm *bprm)
+{
+	return process_measurement(bprm->file, NULL, 0, MAY_EXEC,
+				   CREDS_CHECK, 0);
+}
+
 /**
  * ima_path_check - based on policy, collect/store measurement.
  * @file: pointer to the file to be measured
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 95209a5f8595..a6e14c532627 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -339,6 +339,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
 		return IMA_MMAP_APPRAISE;
 	case BPRM_CHECK:
 		return IMA_BPRM_APPRAISE;
+	case CREDS_CHECK:
+		return IMA_CREDS_APPRAISE;
 	case FILE_CHECK:
 	case POST_SETATTR:
 		return IMA_FILE_APPRAISE;
@@ -691,6 +693,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->func = MMAP_CHECK;
 			else if (strcmp(args[0].from, "BPRM_CHECK") == 0)
 				entry->func = BPRM_CHECK;
+			else if (strcmp(args[0].from, "CREDS_CHECK") == 0)
+				entry->func = CREDS_CHECK;
 			else if (strcmp(args[0].from, "KEXEC_KERNEL_CHECK") ==
 				 0)
 				entry->func = KEXEC_KERNEL_CHECK;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 0a721c110e92..5413eb25b440 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -48,10 +48,14 @@
 #define IMA_BPRM_APPRAISED	0x00002000
 #define IMA_READ_APPRAISE	0x00004000
 #define IMA_READ_APPRAISED	0x00008000
+#define IMA_CREDS_APPRAISE	0x00010000
+#define IMA_CREDS_APPRAISED	0x00020000
 #define IMA_APPRAISE_SUBMASK	(IMA_FILE_APPRAISE | IMA_MMAP_APPRAISE | \
-				 IMA_BPRM_APPRAISE | IMA_READ_APPRAISE)
+				 IMA_BPRM_APPRAISE | IMA_READ_APPRAISE | \
+				 IMA_CREDS_APPRAISE)
 #define IMA_APPRAISED_SUBMASK	(IMA_FILE_APPRAISED | IMA_MMAP_APPRAISED | \
-				 IMA_BPRM_APPRAISED | IMA_READ_APPRAISED)
+				 IMA_BPRM_APPRAISED | IMA_READ_APPRAISED | \
+				 IMA_CREDS_APPRAISED)
 
 enum evm_ima_xattr_type {
 	IMA_XATTR_DIGEST = 0x01,
@@ -109,6 +113,7 @@ struct integrity_iint_cache {
 	enum integrity_status ima_mmap_status:4;
 	enum integrity_status ima_bprm_status:4;
 	enum integrity_status ima_read_status:4;
+	enum integrity_status ima_creds_status:4;
 	enum integrity_status evm_status:4;
 	struct ima_digest_data *ima_hash;
 };
diff --git a/security/security.c b/security/security.c
index 4bf0f571b4ef..c2c5017e3477 100644
--- a/security/security.c
+++ b/security/security.c
@@ -346,9 +346,10 @@ void security_bprm_committing_creds(struct linux_binprm *bprm)
 	call_void_hook(bprm_committing_creds, bprm);
 }
 
-void security_bprm_committed_creds(struct linux_binprm *bprm)
+int security_bprm_committed_creds(struct linux_binprm *bprm)
 {
 	call_void_hook(bprm_committed_creds, bprm);
+	return ima_creds_check(bprm);
 }
 
 int security_sb_alloc(struct super_block *sb)
-- 
2.15.0.rc0.271.g36b669edcc-goog

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH V3 2/2] Add a bprm_interpreted security hook and call into IMA from it
  2017-10-11 19:12 [PATCH V3 1/2] IMA: Add support for IMA validation after credentials are committed Matthew Garrett
@ 2017-10-11 19:12 ` Matthew Garrett
  2017-10-15 14:08   ` Mimi Zohar
  0 siblings, 1 reply; 4+ messages in thread
From: Matthew Garrett @ 2017-10-11 19:12 UTC (permalink / raw)
  To: linux-integrity; +Cc: zohar, james.l.morris, keescook, oleg, Matthew Garrett

IMA has support for validating files during execution using the
bprm_check functionality. However, this is called before new credentials
have been applied to the task. The previous patch in this series added
an additional IMA check in the bprm_committed_creds hook - however, if a
file is handled by binfmt_misc or binfmt_script (or, in an extremely
unlikely corner case, binfmt_em86), only the interpreter will be
appraised since bprm_committed_creds is never called for the initial
executable.

This patch adds an additional bprm_interpreted hook and calls it from
the end of the relevant binfmt implementations. It is effectively
identical to the bprm_committed_creds hook, except that bprm->file
points to the initial file rather than to the interpreter.

Signed-off-by: Matthew Garrett <mjg59@google.com>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Oleg Nesterov <oleg@redhat.com>
---
 fs/binfmt_em86.c          | 31 ++++++++++++++++++++++---------
 fs/binfmt_misc.c          | 11 +++++++----
 fs/binfmt_script.c        | 45 +++++++++++++++++++++++++++++++--------------
 include/linux/lsm_hooks.h |  7 +++++++
 include/linux/security.h  |  1 +
 security/security.c       | 11 +++++++++++
 6 files changed, 79 insertions(+), 27 deletions(-)

diff --git a/fs/binfmt_em86.c b/fs/binfmt_em86.c
index dd2d3f0cd55d..e954ec123d27 100644
--- a/fs/binfmt_em86.c
+++ b/fs/binfmt_em86.c
@@ -26,7 +26,7 @@ static int load_em86(struct linux_binprm *bprm)
 {
 	const char *i_name, *i_arg;
 	char *interp;
-	struct file * file;
+	struct file *file, *old;
 	int retval;
 	struct elfhdr	elf_ex;
 
@@ -47,8 +47,7 @@ static int load_em86(struct linux_binprm *bprm)
 	if (bprm->interp_flags & BINPRM_FLAGS_PATH_INACCESSIBLE)
 		return -ENOENT;
 
-	allow_write_access(bprm->file);
-	fput(bprm->file);
+	old = bprm->file;
 	bprm->file = NULL;
 
 	/* Unlike in the script case, we don't have to do any hairy
@@ -72,11 +71,13 @@ static int load_em86(struct linux_binprm *bprm)
 	bprm->argc++;
 	if (i_arg) {
 		retval = copy_strings_kernel(1, &i_arg, bprm);
-		if (retval < 0) return retval; 
+		if (retval < 0)
+			goto ret;
 		bprm->argc++;
 	}
 	retval = copy_strings_kernel(1, &i_name, bprm);
-	if (retval < 0)	return retval;
+	if (retval < 0)
+		goto ret;
 	bprm->argc++;
 
 	/*
@@ -85,16 +86,28 @@ static int load_em86(struct linux_binprm *bprm)
 	 * space, and we don't need to copy it.
 	 */
 	file = open_exec(interp);
-	if (IS_ERR(file))
-		return PTR_ERR(file);
+	if (IS_ERR(file)) {
+		retval = PTR_ERR(file);
+		goto ret;
+	}
 
 	bprm->file = file;
 
 	retval = prepare_binprm(bprm);
 	if (retval < 0)
-		return retval;
+		goto ret;
 
-	return search_binary_handler(bprm);
+	retval = search_binary_handler(bprm);
+	if (retval < 0)
+		goto ret;
+
+	bprm->file = old;
+	retval = security_bprm_interpreted(bprm);
+	bprm->file = file;
+ret:
+	allow_write_access(old);
+	fput(old);
+	return retval;
 }
 
 static struct linux_binfmt em86_format = {
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 2a46762def31..81e6e02348f9 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -130,7 +130,7 @@ static Node *check_file(struct linux_binprm *bprm)
 static int load_misc_binary(struct linux_binprm *bprm)
 {
 	Node *fmt;
-	struct file *interp_file = NULL;
+	struct file *interp_file = NULL, *old = bprm->file;
 	int retval;
 	int fd_binary = -1;
 
@@ -175,7 +175,6 @@ static int load_misc_binary(struct linux_binprm *bprm)
 		   regardless of the interpreter's permissions */
 		would_dump(bprm, bprm->file);
 
-		allow_write_access(bprm->file);
 		bprm->file = NULL;
 
 		/* mark the bprm that fd should be passed to interp */
@@ -183,8 +182,6 @@ static int load_misc_binary(struct linux_binprm *bprm)
 		bprm->interp_data = fd_binary;
 
 	} else {
-		allow_write_access(bprm->file);
-		fput(bprm->file);
 		bprm->file = NULL;
 	}
 	/* make argv[1] be the path to the binary */
@@ -236,7 +233,13 @@ static int load_misc_binary(struct linux_binprm *bprm)
 	if (retval < 0)
 		goto error;
 
+	bprm->file = old;
+	retval = security_bprm_interpreted(bprm);
+	bprm->file = interp_file;
 ret:
+	allow_write_access(old);
+	if (fd_binary < 0)
+		fput(old);
 	dput(fmt->dentry);
 	return retval;
 error:
diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index 7cde3f46ad26..f2bd2aa15702 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -13,12 +13,13 @@
 #include <linux/file.h>
 #include <linux/err.h>
 #include <linux/fs.h>
+#include <linux/security.h>
 
 static int load_script(struct linux_binprm *bprm)
 {
 	const char *i_arg, *i_name;
 	char *cp;
-	struct file *file;
+	struct file *file, *old;
 	int retval;
 
 	if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!'))
@@ -38,8 +39,7 @@ static int load_script(struct linux_binprm *bprm)
 	 * Sorta complicated, but hopefully it will work.  -TYT
 	 */
 
-	allow_write_access(bprm->file);
-	fput(bprm->file);
+	old = bprm->file;
 	bprm->file = NULL;
 
 	bprm->buf[BINPRM_BUF_SIZE - 1] = '\0';
@@ -54,8 +54,11 @@ static int load_script(struct linux_binprm *bprm)
 			break;
 	}
 	for (cp = bprm->buf+2; (*cp == ' ') || (*cp == '\t'); cp++);
-	if (*cp == '\0')
-		return -ENOEXEC; /* No interpreter name found */
+	if (*cp == '\0') {
+		retval = -ENOEXEC; /* No interpreter name found */
+		goto ret;
+	}
+
 	i_name = cp;
 	i_arg = NULL;
 	for ( ; *cp && (*cp != ' ') && (*cp != '\t'); cp++)
@@ -76,37 +79,51 @@ static int load_script(struct linux_binprm *bprm)
 	 */
 	retval = remove_arg_zero(bprm);
 	if (retval)
-		return retval;
+		goto ret;
 	retval = copy_strings_kernel(1, &bprm->interp, bprm);
 	if (retval < 0)
-		return retval;
+		goto ret;
 	bprm->argc++;
 	if (i_arg) {
 		retval = copy_strings_kernel(1, &i_arg, bprm);
 		if (retval < 0)
-			return retval;
+			goto ret;
 		bprm->argc++;
 	}
 	retval = copy_strings_kernel(1, &i_name, bprm);
 	if (retval)
-		return retval;
+		goto ret;
 	bprm->argc++;
 	retval = bprm_change_interp(i_name, bprm);
 	if (retval < 0)
-		return retval;
+		goto ret;
 
 	/*
 	 * OK, now restart the process with the interpreter's dentry.
 	 */
 	file = open_exec(i_name);
-	if (IS_ERR(file))
-		return PTR_ERR(file);
+	if (IS_ERR(file)) {
+		retval = PTR_ERR(file);
+		goto ret;
+	}
 
 	bprm->file = file;
 	retval = prepare_binprm(bprm);
 	if (retval < 0)
-		return retval;
-	return search_binary_handler(bprm);
+		goto ret;
+	retval = search_binary_handler(bprm);
+	if (retval)
+		goto ret;
+	/*
+	 * Allow for validation of the script as well as the interpreter
+	 */
+	bprm->file = old;
+	retval = security_bprm_interpreted(bprm);
+	bprm->file = file;
+ret:
+	allow_write_access(old);
+	fput(old);
+	return retval;
 }
 
 static struct linux_binfmt script_format = {
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c9258124e417..fd23b4098098 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -75,6 +75,11 @@
  *	linux_binprm structure.  This hook is a good place to perform state
  *	changes on the process such as clearing out non-inheritable signal
  *	state.  This is called immediately after commit_creds().
+ * @bprm_interpreted:
+ *	Validate the credentials of an executable that was interpreted via
+ *	binfmt_misc or binfmt_script. This occurs after the new credentials
+ *	have been committed to @current->cred, but @bprm->file points to the
+ *	original file rather than the interpreter that was used to execute it.
  *
  * Security hooks for filesystem operations.
  *
@@ -1383,6 +1388,7 @@ union security_list_options {
 	int (*bprm_check_security)(struct linux_binprm *bprm);
 	void (*bprm_committing_creds)(struct linux_binprm *bprm);
 	void (*bprm_committed_creds)(struct linux_binprm *bprm);
+	int (*bprm_interpreted)(struct linux_binprm *bprm);
 
 	int (*sb_alloc_security)(struct super_block *sb);
 	void (*sb_free_security)(struct super_block *sb);
@@ -1703,6 +1709,7 @@ struct security_hook_heads {
 	struct list_head bprm_check_security;
 	struct list_head bprm_committing_creds;
 	struct list_head bprm_committed_creds;
+	struct list_head bprm_interpreted;
 	struct list_head sb_alloc_security;
 	struct list_head sb_free_security;
 	struct list_head sb_copy_data;
diff --git a/include/linux/security.h b/include/linux/security.h
index ad970a4f19f6..e48c38379c64 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -233,6 +233,7 @@ int security_bprm_set_creds(struct linux_binprm *bprm);
 int security_bprm_check(struct linux_binprm *bprm);
 void security_bprm_committing_creds(struct linux_binprm *bprm);
 int security_bprm_committed_creds(struct linux_binprm *bprm);
+int security_bprm_interpreted(struct linux_binprm *bprm);
 int security_sb_alloc(struct super_block *sb);
 void security_sb_free(struct super_block *sb);
 int security_sb_copy_data(char *orig, char *copy);
diff --git a/security/security.c b/security/security.c
index c2c5017e3477..6b9cb108ff61 100644
--- a/security/security.c
+++ b/security/security.c
@@ -352,6 +352,17 @@ int security_bprm_committed_creds(struct linux_binprm *bprm)
 	return ima_creds_check(bprm);
 }
 
+int security_bprm_interpreted(struct linux_binprm *bprm)
+{
+	int ret;
+
+	ret = call_int_hook(bprm_interpreted, 0, bprm);
+	if (ret)
+		return ret;
+
+	return ima_creds_check(bprm);
+}
+
 int security_sb_alloc(struct super_block *sb)
 {
 	return call_int_hook(sb_alloc_security, 0, sb);
-- 
2.15.0.rc0.271.g36b669edcc-goog

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH V3 2/2] Add a bprm_interpreted security hook and call into IMA from it
  2017-10-11 19:12 ` [PATCH V3 2/2] Add a bprm_interpreted security hook and call into IMA from it Matthew Garrett
@ 2017-10-15 14:08   ` Mimi Zohar
  2017-10-16 18:13     ` Matthew Garrett
  0 siblings, 1 reply; 4+ messages in thread
From: Mimi Zohar @ 2017-10-15 14:08 UTC (permalink / raw)
  To: Matthew Garrett, linux-integrity; +Cc: james.l.morris, keescook, oleg

On Wed, 2017-10-11 at 12:12 -0700, Matthew Garrett wrote:
> IMA has support for validating files during execution using the
> bprm_check functionality. However, this is called before new credentials
> have been applied to the task. The previous patch in this series added
> an additional IMA check in the bprm_committed_creds hook - however, if a
> file is handled by binfmt_misc or binfmt_script (or, in an extremely
> unlikely corner case, binfmt_em86), only the interpreter will be
> appraised since bprm_committed_creds is never called for the initial
> executable.

Even with this additional patch, there are still potentially missing measurements/appraisals as search_binary_handler is exported.  The original search_binary_handler is called twice, once for the original file and again for the interpreter.  With these patches, the security hooks are deferred, requiring calls in the specific binary handler.

For your usecase scenario this might be enough, but for the general case the security_bprm_check hooks would still be needed.

> This patch adds an additional bprm_interpreted hook and calls it from
> the end of the relevant binfmt implementations. It is effectively
> identical to the bprm_committed_creds hook, except that bprm->file
> points to the initial file rather than to the interpreter.

->load_binary() seems to do a lot more than just load the binary
handler.  There's a comment in load_elf_binary(), before the call to
arch_check_elf(), indicating this is the last opportunity to reject
the ELF.  Are you sure that deferring verifying the initial file like
this isn't too late?

Mimi

> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> ---
>  fs/binfmt_em86.c          | 31 ++++++++++++++++++++++---------
>  fs/binfmt_misc.c          | 11 +++++++----
>  fs/binfmt_script.c        | 45 +++++++++++++++++++++++++++++++--------------
>  include/linux/lsm_hooks.h |  7 +++++++
>  include/linux/security.h  |  1 +
>  security/security.c       | 11 +++++++++++
>  6 files changed, 79 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/binfmt_em86.c b/fs/binfmt_em86.c
> index dd2d3f0cd55d..e954ec123d27 100644
> --- a/fs/binfmt_em86.c
> +++ b/fs/binfmt_em86.c
> @@ -26,7 +26,7 @@ static int load_em86(struct linux_binprm *bprm)
>  {
>  	const char *i_name, *i_arg;
>  	char *interp;
> -	struct file * file;
> +	struct file *file, *old;
>  	int retval;
>  	struct elfhdr	elf_ex;
> 
> @@ -47,8 +47,7 @@ static int load_em86(struct linux_binprm *bprm)
>  	if (bprm->interp_flags & BINPRM_FLAGS_PATH_INACCESSIBLE)
>  		return -ENOENT;
> 
> -	allow_write_access(bprm->file);
> -	fput(bprm->file);
> +	old = bprm->file;
>  	bprm->file = NULL;
> 
>  	/* Unlike in the script case, we don't have to do any hairy
> @@ -72,11 +71,13 @@ static int load_em86(struct linux_binprm *bprm)
>  	bprm->argc++;
>  	if (i_arg) {
>  		retval = copy_strings_kernel(1, &i_arg, bprm);
> -		if (retval < 0) return retval; 
> +		if (retval < 0)
> +			goto ret;
>  		bprm->argc++;
>  	}
>  	retval = copy_strings_kernel(1, &i_name, bprm);
> -	if (retval < 0)	return retval;
> +	if (retval < 0)
> +		goto ret;
>  	bprm->argc++;
> 
>  	/*
> @@ -85,16 +86,28 @@ static int load_em86(struct linux_binprm *bprm)
>  	 * space, and we don't need to copy it.
>  	 */
>  	file = open_exec(interp);
> -	if (IS_ERR(file))
> -		return PTR_ERR(file);
> +	if (IS_ERR(file)) {
> +		retval = PTR_ERR(file);
> +		goto ret;
> +	}
> 
>  	bprm->file = file;
> 
>  	retval = prepare_binprm(bprm);
>  	if (retval < 0)
> -		return retval;
> +		goto ret;
> 
> -	return search_binary_handler(bprm);
> +	retval = search_binary_handler(bprm);
> +	if (retval < 0)
> +		goto ret;
> +
> +	bprm->file = old;
> +	retval = security_bprm_interpreted(bprm);
> +	bprm->file = file;
> +ret:
> +	allow_write_access(old);
> +	fput(old);
> +	return retval;
>  }
> 
>  static struct linux_binfmt em86_format = {
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index 2a46762def31..81e6e02348f9 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -130,7 +130,7 @@ static Node *check_file(struct linux_binprm *bprm)
>  static int load_misc_binary(struct linux_binprm *bprm)
>  {
>  	Node *fmt;
> -	struct file *interp_file = NULL;
> +	struct file *interp_file = NULL, *old = bprm->file;
>  	int retval;
>  	int fd_binary = -1;
> 
> @@ -175,7 +175,6 @@ static int load_misc_binary(struct linux_binprm *bprm)
>  		   regardless of the interpreter's permissions */
>  		would_dump(bprm, bprm->file);
> 
> -		allow_write_access(bprm->file);
>  		bprm->file = NULL;
> 
>  		/* mark the bprm that fd should be passed to interp */
> @@ -183,8 +182,6 @@ static int load_misc_binary(struct linux_binprm *bprm)
>  		bprm->interp_data = fd_binary;
> 
>  	} else {
> -		allow_write_access(bprm->file);
> -		fput(bprm->file);
>  		bprm->file = NULL;
>  	}
>  	/* make argv[1] be the path to the binary */
> @@ -236,7 +233,13 @@ static int load_misc_binary(struct linux_binprm *bprm)
>  	if (retval < 0)
>  		goto error;
> 
> +	bprm->file = old;
> +	retval = security_bprm_interpreted(bprm);
> +	bprm->file = interp_file;
>  ret:
> +	allow_write_access(old);
> +	if (fd_binary < 0)
> +		fput(old);
>  	dput(fmt->dentry);
>  	return retval;
>  error:
> diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
> index 7cde3f46ad26..f2bd2aa15702 100644
> --- a/fs/binfmt_script.c
> +++ b/fs/binfmt_script.c
> @@ -13,12 +13,13 @@
>  #include <linux/file.h>
>  #include <linux/err.h>
>  #include <linux/fs.h>
> +#include <linux/security.h>
> 
>  static int load_script(struct linux_binprm *bprm)
>  {
>  	const char *i_arg, *i_name;
>  	char *cp;
> -	struct file *file;
> +	struct file *file, *old;
>  	int retval;
> 
>  	if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!'))
> @@ -38,8 +39,7 @@ static int load_script(struct linux_binprm *bprm)
>  	 * Sorta complicated, but hopefully it will work.  -TYT
>  	 */
> 
> -	allow_write_access(bprm->file);
> -	fput(bprm->file);
> +	old = bprm->file;
>  	bprm->file = NULL;
> 
>  	bprm->buf[BINPRM_BUF_SIZE - 1] = '\0';
> @@ -54,8 +54,11 @@ static int load_script(struct linux_binprm *bprm)
>  			break;
>  	}
>  	for (cp = bprm->buf+2; (*cp == ' ') || (*cp == '\t'); cp++);
> -	if (*cp == '\0')
> -		return -ENOEXEC; /* No interpreter name found */
> +	if (*cp == '\0') {
> +		retval = -ENOEXEC; /* No interpreter name found */
> +		goto ret;
> +	}
> +
>  	i_name = cp;
>  	i_arg = NULL;
>  	for ( ; *cp && (*cp != ' ') && (*cp != '\t'); cp++)
> @@ -76,37 +79,51 @@ static int load_script(struct linux_binprm *bprm)
>  	 */
>  	retval = remove_arg_zero(bprm);
>  	if (retval)
> -		return retval;
> +		goto ret;
>  	retval = copy_strings_kernel(1, &bprm->interp, bprm);
>  	if (retval < 0)
> -		return retval;
> +		goto ret;
>  	bprm->argc++;
>  	if (i_arg) {
>  		retval = copy_strings_kernel(1, &i_arg, bprm);
>  		if (retval < 0)
> -			return retval;
> +			goto ret;
>  		bprm->argc++;
>  	}
>  	retval = copy_strings_kernel(1, &i_name, bprm);
>  	if (retval)
> -		return retval;
> +		goto ret;
>  	bprm->argc++;
>  	retval = bprm_change_interp(i_name, bprm);
>  	if (retval < 0)
> -		return retval;
> +		goto ret;
> 
>  	/*
>  	 * OK, now restart the process with the interpreter's dentry.
>  	 */
>  	file = open_exec(i_name);
> -	if (IS_ERR(file))
> -		return PTR_ERR(file);
> +	if (IS_ERR(file)) {
> +		retval = PTR_ERR(file);
> +		goto ret;
> +	}
> 
>  	bprm->file = file;
>  	retval = prepare_binprm(bprm);
>  	if (retval < 0)
> -		return retval;
> -	return search_binary_handler(bprm);
> +		goto ret;
> +	retval = search_binary_handler(bprm);
> +	if (retval)
> +		goto ret;
> +	/*
> +	 * Allow for validation of the script as well as the interpreter
> +	 */
> +	bprm->file = old;
> +	retval = security_bprm_interpreted(bprm);
> +	bprm->file = file;
> +ret:
> +	allow_write_access(old);
> +	fput(old);
> +	return retval;
>  }
> 
>  static struct linux_binfmt script_format = {
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index c9258124e417..fd23b4098098 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -75,6 +75,11 @@
>   *	linux_binprm structure.  This hook is a good place to perform state
>   *	changes on the process such as clearing out non-inheritable signal
>   *	state.  This is called immediately after commit_creds().
> + * @bprm_interpreted:
> + *	Validate the credentials of an executable that was interpreted via
> + *	binfmt_misc or binfmt_script. This occurs after the new credentials
> + *	have been committed to @current->cred, but @bprm->file points to the
> + *	original file rather than the interpreter that was used to execute it.
>   *
>   * Security hooks for filesystem operations.
>   *
> @@ -1383,6 +1388,7 @@ union security_list_options {
>  	int (*bprm_check_security)(struct linux_binprm *bprm);
>  	void (*bprm_committing_creds)(struct linux_binprm *bprm);
>  	void (*bprm_committed_creds)(struct linux_binprm *bprm);
> +	int (*bprm_interpreted)(struct linux_binprm *bprm);
> 
>  	int (*sb_alloc_security)(struct super_block *sb);
>  	void (*sb_free_security)(struct super_block *sb);
> @@ -1703,6 +1709,7 @@ struct security_hook_heads {
>  	struct list_head bprm_check_security;
>  	struct list_head bprm_committing_creds;
>  	struct list_head bprm_committed_creds;
> +	struct list_head bprm_interpreted;
>  	struct list_head sb_alloc_security;
>  	struct list_head sb_free_security;
>  	struct list_head sb_copy_data;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ad970a4f19f6..e48c38379c64 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -233,6 +233,7 @@ int security_bprm_set_creds(struct linux_binprm *bprm);
>  int security_bprm_check(struct linux_binprm *bprm);
>  void security_bprm_committing_creds(struct linux_binprm *bprm);
>  int security_bprm_committed_creds(struct linux_binprm *bprm);
> +int security_bprm_interpreted(struct linux_binprm *bprm);
>  int security_sb_alloc(struct super_block *sb);
>  void security_sb_free(struct super_block *sb);
>  int security_sb_copy_data(char *orig, char *copy);
> diff --git a/security/security.c b/security/security.c
> index c2c5017e3477..6b9cb108ff61 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -352,6 +352,17 @@ int security_bprm_committed_creds(struct linux_binprm *bprm)
>  	return ima_creds_check(bprm);
>  }
> 
> +int security_bprm_interpreted(struct linux_binprm *bprm)
> +{
> +	int ret;
> +
> +	ret = call_int_hook(bprm_interpreted, 0, bprm);
> +	if (ret)
> +		return ret;
> +
> +	return ima_creds_check(bprm);
> +}
> +
>  int security_sb_alloc(struct super_block *sb)
>  {
>  	return call_int_hook(sb_alloc_security, 0, sb);

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH V3 2/2] Add a bprm_interpreted security hook and call into IMA from it
  2017-10-15 14:08   ` Mimi Zohar
@ 2017-10-16 18:13     ` Matthew Garrett
  0 siblings, 0 replies; 4+ messages in thread
From: Matthew Garrett @ 2017-10-16 18:13 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, james.l.morris, keescook, oleg

On Sun, Oct 15, 2017 at 7:08 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Even with this additional patch, there are still potentially missing measurements/appraisals as search_binary_handler is exported.  The original search_binary_handler is called twice, once for the original file and again for the interpreter.  With these patches, the security hooks are deferred, requiring calls in the specific binary handler.
>
> For your usecase scenario this might be enough, but for the general case the security_bprm_check hooks would still be needed.

Mm. Yeah. Ok let me try tackling this in a different way.

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-16 18:13 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-11 19:12 [PATCH V3 1/2] IMA: Add support for IMA validation after credentials are committed Matthew Garrett
2017-10-11 19:12 ` [PATCH V3 2/2] Add a bprm_interpreted security hook and call into IMA from it Matthew Garrett
2017-10-15 14:08   ` Mimi Zohar
2017-10-16 18:13     ` Matthew Garrett

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.