[v2,09/11] ima: Move validation of the keyrings conditional into ima_validate_rule()
diff mbox series

Message ID 20200626223900.253615-10-tyhicks@linux.microsoft.com
State New
Headers show
Series
  • ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
Related show

Commit Message

Tyler Hicks June 26, 2020, 10:38 p.m. UTC
Use ima_validate_rule() to ensure that the combination of a hook
function and the keyrings conditional is valid and that the keyrings
conditional is not specified without an explicit KEY_CHECK func
conditional. This is a code cleanup and has no user-facing change.

Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---

* v2
  - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
    IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
    present in the rule entry flags for non-buffer hook functions.

 security/integrity/ima/ima_policy.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

Lakshmi Ramasubramanian June 27, 2020, 11:49 p.m. UTC | #1
On 6/26/20 3:38 PM, Tyler Hicks wrote:

> Use ima_validate_rule() to ensure that the combination of a hook
> function and the keyrings conditional is valid and that the keyrings
> conditional is not specified without an explicit KEY_CHECK func
> conditional. This is a code cleanup and has no user-facing change.

In addition to checking for func=KEY_CHECK and the keyrings conditional, 
the patch also validates the flags for other IMA hooks (such as 
KEXEC_KERNEL_CHECK, POLICY_CHECK, etc.) Would be good to mention that in 
the patch description.

  -lakshmi

> 
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
> 
> * v2
>    - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
>      IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
>      present in the rule entry flags for non-buffer hook functions.
> 
>   security/integrity/ima/ima_policy.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 8cdca2399d59..43d49ad958fb 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>   		case KEXEC_KERNEL_CHECK:
>   		case KEXEC_INITRAMFS_CHECK:
>   		case POLICY_CHECK:
> +			if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
> +					     IMA_UID | IMA_FOWNER | IMA_FSUUID |
> +					     IMA_INMASK | IMA_EUID | IMA_PCR |
> +					     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
> +					     IMA_PERMIT_DIRECTIO |
> +					     IMA_MODSIG_ALLOWED |
> +					     IMA_CHECK_BLACKLIST))
> +				return false;
> +
>   			break;
>   		case KEXEC_CMDLINE:
>   			if (entry->action & ~(MEASURE | DONT_MEASURE))
> @@ -1027,7 +1036,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>   		default:
>   			return false;
>   		}
> -	}
> +	} else if (entry->flags & IMA_KEYRINGS)
> +		return false;
>   
>   	return true;
>   }
> @@ -1209,7 +1219,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>   			keyrings_len = strlen(args[0].from) + 1;
>   
>   			if ((entry->keyrings) ||
> -			    (entry->func != KEY_CHECK) ||
>   			    (keyrings_len < 2)) {
>   				result = -EINVAL;
>   				break;
>
Tyler Hicks June 29, 2020, 2:16 p.m. UTC | #2
On 2020-06-27 16:49:46, Lakshmi Ramasubramanian wrote:
> On 6/26/20 3:38 PM, Tyler Hicks wrote:
> 
> > Use ima_validate_rule() to ensure that the combination of a hook
> > function and the keyrings conditional is valid and that the keyrings
> > conditional is not specified without an explicit KEY_CHECK func
> > conditional. This is a code cleanup and has no user-facing change.
> 
> In addition to checking for func=KEY_CHECK and the keyrings conditional, the
> patch also validates the flags for other IMA hooks (such as
> KEXEC_KERNEL_CHECK, POLICY_CHECK, etc.) Would be good to mention that in the
> patch description.

It actually doesn't do any additional validation of other IMA hooks at
this time. That check on entry->flags is an allowlist of every possible
conditional flag except IMA_KEYRINGS. The ima_parse_rule() function is
already validating all of these conditional flags.

Tyler

> 
>  -lakshmi
> 
> > 
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > ---
> > 
> > * v2
> >    - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
> >      IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
> >      present in the rule entry flags for non-buffer hook functions.
> > 
> >   security/integrity/ima/ima_policy.c | 13 +++++++++++--
> >   1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 8cdca2399d59..43d49ad958fb 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >   		case KEXEC_KERNEL_CHECK:
> >   		case KEXEC_INITRAMFS_CHECK:
> >   		case POLICY_CHECK:
> > +			if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
> > +					     IMA_UID | IMA_FOWNER | IMA_FSUUID |
> > +					     IMA_INMASK | IMA_EUID | IMA_PCR |
> > +					     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
> > +					     IMA_PERMIT_DIRECTIO |
> > +					     IMA_MODSIG_ALLOWED |
> > +					     IMA_CHECK_BLACKLIST))
> > +				return false;
> > +
> >   			break;
> >   		case KEXEC_CMDLINE:
> >   			if (entry->action & ~(MEASURE | DONT_MEASURE))
> > @@ -1027,7 +1036,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >   		default:
> >   			return false;
> >   		}
> > -	}
> > +	} else if (entry->flags & IMA_KEYRINGS)
> > +		return false;
> >   	return true;
> >   }
> > @@ -1209,7 +1219,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> >   			keyrings_len = strlen(args[0].from) + 1;
> >   			if ((entry->keyrings) ||
> > -			    (entry->func != KEY_CHECK) ||
> >   			    (keyrings_len < 2)) {
> >   				result = -EINVAL;
> >   				break;
> >
Mimi Zohar June 30, 2020, 11:07 p.m. UTC | #3
On Fri, 2020-06-26 at 17:38 -0500, Tyler Hicks wrote:
> Use ima_validate_rule() to ensure that the combination of a hook
> function and the keyrings conditional is valid and that the keyrings
> conditional is not specified without an explicit KEY_CHECK func
> conditional. This is a code cleanup and has no user-facing change.
> 
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
> 
> * v2
>   - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
>     IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
>     present in the rule entry flags for non-buffer hook functions.
> 
>  security/integrity/ima/ima_policy.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 8cdca2399d59..43d49ad958fb 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>  		case KEXEC_KERNEL_CHECK:
>  		case KEXEC_INITRAMFS_CHECK:
>  		case POLICY_CHECK:
> +			if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
> +					     IMA_UID | IMA_FOWNER | IMA_FSUUID |
> +					     IMA_INMASK | IMA_EUID | IMA_PCR |
> +					     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
> +					     IMA_PERMIT_DIRECTIO |
> +					     IMA_MODSIG_ALLOWED |
> +					     IMA_CHECK_BLACKLIST))

