[RFC PATCH 0/4] IMA: Export functions for file integrity verification

[RFC PATCH 0/4] IMA: Export functions for file integrity verification

[Vivek Goyal]

Hi,

This is just a proof of concept RFC to export some functions from IMA for
file integrity verification. And there is a patch which modified binfmt_elf.c
to show how a IMA subsystem user can call into IMA to verify integrity
of a file.

This patch set is far from being done. I am just throwing it out so that
we can start a discussion on whether exporting IMA functions makes sense
and if it does, then how those functions should look like.

Thanks
Vivek

Vivek Goyal (4):
  integrity: Identify asymmetric digital signature using new type
  ima: export new IMA functions for signature verification
  capability: Create a new capability CAP_SIGNED
  binfmt_elf: Elf executable signature verification

 fs/Kconfig.binfmt                     |   12 ++++++++
 fs/binfmt_elf.c                       |   44 +++++++++++++++++++++++++++++++
 include/linux/ima.h                   |   24 ++++++++++++++++-
 include/linux/integrity.h             |    7 +++++
 include/uapi/linux/capability.h       |   12 ++++++++-
 kernel/cred.c                         |    7 +++++
 security/commoncap.c                  |    2 +
 security/integrity/digsig.c           |   11 +++++---
 security/integrity/evm/evm_main.c     |    4 ++-
 security/integrity/ima/ima_api.c      |   16 +++++++++++
 security/integrity/ima/ima_appraise.c |   46 +++++++++++++++++++++++++++++++-
 security/integrity/integrity.h        |   14 +++------
 12 files changed, 181 insertions(+), 18 deletions(-)

-- 
1.7.7.6

[PATCH 1/4] integrity: Identify asymmetric digital signature using new type

[Vivek Goyal]

Currently there seem to be two types of digital signatures. Old one and
that is RSA and new one which is being called asymmetric. Right now they
both fall in the categorty of EVM_IMA_XATTR_DIGSIG and one differentiates
between two using signature version. Version 1 is old type and version 2
is new type.

How about asymmetric signature using a new type say
EVM_IMA_XATTR_DIGSIG_ASYMMETRIC. And version numbering can be used for
structure variation with-in signature type.

This can allow to export signature type to other subsystems. And these
subsystems will call into ima/integrity for verification only if signature
are of certain type.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 security/integrity/digsig.c           |   11 +++++++----
 security/integrity/evm/evm_main.c     |    4 +++-
 security/integrity/ima/ima_appraise.c |    7 +++++--
 security/integrity/integrity.h        |   14 +++++---------
 4 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 0b759e1..268c4cc 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -27,7 +27,8 @@ static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
 	"_ima",
 };
 
-int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
+int integrity_digsig_verify(enum evm_ima_xattr_type sig_type,
+			const unsigned int id, const char *sig, int siglen,
 					const char *digest, int digestlen)
 {
 	if (id >= INTEGRITY_KEYRING_MAX)
@@ -44,13 +45,15 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 		}
 	}
 
-	switch (sig[0]) {
-	case 1:
+	switch (sig_type) {
+	case EVM_IMA_XATTR_DIGSIG:
 		return digsig_verify(keyring[id], sig, siglen,
 				     digest, digestlen);
-	case 2:
+	case EVM_IMA_XATTR_DIGSIG_ASYMMETRIC:
 		return asymmetric_verify(keyring[id], sig, siglen,
 					 digest, digestlen);
+	default:
+		break;
 	}
 
 	return -EOPNOTSUPP;
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index cdbde17..cf798cb 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -134,11 +134,13 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 			rc = -EINVAL;
 		break;
 	case EVM_IMA_XATTR_DIGSIG:
+	case EVM_IMA_XATTR_DIGSIG_ASYMMETRIC:
 		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
 				xattr_value_len, calc.digest);
 		if (rc)
 			break;
-		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
+		rc = integrity_digsig_verify(xattr_data->type,
+					INTEGRITY_KEYRING_EVM,
 					xattr_data->digest, xattr_len,
 					calc.digest, sizeof(calc.digest));
 		if (!rc) {
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 2d4beca..9bc0723 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -169,8 +169,10 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
 		status = INTEGRITY_PASS;
 		break;
 	case EVM_IMA_XATTR_DIGSIG:
+	case EVM_IMA_XATTR_DIGSIG_ASYMMETRIC:
 		iint->flags |= IMA_DIGSIG;
-		rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
+		rc = integrity_digsig_verify(xattr_value->type,
+					     INTEGRITY_KEYRING_IMA,
 					     xattr_value->digest, rc - 1,
 					     iint->ima_xattr.digest,
 					     IMA_DIGEST_SIZE);
@@ -193,7 +195,8 @@ out:
 	if (status != INTEGRITY_PASS) {
 		if ((ima_appraise & IMA_APPRAISE_FIX) &&
 		    (!xattr_value ||
-		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
+		     (xattr_value->type != EVM_IMA_XATTR_DIGSIG &&
+		      xattr_value->type != EVM_IMA_XATTR_DIGSIG_ASYMMETRIC))) {
 			if (!ima_fix_xattr(dentry, iint))
 				status = INTEGRITY_PASS;
 		}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 84c37c4..8392d18 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -50,12 +50,6 @@
 #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,
-	EVM_XATTR_HMAC,
-	EVM_IMA_XATTR_DIGSIG,
-};
-
 struct evm_ima_xattr_data {
 	u8 type;
 	u8 digest[SHA1_DIGEST_SIZE];
@@ -88,12 +82,14 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
 
 #ifdef CONFIG_INTEGRITY_SIGNATURE
 
-int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
-					const char *digest, int digestlen);
+int integrity_digsig_verify(enum evm_ima_xattr_type sig_type,
+	const unsigned int id, const char *sig, int siglen,
+	const char *digest, int digestlen);
 
 #else
 
-static inline int integrity_digsig_verify(const unsigned int id,
+static inline int integrity_digsig_verify(enum evm_ima_xattr_type sig_type,
+					  const unsigned int id,
 					  const char *sig, int siglen,
 					  const char *digest, int digestlen)
 {
-- 
1.7.7.6

[PATCH 2/4] ima: export new IMA functions for signature verification

[Vivek Goyal]

Export IMA functions so that other subsystems can use IMA for file
signature verification.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/linux/ima.h                   |   24 +++++++++++++++++++-
 include/linux/integrity.h             |    7 ++++++
 security/integrity/ima/ima_api.c      |   16 +++++++++++++
 security/integrity/ima/ima_appraise.c |   39 +++++++++++++++++++++++++++++++++
 4 files changed, 85 insertions(+), 1 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 86c361e..846e784 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -11,6 +11,7 @@
 #define _LINUX_IMA_H
 
 #include <linux/fs.h>
+#include <linux/integrity.h>
 struct linux_binprm;
 
 #ifdef CONFIG_IMA
@@ -19,7 +20,8 @@ extern int ima_file_check(struct file *file, int mask);
 extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
 extern int ima_module_check(struct file *file);
-
+extern int ima_file_signature_alloc(struct file *file, char **sig);
+extern int ima_signature_type(char *sig, enum evm_ima_xattr_type *sig_type);
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
 {
@@ -46,6 +48,18 @@ static inline int ima_module_check(struct file *file)
 	return 0;
 }
 
+static inline int ima_file_signature_alloc(struct file *file, char **sig)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int ima_signature_type(char *sig,
+					enum evm_ima_xattr_type *sig_type)
+{
+	return -EOPNOTSUPP;
+}
+
+
 #endif /* CONFIG_IMA_H */
 
 #ifdef CONFIG_IMA_APPRAISE
@@ -53,6 +67,7 @@ extern void ima_inode_post_setattr(struct dentry *dentry);
 extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		       const void *xattr_value, size_t xattr_value_len);
 extern int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name);
+extern int ima_appraise_file(struct file *file, char *sig, unsigned int siglen);
 #else
 static inline void ima_inode_post_setattr(struct dentry *dentry)
 {
@@ -72,5 +87,12 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
 {
 	return 0;
 }
+
+static inline int ima_appraise_file(struct file *file, char *sig,
+			unsigned int siglen)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif /* CONFIG_IMA_APPRAISE_H */
 #endif /* _LINUX_IMA_H */
diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index 66c5fe9..0aa1b42 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -20,6 +20,13 @@ enum integrity_status {
 	INTEGRITY_UNKNOWN,
 };
 
+enum evm_ima_xattr_type {
+	IMA_XATTR_DIGEST = 0x01,
+	EVM_XATTR_HMAC,
+	EVM_IMA_XATTR_DIGSIG,
+	EVM_IMA_XATTR_DIGSIG_ASYMMETRIC,
+};
+
 /* List of EVM protected security xattrs */
 #ifdef CONFIG_INTEGRITY
 extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 1c03e8f..cb23538 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -254,3 +254,19 @@ const char *ima_d_path(struct path *path, char **pathbuf)
 	}
 	return pathname;
 }
