trust: Certificate CKA_ID is SubjectKeyIdentifier if possible

The PKCS#11 spec states that the CKA_ID should match the
SubjectKeyIdentifier if such an extension is present.

We delay the filling of CKA_ID until the builder phase of populating
attributes which allows us to have more control over how this works.

Note that we don't make CKA_ID reflect SubjectKeyIdentifier *attached*
extensions. The CKA_ID isn't supposed to change after object creation.
Making it dependent on attached extensions would be making promises
we cannot keep, since attached extensions can be added/removed at any
time.

This also means the CKA_ID of attached extensions and certificates
won't necessarily match up, but that was never promised, and not how
attached extensions should be matched to their certificate anyway.

Based on a patch and research done by David Woodhouse.

https://bugs.freedesktop.org/show_bug.cgi?id=84761

7 files changed, 91 insertions, 46 deletions

diff --git a/trust/builder.c b/trust/builder.c
index 4c62fac..6e8a1e4 100644
--- a/trust/builder.c
+++ b/trust/builder.c
@@ -610,13 +610,18 @@ calc_certificate_category (p11_builder *builder,
 }
 
 static CK_ATTRIBUTE *
-certificate_value_attrs (CK_ATTRIBUTE *attrs,
+certificate_value_attrs (p11_builder *builder,
+                         CK_ATTRIBUTE *attrs,
                          node_asn *node,
                          const unsigned char *der,
                          size_t der_len,
                          CK_ATTRIBUTE *public_key)
 {
 	unsigned char checksum[P11_DIGEST_SHA1_LEN];
+	unsigned char *keyid = NULL;
+	size_t keyid_len;
+	unsigned char *ext = NULL;
+	size_t ext_len;
 	CK_BBOOL falsev = CK_FALSE;
 	CK_ULONG zero = 0UL;
 	CK_BYTE checkv[3];
@@ -637,7 +642,7 @@ certificate_value_attrs (CK_ATTRIBUTE *attrs,
 	CK_ATTRIBUTE issuer = { CKA_ISSUER, "", 0 };
 	CK_ATTRIBUTE serial_number = { CKA_SERIAL_NUMBER, "", 0 };
 	CK_ATTRIBUTE label = { CKA_LABEL };
-	CK_ATTRIBUTE id = { CKA_ID, checksum, sizeof (checksum) };
+	CK_ATTRIBUTE id = { CKA_ID, NULL, 0 };
 
 	return_val_if_fail (attrs != NULL, NULL);
 
@@ -660,9 +665,23 @@ certificate_value_attrs (CK_ATTRIBUTE *attrs,
 		subject.type = CKA_INVALID;
 	calc_element (node, der, der_len, "tbsCertificate.serialNumber", &serial_number);
 
-	if (!node || !p11_x509_calc_keyid (node, der, der_len, checksum)) {
+	/* Try to build a keyid from an extension */
+	if (node) {
+		ext = p11_x509_find_extension (node, P11_OID_SUBJECT_KEY_IDENTIFIER, der, der_len, &ext_len);
+		if (ext) {
+			keyid = p11_x509_parse_subject_key_identifier (builder->asn1_defs, ext,
+			                                               ext_len, &keyid_len);
+			id.pValue = keyid;
+			id.ulValueLen = keyid_len;
+		}
+	}
+
+	if (!node || !p11_x509_hash_subject_public_key (node, der, der_len, checksum))
 		hash_of_subject_public_key.ulValueLen = 0;
-		id.type = CKA_INVALID;
+
+	if (id.pValue == NULL) {
+		id.pValue = hash_of_subject_public_key.pValue;
+		id.ulValueLen = hash_of_subject_public_key.ulValueLen;
 	}
 
 	if (node) {
@@ -690,6 +709,8 @@ certificate_value_attrs (CK_ATTRIBUTE *attrs,
 				 NULL);
 	return_val_if_fail (attrs != NULL, NULL);
 
+	free (ext);
+	free (keyid);
 	free (labelv);
 	return attrs;
 }
@@ -716,7 +737,7 @@ certificate_populate (p11_builder *builder,
 	if (der != NULL)
 		node = decode_or_get_asn1 (builder, "PKIX1.Certificate", der, der_len);
 
-	attrs = certificate_value_attrs (attrs, node, der, der_len, &public_key);
+	attrs = certificate_value_attrs (builder, attrs, node, der, der_len, &public_key);
 	return_val_if_fail (attrs != NULL, NULL);
 
 	if (!calc_certificate_category (builder, index, cert, &public_key, &categoryv))
@@ -794,8 +815,11 @@ extension_populate (p11_builder *builder,
                     p11_index *index,
                     CK_ATTRIBUTE *extension)
 {
-	CK_ATTRIBUTE object_id = { CKA_OBJECT_ID };
+	unsigned char checksum[P11_DIGEST_SHA1_LEN];
+	CK_ATTRIBUTE object_id = { CKA_INVALID };
+	CK_ATTRIBUTE id = { CKA_INVALID };
 	CK_ATTRIBUTE *attrs = NULL;
+
 	void *der;
 	size_t len;
 	node_asn *asn;
@@ -803,6 +827,16 @@ extension_populate (p11_builder *builder,
 	attrs = common_populate (builder, index, extension);
 	return_val_if_fail (attrs != NULL, NULL);
 
+	if (!p11_attrs_find_valid (attrs, CKA_ID)) {
+		der = p11_attrs_find_value (extension, CKA_PUBLIC_KEY_INFO, &len);
+		return_val_if_fail (der != NULL, NULL);
+
+		p11_digest_sha1 (checksum, der, len, NULL);
+		id.pValue = checksum;
+		id.ulValueLen = sizeof (checksum);
+		id.type = CKA_ID;
+	}
+
 	/* Pull the object id out of the extension if not present */
 	if (!p11_attrs_find_valid (attrs, CKA_OBJECT_ID)) {
 		der = p11_attrs_find_value (extension, CKA_VALUE, &len);
@@ -811,12 +845,13 @@ extension_populate (p11_builder *builder,
 		asn = decode_or_get_asn1 (builder, "PKIX1.Extension", der, len);
 		return_val_if_fail (asn != NULL, NULL);
 
-		if (calc_element (asn, der, len, "extnID", &object_id)) {
-			attrs = p11_attrs_build (attrs, &object_id, NULL);
-			return_val_if_fail (attrs != NULL, NULL);
-		}
+		if (calc_element (asn, der, len, "extnID", &object_id))
+			object_id.type = CKA_OBJECT_ID;
 	}
 
+	attrs = p11_attrs_build (attrs, &object_id, &id, NULL);
+	return_val_if_fail (attrs != NULL, NULL);
+
 	return attrs;
 }
 
diff --git a/trust/parser.c b/trust/parser.c
index 7f523e9..7b569d9 100644
--- a/trust/parser.c
+++ b/trust/parser.c
@@ -152,7 +152,6 @@ sink_object (p11_parser *parser,
 
 static CK_ATTRIBUTE *
 certificate_attrs (p11_parser *parser,
-                   CK_ATTRIBUTE *id,
                    const unsigned char *der,
                    size_t der_len)
 {
@@ -165,7 +164,7 @@ certificate_attrs (p11_parser *parser,
 	CK_ATTRIBUTE certificate_type = { CKA_CERTIFICATE_TYPE, &x509, sizeof (x509) };
 	CK_ATTRIBUTE value = { CKA_VALUE, (void *)der, der_len };
 
-	return p11_attrs_build (NULL, &klass, &modifiable, &certificate_type, &value, id, NULL);
+	return p11_attrs_build (NULL, &klass, &modifiable, &certificate_type, &value, NULL);
 }
 
 int
@@ -174,8 +173,6 @@ p11_parser_format_x509 (p11_parser *parser,
                         size_t length)
 {
 	char message[ASN1_MAX_ERROR_DESCRIPTION_SIZE];
-	CK_BYTE idv[ID_LENGTH];
-	CK_ATTRIBUTE id = { CKA_ID, idv, sizeof (idv) };
 	CK_ATTRIBUTE *attrs;
 	CK_ATTRIBUTE *value;
 	node_asn *cert;
@@ -184,11 +181,7 @@ p11_parser_format_x509 (p11_parser *parser,
 	if (cert == NULL)
 		return P11_PARSE_UNRECOGNIZED;
 
-	/* The CKA_ID links related objects */
-	if (!p11_x509_calc_keyid (cert, data, length, idv))
-		id.type = CKA_INVALID;
-
-	attrs = certificate_attrs (parser, &id, data, length);
+	attrs = certificate_attrs (parser, data, length);
 	return_val_if_fail (attrs != NULL, P11_PARSE_FAILURE);
 
 	value = p11_attrs_find_valid (attrs, CKA_VALUE);
@@ -202,7 +195,6 @@ p11_parser_format_x509 (p11_parser *parser,
 
 static CK_ATTRIBUTE *
 extension_attrs (p11_parser *parser,
-                 CK_ATTRIBUTE *id,
                  CK_ATTRIBUTE *public_key_info,
                  const char *oid_str,
                  const unsigned char *oid_der,
@@ -223,7 +215,7 @@ extension_attrs (p11_parser *parser,
 	size_t len;
 	int ret;
 
-	attrs = p11_attrs_build (NULL, id, public_key_info, &klass, &modifiable, &oid, NULL);
+	attrs = p11_attrs_build (NULL, public_key_info, &klass, &modifiable, &oid, NULL);
 	return_val_if_fail (attrs != NULL, NULL);
 
 	dest = p11_asn1_create (parser->asn1_defs, "PKIX1.Extension");
@@ -252,7 +244,6 @@ extension_attrs (p11_parser *parser,
 
 static CK_ATTRIBUTE *
 attached_attrs (p11_parser *parser,
-                CK_ATTRIBUTE *id,
                 CK_ATTRIBUTE *public_key_info,
                 const char *oid_str,
                 const unsigned char *oid_der,
@@ -266,7 +257,7 @@ attached_attrs (p11_parser *parser,
 	der = p11_asn1_encode (ext, &len);
 	return_val_if_fail (der != NULL, NULL);
 
-	attrs = extension_attrs (parser, id, public_key_info, oid_str, oid_der,
+	attrs = extension_attrs (parser, public_key_info, oid_str, oid_der,
 	                         critical, der, len);
 	return_val_if_fail (attrs != NULL, NULL);
 
@@ -303,7 +294,6 @@ load_seq_of_oid_str (node_asn *node,
 
 static CK_ATTRIBUTE *
 attached_eku_attrs (p11_parser *parser,
-                    CK_ATTRIBUTE *id,
                     CK_ATTRIBUTE *public_key_info,
                     const char *oid_str,
                     const unsigned char *oid_der,
@@ -353,7 +343,7 @@ attached_eku_attrs (p11_parser *parser,
 	}
 
 
-	attrs = attached_attrs (parser, id, public_key_info, oid_str, oid_der, critical, dest);
+	attrs = attached_attrs (parser, public_key_info, oid_str, oid_der, critical, dest);
 	asn1_delete_structure (&dest);
 
 	return attrs;
@@ -362,7 +352,6 @@ attached_eku_attrs (p11_parser *parser,
 static CK_ATTRIBUTE *
 build_openssl_extensions (p11_parser *parser,
                           CK_ATTRIBUTE *cert,
-                          CK_ATTRIBUTE *id,
                           CK_ATTRIBUTE *public_key_info,
                           node_asn *aux,
                           const unsigned char *aux_der,
@@ -416,7 +405,7 @@ build_openssl_extensions (p11_parser *parser,
 	 */
 
 	if (trust) {
-		attrs = attached_eku_attrs (parser, id, public_key_info,
+		attrs = attached_eku_attrs (parser, public_key_info,
 		                            P11_OID_EXTENDED_KEY_USAGE_STR,
 		                            P11_OID_EXTENDED_KEY_USAGE,
 		                            true, trust);
@@ -433,7 +422,7 @@ build_openssl_extensions (p11_parser *parser,
 	 */
 
 	if (reject && p11_dict_size (reject) > 0) {
-		attrs = attached_eku_attrs (parser, id, public_key_info,
+		attrs = attached_eku_attrs (parser, public_key_info,
 		                            P11_OID_OPENSSL_REJECT_STR,
 		                            P11_OID_OPENSSL_REJECT,
 		                            false, reject);
@@ -482,7 +471,7 @@ build_openssl_extensions (p11_parser *parser,
 	return_val_if_fail (ret == ASN1_SUCCESS || ret == ASN1_ELEMENT_NOT_FOUND, NULL);
 
 	if (ret == ASN1_SUCCESS) {
-		attrs = extension_attrs (parser, id, public_key_info,
+		attrs = extension_attrs (parser, public_key_info,
 		                         P11_OID_SUBJECT_KEY_IDENTIFIER_STR,
 		                         P11_OID_SUBJECT_KEY_IDENTIFIER,
 		                         false, aux_der + start, (end - start) + 1);
@@ -501,8 +490,6 @@ parse_openssl_trusted_certificate (p11_parser *parser,
 {
 	char message[ASN1_MAX_ERROR_DESCRIPTION_SIZE];
 	CK_ATTRIBUTE *attrs;
-	CK_BYTE idv[ID_LENGTH];
-	CK_ATTRIBUTE id = { CKA_ID, idv, sizeof (idv) };
 	CK_ATTRIBUTE public_key_info = { CKA_PUBLIC_KEY_INFO };
 	CK_ATTRIBUTE *value;
 	char *label = NULL;
@@ -539,11 +526,7 @@ parse_openssl_trusted_certificate (p11_parser *parser,
 		}
 	}
 
-	/* The CKA_ID links related objects */
-	if (!p11_x509_calc_keyid (cert, data, cert_len, idv))
-		id.type = CKA_INVALID;
-
-	attrs = certificate_attrs (parser, &id, data, cert_len);
+	attrs = certificate_attrs (parser, data, cert_len);
 	return_val_if_fail (attrs != NULL, P11_PARSE_FAILURE);
 
 	/* Cache the parsed certificate ASN.1 for later use by the builder */
@@ -570,7 +553,7 @@ parse_openssl_trusted_certificate (p11_parser *parser,
 			return_val_if_fail (attrs != NULL, P11_PARSE_FAILURE);
 		}
 
-		attrs = build_openssl_extensions (parser, attrs, &id, &public_key_info, aux,
+		attrs = build_openssl_extensions (parser, attrs, &public_key_info, aux,
 		                                  data + cert_len, length - cert_len);
 		return_val_if_fail (attrs != NULL, P11_PARSE_FAILURE);
 	}
diff --git a/trust/test-builder.c b/trust/test-builder.c
index bf1eed1..5f4b823 100644
--- a/trust/test-builder.c
+++ b/trust/test-builder.c
@@ -160,7 +160,7 @@ test_build_certificate (void)
 		{ CKA_ISSUER, (void *)test_cacert3_ca_issuer, sizeof (test_cacert3_ca_issuer) },
 		{ CKA_SERIAL_NUMBER, (void *)test_cacert3_ca_serial, sizeof (test_cacert3_ca_serial) },
 		{ CKA_LABEL, "the label", 9 },
-		{ CKA_ID, "\xf0""a\xd8?\x95\x8fMx\xb1G\xb3\x13""9\x97\x8e\xa9\xc2Q\xba\x9b", 20},
+		{ CKA_ID, "u\xa8q`L\x88\x13\xf0x\xd9\x89w\xb5m\xc5\x89\xdf\xbc\xb1z", 20},
 		{ CKA_INVALID },
 	};
 
diff --git a/trust/test-parser.c b/trust/test-parser.c
index 201ed81..b5c2525 100644
--- a/trust/test-parser.c
+++ b/trust/test-parser.c
@@ -247,7 +247,6 @@ test_parse_openssl_trusted (void)
 		assert_ptr_not_null (object);
 
 		test_check_attrs (expected[i], object);
-		test_check_id (cert, object);
 	}
 }
 
@@ -329,7 +328,6 @@ test_parse_openssl_distrusted (void)
 		assert_ptr_not_null (object);
 
 		test_check_attrs (expected[i], object);
-		test_check_id (cert, object);
 	}
 }
 
diff --git a/trust/test-trust.c b/trust/test-trust.c
index 20306e0..802007d 100644
--- a/trust/test-trust.c
+++ b/trust/test-trust.c
@@ -131,6 +131,8 @@ test_check_attrs_msg (const char *file,
 	CK_OBJECT_CLASS klass;
 	CK_ATTRIBUTE *attr;
 
+	assert (expected != NULL);
+
 	if (!p11_attrs_find_ulong (expected, CKA_CLASS, &klass))
 		klass = CKA_INVALID;
 
diff --git a/trust/x509.c b/trust/x509.c
index b93d26c..3b4fb2d 100644
--- a/trust/x509.c
+++ b/trust/x509.c
@@ -92,10 +92,10 @@ p11_x509_find_extension (node_asn *cert,
 }
 
 bool
-p11_x509_calc_keyid (node_asn *cert,
-                     const unsigned char *der,
-                     size_t der_len,
-                     unsigned char *keyid)
+p11_x509_hash_subject_public_key (node_asn *cert,
+                                  const unsigned char *der,
+                                  size_t der_len,
+                                  unsigned char *keyid)
 {
 	int start, end;
 	size_t len;
@@ -103,7 +103,6 @@ p11_x509_calc_keyid (node_asn *cert,
 
 	return_val_if_fail (cert != NULL, NULL);
 	return_val_if_fail (der != NULL, NULL);
-	return_val_if_fail (keyid != NULL, NULL);
 
 	ret = asn1_der_decoding_startEnd (cert, der, der_len, "tbsCertificate.subjectPublicKeyInfo", &start, &end);
 	return_val_if_fail (ret == ASN1_SUCCESS, false);
@@ -114,6 +113,29 @@ p11_x509_calc_keyid (node_asn *cert,
 	return true;
 }
 
+unsigned char *
+p11_x509_parse_subject_key_identifier  (p11_dict *asn1_defs,
+                                        const unsigned char *ext_der,
+                                        size_t ext_len,
+                                        size_t *keyid_len)
+{
+	unsigned char *keyid;
+	node_asn *ext;
+
+	return_val_if_fail (keyid_len != NULL, false);
+
+	ext = p11_asn1_decode (asn1_defs, "PKIX1.SubjectKeyIdentifier", ext_der, ext_len, NULL);
+	if (ext == NULL)
+		return NULL;
+
+	keyid = p11_asn1_read (ext, "", keyid_len);
+	return_val_if_fail (keyid != NULL, NULL);
+
+	asn1_delete_structure (&ext);
+
+	return keyid;
+}
+
 bool
 p11_x509_parse_basic_constraints (p11_dict *asn1_defs,
                                   const unsigned char *ext_der,
diff --git a/trust/x509.h b/trust/x509.h
index af91c28..45fa628 100644
--- a/trust/x509.h
+++ b/trust/x509.h
@@ -46,7 +46,7 @@ unsigned char *  p11_x509_find_extension            (node_asn *cert,
                                                      size_t der_len,
                                                      size_t *ext_len);
 
-bool             p11_x509_calc_keyid                (node_asn *cert,
+bool             p11_x509_hash_subject_public_key   (node_asn *cert,
                                                      const unsigned char *der,
                                                      size_t der_len,
                                                      unsigned char *keyid);
@@ -65,6 +65,11 @@ p11_array *      p11_x509_parse_extended_key_usage  (p11_dict *asn1_defs,
                                                      const unsigned char *ext_der,
                                                      size_t ext_len);
 
+unsigned char *  p11_x509_parse_subject_key_identifier  (p11_dict *asn1_defs,
+                                                         const unsigned char *ext_der,
+                                                         size_t ext_len,
+                                                         size_t *keyid_len);
+
 char *           p11_x509_parse_dn_name             (p11_dict *asn_defs,
                                                      const unsigned char *der,
                                                      size_t der_len,