Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <456fea5f63f14a342791388e6e8d34605ef13eb5.camel@linux.ibm.com>
Subject: Re: [PATCH RFC v2 02/12] KEYS: CA link restriction
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Eric Snowberg <eric.snowberg@oracle.com>, keyrings@vger.kernel.org,
        linux-integrity@vger.kernel.org, dhowells@redhat.com,
        dwmw2@infradead.org, herbert@gondor.apana.org.au,
        davem@davemloft.net, jarkko@kernel.org, jmorris@namei.org,
        serge@hallyn.com
Cc:     keescook@chromium.org, gregkh@linuxfoundation.org,
        torvalds@linux-foundation.org, scott.branden@broadcom.com,
        weiyongjun1@huawei.com, nayna@linux.ibm.com, ebiggers@google.com,
        ardb@kernel.org, nramas@linux.microsoft.com, lszubowi@redhat.com,
        linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        James.Bottomley@HansenPartnership.com, pjones@redhat.com,
        glin@suse.com, konrad.wilk@oracle.com
Date:   Thu, 05 Aug 2021 10:00:18 -0400
In-Reply-To: <20210726171319.3133879-3-eric.snowberg@oracle.com>
References: <20210726171319.3133879-1-eric.snowberg@oracle.com>
         <20210726171319.3133879-3-eric.snowberg@oracle.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 2021-07-26 at 13:13 -0400, Eric Snowberg wrote:
> Add a new link restriction.  Restrict the addition of keys in a keyring
> based on the key to be added being a CA (self-signed) or by being
> vouched for by a key in either the built-in or the secondary trusted
> keyrings.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

As discussed, please remove "or by being vouched for by a key in
either the built-in or the secondary trusted keyrings."

If these keys were to be loaded onto a keyring other than the platform
keyring, they should be loaded onto the secondary keyring.  The
secondary restriction currently allows certificates signed by keys on
either the builtin or the secondary keyring, to be loaded onto the
secondary keyring.   A new restriction would also allow certificates
signed by keys on the ".mok" keyring.

> +/**
> + * restrict_link_by_ca - Restrict additions to a ring of public keys
> + * based on it being a CA
> + * @dest_keyring: Keyring being linked to.
> + * @type: The type of key being added.
> + * @payload: The payload of the new key.
> + * @trusted: A key or ring of keys that can be used to vouch for the new cert.
> + *
> + * Check if the new certificate is a CA or if they key can be vouched for
> + * by keys already linked in the destination keyring or the trusted
> + * keyring.  If one of those is the signing key or it is self signed, then
> + * mark the new certificate as being ok to link.
> + *
> + * Returns 0 if the new certificate was accepted, -ENOKEY if we could not find
> + * a matching parent certificate in the trusted list.  -ENOPKG if the signature
> + * uses unsupported crypto, or some other error if there is a matching
> + * certificate  but the signature check cannot be performed.
> + */

Please update the function description as discussed, removing "or if they
key can be vouched for by keys already linked in the destination
keyring or the trusted keyring."

The kernel doc "Brief description of function" should not wrap.  The
variable definition shouldn't be suffixed with a period.  Please refer
to Documentation/doc-guide/kernel-doc.rst.

> +int restrict_link_by_ca(struct key *dest_keyring,

The UEFI db CA keys should only be loaded on boot.  Should this be
annotated as __init?

> +			const struct key_type *type,
> +			const union key_payload *payload,
> +			struct key *trust_keyring)
> +{
> +	const struct public_key_signature *sig;
> +	const struct public_key *pkey;
> +	struct key *key;
> +	int ret;
> +
> +	if (type != &key_type_asymmetric)
> +		return -EOPNOTSUPP;
> +
> +	sig = payload->data[asym_auth];
> +	if (!sig)
> +		return -ENOPKG;
> +
> +	if (!sig->auth_ids[0] && !sig->auth_ids[1])
> +		return -ENOKEY;
> +
> +	pkey = payload->data[asym_crypto];
> +	if (!pkey)
> +		return -ENOPKG;
> +
> +	ret = public_key_verify_signature(pkey, sig);
> +	if (!ret)
> +		return 0;
> +
> +	if (!trust_keyring)
> +		return -ENOKEY;
> +
> +	key = find_asymmetric_key(trust_keyring,
> +				  sig->auth_ids[0], sig->auth_ids[1],
> +				  false);
> +	if (IS_ERR(key))
> +		return -ENOKEY;
> +
> +	ret = verify_signature(key, sig);
> +	key_put(key);
> +	return ret;
> +}
> +
>  static bool match_either_id(const struct asymmetric_key_ids *pair,
>  			    const struct asymmetric_key_id *single)
>  {


> +extern int restrict_link_by_system_trusted_or_ca(
> +	struct key *dest_keyring,
> +	const struct key_type *type,
> +	const union key_payload *payload,
> +	struct key *restrict_key);

After the discussed change, this shouldn't be needed.

thanks,

Mimi

> +
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>  extern int restrict_link_by_builtin_and_secondary_trusted(
>  	struct key *keyring,