+
+/*
+ * Get ima signature.
+ */
+int ima_file_signature_alloc(struct file *file, char **sig)
+{
+	struct dentry *dentry = file->f_dentry;
+
+	return vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, sig, 0, GFP_NOFS);
+}
+
+int ima_signature_type(char *sig, enum evm_ima_xattr_type *sig_type)
+{
+	*sig_type = ((struct evm_ima_xattr_data *)sig)->type;
+	return 0;
+}
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 9bc0723..4e44874 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -108,6 +108,45 @@ static void ima_cache_flags(struct integrity_iint_cache *iint, int func)
 }
 
 /*
+ * Appraise a file with a given signature
+ * sig: signature
+ * siglen: length of signature
+ *
+ * Returns 0 on successful appraisal, error otherwise.
+ */
+int ima_appraise_file(struct file *file, char *sig, unsigned int siglen)
+{
+	struct evm_ima_xattr_data *xattr_value;
+	int ret = 0, rc;
+	/* FIX Me. retrieve hash algo from sig */
+	char digest[SHA1_DIGEST_SIZE];
+
+	xattr_value = (struct evm_ima_xattr_data*) sig;
+	ret = ima_calc_file_hash(file, digest);
+	if (ret)
+		return ret;
+
+	switch (xattr_value->type) {
+	case IMA_XATTR_DIGEST:
+		rc = memcmp(xattr_value->digest, digest, SHA1_DIGEST_SIZE);
+		if (!rc)
+			ret = -EKEYREJECTED;
+		break;
+	case EVM_IMA_XATTR_DIGSIG:
+	case EVM_IMA_XATTR_DIGSIG_ASYMMETRIC:
+		return integrity_digsig_verify(xattr_value->type,
+					     INTEGRITY_KEYRING_IMA,
+					     xattr_value->digest, siglen - 1,
+					     digest, SHA1_DIGEST_SIZE);
+	default:
+		ret = -EBADMSG;
+		break;
+	}
+
+	return ret;
+}
+
+/*
  * ima_appraise_measurement - appraise file measurement
  *
  * Call evm_verifyxattr() to verify the integrity of 'security.ima'.
-- 
1.7.7.6

[PATCH 3/4] capability: Create a new capability CAP_SIGNED

[Vivek Goyal]

Create a new capability CAP_SIGNED which can be given to signed executables.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/uapi/linux/capability.h |   12 +++++++++++-
 kernel/cred.c                   |    7 +++++++
 security/commoncap.c            |    2 ++
 3 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index ba478fa..1bbe671 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -343,7 +343,17 @@ struct vfs_cap_data {
 
 #define CAP_BLOCK_SUSPEND    36
 
-#define CAP_LAST_CAP         CAP_BLOCK_SUSPEND
+/*
+ * Allow certain kernel services with secureboot enabled. One of such
+ * service is sys_kexec() which can be invoked by process only if it
+ * has CAP_SIGNED capability (with secureboot enabled).
+ *
+ * This capability is given by kernel automatically if executable
+ * file is validly signed.
+ */
+#define CAP_SIGNED    37
+
+#define CAP_LAST_CAP         CAP_SIGNED
 
 #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
 
diff --git a/kernel/cred.c b/kernel/cred.c
index e0573a4..f554d1b 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -555,6 +555,12 @@ void revert_creds(const struct cred *old)
 }
 EXPORT_SYMBOL(revert_creds);
 
+static void remove_cap_signed_init_cred(void)
+{
+	cap_lower((&init_cred)->cap_bset, CAP_SIGNED);
+	cap_lower((&init_cred)->cap_permitted, CAP_SIGNED);
+}
+
 /*
  * initialise the credentials stuff
  */
@@ -563,6 +569,7 @@ void __init cred_init(void)
 	/* allocate a slab in which we can store credentials */
 	cred_jar = kmem_cache_create("cred_jar", sizeof(struct cred),
 				     0, SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL);
