[PATCH v2 0/4] integrity: improve user feedback for invalid bootparams

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH v2 0/4] integrity: improve user feedback for invalid bootparams
@ 2020-09-04 19:40 Bruno Meneguele
  2020-09-04 19:40 ` [PATCH v2 1/4] ima: add check for enforced appraise option Bruno Meneguele
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Bruno Meneguele @ 2020-09-04 19:40 UTC (permalink / raw)
  To: linux-integrity, linux-kernel; +Cc: zohar, Bruno Meneguele

Some boot paramenters under integrity/ don't report any feedback to the user
in case an invalid/unknown option is passed. With this patch, try to be more
informative about what went wrong, including a more strict secure boot
feedback.

Bruno Meneguele (4):
  ima: add check for enforced appraise option
  integrity: invalid kernel parameters feedback
  ima: limit secure boot feedback scope for appraise
  integrity: prompt keyring name for unknown key request

 security/integrity/digsig_asymmetric.c | 10 ++++++++--
 security/integrity/evm/evm_main.c      |  3 +++
 security/integrity/ima/ima_appraise.c  | 27 ++++++++++++++++++--------
 security/integrity/ima/ima_main.c      | 13 +++++++++----
 security/integrity/ima/ima_policy.c    |  2 ++
 5 files changed, 41 insertions(+), 14 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/4] ima: add check for enforced appraise option
  2020-09-04 19:40 [PATCH v2 0/4] integrity: improve user feedback for invalid bootparams Bruno Meneguele
@ 2020-09-04 19:40 ` Bruno Meneguele
  2020-09-04 19:40 ` [PATCH v2 2/4] integrity: invalid kernel parameters feedback Bruno Meneguele
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Bruno Meneguele @ 2020-09-04 19:40 UTC (permalink / raw)
  To: linux-integrity, linux-kernel; +Cc: zohar, Bruno Meneguele

The "enforce" string is allowed as an option for ima_appraise= kernel
paramenter per kernel-paramenters.txt and should be considered on the
parameter setup checking as a matter of completeness. Also it allows futher
checking on the options being passed by the user.

Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
---
 security/integrity/ima/ima_appraise.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 372d16382960..580b771e3458 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -31,6 +31,8 @@ static int __init default_appraise_setup(char *str)
 		ima_appraise = IMA_APPRAISE_LOG;
 	else if (strncmp(str, "fix", 3) == 0)
 		ima_appraise = IMA_APPRAISE_FIX;
+	else if (strncmp(str, "enforce", 7) == 0)
+		ima_appraise = IMA_APPRAISE_ENFORCE;
 #endif
 	return 1;
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/4] integrity: invalid kernel parameters feedback
  2020-09-04 19:40 [PATCH v2 0/4] integrity: improve user feedback for invalid bootparams Bruno Meneguele
  2020-09-04 19:40 ` [PATCH v2 1/4] ima: add check for enforced appraise option Bruno Meneguele