Other than KEYRINGS, this patch should continue to behave the same.
 However, this list gives the impressions that all of these flags are
permitted on all of the above flags, which isn't true.

For example, both IMA_MODSIG_ALLOWED & IMA_CHECK_BLACKLIST are limited
to appended signatures, meaning KERNEL_CHECK and KEXEC_KERNEL_CHECK.
 Both should only be allowed on APPRAISE action rules.

IMA_PCR should be limited to MEASURE action rules.

IMA_DIGSIG_REQUIRED should be limited to APPRAISE action rules.

> +				return false;
> +
>  			break;
>  		case KEXEC_CMDLINE:
>  			if (entry->action & ~(MEASURE | DONT_MEASURE))
> @@ -1027,7 +1036,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>  		default:
>  			return false;
>  		}
> -	}
> +	} else if (entry->flags & IMA_KEYRINGS)
> +		return false;

IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST need to be added here as
well.

Mimi

>  
>  	return true;
>  }
> @@ -1209,7 +1219,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  			keyrings_len = strlen(args[0].from) + 1;
>  
>  			if ((entry->keyrings) ||
> -			    (entry->func != KEY_CHECK) ||
>  			    (keyrings_len < 2)) {
>  				result = -EINVAL;
>  				break;
Tyler Hicks July 2, 2020, 10:16 p.m. UTC | #4
On 2020-06-30 19:07:29, Mimi Zohar wrote:
> On Fri, 2020-06-26 at 17:38 -0500, Tyler Hicks wrote:
> > Use ima_validate_rule() to ensure that the combination of a hook
> > function and the keyrings conditional is valid and that the keyrings
> > conditional is not specified without an explicit KEY_CHECK func
> > conditional. This is a code cleanup and has no user-facing change.
> > 
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > ---
> > 
> > * v2
> >   - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
> >     IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
> >     present in the rule entry flags for non-buffer hook functions.
> > 
> >  security/integrity/ima/ima_policy.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 8cdca2399d59..43d49ad958fb 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >  		case KEXEC_KERNEL_CHECK:
> >  		case KEXEC_INITRAMFS_CHECK:
> >  		case POLICY_CHECK:
> > +			if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
> > +					     IMA_UID | IMA_FOWNER | IMA_FSUUID |
> > +					     IMA_INMASK | IMA_EUID | IMA_PCR |
> > +					     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
> > +					     IMA_PERMIT_DIRECTIO |
> > +					     IMA_MODSIG_ALLOWED |
> > +					     IMA_CHECK_BLACKLIST))
> 
> Other than KEYRINGS, this patch should continue to behave the same.
>  However, this list gives the impressions that all of these flags are
> permitted on all of the above flags, which isn't true.
> 
> For example, both IMA_MODSIG_ALLOWED & IMA_CHECK_BLACKLIST are limited
> to appended signatures, meaning KERNEL_CHECK and KEXEC_KERNEL_CHECK.