+	remove_cap_signed_init_cred();
 }
 
 /**
diff --git a/security/commoncap.c b/security/commoncap.c
index c44b6fe..4190eb9 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -272,6 +272,8 @@ int cap_capset(struct cred *new,
 	new->cap_effective   = *effective;
 	new->cap_inheritable = *inheritable;
 	new->cap_permitted   = *permitted;
+	if (cap_raised(old->cap_effective, CAP_SIGNED))
+		cap_raise(new->cap_effective, CAP_SIGNED);
 	return 0;
 }
 
-- 
1.7.7.6

Re: [PATCH 3/4] capability: Create a new capability CAP_SIGNED

[Casey Schaufler]

On 3/15/2013 1:35 PM, Vivek Goyal wrote:
> Create a new capability CAP_SIGNED which can be given to signed executables.

This would drive anyone who is trying to use
capabilities as the privilege mechanism it is
intended to be absolutely crazy.

Capabilities aren't just random attribute bits. They
indicate that a task has permission to violate a
system policy (e.g. change the mode bits of a file
the user doesn't own). Think about how this will
interact with programs using file based capabilities.

>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  include/uapi/linux/capability.h |   12 +++++++++++-
>  kernel/cred.c                   |    7 +++++++
>  security/commoncap.c            |    2 ++
>  3 files changed, 20 insertions(+), 1 deletions(-)
>
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index ba478fa..1bbe671 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -343,7 +343,17 @@ struct vfs_cap_data {
>  
>  #define CAP_BLOCK_SUSPEND    36
>  
> -#define CAP_LAST_CAP         CAP_BLOCK_SUSPEND
> +/*
> + * Allow certain kernel services with secureboot enabled. One of such
> + * service is sys_kexec() which can be invoked by process only if it
> + * has CAP_SIGNED capability (with secureboot enabled).
> + *
> + * This capability is given by kernel automatically if executable
> + * file is validly signed.
> + */
> +#define CAP_SIGNED    37
> +
> +#define CAP_LAST_CAP         CAP_SIGNED
>  
>  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>  
> diff --git a/kernel/cred.c b/kernel/cred.c
> index e0573a4..f554d1b 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -555,6 +555,12 @@ void revert_creds(const struct cred *old)
>  }
>  EXPORT_SYMBOL(revert_creds);
>  
> +static void remove_cap_signed_init_cred(void)
> +{
> +	cap_lower((&init_cred)->cap_bset, CAP_SIGNED);
> +	cap_lower((&init_cred)->cap_permitted, CAP_SIGNED);
> +}
> +
>  /*
>   * initialise the credentials stuff
>   */
> @@ -563,6 +569,7 @@ void __init cred_init(void)
>  	/* allocate a slab in which we can store credentials */
>  	cred_jar = kmem_cache_create("cred_jar", sizeof(struct cred),
>  				     0, SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL);
> +	remove_cap_signed_init_cred();
>  }
>  
>  /**
> diff --git a/security/commoncap.c b/security/commoncap.c
> index c44b6fe..4190eb9 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -272,6 +272,8 @@ int cap_capset(struct cred *new,
>  	new->cap_effective   = *effective;
>  	new->cap_inheritable = *inheritable;
>  	new->cap_permitted   = *permitted;
> +	if (cap_raised(old->cap_effective, CAP_SIGNED))
> +		cap_raise(new->cap_effective, CAP_SIGNED);
>  	return 0;
>  }
>  

Re: [PATCH 3/4] capability: Create a new capability CAP_SIGNED

[Vivek Goyal]

On Fri, Mar 15, 2013 at 02:12:59PM -0700, Casey Schaufler wrote:
> On 3/15/2013 1:35 PM, Vivek Goyal wrote:
> > Create a new capability CAP_SIGNED which can be given to signed executables.
> 
> This would drive anyone who is trying to use
> capabilities as the privilege mechanism it is
> intended to be absolutely crazy.

Will calling it CAP_SIGNED_SERVICES help. I intend to use it as
capability (and not just as a flag for task attribute).

I think primary difference here is that this capability is controlled
by kernel and only validly signed processes get it.

> 
> Capabilities aren't just random attribute bits. They
> indicate that a task has permission to violate a
> system policy (e.g. change the mode bits of a file
> the user doesn't own). Think about how this will
> interact with programs using file based capabilities.

It is a separate capability. I am not sure why it would
interfere with other capabilities or functionality out there.

Thanks
Vivek

> 
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  include/uapi/linux/capability.h |   12 +++++++++++-
> >  kernel/cred.c                   |    7 +++++++
> >  security/commoncap.c            |    2 ++
> >  3 files changed, 20 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> > index ba478fa..1bbe671 100644
> > --- a/include/uapi/linux/capability.h
> > +++ b/include/uapi/linux/capability.h
> > @@ -343,7 +343,17 @@ struct vfs_cap_data {
> >  
> >  #define CAP_BLOCK_SUSPEND    36
> >  
> > -#define CAP_LAST_CAP         CAP_BLOCK_SUSPEND
> > +/*
> > + * Allow certain kernel services with secureboot enabled. One of such
> > + * service is sys_kexec() which can be invoked by process only if it
> > + * has CAP_SIGNED capability (with secureboot enabled).
> > + *
> > + * This capability is given by kernel automatically if executable
> > + * file is validly signed.
> > + */
> > +#define CAP_SIGNED    37
> > +
> > +#define CAP_LAST_CAP         CAP_SIGNED
> >  
> >  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
> >  
> > diff --git a/kernel/cred.c b/kernel/cred.c
> > index e0573a4..f554d1b 100644
> > --- a/kernel/cred.c
> > +++ b/kernel/cred.c
> > @@ -555,6 +555,12 @@ void revert_creds(const struct cred *old)
> >  }
> >  EXPORT_SYMBOL(revert_creds);
> >  
> > +static void remove_cap_signed_init_cred(void)
> > +{
> > +	cap_lower((&init_cred)->cap_bset, CAP_SIGNED);
> > +	cap_lower((&init_cred)->cap_permitted, CAP_SIGNED);
> > +}
> > +
> >  /*
> >   * initialise the credentials stuff
> >   */
> > @@ -563,6 +569,7 @@ void __init cred_init(void)
> >  	/* allocate a slab in which we can store credentials */
> >  	cred_jar = kmem_cache_create("cred_jar", sizeof(struct cred),
> >  				     0, SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL);
> > +	remove_cap_signed_init_cred();
> >  }
> >  
> >  /**
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index c44b6fe..4190eb9 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -272,6 +272,8 @@ int cap_capset(struct cred *new,
> >  	new->cap_effective   = *effective;
> >  	new->cap_inheritable = *inheritable;
> >  	new->cap_permitted   = *permitted;
> > +	if (cap_raised(old->cap_effective, CAP_SIGNED))
> > +		cap_raise(new->cap_effective, CAP_SIGNED);
> >  	return 0;
> >  }
> >  

Re: [PATCH 3/4] capability: Create a new capability CAP_SIGNED

[Casey Schaufler]

On 3/18/2013 10:05 AM, Vivek Goyal wrote:
> On Fri, Mar 15, 2013 at 02:12:59PM -0700, Casey Schaufler wrote:
>> On 3/15/2013 1:35 PM, Vivek Goyal wrote:
>>> Create a new capability CAP_SIGNED which can be given to signed executables.
>> This would drive anyone who is trying to use
>> capabilities as the privilege mechanism it is
>> intended to be absolutely crazy.
> Will calling it CAP_SIGNED_SERVICES help. I intend to use it as
> capability (and not just as a flag for task attribute).

No, the name is not the issue.

> I think primary difference here is that this capability is controlled
> by kernel and only validly signed processes get it.

Applications are allowed to manipulate their capability sets
in well defined ways. The behavior of file based capabilities
is also explicitly defined. The behavior you are proposing would
violate both of these mechanisms. 

>> Capabilities aren't just random attribute bits. They
>> indicate that a task has permission to violate a
>> system policy (e.g. change the mode bits of a file
>> the user doesn't own). Think about how this will
>> interact with programs using file based capabilities.
> It is a separate capability. I am not sure why it would
> interfere with other capabilities or functionality out there.

The behavior of capabilities is uniform. You can't have one
capability that behaves differently from the others. If a
file is unsigned but has CAP_SIGNED in the file capability
set what do you expect to happen? Do you want a signed
application to be able to drop and raise the fact that it
is signed?

I expect that you don't want your attribute that indicates
that the binary was signed to behave the same way that
capabilities do. Like I said, capabilities are not just
attribute bits. You need a different kind of process attribute
to indicate that the binary was signed.

When (if ever) we have multiple LSM support you might consider
doing this as a small LSM. Until then, you're going to need a
different way to express the signature attribute.


>
> Thanks
> Vivek
>
>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>>> ---
>>>  include/uapi/linux/capability.h |   12 +++++++++++-
>>>  kernel/cred.c                   |    7 +++++++
>>>  security/commoncap.c            |    2 ++
>>>  3 files changed, 20 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
>>> index ba478fa..1bbe671 100644
>>> --- a/include/uapi/linux/capability.h
>>> +++ b/include/uapi/linux/capability.h
>>> @@ -343,7 +343,17 @@ struct vfs_cap_data {
>>>  
>>>  #define CAP_BLOCK_SUSPEND    36
>>>  
>>> -#define CAP_LAST_CAP         CAP_BLOCK_SUSPEND
>>> +/*
>>> + * Allow certain kernel services with secureboot enabled. One of such
>>> + * service is sys_kexec() which can be invoked by process only if it
>>> + * has CAP_SIGNED capability (with secureboot enabled).
>>> + *
>>> + * This capability is given by kernel automatically if executable
>>> + * file is validly signed.
>>> + */
>>> +#define CAP_SIGNED    37
>>> +
>>> +#define CAP_LAST_CAP         CAP_SIGNED
>>>  
>>>  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>>>  
>>> diff --git a/kernel/cred.c b/kernel/cred.c
>>> index e0573a4..f554d1b 100644
>>> --- a/kernel/cred.c
>>> +++ b/kernel/cred.c
>>> @@ -555,6 +555,12 @@ void revert_creds(const struct cred *old)
>>>  }
>>>  EXPORT_SYMBOL(revert_creds);
>>>  
>>> +static void remove_cap_signed_init_cred(void)
>>> +{
>>> +	cap_lower((&init_cred)->cap_bset, CAP_SIGNED);
>>> +	cap_lower((&init_cred)->cap_permitted, CAP_SIGNED);
>>> +}
>>> +
>>>  /*
>>>   * initialise the credentials stuff
>>>   */
>>> @@ -563,6 +569,7 @@ void __init cred_init(void)
>>>  	/* allocate a slab in which we can store credentials */
>>>  	cred_jar = kmem_cache_create("cred_jar", sizeof(struct cred),
>>>  				     0, SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL);
>>> +	remove_cap_signed_init_cred();
>>>  }
>>>  
>>>  /**
>>> diff --git a/security/commoncap.c b/security/commoncap.c
>>> index c44b6fe..4190eb9 100644
>>> --- a/security/commoncap.c
>>> +++ b/security/commoncap.c
>>> @@ -272,6 +272,8 @@ int cap_capset(struct cred *new,
>>>  	new->cap_effective   = *effective;
>>>  	new->cap_inheritable = *inheritable;
>>>  	new->cap_permitted   = *permitted;
>>> +	if (cap_raised(old->cap_effective, CAP_SIGNED))
>>> +		cap_raise(new->cap_effective, CAP_SIGNED);
>>>  	return 0;
>>>  }
>>>  

Re: [PATCH 3/4] capability: Create a new capability CAP_SIGNED

[Vivek Goyal]

On Mon, Mar 18, 2013 at 10:50:21AM -0700, Casey Schaufler wrote:
> On 3/18/2013 10:05 AM, Vivek Goyal wrote:
> > On Fri, Mar 15, 2013 at 02:12:59PM -0700, Casey Schaufler wrote:
> >> On 3/15/2013 1:35 PM, Vivek Goyal wrote:
> >>> Create a new capability CAP_SIGNED which can be given to signed executables.
> >> This would drive anyone who is trying to use
> >> capabilities as the privilege mechanism it is
> >> intended to be absolutely crazy.
> > Will calling it CAP_SIGNED_SERVICES help. I intend to use it as
> > capability (and not just as a flag for task attribute).
> 
> No, the name is not the issue.
> 
> > I think primary difference here is that this capability is controlled
> > by kernel and only validly signed processes get it.
> 
> Applications are allowed to manipulate their capability sets
> in well defined ways. The behavior of file based capabilities
> is also explicitly defined. The behavior you are proposing would
> violate both of these mechanisms. 
> 
> >> Capabilities aren't just random attribute bits. They
> >> indicate that a task has permission to violate a
> >> system policy (e.g. change the mode bits of a file
> >> the user doesn't own). Think about how this will
> >> interact with programs using file based capabilities.
> > It is a separate capability. I am not sure why it would
> > interfere with other capabilities or functionality out there.
> 
> The behavior of capabilities is uniform. You can't have one
> capability that behaves differently from the others. If a
> file is unsigned but has CAP_SIGNED in the file capability
> set what do you expect to happen? Do you want a signed
> application to be able to drop and raise the fact that it
> is signed?

I have already removed this capability from bounding set. Behavior
I am looking for is that nobody should be able to set CAP_SIGNED
as file capability. I will look into that.

I am thinking of this more as kernel managed capability. It is
not in bounding set of any process and it can not be set as file
capability. 

It is a new capability, so no existing user application should
be trying to set it.

I think the only surprise would be that they can't drop it. If
that's a concern, may be we can allow dropping the capability.
But the side affect is that there is no way to gain it back for
the life time of process.

> 
> I expect that you don't want your attribute that indicates
> that the binary was signed to behave the same way that
> capabilities do. Like I said, capabilities are not just
> attribute bits. You need a different kind of process attribute
> to indicate that the binary was signed.

I think I need more than process attribute. One of the things
I am looking for is that signed processes run locked in memory
and nobody (i think no unsigned process) is able to do ptrace() on it.
Using the notion of capability might help here.

> 
> When (if ever) we have multiple LSM support you might consider
> doing this as a small LSM. Until then, you're going to need a
> different way to express the signature attribute.

I am not sure why you are viewing it as necessarily as attribute only.
I am thinking more in terms of that in certain situations, user space
processes can't perform certain operations (like kexec) untile and
unless process has the capability CAP_SIGNED_SERVICES. And this capability
is granted if upon exec() process signature are verified.

So yes it is little different from how capabilities are managed
currently. But is it very hard to extend the current capability definition
and include the fact that kernel can give additional capabilities to
processes based on some other factors.

Thanks
Vivek

Re: [PATCH 3/4] capability: Create a new capability CAP_SIGNED

[Casey Schaufler]

On 3/18/2013 11:30 AM, Vivek Goyal wrote:
> On Mon, Mar 18, 2013 at 10:50:21AM -0700, Casey Schaufler wrote:
>> On 3/18/2013 10:05 AM, Vivek Goyal wrote:
>>> On Fri, Mar 15, 2013 at 02:12:59PM -0700, Casey Schaufler wrote:
>>>> On 3/15/2013 1:35 PM, Vivek Goyal wrote:
>>>>> Create a new capability CAP_SIGNED which can be given to signed executables.
>>>> This would drive anyone who is trying to use
>>>> capabilities as the privilege mechanism it is
>>>> intended to be absolutely crazy.
>>> Will calling it CAP_SIGNED_SERVICES help. I intend to use it as
>>> capability (and not just as a flag for task attribute).
>> No, the name is not the issue.
>>
>>> I think primary difference here is that this capability is controlled
>>> by kernel and only validly signed processes get it.
>> Applications are allowed to manipulate their capability sets
>> in well defined ways. The behavior of file based capabilities
>> is also explicitly defined. The behavior you are proposing would
>> violate both of these mechanisms. 
>>
>>>> Capabilities aren't just random attribute bits. They
>>>> indicate that a task has permission to violate a
>>>> system policy (e.g. change the mode bits of a file
>>>> the user doesn't own). Think about how this will
>>>> interact with programs using file based capabilities.
>>> It is a separate capability. I am not sure why it would
>>> interfere with other capabilities or functionality out there.
>> The behavior of capabilities is uniform. You can't have one
>> capability that behaves differently from the others. If a
>> file is unsigned but has CAP_SIGNED in the file capability
>> set what do you expect to happen? Do you want a signed
>> application to be able to drop and raise the fact that it
>> is signed?
> I have already removed this capability from bounding set. Behavior
> I am looking for is that nobody should be able to set CAP_SIGNED
> as file capability. I will look into that.

No! You are not listening. All capabilities work the same way.
If the file capabilities say ALL that means ALL. You do not get
to put a hole in the middle of the file based capabilities.


> I am thinking of this more as kernel managed capability. It is
> not in bounding set of any process and it can not be set as file
> capability.

I heard that. No, you don't get to do that. All capabilities
work the same way. Your attribute does not behave the way
capabilities do, so you have to implement it some other way.


> It is a new capability, so no existing user application should
> be trying to set it.

There are (and will be) applications that raise and drop all
capabilities, and that do so for good reasons.

> I think the only surprise would be that they can't drop it. If
> that's a concern, may be we can allow dropping the capability.
> But the side affect is that there is no way to gain it back for
> the life time of process.

Right. And that is a change to the capability mechanism. No, you
don't get to do that.

You don't want a new capability. You want a new attribute that
behaves differently than capabilities do. You need to come up
with a different way to implement your attribute. You do not get
to change the way capabilities work.

>> I expect that you don't want your attribute that indicates
>> that the binary was signed to behave the same way that
>> capabilities do. Like I said, capabilities are not just
>> attribute bits. You need a different kind of process attribute
>> to indicate that the binary was signed.
> I think I need more than process attribute. One of the things
> I am looking for is that signed processes run locked in memory
> and nobody (i think no unsigned process) is able to do ptrace() on it.
> Using the notion of capability might help here.

There are already capabilities associated with ptrace. It would
be simple to add a check for signatures in cap_ptrace_access_check.


>> When (if ever) we have multiple LSM support you might consider
>> doing this as a small LSM. Until then, you're going to need a
>> different way to express the signature attribute.
> I am not sure why you are viewing it as necessarily as attribute only.
> I am thinking more in terms of that in certain situations, user space
> processes can't perform certain operations (like kexec) untile and
> unless process has the capability CAP_SIGNED_SERVICES. And this capability
> is granted if upon exec() process signature are verified.

Sigh. You need the process attribute to make the checks against. The
process capability set, uids and groups are all examples of process
attributes that exist today.

> So yes it is little different from how capabilities are managed
> currently. But is it very hard to extend the current capability definition
> and include the fact that kernel can give additional capabilities to
> processes based on some other factors.

Yes. That is correct. That is why we have the LSM facility. The
unfortunate fact is that you only get one LSM at a time today. I
am working on fixing that, but there is still work to be done
before it will be ready for upstream.

If signed application controls are deemed sufficiently important
and your implementation sound you should be able to get the signature
attribute and the checks on that attribute into the base system.


> Thanks
> Vivek
>

Re: [PATCH 3/4] capability: Create a new capability CAP_SIGNED

[Eric W. Biederman]

Adding Serge as he is the sometimes capabilities maintainer to this
discussion.

Casey Schaufler <casey@schaufler-ca.com> writes:

> On 3/18/2013 11:30 AM, Vivek Goyal wrote:
>> On Mon, Mar 18, 2013 at 10:50:21AM -0700, Casey Schaufler wrote:
>>> On 3/18/2013 10:05 AM, Vivek Goyal wrote:
>>>> On Fri, Mar 15, 2013 at 02:12:59PM -0700, Casey Schaufler wrote:
>>>>> On 3/15/2013 1:35 PM, Vivek Goyal wrote:
>>>>>> Create a new capability CAP_SIGNED which can be given to signed executables.
>>>>> This would drive anyone who is trying to use
>>>>> capabilities as the privilege mechanism it is
>>>>> intended to be absolutely crazy.
>>>> Will calling it CAP_SIGNED_SERVICES help. I intend to use it as
>>>> capability (and not just as a flag for task attribute).
>>> No, the name is not the issue.
>>>
>>>> I think primary difference here is that this capability is controlled
>>>> by kernel and only validly signed processes get it.
>>> Applications are allowed to manipulate their capability sets
>>> in well defined ways. The behavior of file based capabilities
>>> is also explicitly defined. The behavior you are proposing would
>>> violate both of these mechanisms. 
>>>
>>>>> Capabilities aren't just random attribute bits. They
>>>>> indicate that a task has permission to violate a
>>>>> system policy (e.g. change the mode bits of a file
>>>>> the user doesn't own). Think about how this will
>>>>> interact with programs using file based capabilities.
>>>> It is a separate capability. I am not sure why it would
>>>> interfere with other capabilities or functionality out there.
>>> The behavior of capabilities is uniform. You can't have one
>>> capability that behaves differently from the others. If a
>>> file is unsigned but has CAP_SIGNED in the file capability
>>> set what do you expect to happen? Do you want a signed
>>> application to be able to drop and raise the fact that it
>>> is signed?
>> I have already removed this capability from bounding set. Behavior
>> I am looking for is that nobody should be able to set CAP_SIGNED
>> as file capability. I will look into that.
>
> No! You are not listening. All capabilities work the same way.
> If the file capabilities say ALL that means ALL. You do not get
> to put a hole in the middle of the file based capabilities.
>
>
>> I am thinking of this more as kernel managed capability. It is
>> not in bounding set of any process and it can not be set as file
>> capability.
>
> I heard that. No, you don't get to do that. All capabilities
> work the same way. Your attribute does not behave the way
> capabilities do, so you have to implement it some other way.
>
>
>> It is a new capability, so no existing user application should
>> be trying to set it.
>
> There are (and will be) applications that raise and drop all
> capabilities, and that do so for good reasons.
>
>> I think the only surprise would be that they can't drop it. If
>> that's a concern, may be we can allow dropping the capability.
>> But the side affect is that there is no way to gain it back for
>> the life time of process.
>
> Right. And that is a change to the capability mechanism. No, you
> don't get to do that.
>
> You don't want a new capability. You want a new attribute that
> behaves differently than capabilities do. You need to come up
> with a different way to implement your attribute. You do not get
> to change the way capabilities work.
>
>>> I expect that you don't want your attribute that indicates
>>> that the binary was signed to behave the same way that
>>> capabilities do. Like I said, capabilities are not just
>>> attribute bits. You need a different kind of process attribute
>>> to indicate that the binary was signed.
>> I think I need more than process attribute. One of the things
>> I am looking for is that signed processes run locked in memory
>> and nobody (i think no unsigned process) is able to do ptrace() on it.
>> Using the notion of capability might help here.
>
> There are already capabilities associated with ptrace. It would
> be simple to add a check for signatures in cap_ptrace_access_check.
>
>
>>> When (if ever) we have multiple LSM support you might consider
>>> doing this as a small LSM. Until then, you're going to need a
>>> different way to express the signature attribute.
>> I am not sure why you are viewing it as necessarily as attribute only.
>> I am thinking more in terms of that in certain situations, user space
>> processes can't perform certain operations (like kexec) untile and
>> unless process has the capability CAP_SIGNED_SERVICES. And this capability
>> is granted if upon exec() process signature are verified.
>
> Sigh. You need the process attribute to make the checks against. The
> process capability set, uids and groups are all examples of process
> attributes that exist today.
>
>> So yes it is little different from how capabilities are managed
>> currently. But is it very hard to extend the current capability definition
>> and include the fact that kernel can give additional capabilities to
>> processes based on some other factors.
>
> Yes. That is correct. That is why we have the LSM facility. The
> unfortunate fact is that you only get one LSM at a time today. I
> am working on fixing that, but there is still work to be done
> before it will be ready for upstream.
>
> If signed application controls are deemed sufficiently important
> and your implementation sound you should be able to get the signature
> attribute and the checks on that attribute into the base system.

Vivek the desired semantics for today for kexec is that you have an
application that is allowed CAP_SYS_BOOT in it's file capabilities.

In a context where root is not trusted with all capabilities by default
you want one or a couple of capabilities to only be possible when coming
from file capabilities.  So that you can say.  "I trust you oh great and
blessed executable do what you will."

I don't think those are contentious semantics.

Eric

Re: [PATCH 3/4] capability: Create a new capability CAP_SIGNED

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm@xmission.com):
> 
> Adding Serge as he is the sometimes capabilities maintainer to this
> discussion.

Thanks, Eric.

> Casey Schaufler <casey@schaufler-ca.com> writes:
> 
> > On 3/18/2013 11:30 AM, Vivek Goyal wrote:
> >> On Mon, Mar 18, 2013 at 10:50:21AM -0700, Casey Schaufler wrote:
> >>> On 3/18/2013 10:05 AM, Vivek Goyal wrote:
> >>>> On Fri, Mar 15, 2013 at 02:12:59PM -0700, Casey Schaufler wrote:
> >>>>> On 3/15/2013 1:35 PM, Vivek Goyal wrote:
> >>>>>> Create a new capability CAP_SIGNED which can be given to signed executables.
> >>>>> This would drive anyone who is trying to use
> >>>>> capabilities as the privilege mechanism it is
> >>>>> intended to be absolutely crazy.
> >>>> Will calling it CAP_SIGNED_SERVICES help. I intend to use it as
> >>>> capability (and not just as a flag for task attribute).
> >>> No, the name is not the issue.
> >>>
> >>>> I think primary difference here is that this capability is controlled
> >>>> by kernel and only validly signed processes get it.
> >>> Applications are allowed to manipulate their capability sets
> >>> in well defined ways. The behavior of file based capabilities
> >>> is also explicitly defined. The behavior you are proposing would
> >>> violate both of these mechanisms. 
> >>>
> >>>>> Capabilities aren't just random attribute bits. They
> >>>>> indicate that a task has permission to violate a
> >>>>> system policy (e.g. change the mode bits of a file
> >>>>> the user doesn't own). Think about how this will
> >>>>> interact with programs using file based capabilities.
> >>>> It is a separate capability. I am not sure why it would
> >>>> interfere with other capabilities or functionality out there.
> >>> The behavior of capabilities is uniform. You can't have one
> >>> capability that behaves differently from the others. If a
> >>> file is unsigned but has CAP_SIGNED in the file capability
> >>> set what do you expect to happen? Do you want a signed
> >>> application to be able to drop and raise the fact that it
> >>> is signed?
> >> I have already removed this capability from bounding set. Behavior
> >> I am looking for is that nobody should be able to set CAP_SIGNED
> >> as file capability. I will look into that.
> >
> > No! You are not listening. All capabilities work the same way.
> > If the file capabilities say ALL that means ALL. You do not get
> > to put a hole in the middle of the file based capabilities.
> >
> >
> >> I am thinking of this more as kernel managed capability. It is
> >> not in bounding set of any process and it can not be set as file
> >> capability.
> >
> > I heard that. No, you don't get to do that. All capabilities
> > work the same way. Your attribute does not behave the way
> > capabilities do, so you have to implement it some other way.
> >
> >
> >> It is a new capability, so no existing user application should
> >> be trying to set it.
> >
> > There are (and will be) applications that raise and drop all
> > capabilities, and that do so for good reasons.
> >
> >> I think the only surprise would be that they can't drop it. If
> >> that's a concern, may be we can allow dropping the capability.
> >> But the side affect is that there is no way to gain it back for
> >> the life time of process.
> >
> > Right. And that is a change to the capability mechanism. No, you
> > don't get to do that.
> >
> > You don't want a new capability. You want a new attribute that
> > behaves differently than capabilities do. You need to come up
> > with a different way to implement your attribute. You do not get
> > to change the way capabilities work.
> >
> >>> I expect that you don't want your attribute that indicates
> >>> that the binary was signed to behave the same way that
> >>> capabilities do. Like I said, capabilities are not just
> >>> attribute bits. You need a different kind of process attribute
> >>> to indicate that the binary was signed.
> >> I think I need more than process attribute. One of the things
> >> I am looking for is that signed processes run locked in memory
> >> and nobody (i think no unsigned process) is able to do ptrace() on it.
> >> Using the notion of capability might help here.
> >
> > There are already capabilities associated with ptrace. It would
> > be simple to add a check for signatures in cap_ptrace_access_check.
> >
> >
> >>> When (if ever) we have multiple LSM support you might consider
> >>> doing this as a small LSM. Until then, you're going to need a
> >>> different way to express the signature attribute.
> >> I am not sure why you are viewing it as necessarily as attribute only.
> >> I am thinking more in terms of that in certain situations, user space
> >> processes can't perform certain operations (like kexec) untile and
> >> unless process has the capability CAP_SIGNED_SERVICES. And this capability
> >> is granted if upon exec() process signature are verified.
> >
> > Sigh. You need the process attribute to make the checks against. The
> > process capability set, uids and groups are all examples of process
> > attributes that exist today.
> >
> >> So yes it is little different from how capabilities are managed
> >> currently. But is it very hard to extend the current capability definition
> >> and include the fact that kernel can give additional capabilities to
> >> processes based on some other factors.
> >
> > Yes. That is correct. That is why we have the LSM facility. The
> > unfortunate fact is that you only get one LSM at a time today. I
> > am working on fixing that, but there is still work to be done
> > before it will be ready for upstream.
> >
> > If signed application controls are deemed sufficiently important
> > and your implementation sound you should be able to get the signature
> > attribute and the checks on that attribute into the base system.
> 
> Vivek the desired semantics for today for kexec is that you have an
> application that is allowed CAP_SYS_BOOT in it's file capabilities.
> 
> In a context where root is not trusted with all capabilities by default
> you want one or a couple of capabilities to only be possible when coming
> from file capabilities.  So that you can say.  "I trust you oh great and
> blessed executable do what you will."
> 
> I don't think those are contentious semantics.

[ keeping all context as I've just cc:d amorgan who may have input -
though i needed to skim the thread to understand ]

There are many ways that come to mind that we could use knowledge of a
signed binary.  We might actually want a securebit which says "you need
to be running a signed executable to get capabilities."  Or a capset
akin to the bounding set, saying you can only have the caps in this set
if the running binary was a signed one.

But I don't think CAP_SIGNED is the right way to expose and use this
state.

-serge

Re: [PATCH 3/4] capability: Create a new capability CAP_SIGNED

[James Morris]

On Fri, 15 Mar 2013, Casey Schaufler wrote:

> Capabilities aren't just random attribute bits. They
> indicate that a task has permission to violate a
> system policy (e.g. change the mode bits of a file
> the user doesn't own).

Casey's right here, as well he should be.


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 3/4] capability: Create a new capability CAP_SIGNED

[Vivek Goyal]

On Wed, Mar 20, 2013 at 04:07:58PM +1100, James Morris wrote:
> On Fri, 15 Mar 2013, Casey Schaufler wrote:
> 
> > Capabilities aren't just random attribute bits. They
> > indicate that a task has permission to violate a
> > system policy (e.g. change the mode bits of a file
> > the user doesn't own).
> 
> Casey's right here, as well he should be.
> 

Ok, so how do I go about it (Though I have yet to spend more time
understanding the suggestion in couple of other mails. I will do that
now)

I am not sure why CAP_COMPROMISE_KERNEL(CAP_MODIFY_KERNEL) is any
different. When secureboot is enabled, kernel will take away that
capability from all the processes. So kernel became a decision maker
too whether processes have CAP_COMPROMISE_KERNEL or not based on
certain other factors like secureboot is enabled or not.

If I draw a parallel, then based on certain other factors (binary is
signed and secureboot trust has been extended to this binary), why
can't kernel take a decision to give extra capability to this binary.

In fact instead of new capabiilty, I guess upon successful signature
verification, one could just give CAP_MODIFY_KERNEL to process.

I am just trying to understand better that why capability is not
a good fit here (Especially given the fact that CAP_MODIFY_KERNEL
is making progress and it seems reasonable to me to extend the
secureboot trust to validly signed processes. Like modules, their
signatures have been verified and they should be allowed to modify
kernel).

Thanks
Vivek

Re: [PATCH 3/4] capability: Create a new capability CAP_SIGNED

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1064 bytes --]

On Wed, 2013-03-20 at 10:41 -0400, Vivek Goyal wrote:

> I am not sure why CAP_COMPROMISE_KERNEL(CAP_MODIFY_KERNEL) is any
> different. When secureboot is enabled, kernel will take away that
> capability from all the processes. So kernel became a decision maker
> too whether processes have CAP_COMPROMISE_KERNEL or not based on
> certain other factors like secureboot is enabled or not.

No, that's a limited case. Outside of that, it's an access control
capability in exactly the same way as CAP_SYS_RAWIO is. The easiest way
to think of this is probably whether it ever makes sense for an
arbitrary binary run as root to possess that capability. For
CAP_COMPROMISE_KERNEL the answer is yes - for CAP_SIGNED, the answer is
no.

Just have a flag somewhere in the process structure that indicates
whether it was signed. There's no need to use capabilities here.
-- 
Matthew Garrett | mjg59@srcf.ucam.org
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH 4/4] binfmt_elf: Elf executable signature verification

[Vivek Goyal]

Do elf executable signature verification (if one is present). If signature
is present, it should be valid. Validly signed files are given a capability
CAP_SIGNED.

If file is unsigned, it can execute but it does not get the capability
CAP_SIGNED.

This is work in progress. This patch is just an RFC to show how one
can go about making use of IMA APIs for executable signature
verification.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/Kconfig.binfmt |   12 ++++++++++++
 fs/binfmt_elf.c   |   44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
index 0efd152..cbb1d4a 100644
--- a/fs/Kconfig.binfmt
+++ b/fs/Kconfig.binfmt
@@ -23,6 +23,18 @@ config BINFMT_ELF
 	  ld.so (check the file <file:Documentation/Changes> for location and
 	  latest version).
 
+config BINFMT_ELF_SIG
+	bool "ELF binary signature verification"
+	depends on BINFMT_ELF
+	select INTEGRITY
+	select INTEGRITY_SIGNATURE
+	select INTEGRITY_ASYMMETRIC_KEYS
+	select IMA
+	select IMA_APPRAISE
+	default n
+	---help---
+	  Check ELF binary signature verfication.
+
 config COMPAT_BINFMT_ELF
 	bool
 	depends on COMPAT && BINFMT_ELF
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 3939829..820ceb9 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -34,6 +34,7 @@
 #include <linux/utsname.h>
 #include <linux/coredump.h>
 #include <linux/sched.h>
+#include <linux/ima.h>
 #include <asm/uaccess.h>
 #include <asm/param.h>
 #include <asm/page.h>
@@ -581,6 +582,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	int executable_stack = EXSTACK_DEFAULT;
 	unsigned long def_flags = 0;
 	struct pt_regs *regs = current_pt_regs();
+	char *signature = NULL;
+	unsigned int siglen = 0;
+	enum evm_ima_xattr_type sig_type;
+	bool mlock_mappings = false;
+
 	struct {
 		struct elfhdr elf_ex;
 		struct elfhdr interp_elf_ex;
@@ -722,6 +728,29 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	/* OK, This is the point of no return */
 	current->mm->def_flags = def_flags;
 
