diff mbox series

[RFC,1/2] ima: Implement support for uncompressed module appended signatures

Message ID 20200206164226.24875-2-eric.snowberg@oracle.com (mailing list archive)
State New, archived
Headers show
Series ima: uncompressed module appraisal support | expand

Commit Message

Eric Snowberg Feb. 6, 2020, 4:42 p.m. UTC 
Currently IMA can validate compressed modules containing appended
signatures.  This adds the ability to also validate uncompressed
modules when appraise_type=imasig|modsig.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 security/integrity/digsig.c           | 9 +++++++--
 security/integrity/ima/ima_appraise.c | 3 +++
 security/integrity/integrity.h        | 3 ++-
 3 files changed, 12 insertions(+), 3 deletions(-)

Comments

Lakshmi Ramasubramanian Feb. 6, 2020, 5:07 p.m. UTC | #1 
On 2/6/2020 8:42 AM, Eric Snowberg wrote:

>   
> @@ -31,6 +32,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
>   	".ima",
>   #endif
>   	".platform",
> +	".builtin_trusted_keys",
>   };
>   
>   #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> @@ -45,8 +47,11 @@ static struct key *integrity_keyring_from_id(const unsigned int id)
>   		return ERR_PTR(-EINVAL);
>   
>   	if (!keyring[id]) {
> -		keyring[id] =
> -			request_key(&key_type_keyring, keyring_name[id], NULL);
> +		if (id == INTEGRITY_KEYRING_KERNEL)
> +			keyring[id] = VERIFY_USE_SECONDARY_KEYRING;

Since "Built-In Trusted Keyring" or "Secondary Trusted Keyring" is used, 
would it be more appropriate to name this identifier 
INTEGRITY_KEYRING_BUILTIN_OR_SECONDARY?

> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 73fc286834d7..63f0e6bff0e0 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -145,7 +145,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
>   #define INTEGRITY_KEYRING_EVM		0
>   #define INTEGRITY_KEYRING_IMA		1
>   #define INTEGRITY_KEYRING_PLATFORM	2
> -#define INTEGRITY_KEYRING_MAX		3
> +#define INTEGRITY_KEYRING_KERNEL	3
> +#define INTEGRITY_KEYRING_MAX		4


  -lakshmi
Eric Snowberg Feb. 6, 2020, 5:30 p.m. UTC | #2 
> On Feb 6, 2020, at 10:07 AM, Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote:
> 
> On 2/6/2020 8:42 AM, Eric Snowberg wrote:
> 
>>  @@ -31,6 +32,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
>>  	".ima",
>>  #endif
>>  	".platform",
>> +	".builtin_trusted_keys",
>>  };
>>    #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
>> @@ -45,8 +47,11 @@ static struct key *integrity_keyring_from_id(const unsigned int id)
>>  		return ERR_PTR(-EINVAL);
>>    	if (!keyring[id]) {
>> -		keyring[id] =
>> -			request_key(&key_type_keyring, keyring_name[id], NULL);
>> +		if (id == INTEGRITY_KEYRING_KERNEL)
>> +			keyring[id] = VERIFY_USE_SECONDARY_KEYRING;
> 
> Since "Built-In Trusted Keyring" or "Secondary Trusted Keyring" is used, would it be more appropriate to name this identifier INTEGRITY_KEYRING_BUILTIN_OR_SECONDARY?

I’m open to changing INTEGRITY_KEYRING_KERNEL to INTEGRITY_KEYRING_BUILTIN_OR_SECONDARY if that seems more appropriate.
Mimi Zohar Feb. 6, 2020, 6:05 p.m. UTC | #3 
Hi Eric,

On Thu, 2020-02-06 at 11:42 -0500, Eric Snowberg wrote:
> Currently IMA can validate compressed modules containing appended
> signatures.  This adds the ability to also validate uncompressed
> modules when appraise_type=imasig|modsig.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

Your patch description in no way matches the code.

Mimi

> ---
>  security/integrity/digsig.c           | 9 +++++++--
>  security/integrity/ima/ima_appraise.c | 3 +++
>  security/integrity/integrity.h        | 3 ++-
>  3 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index ea1aae3d07b3..5e0c4d04ab9d 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -15,6 +15,7 @@
>  #include <linux/key-type.h>
>  #include <linux/digsig.h>
>  #include <linux/vmalloc.h>
> +#include <linux/verification.h>
>  #include <crypto/public_key.h>
>  #include <keys/system_keyring.h>
>  
> @@ -31,6 +32,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
>  	".ima",
>  #endif
>  	".platform",
> +	".builtin_trusted_keys",
>  };
>  
>  #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> @@ -45,8 +47,11 @@ static struct key *integrity_keyring_from_id(const unsigned int id)
>  		return ERR_PTR(-EINVAL);
>  
>  	if (!keyring[id]) {
> -		keyring[id] =
> -			request_key(&key_type_keyring, keyring_name[id], NULL);
> +		if (id == INTEGRITY_KEYRING_KERNEL)
> +			keyring[id] = VERIFY_USE_SECONDARY_KEYRING;
> +		else
> +			keyring[id] = request_key(&key_type_keyring,
> +						  keyring_name[id], NULL);
>  		if (IS_ERR(keyring[id])) {
>  			int err = PTR_ERR(keyring[id]);
>  			pr_err("no %s keyring: %d\n", keyring_name[id], err);
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 300c8d2943c5..4c009c55d620 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -294,6 +294,9 @@ static int modsig_verify(enum ima_hooks func, const struct modsig *modsig,
>  	    func == KEXEC_KERNEL_CHECK)
>  		rc = integrity_modsig_verify(INTEGRITY_KEYRING_PLATFORM,
>  					     modsig);
> +	if (rc && func == MODULE_CHECK)
> +		rc = integrity_modsig_verify(INTEGRITY_KEYRING_KERNEL, modsig);
> +
>  	if (rc) {
>  		*cause = "invalid-signature";
>  		*status = INTEGRITY_FAIL;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 73fc286834d7..63f0e6bff0e0 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -145,7 +145,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
>  #define INTEGRITY_KEYRING_EVM		0
>  #define INTEGRITY_KEYRING_IMA		1
>  #define INTEGRITY_KEYRING_PLATFORM	2
> -#define INTEGRITY_KEYRING_MAX		3
> +#define INTEGRITY_KEYRING_KERNEL	3
> +#define INTEGRITY_KEYRING_MAX		4
>  
>  extern struct dentry *integrity_dir;
>
Eric Snowberg Feb. 6, 2020, 7:01 p.m. UTC | #4 
> On Feb 6, 2020, at 11:05 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Thu, 2020-02-06 at 11:42 -0500, Eric Snowberg wrote:
>> Currently IMA can validate compressed modules containing appended
>> signatures.  This adds the ability to also validate uncompressed
>> modules when appraise_type=imasig|modsig.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> 
> Your patch description in no way matches the code.
> 

How about if I changed the description to the following:

Currently IMA can only validate compressed modules containing appended
signatures when appraise_type=imasig|modsig.  An uncompressed module that 
is internally signed must still be ima signed.  

Add the ability to validate the uncompress module by validating it against
keys contained within the .builtin_trusted_keys keyring. Now when using a
policy such as:

appraise func=MODULE_CHECK appraise_type=imasig|modsig

It will load modules containing an appended signature when either compressed
or uncompressed.


>> ---
>> security/integrity/digsig.c           | 9 +++++++--
>> security/integrity/ima/ima_appraise.c | 3 +++
>> security/integrity/integrity.h        | 3 ++-
>> 3 files changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
>> index ea1aae3d07b3..5e0c4d04ab9d 100644
>> --- a/security/integrity/digsig.c
>> +++ b/security/integrity/digsig.c
>> @@ -15,6 +15,7 @@
>> #include <linux/key-type.h>
>> #include <linux/digsig.h>
>> #include <linux/vmalloc.h>
>> +#include <linux/verification.h>
>> #include <crypto/public_key.h>
>> #include <keys/system_keyring.h>
>> 
>> @@ -31,6 +32,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
>> 	".ima",
>> #endif
>> 	".platform",
>> +	".builtin_trusted_keys",
>> };
>> 
>> #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
>> @@ -45,8 +47,11 @@ static struct key *integrity_keyring_from_id(const unsigned int id)
>> 		return ERR_PTR(-EINVAL);
>> 
>> 	if (!keyring[id]) {
>> -		keyring[id] =
>> -			request_key(&key_type_keyring, keyring_name[id], NULL);
>> +		if (id == INTEGRITY_KEYRING_KERNEL)
>> +			keyring[id] = VERIFY_USE_SECONDARY_KEYRING;
>> +		else
>> +			keyring[id] = request_key(&key_type_keyring,
>> +						  keyring_name[id], NULL);
>> 		if (IS_ERR(keyring[id])) {
>> 			int err = PTR_ERR(keyring[id]);
>> 			pr_err("no %s keyring: %d\n", keyring_name[id], err);
>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>> index 300c8d2943c5..4c009c55d620 100644
>> --- a/security/integrity/ima/ima_appraise.c
>> +++ b/security/integrity/ima/ima_appraise.c
>> @@ -294,6 +294,9 @@ static int modsig_verify(enum ima_hooks func, const struct modsig *modsig,
>> 	    func == KEXEC_KERNEL_CHECK)
>> 		rc = integrity_modsig_verify(INTEGRITY_KEYRING_PLATFORM,
>> 					     modsig);
>> +	if (rc && func == MODULE_CHECK)
>> +		rc = integrity_modsig_verify(INTEGRITY_KEYRING_KERNEL, modsig);
>> +
>> 	if (rc) {
>> 		*cause = "invalid-signature";
>> 		*status = INTEGRITY_FAIL;
>> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
>> index 73fc286834d7..63f0e6bff0e0 100644
>> --- a/security/integrity/integrity.h
>> +++ b/security/integrity/integrity.h
>> @@ -145,7 +145,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
>> #define INTEGRITY_KEYRING_EVM		0
>> #define INTEGRITY_KEYRING_IMA		1
>> #define INTEGRITY_KEYRING_PLATFORM	2
>> -#define INTEGRITY_KEYRING_MAX		3
>> +#define INTEGRITY_KEYRING_KERNEL	3
>> +#define INTEGRITY_KEYRING_MAX		4
>> 
>> extern struct dentry *integrity_dir;
>> 
>
Mimi Zohar Feb. 6, 2020, 7:10 p.m. UTC | #5 
On Thu, 2020-02-06 at 12:01 -0700, Eric Snowberg wrote:
> > On Feb 6, 2020, at 11:05 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Thu, 2020-02-06 at 11:42 -0500, Eric Snowberg wrote:
> >> Currently IMA can validate compressed modules containing appended
> >> signatures.  This adds the ability to also validate uncompressed
> >> modules when appraise_type=imasig|modsig.
> >> 
> >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> > 
> > Your patch description in no way matches the code.
> > 
> 
> How about if I changed the description to the following:
> 
> Currently IMA can only validate compressed modules containing appended
> signatures when appraise_type=imasig|modsig.  An uncompressed module that 
> is internally signed must still be ima signed.  
> 
> Add the ability to validate the uncompress module by validating it against
> keys contained within the .builtin_trusted_keys keyring. Now when using a
> policy such as:
> 
> appraise func=MODULE_CHECK appraise_type=imasig|modsig
> 
> It will load modules containing an appended signature when either compressed
> or uncompressed.

We - Nayna and I - will be commenting on the cover letter shortly.  I
think that will help clarify the problem(s).

Mimi
diff mbox series

Patch

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index ea1aae3d07b3..5e0c4d04ab9d 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -15,6 +15,7 @@ 
 #include <linux/key-type.h>
 #include <linux/digsig.h>
 #include <linux/vmalloc.h>
+#include <linux/verification.h>
 #include <crypto/public_key.h>
 #include <keys/system_keyring.h>
 
@@ -31,6 +32,7 @@  static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
 	".ima",
 #endif
 	".platform",
+	".builtin_trusted_keys",
 };
 
 #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
@@ -45,8 +47,11 @@  static struct key *integrity_keyring_from_id(const unsigned int id)
 		return ERR_PTR(-EINVAL);
 
 	if (!keyring[id]) {
-		keyring[id] =
-			request_key(&key_type_keyring, keyring_name[id], NULL);
+		if (id == INTEGRITY_KEYRING_KERNEL)
+			keyring[id] = VERIFY_USE_SECONDARY_KEYRING;
+		else
+			keyring[id] = request_key(&key_type_keyring,
+						  keyring_name[id], NULL);
 		if (IS_ERR(keyring[id])) {
 			int err = PTR_ERR(keyring[id]);
 			pr_err("no %s keyring: %d\n", keyring_name[id], err);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 300c8d2943c5..4c009c55d620 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -294,6 +294,9 @@  static int modsig_verify(enum ima_hooks func, const struct modsig *modsig,
 	    func == KEXEC_KERNEL_CHECK)
 		rc = integrity_modsig_verify(INTEGRITY_KEYRING_PLATFORM,
 					     modsig);
+	if (rc && func == MODULE_CHECK)
+		rc = integrity_modsig_verify(INTEGRITY_KEYRING_KERNEL, modsig);
+
 	if (rc) {
 		*cause = "invalid-signature";
 		*status = INTEGRITY_FAIL;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 73fc286834d7..63f0e6bff0e0 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -145,7 +145,8 @@  int integrity_kernel_read(struct file *file, loff_t offset,
 #define INTEGRITY_KEYRING_EVM		0
 #define INTEGRITY_KEYRING_IMA		1
 #define INTEGRITY_KEYRING_PLATFORM	2
-#define INTEGRITY_KEYRING_MAX		3
+#define INTEGRITY_KEYRING_KERNEL	3
+#define INTEGRITY_KEYRING_MAX		4
 
 extern struct dentry *integrity_dir;