@ 2020-09-04 19:40 ` Bruno Meneguele
  2020-09-04 19:40 ` [PATCH v2 3/4] ima: limit secure boot feedback scope for appraise Bruno Meneguele
  2020-09-04 19:41 ` [PATCH v2 4/4] integrity: prompt keyring name for unknown key request Bruno Meneguele
  3 siblings, 0 replies; 10+ messages in thread
From: Bruno Meneguele @ 2020-09-04 19:40 UTC (permalink / raw)
  To: linux-integrity, linux-kernel; +Cc: zohar, Bruno Meneguele

Don't silently ignore unknown or invalid ima_{policy,appraise,hash} and evm
kernel boot command line options.

Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
---
Changelog:
v2: update commit message (Mimi)

 security/integrity/evm/evm_main.c     |  3 +++
 security/integrity/ima/ima_appraise.c |  2 ++
 security/integrity/ima/ima_main.c     | 13 +++++++++----
 security/integrity/ima/ima_policy.c   |  2 ++
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 0d36259b690d..6ae00fee1d34 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -59,6 +59,9 @@ static int __init evm_set_fixmode(char *str)
 {
 	if (strncmp(str, "fix", 3) == 0)
 		evm_fixmode = 1;
+	else
+		pr_err("invalid \"%s\" mode", str);
+
 	return 0;
 }
 __setup("evm=", evm_set_fixmode);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 580b771e3458..2193b51c2743 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -33,6 +33,8 @@ static int __init default_appraise_setup(char *str)
 		ima_appraise = IMA_APPRAISE_FIX;
 	else if (strncmp(str, "enforce", 7) == 0)
 		ima_appraise = IMA_APPRAISE_ENFORCE;
+	else
+		pr_err("invalid \"%s\" appraise option", str);
 #endif
 	return 1;
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8a91711ca79b..2b22932b140d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -50,18 +50,23 @@ static int __init hash_setup(char *str)
 		return 1;
 
 	if (strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) == 0) {
-		if (strncmp(str, "sha1", 4) == 0)
+		if (strncmp(str, "sha1", 4) == 0) {
 			ima_hash_algo = HASH_ALGO_SHA1;
-		else if (strncmp(str, "md5", 3) == 0)
+		} else if (strncmp(str, "md5", 3) == 0) {
 			ima_hash_algo = HASH_ALGO_MD5;
-		else
+		} else {
+			pr_err("invalid hash algorithm \"%s\" for template \"%s\"",
+				str, IMA_TEMPLATE_IMA_NAME);
 			return 1;
+		}
 		goto out;
 	}
 
 	i = match_string(hash_algo_name, HASH_ALGO__LAST, str);
-	if (i < 0)
+	if (i < 0) {
+		pr_err("invalid hash algorithm \"%s\"", str);
 		return 1;
+	}
 
 	ima_hash_algo = i;
 out:
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 07f033634b27..880d10887de8 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -240,6 +240,8 @@ static int __init policy_setup(char *str)
 			ima_use_secure_boot = true;
 		else if (strcmp(p, "fail_securely") == 0)
 			ima_fail_unverifiable_sigs = true;
+		else
+			pr_err("policy \"%s\" not found", p);
 	}
 
 	return 1;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/4] ima: limit secure boot feedback scope for appraise
  2020-09-04 19:40 [PATCH v2 0/4] integrity: improve user feedback for invalid bootparams Bruno Meneguele
  2020-09-04 19:40 ` [PATCH v2 1/4] ima: add check for enforced appraise option Bruno Meneguele
  2020-09-04 19:40 ` [PATCH v2 2/4] integrity: invalid kernel parameters feedback Bruno Meneguele
@ 2020-09-04 19:40 ` Bruno Meneguele
  2020-09-04 21:07   ` Mimi Zohar
  2020-09-05  1:20   ` [PATCH v3 " Bruno Meneguele
  2020-09-04 19:41 ` [PATCH v2 4/4] integrity: prompt keyring name for unknown key request Bruno Meneguele
  3 siblings, 2 replies; 10+ messages in thread
From: Bruno Meneguele @ 2020-09-04 19:40 UTC (permalink / raw)
  To: linux-integrity, linux-kernel; +Cc: zohar, Bruno Meneguele

Only prompt the unknown/invalid appraisal option if secureboot is enabled and
if the current state differentiates from the original one.

Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
---
Changelog:
v2: 
- update commit message (Mimi)
- work with a temporary var instead of directly with ima_appraise (Mimi)

 security/integrity/ima/ima_appraise.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 2193b51c2743..d17808245592 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -19,22 +19,29 @@
 static int __init default_appraise_setup(char *str)
 {
 #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
-	if (arch_ima_get_secureboot()) {
-		pr_info("Secure boot enabled: ignoring ima_appraise=%s boot parameter option",
-			str);
-		return 1;
-	}
+	bool sb_state = arch_ima_get_secureboot();
+	int appraisal_state = ima_appraise;
 
 	if (strncmp(str, "off", 3) == 0)
-		ima_appraise = 0;
+		appraisal_state = 0;
 	else if (strncmp(str, "log", 3) == 0)
-		ima_appraise = IMA_APPRAISE_LOG;
+		appraisal_state = IMA_APPRAISE_LOG;
 	else if (strncmp(str, "fix", 3) == 0)
-		ima_appraise = IMA_APPRAISE_FIX;
+		appraisal_state = IMA_APPRAISE_FIX;
 	else if (strncmp(str, "enforce", 7) == 0)
-		ima_appraise = IMA_APPRAISE_ENFORCE;
+		appraisal_state = IMA_APPRAISE_ENFORCE;
 	else
 		pr_err("invalid \"%s\" appraise option", str);
+
+	/* If appraisal state was changed, but secure boot is enabled,
+	 * keep its default */
+	if (sb_state) {
+		if (!(appraisal_state & IMA_APPRAISE_ENFORCE))
+			pr_info("Secure boot enabled: ignoring ima_appraise=%s option",
+				str);
+		else
+			ima_appraise = appraisal_state;
+	}
 #endif
 	return 1;
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/4] ima: limit secure boot feedback scope for appraise
  2020-09-04 19:40 ` [PATCH v2 3/4] ima: limit secure boot feedback scope for appraise Bruno Meneguele
@ 2020-09-04 21:07   ` Mimi Zohar
  2020-09-05  1:17     ` Bruno Meneguele
  2020-09-05  1:20   ` [PATCH v3 " Bruno Meneguele
  1 sibling, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2020-09-04 21:07 UTC (permalink / raw)
  To: Bruno Meneguele, linux-integrity, linux-kernel

Hi Bruno,

> +	bool sb_state = arch_ima_get_secureboot();
> +	int appraisal_state = ima_appraise;
>  
>  	if (strncmp(str, "off", 3) == 0)
> -		ima_appraise = 0;
> +		appraisal_state = 0;
>  	else if (strncmp(str, "log", 3) == 0)
> -		ima_appraise = IMA_APPRAISE_LOG;
> +		appraisal_state = IMA_APPRAISE_LOG;
>  	else if (strncmp(str, "fix", 3) == 0)
> -		ima_appraise = IMA_APPRAISE_FIX;
> +		appraisal_state = IMA_APPRAISE_FIX;
>  	else if (strncmp(str, "enforce", 7) == 0)
> -		ima_appraise = IMA_APPRAISE_ENFORCE;
> +		appraisal_state = IMA_APPRAISE_ENFORCE;
>  	else
>  		pr_err("invalid \"%s\" appraise option", str);
> +
> +	/* If appraisal state was changed, but secure boot is enabled,
> +	 * keep its default */
> +	if (sb_state) {
> +		if (!(appraisal_state & IMA_APPRAISE_ENFORCE))
> +			pr_info("Secure boot enabled: ignoring ima_appraise=%s option",
> +				str);
> +		else
> +			ima_appraise = appraisal_state;
> +	}

Shouldn't the "else" clause be here.   No need to re-post the entire
patch set.

thanks,

Mimi

>  #endif
>  	return 1;
>  }



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/4] ima: limit secure boot feedback scope for appraise
  2020-09-04 21:07   ` Mimi Zohar
@ 2020-09-05  1:17     ` Bruno Meneguele
  0 siblings, 0 replies; 10+ messages in thread
From: Bruno Meneguele @ 2020-09-05  1:17 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

On Fri, Sep 04, 2020 at 05:07:08PM -0400, Mimi Zohar wrote:
> Hi Bruno,
> 
> > +	bool sb_state = arch_ima_get_secureboot();
> > +	int appraisal_state = ima_appraise;
> >  
> >  	if (strncmp(str, "off", 3) == 0)
> > -		ima_appraise = 0;
> > +		appraisal_state = 0;
> >  	else if (strncmp(str, "log", 3) == 0)
> > -		ima_appraise = IMA_APPRAISE_LOG;
> > +		appraisal_state = IMA_APPRAISE_LOG;
> >  	else if (strncmp(str, "fix", 3) == 0)
> > -		ima_appraise = IMA_APPRAISE_FIX;
> > +		appraisal_state = IMA_APPRAISE_FIX;
> >  	else if (strncmp(str, "enforce", 7) == 0)
> > -		ima_appraise = IMA_APPRAISE_ENFORCE;
> > +		appraisal_state = IMA_APPRAISE_ENFORCE;
> >  	else
> >  		pr_err("invalid \"%s\" appraise option", str);
> > +
> > +	/* If appraisal state was changed, but secure boot is enabled,
> > +	 * keep its default */
> > +	if (sb_state) {
> > +		if (!(appraisal_state & IMA_APPRAISE_ENFORCE))
> > +			pr_info("Secure boot enabled: ignoring ima_appraise=%s option",
> > +				str);
> > +		else
> > +			ima_appraise = appraisal_state;
> > +	}
> 
> Shouldn't the "else" clause be here.   No need to re-post the entire
> patch set.

Yes, of course it should.
Sorry. Sending the v3 for this patch.

> 
> thanks,
> 
> Mimi
> 
> >  #endif
> >  	return 1;
> >  }
> 
> 

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 3/4] ima: limit secure boot feedback scope for appraise
  2020-09-04 19:40 ` [PATCH v2 3/4] ima: limit secure boot feedback scope for appraise Bruno Meneguele
  2020-09-04 21:07   ` Mimi Zohar
@ 2020-09-05  1:20   ` Bruno Meneguele
  2020-09-11 15:07     ` Mimi Zohar
  1 sibling, 1 reply; 10+ messages in thread
From: Bruno Meneguele @ 2020-09-05  1:20 UTC (permalink / raw)
  To: linux-integrity, linux-kernel; +Cc: zohar, Bruno Meneguele

Only prompt the unknown/invalid appraisal option if secureboot is enabled and
if the current appraisal state is different from the original one.

Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
---
Changelog:
v3:
- fix sb_state conditional (Mimi)
v2: 
- update commit message (Mimi)
- work with a temporary var instead of directly with ima_appraise (Mimi)

 security/integrity/ima/ima_appraise.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 2193b51c2743..4f028f6e8f8d 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -19,22 +19,29 @@
 static int __init default_appraise_setup(char *str)
 {
 #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
-	if (arch_ima_get_secureboot()) {
-		pr_info("Secure boot enabled: ignoring ima_appraise=%s boot parameter option",
-			str);
-		return 1;
-	}
+	bool sb_state = arch_ima_get_secureboot();
+	int appraisal_state = ima_appraise;
 
 	if (strncmp(str, "off", 3) == 0)
-		ima_appraise = 0;
+		appraisal_state = 0;
 	else if (strncmp(str, "log", 3) == 0)
-		ima_appraise = IMA_APPRAISE_LOG;
+		appraisal_state = IMA_APPRAISE_LOG;
 	else if (strncmp(str, "fix", 3) == 0)
-		ima_appraise = IMA_APPRAISE_FIX;
+		appraisal_state = IMA_APPRAISE_FIX;
 	else if (strncmp(str, "enforce", 7) == 0)
-		ima_appraise = IMA_APPRAISE_ENFORCE;
+		appraisal_state = IMA_APPRAISE_ENFORCE;
 	else
 		pr_err("invalid \"%s\" appraise option", str);
+
+	/* If appraisal state was changed, but secure boot is enabled,
+	 * keep its default */
+	if (sb_state) {
+		if (!(appraisal_state & IMA_APPRAISE_ENFORCE))
+			pr_info("Secure boot enabled: ignoring ima_appraise=%s option",
+				str);
+	} else {
+		ima_appraise = appraisal_state;
+	}
 #endif
 	return 1;
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/4] ima: limit secure boot feedback scope for appraise
  2020-09-05  1:20   ` [PATCH v3 " Bruno Meneguele
@ 2020-09-11 15:07     ` Mimi Zohar
  2020-09-11 18:03       ` Bruno Meneguele
  0 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2020-09-11 15:07 UTC (permalink / raw)
  To: Bruno Meneguele, linux-integrity, linux-kernel

Hi Bruno,

On Fri, 2020-09-04 at 22:20 -0300, Bruno Meneguele wrote:
> Only prompt the unknown/invalid appraisal option if secureboot is enabled and
> if the current appraisal state is different from the original one.
> 
> Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>

Thanks.  I tweaked this patch description and that of 4/4.  This patch
set is in next-integrity-testing.  Please take a look.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/4] ima: limit secure boot feedback scope for appraise
  2020-09-11 15:07     ` Mimi Zohar
@ 2020-09-11 18:03       ` Bruno Meneguele
  0 siblings, 0 replies; 10+ messages in thread
From: Bruno Meneguele @ 2020-09-11 18:03 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: linux-integrity, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 641 bytes --]