+#ifdef CONFIG_BINFMT_ELF_SIG
+	/* If executable is digitally signed. Lock down in memory */
+	/* Get file signature, if any */
+	retval = ima_file_signature_alloc(bprm->file, &signature);
+
+	/*
+	 * If there is an error getting signature, bail out. Having
+	 * no signature is fine though.
+	 */
+	if (retval < 0 && retval != -ENODATA && retval != -EOPNOTSUPP)
+		goto out_free_dentry;
+
+	if (signature != NULL) {
+		siglen = retval;
+		retval = ima_signature_type(signature, &sig_type);
+		if (!retval && sig_type == EVM_IMA_XATTR_DIGSIG_ASYMMETRIC)
+			mlock_mappings = true;
+	}
+
+	if (mlock_mappings)
+		current->mm->def_flags |= VM_LOCKED;
+
+#endif
 	/* Do this immediately, since STACK_TOP as used in setup_arg_pages
 	   may depend on the personality.  */
 	SET_PERSONALITY(loc->elf_ex);
@@ -893,6 +922,18 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		goto out_free_dentry;
 	}
 
+#ifdef CONFIG_BINFMT_ELF_SIG
+	if (mlock_mappings) {
+		retval = ima_appraise_file(bprm->file, signature, siglen);
+		if (retval) {
+			send_sig(SIGKILL, current, 0);
+			goto out_free_dentry;
+		}
+		/* Signature verification successful */
+		cap_raise(bprm->cred->cap_effective, CAP_SIGNED);
+	}
+#endif
+
 	if (elf_interpreter) {
 		unsigned long interp_map_addr = 0;
 
@@ -986,11 +1027,14 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	 */
 	ELF_PLAT_INIT(regs, reloc_func_desc);
 #endif
+	if (mlock_mappings)
+		current->mm->def_flags &= ~VM_LOCKED;
 
 	start_thread(regs, elf_entry, bprm->p);
 	retval = 0;
 out:
 	kfree(loc);
+	kfree(signature);
 out_ret:
 	return retval;
 
-- 
1.7.7.6

Re: [PATCH 4/4] binfmt_elf: Elf executable signature verification

[Josh Boyer]

On Fri, Mar 15, 2013 at 4:35 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Do elf executable signature verification (if one is present). If signature
> is present, it should be valid. Validly signed files are given a capability
> CAP_SIGNED.
>
> If file is unsigned, it can execute but it does not get the capability
> CAP_SIGNED.
>
> This is work in progress. This patch is just an RFC to show how one
> can go about making use of IMA APIs for executable signature
> verification.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/Kconfig.binfmt |   12 ++++++++++++
>  fs/binfmt_elf.c   |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+), 0 deletions(-)
>
> diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
> index 0efd152..cbb1d4a 100644
> --- a/fs/Kconfig.binfmt
> +++ b/fs/Kconfig.binfmt
> @@ -23,6 +23,18 @@ config BINFMT_ELF
>           ld.so (check the file <file:Documentation/Changes> for location and
>           latest version).
>
> +config BINFMT_ELF_SIG
> +       bool "ELF binary signature verification"
> +       depends on BINFMT_ELF
> +       select INTEGRITY
> +       select INTEGRITY_SIGNATURE
> +       select INTEGRITY_ASYMMETRIC_KEYS
> +       select IMA
> +       select IMA_APPRAISE
> +       default n
> +       ---help---
> +         Check ELF binary signature verfication.
> +

