Re: [PATCH v2] keys: X.509 public key issuer lookup without AKID

[Jarkko Sakkinen <jarkko@kernel.org>]

On Thu, Jan 14, 2021 at 09:40:35PM +0100, Andrew Zaborowski wrote:
> There are non-root X.509 v3 certificates in use there that contain no
> Authority Key Identifier extension (RFC5280 section 4.2.1.1).  The
> asymmetric key's issuer key IDs 1 and 2 for those certificates are
> generated from the AKID data so current code has no way to look up the
> issuer certificate for verification.  Add a third ID blob to the arrays
> in both asymmetric_key_ids (for certficate subject) and in the
> public_keys_signature's auth_ids (for issuer lookup), using just raw
> subject and issuer DNs from the certificate.  There's no other place
> this data is currently saved that could be used in find_asymmetric_key.
> Adapt asymmetric_key_ids() and the callers to use the third ID for
> lookups when none of the other two are available.  Attempt to keep the
> logic intact when they are, to minimise behaviour changes.  Adapt the
> restrict functions' NULL-checks to include that ID too.  The lookup
> logic in pkcs7_verify.c is not modified, the AKID extensions are still
> required there.
> 
> I believe this implements what (2) in the struct asymmetric_key_id
> comment (include/keys/asymmetric-type.h) talks about already, so that
> comment isn't modified.  It's also how "openssl verify" looks up issuer
> certificates without the AKID available.

Preferably no language in persons.