On Fri, Sep 11, 2020 at 11:07:43AM -0400, Mimi Zohar wrote:
> Hi Bruno,
> 
> On Fri, 2020-09-04 at 22:20 -0300, Bruno Meneguele wrote:
> > Only prompt the unknown/invalid appraisal option if secureboot is enabled and
> > if the current appraisal state is different from the original one.
> > 
> > Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
> 
> Thanks.  I tweaked this patch description and that of 4/4.  This patch
> set is in next-integrity-testing.  Please take a look.
> 

Thanks Mimi! Just checked in the branch and they're fine.

> thanks,
> 
> Mimi
> 

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 4/4] integrity: prompt keyring name for unknown key request
  2020-09-04 19:40 [PATCH v2 0/4] integrity: improve user feedback for invalid bootparams Bruno Meneguele
                   ` (2 preceding siblings ...)
  2020-09-04 19:40 ` [PATCH v2 3/4] ima: limit secure boot feedback scope for appraise Bruno Meneguele
@ 2020-09-04 19:41 ` Bruno Meneguele
  3 siblings, 0 replies; 10+ messages in thread
From: Bruno Meneguele @ 2020-09-04 19:41 UTC (permalink / raw)
  To: linux-integrity, linux-kernel; +Cc: zohar, Bruno Meneguele

Depending on the IMA policy a key can be searched in multiple keyrings (e.g.
.ima and .platform) and possibly failing for both. However, for the user not
aware of the searching order it's not clear what's the keyring the kernel
didn't find the key. With this patch we improve this feedback by printing
the keyring "description" (name).

Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
---
 security/integrity/digsig_asymmetric.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
index cfa4127d0518..14de98ef67f6 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -55,8 +55,14 @@ static struct key *request_asymmetric_key(struct key *keyring, uint32_t keyid)
 	}
 
 	if (IS_ERR(key)) {
-		pr_err_ratelimited("Request for unknown key '%s' err %ld\n",
-				   name, PTR_ERR(key));
+		if (keyring)
+			pr_err_ratelimited("Request for unknown key '%s' in '%s' keyring. err %ld\n",
+					   name, keyring->description,
+					   PTR_ERR(key));
+		else
+			pr_err_ratelimited("Request for unknown key '%s' err %ld\n",
+					   name, PTR_ERR(key));
+
 		switch (PTR_ERR(key)) {
 			/* Hide some search errors */
 		case -EACCES:
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-09-11 18:03 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-04 19:40 [PATCH v2 0/4] integrity: improve user feedback for invalid bootparams Bruno Meneguele
2020-09-04 19:40 ` [PATCH v2 1/4] ima: add check for enforced appraise option Bruno Meneguele
2020-09-04 19:40 ` [PATCH v2 2/4] integrity: invalid kernel parameters feedback Bruno Meneguele
2020-09-04 19:40 ` [PATCH v2 3/4] ima: limit secure boot feedback scope for appraise Bruno Meneguele
2020-09-04 21:07   ` Mimi Zohar
2020-09-05  1:17     ` Bruno Meneguele
2020-09-05  1:20   ` [PATCH v3 " Bruno Meneguele
2020-09-11 15:07     ` Mimi Zohar
2020-09-11 18:03       ` Bruno Meneguele
2020-09-04 19:41 ` [PATCH v2 4/4] integrity: prompt keyring name for unknown key request Bruno Meneguele

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.