Just to clarify, are both IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST
limited to KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK, and MODULE_CHECK?
That's what ima_hook_supports_modsig() suggests.

>  Both should only be allowed on APPRAISE action rules.

For completeness, it looks like DONT_APPRAISE should not be allowed.

> IMA_PCR should be limited to MEASURE action rules.

It looks like DONT_MEASURE should not be allowed.

> IMA_DIGSIG_REQUIRED should be limited to APPRAISE action rules.

It looks like DONT_APPRAISE should not be allowed.

> 
> > +				return false;
> > +
> >  			break;
> >  		case KEXEC_CMDLINE:
> >  			if (entry->action & ~(MEASURE | DONT_MEASURE))
> > @@ -1027,7 +1036,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >  		default:
> >  			return false;
> >  		}
> > -	}
> > +	} else if (entry->flags & IMA_KEYRINGS)
> > +		return false;
> 
> IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST need to be added here as
> well.

That makes sense.

Tyler

> 
> Mimi
> 
> >  
> >  	return true;
> >  }
> > @@ -1209,7 +1219,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> >  			keyrings_len = strlen(args[0].from) + 1;
> >  
> >  			if ((entry->keyrings) ||
> > -			    (entry->func != KEY_CHECK) ||
> >  			    (keyrings_len < 2)) {
> >  				result = -EINVAL;
> >  				break;
Mimi Zohar July 3, 2020, 2:15 p.m. UTC | #5
On Thu, 2020-07-02 at 17:16 -0500, Tyler Hicks wrote:
> On 2020-06-30 19:07:29, Mimi Zohar wrote:
> > On Fri, 2020-06-26 at 17:38 -0500, Tyler Hicks wrote:
> > > Use ima_validate_rule() to ensure that the combination of a hook
> > > function and the keyrings conditional is valid and that the keyrings
> > > conditional is not specified without an explicit KEY_CHECK func
> > > conditional. This is a code cleanup and has no user-facing change.
> > > 
> > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > > ---
> > > 
> > > * v2
> > >   - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
> > >     IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
> > >     present in the rule entry flags for non-buffer hook functions.
> > > 
> > >  security/integrity/ima/ima_policy.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > > index 8cdca2399d59..43d49ad958fb 100644
> > > --- a/security/integrity/ima/ima_policy.c
> > > +++ b/security/integrity/ima/ima_policy.c
> > > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> > >  		case KEXEC_KERNEL_CHECK:
> > >  		case KEXEC_INITRAMFS_CHECK:
> > >  		case POLICY_CHECK:
> > > +			if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
> > > +					     IMA_UID | IMA_FOWNER | IMA_FSUUID |
> > > +					     IMA_INMASK | IMA_EUID | IMA_PCR |
> > > +					     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
> > > +					     IMA_PERMIT_DIRECTIO |
> > > +					     IMA_MODSIG_ALLOWED |
> > > +					     IMA_CHECK_BLACKLIST))
> > 
> > Other than KEYRINGS, this patch should continue to behave the same.
> >  However, this list gives the impressions that all of these flags are
> > permitted on all of the above flags, which isn't true.
> > 
> > For example, both IMA_MODSIG_ALLOWED & IMA_CHECK_BLACKLIST are limited
> > to appended signatures, meaning KERNEL_CHECK and KEXEC_KERNEL_CHECK.
> 
> Just to clarify, are both IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST
> limited to KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK, and MODULE_CHECK?
> That's what ima_hook_supports_modsig() suggests.

Theoretically that is true, but I have no idea how you would append a
signature to the kexec boot command line.  The only users of appended
signatures are currently kernel modules and the kexec'ed kernel image.