> Internally the search specifier string generated for the key lookup in
> find_asymmetric_key() uses a new prefix that tells
> asymmetric_key_match_preparse to only match the data against the raw
> Distinguished Name in the third ID and shouldn't conflict with search
> specifiers already in use.
> 
> Lookups are unambiguous only provided that the CAs respect the condition
> in RFC5280 4.2.1.1 that the AKID may only be omitted if the CA uses a
> single signing key.
> 
> Signed-off-by: Andrew Zaborowski <andrew.zaborowski@intel.com>
> ---
> v2:
>  - focus the commit message on the implementation
>  - shorten the prefix used in find_asymmetric_key
>  - clarify find_asymmetric_key comment
> 
>  crypto/asymmetric_keys/asymmetric_type.c  | 52 +++++++++++++++++------
>  crypto/asymmetric_keys/pkcs7_trust.c      |  6 +--
>  crypto/asymmetric_keys/restrict.c         | 48 ++++++++++++---------
>  crypto/asymmetric_keys/x509_cert_parser.c | 10 +++++
>  crypto/asymmetric_keys/x509_public_key.c  | 10 +++++
>  include/crypto/public_key.h               |  2 +-
>  include/keys/asymmetric-type.h            |  3 +-
>  7 files changed, 95 insertions(+), 36 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
> index 33e77d846ca..e411f2a7a84 100644
> --- a/crypto/asymmetric_keys/asymmetric_type.c
> +++ b/crypto/asymmetric_keys/asymmetric_type.c
> @@ -36,8 +36,11 @@ static DECLARE_RWSEM(asymmetric_key_parsers_sem);
>   * find_asymmetric_key - Find a key by ID.
>   * @keyring: The keys to search.
>   * @id_0: The first ID to look for or NULL.
> - * @id_1: The second ID to look for or NULL.
> - * @partial: Use partial match if true, exact if false.
> + * @id_1: The second ID to look for or NULL, matched together with @id_0
> + * against @keyring keys' id[0] and id[1].
> + * @id_2: The fallback ID to match against @keyring keys' id[2] if both of the
> + * other IDs are NULL.
> + * @partial: Use partial match for @id_0 and @id_1 if true, exact if false.
>   *
>   * Find a key in the given keyring by identifier.  The preferred identifier is
>   * the id_0 and the fallback identifier is the id_1.  If both are given, the
> @@ -46,6 +49,7 @@ static DECLARE_RWSEM(asymmetric_key_parsers_sem);
>  struct key *find_asymmetric_key(struct key *keyring,
>  				const struct asymmetric_key_id *id_0,
>  				const struct asymmetric_key_id *id_1,
> +				const struct asymmetric_key_id *id_2,
>  				bool partial)
>  {
>  	struct key *key;
> @@ -54,14 +58,17 @@ struct key *find_asymmetric_key(struct key *keyring,
>  	char *req, *p;
>  	int len;
>  
> -	BUG_ON(!id_0 && !id_1);
> +	BUG_ON(!id_0 && !id_1 && !id_2);
>  
>  	if (id_0) {
>  		lookup = id_0->data;
>  		len = id_0->len;
> -	} else {
> +	} else if (id_1) {
>  		lookup = id_1->data;
>  		len = id_1->len;
> +	} else {
> +		lookup = id_2->data;
> +		len = id_2->len;
>  	}
>  
>  	/* Construct an identifier "id:<keyid>". */
> @@ -69,7 +76,10 @@ struct key *find_asymmetric_key(struct key *keyring,
>  	if (!req)
>  		return ERR_PTR(-ENOMEM);
>  
> -	if (partial) {
> +	if (!id_0 && !id_1) {
> +		*p++ = 'd';
> +		*p++ = 'n';
> +	} else if (partial) {
>  		*p++ = 'i';
>  		*p++ = 'd';
>  	} else {
> @@ -183,8 +193,8 @@ bool asymmetric_key_id_partial(const struct asymmetric_key_id *kid1,
>  EXPORT_SYMBOL_GPL(asymmetric_key_id_partial);
>  
>  /**
> - * asymmetric_match_key_ids - Search asymmetric key IDs
> - * @kids: The list of key IDs to check
> + * asymmetric_match_key_ids - Search asymmetric key IDs 1 & 2
> + * @kids: The pair of key IDs to check
>   * @match_id: The key ID we're looking for
>   * @match: The match function to use
>   */
> @@ -198,7 +208,7 @@ static bool asymmetric_match_key_ids(
>  
>  	if (!kids || !match_id)
>  		return false;
> -	for (i = 0; i < ARRAY_SIZE(kids->id); i++)
> +	for (i = 0; i < 2; i++)
>  		if (match(kids->id[i], match_id))
>  			return true;
>  	return false;

Why are key ID 2 and key ID 3 handled differently? They are both
optional.

> @@ -242,7 +252,7 @@ struct asymmetric_key_id *asymmetric_key_hex_to_key_id(const char *id)
>  }
>  
>  /*
> - * Match asymmetric keys by an exact match on an ID.
> + * Match asymmetric keys by an exact match on one of the first two IDs.
>   */
>  static bool asymmetric_key_cmp(const struct key *key,
>  			       const struct key_match_data *match_data)
> @@ -255,7 +265,7 @@ static bool asymmetric_key_cmp(const struct key *key,
>  }
>  
>  /*
> - * Match asymmetric keys by a partial match on an IDs.
> + * Match asymmetric keys by a partial match on one of the first two IDs.
>   */
>  static bool asymmetric_key_cmp_partial(const struct key *key,
>  				       const struct key_match_data *match_data)
> @@ -267,6 +277,18 @@ static bool asymmetric_key_cmp_partial(const struct key *key,
>  					asymmetric_key_id_partial);
>  }
>  
> +/*
> + * Match asymmetric keys by an exact match on the third IDs.
> + */
> +static bool asymmetric_key_cmp_name(const struct key *key,
> +				    const struct key_match_data *match_data)
> +{
> +	const struct asymmetric_key_ids *kids = asymmetric_key_ids(key);
> +	const struct asymmetric_key_id *match_id = match_data->preparsed;
> +
> +	return kids && asymmetric_key_id_same(kids->id[2], match_id);
> +}
> +
>  /*
>   * Preparse the match criterion.  If we don't set lookup_type and cmp,
>   * the default will be an exact match on the key description.
> @@ -274,8 +296,9 @@ static bool asymmetric_key_cmp_partial(const struct key *key,
>   * There are some specifiers for matching key IDs rather than by the key
>   * description:
>   *
> - *	"id:<id>" - find a key by partial match on any available ID
> - *	"ex:<id>" - find a key by exact match on any available ID
> + *	"id:<id>" - find a key by partial match on one of the first two IDs
> + *	"ex:<id>" - find a key by exact match on one of the first two IDs
> + *	"dn:<id>" - find a key by exact match on the third ID
>   *
>   * These have to be searched by iteration rather than by direct lookup because
>   * the key is hashed according to its description.
> @@ -299,6 +322,11 @@ static int asymmetric_key_match_preparse(struct key_match_data *match_data)
>  		   spec[1] == 'x' &&
>  		   spec[2] == ':') {
>  		id = spec + 3;
> +	} else if (spec[0] == 'd' &&
> +		   spec[1] == 'n' &&
> +		   spec[2] == ':') {
> +		id = spec + 3;
> +		cmp = asymmetric_key_cmp_name;
>  	} else {
>  		goto default_match;
>  	}
> diff --git a/crypto/asymmetric_keys/pkcs7_trust.c b/crypto/asymmetric_keys/pkcs7_trust.c
> index 61af3c4d82c..998ba0e2ffb 100644
> --- a/crypto/asymmetric_keys/pkcs7_trust.c
> +++ b/crypto/asymmetric_keys/pkcs7_trust.c
> @@ -48,7 +48,7 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
>  		 * keys.
>  		 */
>  		key = find_asymmetric_key(trust_keyring,
> -					  x509->id, x509->skid, false);
> +					  x509->id, x509->skid, NULL, false);
>  		if (!IS_ERR(key)) {
>  			/* One of the X.509 certificates in the PKCS#7 message
>  			 * is apparently the same as one we already trust.
> @@ -82,7 +82,7 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
>  		key = find_asymmetric_key(trust_keyring,
>  					  last->sig->auth_ids[0],
>  					  last->sig->auth_ids[1],
> -					  false);
> +					  NULL, false);
>  		if (!IS_ERR(key)) {
>  			x509 = last;
>  			pr_devel("sinfo %u: Root cert %u signer is key %x\n",
> @@ -97,7 +97,7 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
>  	 * the signed info directly.
>  	 */
>  	key = find_asymmetric_key(trust_keyring,
> -				  sinfo->sig->auth_ids[0], NULL, false);
> +				  sinfo->sig->auth_ids[0], NULL, NULL, false);
>  	if (!IS_ERR(key)) {
>  		pr_devel("sinfo %u: Direct signer is key %x\n",
>  			 sinfo->index, key_serial(key));
> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> index 77ebebada29..f77875ec942 100644
> --- a/crypto/asymmetric_keys/restrict.c
> +++ b/crypto/asymmetric_keys/restrict.c
> @@ -87,7 +87,7 @@ int restrict_link_by_signature(struct key *dest_keyring,
>  	sig = payload->data[asym_auth];
>  	if (!sig)
>  		return -ENOPKG;
> -	if (!sig->auth_ids[0] && !sig->auth_ids[1])
> +	if (!sig->auth_ids[0] && !sig->auth_ids[1] && !sig->auth_ids[2])
>  		return -ENOKEY;
>  
>  	if (ca_keyid && !asymmetric_key_id_partial(sig->auth_ids[1], ca_keyid))
> @@ -96,7 +96,7 @@ int restrict_link_by_signature(struct key *dest_keyring,
>  	/* See if we have a key that signed this one. */
>  	key = find_asymmetric_key(trust_keyring,
>  				  sig->auth_ids[0], sig->auth_ids[1],
> -				  false);
> +				  sig->auth_ids[2], false);
>  	if (IS_ERR(key))
>  		return -ENOKEY;
>  
> @@ -108,11 +108,11 @@ int restrict_link_by_signature(struct key *dest_keyring,
>  	return ret;
>  }
>  
> -static bool match_either_id(const struct asymmetric_key_ids *pair,
> +static bool match_either_id(const struct asymmetric_key_id **pair,
>  			    const struct asymmetric_key_id *single)
>  {
> -	return (asymmetric_key_id_same(pair->id[0], single) ||
> -		asymmetric_key_id_same(pair->id[1], single));
> +	return (asymmetric_key_id_same(pair[0], single) ||
> +		asymmetric_key_id_same(pair[1], single));
>  }
>  
>  static int key_or_keyring_common(struct key *dest_keyring,
> @@ -140,20 +140,22 @@ static int key_or_keyring_common(struct key *dest_keyring,
>  	sig = payload->data[asym_auth];
>  	if (!sig)
>  		return -ENOPKG;
> -	if (!sig->auth_ids[0] && !sig->auth_ids[1])
> +	if (!sig->auth_ids[0] && !sig->auth_ids[1] && !sig->auth_ids[2])
>  		return -ENOKEY;
>  
>  	if (trusted) {
>  		if (trusted->type == &key_type_keyring) {
>  			/* See if we have a key that signed this one. */
>  			key = find_asymmetric_key(trusted, sig->auth_ids[0],
> -						  sig->auth_ids[1], false);
> +						  sig->auth_ids[1],
> +						  sig->auth_ids[2], false);
>  			if (IS_ERR(key))
>  				key = NULL;
>  		} else if (trusted->type == &key_type_asymmetric) {
> -			const struct asymmetric_key_ids *signer_ids;
> +			const struct asymmetric_key_id **signer_ids;
>  
> -			signer_ids = asymmetric_key_ids(trusted);
> +			signer_ids = (const struct asymmetric_key_id **)
> +				asymmetric_key_ids(trusted)->id;
>  
>  			/*
>  			 * The auth_ids come from the candidate key (the
> @@ -164,22 +166,29 @@ static int key_or_keyring_common(struct key *dest_keyring,
>  			 * The signer_ids are identifiers for the
>  			 * signing key specified for dest_keyring.
>  			 *
> -			 * The first auth_id is the preferred id, and
> -			 * the second is the fallback. If only one
> -			 * auth_id is present, it may match against
> -			 * either signer_id. If two auth_ids are
> -			 * present, the first auth_id must match one
> -			 * signer_id and the second auth_id must match
> -			 * the second signer_id.
> +			 * The first auth_id is the preferred id, 2nd and
> +			 * 3rd are the fallbacks. If excatly one of
> +			 * auth_ids[0] and auth_ids[1] is present, it may
> +			 * match either signer_ids[0] or signed_ids[1].
> +			 * If both are present the first one may match
> +			 * either signed_id but the second one must match
> +			 * the second signer_id. If neither of them is
> +			 * available, auth_ids[2] is matched against
> +			 * signer_ids[2] as a fallback.
>  			 */
> -			if (!sig->auth_ids[0] || !sig->auth_ids[1]) {
> +			if (!sig->auth_ids[0] && !sig->auth_ids[1]) {
> +				if (asymmetric_key_id_same(signer_ids[2],
> +							   sig->auth_ids[2]))
> +					key = __key_get(trusted);
> +
> +			} else if (!sig->auth_ids[0] || !sig->auth_ids[1]) {
>  				const struct asymmetric_key_id *auth_id;
>  
>  				auth_id = sig->auth_ids[0] ?: sig->auth_ids[1];
>  				if (match_either_id(signer_ids, auth_id))
>  					key = __key_get(trusted);
>  
> -			} else if (asymmetric_key_id_same(signer_ids->id[1],
> +			} else if (asymmetric_key_id_same(signer_ids[1],
>  							  sig->auth_ids[1]) &&
>  				   match_either_id(signer_ids,
>  						   sig->auth_ids[0])) {
> @@ -193,7 +202,8 @@ static int key_or_keyring_common(struct key *dest_keyring,
>  	if (check_dest && !key) {
>  		/* See if the destination has a key that signed this one. */
>  		key = find_asymmetric_key(dest_keyring, sig->auth_ids[0],
> -					  sig->auth_ids[1], false);
> +					  sig->auth_ids[1], sig->auth_ids[2],
> +					  false);
>  		if (IS_ERR(key))
>  			key = NULL;
>  	}
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 52c9b455fc7..59dfffa77cf 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -415,8 +415,18 @@ int x509_note_issuer(void *context, size_t hdrlen,
>  		     const void *value, size_t vlen)
>  {
>  	struct x509_parse_context *ctx = context;
> +	struct asymmetric_key_id *kid;
> +
>  	ctx->cert->raw_issuer = value;
>  	ctx->cert->raw_issuer_size = vlen;
> +
> +	if (!ctx->cert->sig->auth_ids[2]) {
> +		kid = asymmetric_key_generate_id(value, vlen, "", 0);
> +		if (IS_ERR(kid))
> +			return PTR_ERR(kid);
> +		ctx->cert->sig->auth_ids[2] = kid;
> +	}
> +
>  	return x509_fabricate_name(ctx, hdrlen, tag, &ctx->cert->issuer, vlen);
>  }
>  
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index ae450eb8be1..a95cc351518 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -221,6 +221,13 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
>  		goto error_free_desc;
>  	kids->id[0] = cert->id;
>  	kids->id[1] = cert->skid;
> +	kids->id[2] = asymmetric_key_generate_id(cert->raw_subject,
> +						 cert->raw_subject_size,
> +						 "", 0);
> +	if (IS_ERR(kids->id[2])) {
> +		ret = PTR_ERR(kids->id[2]);
> +		goto error_free_kids;
> +	}
>  
>  	/* We're pinning the module by being linked against it */
>  	__module_get(public_key_subtype.owner);
> @@ -237,8 +244,11 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
>  	cert->skid = NULL;
>  	cert->sig = NULL;
>  	desc = NULL;
> +	kids = NULL;
>  	ret = 0;
>  
> +error_free_kids:
> +	kfree(kids);
>  error_free_desc:
>  	kfree(desc);
>  error_free_cert:
> diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> index 948c5203ca9..4819e63a772 100644
> --- a/include/crypto/public_key.h
> +++ b/include/crypto/public_key.h
> @@ -37,7 +37,7 @@ extern void public_key_free(struct public_key *key);
>   * Public key cryptography signature data
>   */
>  struct public_key_signature {
> -	struct asymmetric_key_id *auth_ids[2];
> +	struct asymmetric_key_id *auth_ids[3];
>  	u8 *s;			/* Signature */
>  	u32 s_size;		/* Number of bytes in signature */
>  	u8 *digest;
> diff --git a/include/keys/asymmetric-type.h b/include/keys/asymmetric-type.h
> index a29d3ff2e7e..344ab9f178d 100644
> --- a/include/keys/asymmetric-type.h
> +++ b/include/keys/asymmetric-type.h
> @@ -53,7 +53,7 @@ struct asymmetric_key_id {
>  };
>  
>  struct asymmetric_key_ids {
> -	void		*id[2];
> +	void		*id[3];
>  };
>  
>  extern bool asymmetric_key_id_same(const struct asymmetric_key_id *kid1,
> @@ -75,6 +75,7 @@ const struct asymmetric_key_ids *asymmetric_key_ids(const struct key *key)
>  extern struct key *find_asymmetric_key(struct key *keyring,
>  				       const struct asymmetric_key_id *id_0,
>  				       const struct asymmetric_key_id *id_1,
> +				       const struct asymmetric_key_id *id_2,
>  				       bool partial);
>  
>  /*
> -- 
> 2.27.0
> 
> 

This gives checkpatch error but is not due to this patch. I sent a
bug fix (and also cc'd you to that). I'll apply that to my tree as
soon as I get it ack'd first.

Overally, I don't understand differianting the semantics. Emphasis
on that I haven't been involved with asymmetric keys implementation,
so it is entirely possible that there are good reasons to do this
but the commit message did not shed any light on this.

/Jarkko