I haven't reviewed the whole patch set, but this caught my eye.  There
are a couple things wrong with it.

1) The help text isn't helpful.  It could definitely be more verbose and
should probably point to something in Documentation/ that describes what
this whole thing is.

2) The select mechanism is horrible.  I would really like to see this
option use "depends on" instead of select given that you're selecting in
a whole subsystem that people probably aren't going to have already
enabled.

josh

Re: [PATCH 4/4] binfmt_elf: Elf executable signature verification

[Vivek Goyal]

On Mon, Mar 18, 2013 at 04:23:11PM -0400, Josh Boyer wrote:
> On Fri, Mar 15, 2013 at 4:35 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Do elf executable signature verification (if one is present). If signature
> > is present, it should be valid. Validly signed files are given a capability
> > CAP_SIGNED.
> >
> > If file is unsigned, it can execute but it does not get the capability
> > CAP_SIGNED.
> >
> > This is work in progress. This patch is just an RFC to show how one
> > can go about making use of IMA APIs for executable signature
> > verification.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/Kconfig.binfmt |   12 ++++++++++++
> >  fs/binfmt_elf.c   |   44 ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 56 insertions(+), 0 deletions(-)
> >
> > diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
> > index 0efd152..cbb1d4a 100644
> > --- a/fs/Kconfig.binfmt
> > +++ b/fs/Kconfig.binfmt
> > @@ -23,6 +23,18 @@ config BINFMT_ELF
> >           ld.so (check the file <file:Documentation/Changes> for location and
> >           latest version).
> >
> > +config BINFMT_ELF_SIG
> > +       bool "ELF binary signature verification"
> > +       depends on BINFMT_ELF
> > +       select INTEGRITY
> > +       select INTEGRITY_SIGNATURE
> > +       select INTEGRITY_ASYMMETRIC_KEYS
> > +       select IMA
> > +       select IMA_APPRAISE
> > +       default n
> > +       ---help---
> > +         Check ELF binary signature verfication.
> > +
> 
> I haven't reviewed the whole patch set, but this caught my eye.  There
> are a couple things wrong with it.
> 
> 1) The help text isn't helpful.  It could definitely be more verbose and
> should probably point to something in Documentation/ that describes what
> this whole thing is.

