linux-kernel - Re: [PATCH v2 09/11] ima: Move validation of the keyrings conditional into ima_validate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1593558449.5057.12.camel@linux.ibm.com>
Date:   Tue, 30 Jun 2020 19:07:29 -0400
From:   Mimi Zohar <zohar@...ux.ibm.com>
To:     Tyler Hicks <tyhicks@...ux.microsoft.com>,
        Dmitry Kasatkin <dmitry.kasatkin@...il.com>
Cc:     James Morris <jmorris@...ei.org>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>,
        Prakhar Srivastava <prsriva02@...il.com>,
        linux-kernel@...r.kernel.org, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH v2 09/11] ima: Move validation of the keyrings
 conditional into ima_validate_rule()

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@...ux.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;