> 
> >  Both should only be allowed on APPRAISE action rules.
> 
> For completeness, it looks like DONT_APPRAISE should not be allowed.

Good point.  

> 
> > IMA_PCR should be limited to MEASURE action rules.
> 
> It looks like DONT_MEASURE should not be allowed.

The TPM PCR isn't a file attribute.

> 
> > IMA_DIGSIG_REQUIRED should be limited to APPRAISE action rules.
> 
> It looks like DONT_APPRAISE should not be allowed.

Right, in all of these cases the DONT_XXXX isn't applicable.

Mimi
Tyler Hicks July 6, 2020, 1:18 p.m. UTC | #6
On 2020-07-03 10:15:32, Mimi Zohar wrote:
> On Thu, 2020-07-02 at 17:16 -0500, Tyler Hicks wrote:
> > On 2020-06-30 19:07:29, Mimi Zohar wrote:
> > > On Fri, 2020-06-26 at 17:38 -0500, Tyler Hicks wrote:
> > > > Use ima_validate_rule() to ensure that the combination of a hook
> > > > function and the keyrings conditional is valid and that the keyrings
> > > > conditional is not specified without an explicit KEY_CHECK func
> > > > conditional. This is a code cleanup and has no user-facing change.
> > > > 
> > > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > > > ---
> > > > 
> > > > * v2
> > > >   - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
> > > >     IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
> > > >     present in the rule entry flags for non-buffer hook functions.
> > > > 
> > > >  security/integrity/ima/ima_policy.c | 13 +++++++++++--
> > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > > > index 8cdca2399d59..43d49ad958fb 100644
> > > > --- a/security/integrity/ima/ima_policy.c
> > > > +++ b/security/integrity/ima/ima_policy.c
> > > > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> > > >  		case KEXEC_KERNEL_CHECK:
> > > >  		case KEXEC_INITRAMFS_CHECK:
> > > >  		case POLICY_CHECK:
> > > > +			if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
> > > > +					     IMA_UID | IMA_FOWNER | IMA_FSUUID |
> > > > +					     IMA_INMASK | IMA_EUID | IMA_PCR |
> > > > +					     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
> > > > +					     IMA_PERMIT_DIRECTIO |
> > > > +					     IMA_MODSIG_ALLOWED |
> > > > +					     IMA_CHECK_BLACKLIST))
> > > 
> > > Other than KEYRINGS, this patch should continue to behave the same.
> > >  However, this list gives the impressions that all of these flags are
> > > permitted on all of the above flags, which isn't true.
> > > 
> > > For example, both IMA_MODSIG_ALLOWED & IMA_CHECK_BLACKLIST are limited
> > > to appended signatures, meaning KERNEL_CHECK and KEXEC_KERNEL_CHECK.
> > 
> > Just to clarify, are both IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST
> > limited to KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK, and MODULE_CHECK?
> > That's what ima_hook_supports_modsig() suggests.
> 
> Theoretically that is true, but I have no idea how you would append a
> signature to the kexec boot command line.  The only users of appended
> signatures are currently kernel modules and the kexec'ed kernel image.

The discrepancy was with KEXEC_INITRAMFS_CHECK, not KEXEC_CMDLINE. I now
see that there's no support for initramfs signature verification in the
kexec code so I'll assume that ima_hook_supports_modsig() is wrong and
limit IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST to the
KEXEC_KERNEL_CHECK and MODULE_CHECK actions, as you originally
suggested.

Tyler