Sure, I will fix that. Actually this posting was more for getting the
IMA interfaces sorted out and just wanted to quickly show how new
interfaces will be used in ELF code.

> 
> 2) The select mechanism is horrible.  I would really like to see this
> option use "depends on" instead of select given that you're selecting in
> a whole subsystem that people probably aren't going to have already
>  enabled.

I like "select" better in this context. If you want this feature, then you
need to select a bunch of other features which feature depends on.
Otherwise it is a configuration nightmare. How does one know what are
different parts which need to be enabled before elf binary signature
verification options becomes visible.

And it is very similar to module signing. It selects bunch of options
when user wants to enable modules signing (instead of depending on these
options).


config MODULE_SIG
        bool "Module signature verification"
        depends on MODULES
        select KEYS
        select CRYPTO
        select ASYMMETRIC_KEY_TYPE
        select ASYMMETRIC_PUBLIC_KEY_SUBTYPE
        select PUBLIC_KEY_ALGO_RSA
        select ASN1
        select OID_REGISTRY
        select X509_CERTIFICATE_PARSER

Thanks
Vivek

Re: [PATCH 4/4] binfmt_elf: Elf executable signature verification

[Mimi Zohar]

On Fri, 2013-03-15 at 16:35 -0400, Vivek Goyal wrote:
> Do elf executable signature verification (if one is present). If signature
> is present, it should be valid. Validly signed files are given a capability
> CAP_SIGNED.
> 
> If file is unsigned, it can execute but it does not get the capability
> CAP_SIGNED.
> 
> This is work in progress. This patch is just an RFC to show how one
> can go about making use of IMA APIs for executable signature
> verification.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/Kconfig.binfmt |   12 ++++++++++++
>  fs/binfmt_elf.c   |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
> index 0efd152..cbb1d4a 100644
> --- a/fs/Kconfig.binfmt
> +++ b/fs/Kconfig.binfmt
> @@ -23,6 +23,18 @@ config BINFMT_ELF
>  	  ld.so (check the file <file:Documentation/Changes> for location and
>  	  latest version).
> 
> +config BINFMT_ELF_SIG
> +	bool "ELF binary signature verification"
> +	depends on BINFMT_ELF
> +	select INTEGRITY
> +	select INTEGRITY_SIGNATURE
> +	select INTEGRITY_ASYMMETRIC_KEYS
> +	select IMA
> +	select IMA_APPRAISE
> +	default n
> +	---help---
> +	  Check ELF binary signature verfication.
> +
>  config COMPAT_BINFMT_ELF
>  	bool
>  	depends on COMPAT && BINFMT_ELF
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 3939829..820ceb9 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -34,6 +34,7 @@
>  #include <linux/utsname.h>
>  #include <linux/coredump.h>
>  #include <linux/sched.h>
> +#include <linux/ima.h>
>  #include <asm/uaccess.h>
>  #include <asm/param.h>
>  #include <asm/page.h>
> @@ -581,6 +582,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  	int executable_stack = EXSTACK_DEFAULT;
>  	unsigned long def_flags = 0;
>  	struct pt_regs *regs = current_pt_regs();
> +	char *signature = NULL;
> +	unsigned int siglen = 0;
> +	enum evm_ima_xattr_type sig_type;
> +	bool mlock_mappings = false;
> +
>  	struct {
>  		struct elfhdr elf_ex;
>  		struct elfhdr interp_elf_ex;
> @@ -722,6 +728,29 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  	/* OK, This is the point of no return */
>  	current->mm->def_flags = def_flags;
> 
> +#ifdef CONFIG_BINFMT_ELF_SIG
> +	/* If executable is digitally signed. Lock down in memory */
> +	/* Get file signature, if any */
> +	retval = ima_file_signature_alloc(bprm->file, &signature);
> +
> +	/*
> +	 * If there is an error getting signature, bail out. Having
> +	 * no signature is fine though.
> +	 */
> +	if (retval < 0 && retval != -ENODATA && retval != -EOPNOTSUPP)
> +		goto out_free_dentry;
> +
> +	if (signature != NULL) {
> +		siglen = retval;
> +		retval = ima_signature_type(signature, &sig_type);
> +		if (!retval && sig_type == EVM_IMA_XATTR_DIGSIG_ASYMMETRIC)
> +			mlock_mappings = true;
> +	}
> +
> +	if (mlock_mappings)
> +		current->mm->def_flags |= VM_LOCKED;

Vivek, we've already discussed this.  Hard coding policy in the kernel
is unacceptable, whether it is inlined, here, or as part of IMA.
Defining a policy, that mlocks all signed ELF executables, does not
scale.  The 'ima_appraise_tcb' policy requires all files owned by root
to be signed.  Please define some other mechanism, other than a
signature, for identifying files that you want to mlock.
(Recommendations were previously made, which you rejected.)

Lastly, adding 'VM_LOCKED' here seems to change existing, expected
behavior.  According to the mlock(2) man pages, "Memory locks are not
inherited by a child created via fork(2) and are automatically removed
(unlocked) during an execve(2) or when the process terminates."  Someone
else needs to comment on this sort of change.  Andrew?  Al?

thanks,

Mimi

> +
> +#endif
>  	/* Do this immediately, since STACK_TOP as used in setup_arg_pages
>  	   may depend on the personality.  */
>  	SET_PERSONALITY(loc->elf_ex);
> @@ -893,6 +922,18 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  		goto out_free_dentry;
>  	}
> 
> +#ifdef CONFIG_BINFMT_ELF_SIG
> +	if (mlock_mappings) {
> +		retval = ima_appraise_file(bprm->file, signature, siglen);
> +		if (retval) {
> +			send_sig(SIGKILL, current, 0);
> +			goto out_free_dentry;
> +		}
> +		/* Signature verification successful */
> +		cap_raise(bprm->cred->cap_effective, CAP_SIGNED);
> +	}
> +#endif
> +
>  	if (elf_interpreter) {
>  		unsigned long interp_map_addr = 0;
> 
> @@ -986,11 +1027,14 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  	 */
>  	ELF_PLAT_INIT(regs, reloc_func_desc);
>  #endif
> +	if (mlock_mappings)
> +		current->mm->def_flags &= ~VM_LOCKED;
> 
>  	start_thread(regs, elf_entry, bprm->p);
>  	retval = 0;
>  out:
>  	kfree(loc);
> +	kfree(signature);
>  out_ret:
>  	return retval;
> 

Re: [PATCH 4/4] binfmt_elf: Elf executable signature verification

[Vivek Goyal]

On Tue, Mar 19, 2013 at 10:39:01AM -0400, Mimi Zohar wrote:

[..]
> > +#ifdef CONFIG_BINFMT_ELF_SIG
> > +	/* If executable is digitally signed. Lock down in memory */
> > +	/* Get file signature, if any */
> > +	retval = ima_file_signature_alloc(bprm->file, &signature);
> > +
> > +	/*
> > +	 * If there is an error getting signature, bail out. Having
> > +	 * no signature is fine though.
> > +	 */
> > +	if (retval < 0 && retval != -ENODATA && retval != -EOPNOTSUPP)
> > +		goto out_free_dentry;
> > +
> > +	if (signature != NULL) {
> > +		siglen = retval;
> > +		retval = ima_signature_type(signature, &sig_type);
> > +		if (!retval && sig_type == EVM_IMA_XATTR_DIGSIG_ASYMMETRIC)
> > +			mlock_mappings = true;
> > +	}
> > +
> > +	if (mlock_mappings)
> > +		current->mm->def_flags |= VM_LOCKED;
> 
> Vivek, we've already discussed this.  Hard coding policy in the kernel
> is unacceptable, whether it is inlined, here, or as part of IMA.
> Defining a policy, that mlocks all signed ELF executables, does not
> scale.  The 'ima_appraise_tcb' policy requires all files owned by root
> to be signed.  Please define some other mechanism, other than a
> signature, for identifying files that you want to mlock.
> (Recommendations were previously made, which you rejected.)

Hi Mimi,

How about just just defining another config option CONFIG_BINFMT_MEMLOCK_SIGNED.

So CONFIG_BINFMT_ELF_SIG takes care of signature verification and
CONFIG_BINFMT_MEMLOCK_SIGNED will determine whether executable is locked
in memory or not.

If I define any command line options to enable/disable it, then root can
easily bypass this. And that's the problem I am trying to solve. In
secureboot environment we don't trust root.

And what's the use case for some of the signed executables locked but
not others. I am able to visualize only two states. Either we lock down
every signed process  or we don't lock anything in (Assuming we have
signed all the user space and no unsigned process can execute now so
hopefully it is safe to not lock down process memory).