> 
> > 
> > >  Both should only be allowed on APPRAISE action rules.
> > 
> > For completeness, it looks like DONT_APPRAISE should not be allowed.
> 
> Good point.  
> 
> > 
> > > IMA_PCR should be limited to MEASURE action rules.
> > 
> > It looks like DONT_MEASURE should not be allowed.
> 
> The TPM PCR isn't a file attribute.
> 
> > 
> > > IMA_DIGSIG_REQUIRED should be limited to APPRAISE action rules.
> > 
> > It looks like DONT_APPRAISE should not be allowed.
> 
> Right, in all of these cases the DONT_XXXX isn't applicable.
> 
> Mimi
Mimi Zohar July 7, 2020, 3:18 a.m. UTC | #7
On Mon, 2020-07-06 at 08:18 -0500, Tyler Hicks wrote:
> On 2020-07-03 10:15:32, Mimi Zohar wrote:
> > On Thu, 2020-07-02 at 17:16 -0500, Tyler Hicks wrote:
> > > On 2020-06-30 19:07:29, Mimi Zohar wrote:
> > > > On Fri, 2020-06-26 at 17:38 -0500, Tyler Hicks wrote:
> > > > > Use ima_validate_rule() to ensure that the combination of a hook
> > > > > function and the keyrings conditional is valid and that the keyrings
> > > > > conditional is not specified without an explicit KEY_CHECK func
> > > > > conditional. This is a code cleanup and has no user-facing change.
> > > > > 
> > > > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > > > > ---
> > > > > 
> > > > > * v2
> > > > >   - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
> > > > >     IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
> > > > >     present in the rule entry flags for non-buffer hook functions.
> > > > > 
> > > > >  security/integrity/ima/ima_policy.c | 13 +++++++++++--
> > > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > > > > index 8cdca2399d59..43d49ad958fb 100644
> > > > > --- a/security/integrity/ima/ima_policy.c
> > > > > +++ b/security/integrity/ima/ima_policy.c
> > > > > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> > > > >  		case KEXEC_KERNEL_CHECK:
> > > > >  		case KEXEC_INITRAMFS_CHECK:
> > > > >  		case POLICY_CHECK:
> > > > > +			if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
> > > > > +					     IMA_UID | IMA_FOWNER | IMA_FSUUID |
> > > > > +					     IMA_INMASK | IMA_EUID | IMA_PCR |
> > > > > +					     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
> > > > > +					     IMA_PERMIT_DIRECTIO |
> > > > > +					     IMA_MODSIG_ALLOWED |
> > > > > +					     IMA_CHECK_BLACKLIST))
> > > > 
> > > > Other than KEYRINGS, this patch should continue to behave the same.
> > > >  However, this list gives the impressions that all of these flags are
> > > > permitted on all of the above flags, which isn't true.
> > > > 
> > > > For example, both IMA_MODSIG_ALLOWED & IMA_CHECK_BLACKLIST are limited
> > > > to appended signatures, meaning KERNEL_CHECK and KEXEC_KERNEL_CHECK.
> > > 
> > > Just to clarify, are both IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST
> > > limited to KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK, and MODULE_CHECK?
> > > That's what ima_hook_supports_modsig() suggests.
> > 
> > Theoretically that is true, but I have no idea how you would append a
> > signature to the kexec boot command line.  The only users of appended
> > signatures are currently kernel modules and the kexec'ed kernel image.
> 
> The discrepancy was with KEXEC_INITRAMFS_CHECK, not KEXEC_CMDLINE. I now
> see that there's no support for initramfs signature verification in the
> kexec code so I'll assume that ima_hook_supports_modsig() is wrong and
> limit IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST to the
> KEXEC_KERNEL_CHECK and MODULE_CHECK actions, as you originally
> suggested.

My mistake.  Yes, both the kexec kernel image and the initramfs read
the respective file into memory and can be signed either with an
imasig or modsig.  Refer to kernel_read_file_from_fd().

Mimi

Patch
diff mbox series

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8cdca2399d59..43d49ad958fb 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1000,6 +1000,15 @@  static bool ima_validate_rule(struct ima_rule_entry *entry)
 		case KEXEC_KERNEL_CHECK:
 		case KEXEC_INITRAMFS_CHECK:
 		case POLICY_CHECK:
+			if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
+					     IMA_UID | IMA_FOWNER | IMA_FSUUID |
+					     IMA_INMASK | IMA_EUID | IMA_PCR |
+					     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
+					     IMA_PERMIT_DIRECTIO |
+					     IMA_MODSIG_ALLOWED |
+					     IMA_CHECK_BLACKLIST))
+				return false;
+
 			break;
 		case KEXEC_CMDLINE:
 			if (entry->action & ~(MEASURE | DONT_MEASURE))
@@ -1027,7 +1036,8 @@  static bool ima_validate_rule(struct ima_rule_entry *entry)
 		default:
 			return false;
 		}
-	}
+	} else if (entry->flags & IMA_KEYRINGS)
+		return false;
 
 	return true;
 }
@@ -1209,7 +1219,6 @@  static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			keyrings_len = strlen(args[0].from) + 1;
 
 			if ((entry->keyrings) ||
-			    (entry->func != KEY_CHECK) ||
 			    (keyrings_len < 2)) {
 				result = -EINVAL;
 				break;