That's why I think it is not per file attribute. It is in general system
attribute whether on this system signed files should be locked or not. And
given the fact we don't trust root in secureboot environment, we can't
define external mechanism to trigger policy and it has to be built-in.

So it is not same as ima_appraise_tcb as there you trust root. Even if you
don't there are ways to detect that things are not right (by remote
attestation). But in case of secureboot no such mechanism is there. So one
can not trust root to configure a policy.

> 
> Lastly, adding 'VM_LOCKED' here seems to change existing, expected
> behavior.  According to the mlock(2) man pages, "Memory locks are not
> inherited by a child created via fork(2) and are automatically removed
> (unlocked) during an execve(2) or when the process terminates."  Someone
> else needs to comment on this sort of change.  Andrew?  Al?

I will read more about it. I know that some more work is needed here. For
example, one can say munlock()/munlockall() on already locked pages. I
was thinkingof defining a new flag say VM_LOCKED_PERM. So any pages which
have been locked by kernel are permanent and can not be unlocked by user
space until and unless process exits.

I just need to spend more time in this memory locking area to cover all
the corner cases and make sure kernel mlocked pages are not unlocked.

Thanks
Vivek

Re: [PATCH 4/4] binfmt_elf: Elf executable signature verification

[Mimi Zohar]

On Wed, 2013-03-20 at 11:21 -0400, Vivek Goyal wrote:
> On Tue, Mar 19, 2013 at 10:39:01AM -0400, Mimi Zohar wrote:
> 
> [..]
> > > +#ifdef CONFIG_BINFMT_ELF_SIG
> > > +	/* If executable is digitally signed. Lock down in memory */
> > > +	/* Get file signature, if any */
> > > +	retval = ima_file_signature_alloc(bprm->file, &signature);
> > > +
> > > +	/*
> > > +	 * If there is an error getting signature, bail out. Having
> > > +	 * no signature is fine though.
> > > +	 */
> > > +	if (retval < 0 && retval != -ENODATA && retval != -EOPNOTSUPP)
> > > +		goto out_free_dentry;
> > > +
> > > +	if (signature != NULL) {
> > > +		siglen = retval;
> > > +		retval = ima_signature_type(signature, &sig_type);
> > > +		if (!retval && sig_type == EVM_IMA_XATTR_DIGSIG_ASYMMETRIC)
> > > +			mlock_mappings = true;
> > > +	}
> > > +
> > > +	if (mlock_mappings)
> > > +		current->mm->def_flags |= VM_LOCKED;
> > 
> > Vivek, we've already discussed this.  Hard coding policy in the kernel
> > is unacceptable, whether it is inlined, here, or as part of IMA.
> > Defining a policy, that mlocks all signed ELF executables, does not
> > scale.  The 'ima_appraise_tcb' policy requires all files owned by root
> > to be signed.  Please define some other mechanism, other than a
> > signature, for identifying files that you want to mlock.
> > (Recommendations were previously made, which you rejected.)
> 
> Hi Mimi,
> 
> How about just just defining another config option CONFIG_BINFMT_MEMLOCK_SIGNED.
> 
> So CONFIG_BINFMT_ELF_SIG takes care of signature verification and
> CONFIG_BINFMT_MEMLOCK_SIGNED will determine whether executable is locked
> in memory or not.
> 
> If I define any command line options to enable/disable it, then root can
> easily bypass this. And that's the problem I am trying to solve. In
> secureboot environment we don't trust root.
> 
> And what's the use case for some of the signed executables locked but
> not others. I am able to visualize only two states. Either we lock down
> every signed process  or we don't lock anything in (Assuming we have
> signed all the user space and no unsigned process can execute now so
> hopefully it is safe to not lock down process memory).
> 
> That's why I think it is not per file attribute. It is in general system
> attribute whether on this system signed files should be locked or not. And
> given the fact we don't trust root in secureboot environment, we can't
> define external mechanism to trigger policy and it has to be built-in.
> 
> So it is not same as ima_appraise_tcb as there you trust root. Even if you
> don't there are ways to detect that things are not right (by remote
> attestation). But in case of secureboot no such mechanism is there. So one
> can not trust root to configure a policy.

Defining another Kconfig option will either memlock all signed
executables or none.  If a distro ships with this new Kconfig enabled,
then the 'ima_appraise_tcb' boot command line option would result in all
executables, owned by root, being memlocked.  Giving of privileges based
on the existence of a signature, does not scale.  Again, do not hard
code policy in the kernel.

> > 
> > Lastly, adding 'VM_LOCKED' here seems to change existing, expected
> > behavior.  According to the mlock(2) man pages, "Memory locks are not
> > inherited by a child created via fork(2) and are automatically removed
> > (unlocked) during an execve(2) or when the process terminates."  Someone
> > else needs to comment on this sort of change.  Andrew?  Al?
> 
> I will read more about it. I know that some more work is needed here. For
> example, one can say munlock()/munlockall() on already locked pages. I
> was thinkingof defining a new flag say VM_LOCKED_PERM. So any pages which
> have been locked by kernel are permanent and can not be unlocked by user
> space until and unless process exits.
> 
> I just need to spend more time in this memory locking area to cover all
> the corner cases and make sure kernel mlocked pages are not unlocked.

Vivek, there must be a good reason that execve intentionally,
automatically unlocks the memory.   Are there any interrupt or locking
implications?  I'm not qualified to answer any of these questions.  As
the entire patchset is based on using VM_LOCKED, before we continue any
further discussions, these basic questions need to be answered first.

thanks,

Mimi

Re: [PATCH 4/4] binfmt_elf: Elf executable signature verification

[Vivek Goyal]

On Wed, Mar 20, 2013 at 01:41:30PM -0400, Mimi Zohar wrote:

[..]
> Defining another Kconfig option will either memlock all signed
> executables or none.  If a distro ships with this new Kconfig enabled,
> then the 'ima_appraise_tcb' boot command line option would result in all
> executables, owned by root, being memlocked.

ima_appraise_tcb and elf signing + memlock are offering two
different features. ima_appraise_tcb is offering only disk integrity
of file while elf signing + memlock is offering in memory integrity
of executable file (even if there are root unsigned executables).

So distributrions which support ima_appraise_tcb, can ship with elf
signing and memlock feature disabled. (assuming they don't need 
in memory integrity of executable).

If they do need in-memory integrity of executable (like booting on
secureboot platform), then they do need to enable this feautre and
live with the cost because they do need this feature.

I think one area of optimization is more interesting and that is what
if those distributions are booting on a platform with secureboot disabled.
How about defining a command line option to disable memlock behavior
(only when secureboot is disabled).

> Giving of privileges based
> on the existence of a signature, does not scale.

If an application is signed by right keys, one should be able to
trust it and give priviliges. Agreed locking down executable in memory
inhibits scalability. But in general we don't want to sign whole of the
user space. And for signing selected executables this seems to be only
option (to make things foolproof).

Do you have other ideas on how to achieve in-memory integrity of file without
locking down executable in a selectively signed user space.

> Again, do not hard
> code policy in the kernel.

Do you have suggestions on how to do this while we don't trust root.

Given the fact that it is a config option and possibly a command line
option to disable it, it is not exactly hard-coded.

As per this argument, signature verification can be called though of a as a
policy too and should be configurable.  Then all the module signature code
should be modified to enable module signature only if root configures so
using a command line option. I am sure I can find other things which can be
called policy but are in kernel. I think we also need to look at what is
easy to use and what makes sense given the constraints.

> 
> > > 
> > > Lastly, adding 'VM_LOCKED' here seems to change existing, expected
> > > behavior.  According to the mlock(2) man pages, "Memory locks are not
> > > inherited by a child created via fork(2) and are automatically removed
> > > (unlocked) during an execve(2) or when the process terminates."  Someone
> > > else needs to comment on this sort of change.  Andrew?  Al?
> > 
> > I will read more about it. I know that some more work is needed here. For
> > example, one can say munlock()/munlockall() on already locked pages. I
> > was thinkingof defining a new flag say VM_LOCKED_PERM. So any pages which
> > have been locked by kernel are permanent and can not be unlocked by user
> > space until and unless process exits.
> > 
> > I just need to spend more time in this memory locking area to cover all
> > the corner cases and make sure kernel mlocked pages are not unlocked.
> 
> Vivek, there must be a good reason that execve intentionally,
> automatically unlocks the memory. 

Once a process has called execve, it gets new vma and has no
correlation to old vmas and its locks. So it think it makes sense that
newly setup vmas don't have any locks. But again, I am not an expert
in that area either. So if an expert can provide some insights here, that
would help a lot.

Thanks
Vivek

Re: [PATCH 4/4] binfmt_elf: Elf executable signature verification

[Vivek Goyal]

On Tue, Mar 19, 2013 at 10:39:01AM -0400, Mimi Zohar wrote:

[..]
> 
> Lastly, adding 'VM_LOCKED' here seems to change existing, expected
> behavior.  According to the mlock(2) man pages, "Memory locks are not
> inherited by a child created via fork(2) and are automatically removed
> (unlocked) during an execve(2) or when the process terminates."  Someone
> else needs to comment on this sort of change.  Andrew?  Al?

I think removing locks during execve() makes sense. New executable will
get its own locked memory and it is not dependent on memory areas locked
before execve().

fork() is more interesting though. I guess we could just reset the
"signed" bit of forked process. So it does not inherit it from parent. And
when forked process does exec() it will lock its own memory areas and
get "signed" bit if signatuer verification was successful.

So looks like exeisting memory lock behavior on fork()/execve() will be
fine.

Thanks
Vivek