trust: Use a SHA-1 hash of subjectPublicKeyInfo as CKA_ID by default

This is what's recommended by the spec, and allows stapled extensions
to hang off a predictable CKA_ID.

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

6 files changed, 88 insertions, 39 deletions

diff --git a/trust/builder.c b/trust/builder.c
index 4397b9b..3322157 100644
--- a/trust/builder.c
+++ b/trust/builder.c
@@ -124,7 +124,7 @@ lookup_extension (p11_builder *builder,
 
 	/* Look for a stapled certificate extension */
 	id = p11_attrs_find (cert, CKA_ID);
-	if (id != NULL) {
+	if (id != NULL && id->ulValueLen > 0) {
 		match[0].pValue = id->pValue;
 		match[0].ulValueLen = id->ulValueLen;
 
@@ -417,6 +417,7 @@ certificate_value_attrs (CK_ATTRIBUTE *attrs,
                          const unsigned char *der,
                          size_t der_len)
 {
+	unsigned char checksum[P11_CHECKSUM_SHA1_LENGTH];
 	CK_BBOOL falsev = CK_FALSE;
 	CK_ULONG zero = 0UL;
 	CK_BYTE checkv[3];
@@ -427,7 +428,7 @@ certificate_value_attrs (CK_ATTRIBUTE *attrs,
 	CK_ATTRIBUTE trusted = { CKA_TRUSTED, &falsev, sizeof (falsev) };
 	CK_ATTRIBUTE distrusted = { CKA_X_DISTRUSTED, &falsev, sizeof (falsev) };
 	CK_ATTRIBUTE url = { CKA_URL, "", 0 };
-	CK_ATTRIBUTE hash_of_subject_public_key = { CKA_HASH_OF_SUBJECT_PUBLIC_KEY, "", 0 };
+	CK_ATTRIBUTE hash_of_subject_public_key = { CKA_HASH_OF_SUBJECT_PUBLIC_KEY, checksum, sizeof (checksum) };
 	CK_ATTRIBUTE hash_of_issuer_public_key = { CKA_HASH_OF_ISSUER_PUBLIC_KEY, "", 0 };
 	CK_ATTRIBUTE java_midp_security_domain = { CKA_JAVA_MIDP_SECURITY_DOMAIN, &zero, sizeof (zero) };
 	CK_ATTRIBUTE check_value = { CKA_CHECK_VALUE, &checkv, sizeof (checkv) };
@@ -437,6 +438,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) };
 
 	return_val_if_fail (attrs != NULL, NULL);
 
@@ -455,6 +457,11 @@ 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)) {
+		hash_of_subject_public_key.ulValueLen = 0;
+		id.type = CKA_INVALID;
+	}
+
 	if (node) {
 		labelv = p11_x509_lookup_dn_name (node, "tbsCertificate.subject",
 		                                  der, der_len, P11_OID_CN);
@@ -475,11 +482,12 @@ certificate_value_attrs (CK_ATTRIBUTE *attrs,
 
 	attrs = p11_attrs_build (attrs, &trusted, &distrusted, &url, &hash_of_issuer_public_key,
 	                         &hash_of_subject_public_key, &java_midp_security_domain,
-	                         &check_value, &start_date, &end_date,
+	                         &check_value, &start_date, &end_date, &id,
 				 &subject, &issuer, &serial_number, &label,
 				 NULL);
 	return_val_if_fail (attrs != NULL, NULL);
 
+	free (labelv);
 	return attrs;
 }
 
@@ -1405,7 +1413,7 @@ replace_compat_for_cert (p11_builder *builder,
 
 	value = p11_attrs_find (attrs, CKA_VALUE);
 	id = p11_attrs_find (attrs, CKA_ID);
-	if (value == NULL || id == NULL)
+	if (value == NULL || id == NULL || id->ulValueLen == 0)
 		return;
 
 	/*
@@ -1439,7 +1447,7 @@ replace_compat_for_ext (p11_builder *builder,
 	int i;
 
 	id = p11_attrs_find (attrs, CKA_ID);
-	if (id == NULL)
+	if (id == NULL || id->ulValueLen == 0)
 		return;
 
 	handles = lookup_related (index, CKO_CERTIFICATE, id);
@@ -1470,7 +1478,7 @@ update_related_category (p11_builder *builder,
 	};
 
 	id = p11_attrs_find (attrs, CKA_ID);
-	if (id == NULL)
+	if (id == NULL || id->ulValueLen == 0)
 		return;
 
 	/* Find all other objects with this handle */
diff --git a/trust/parser.c b/trust/parser.c
index 978d30e..0ab9468 100644
--- a/trust/parser.c
+++ b/trust/parser.c
@@ -146,17 +146,9 @@ sink_object (p11_parser *parser,
 		p11_message ("couldn't load file into objects: %s", parser->basename);
 }
 
-static void
-id_generate (p11_parser *parser,
-             CK_BYTE *vid)
-{
-	CK_ULONG val = p11_module_next_id ();
-	p11_checksum_sha1 (vid, &val, sizeof (val), NULL);
-}
-
 static CK_ATTRIBUTE *
 certificate_attrs (p11_parser *parser,
-                   CK_BYTE *idv,
+                   CK_ATTRIBUTE *id,
                    const unsigned char *der,
                    size_t der_len)
 {
@@ -168,9 +160,8 @@ certificate_attrs (p11_parser *parser,
 	CK_ATTRIBUTE klass = { CKA_CLASS, &klassv, sizeof (klassv) };
 	CK_ATTRIBUTE certificate_type = { CKA_CERTIFICATE_TYPE, &x509, sizeof (x509) };
 	CK_ATTRIBUTE value = { CKA_VALUE, (void *)der, der_len };
-	CK_ATTRIBUTE id = { CKA_ID, idv, ID_LENGTH };
 
-	return p11_attrs_build (NULL, &klass, &modifiable, &certificate_type, &value, &id, NULL);
+	return p11_attrs_build (NULL, &klass, &modifiable, &certificate_type, &value, id, NULL);
 }
 
 static int
@@ -179,6 +170,7 @@ parse_der_x509_certificate (p11_parser *parser,
                             size_t length)
 {
 	CK_BYTE idv[ID_LENGTH];
+	CK_ATTRIBUTE id = { CKA_ID, idv, sizeof (idv) };
 	CK_ATTRIBUTE *attrs;
 	CK_ATTRIBUTE *value;
 	node_asn *cert;
@@ -188,9 +180,10 @@ parse_der_x509_certificate (p11_parser *parser,
 		return P11_PARSE_UNRECOGNIZED;
 
 	/* The CKA_ID links related objects */
-	id_generate (parser, idv);
+	if (!p11_x509_calc_keyid (cert, data, length, idv))
+		id.type = CKA_INVALID;
 
-	attrs = certificate_attrs (parser, idv, data, length);
+	attrs = certificate_attrs (parser, &id, data, length);
 	return_val_if_fail (attrs != NULL, P11_PARSE_FAILURE);
 
 	value = p11_attrs_find (attrs, CKA_VALUE);
@@ -204,7 +197,7 @@ parse_der_x509_certificate (p11_parser *parser,
 
 static CK_ATTRIBUTE *
 extension_attrs (p11_parser *parser,
-                 CK_BYTE *idv,
+                 CK_ATTRIBUTE *id,
                  const unsigned char *oid_der,
                  CK_BBOOL vcritical,
                  const unsigned char *ext_der,
@@ -218,17 +211,16 @@ extension_attrs (p11_parser *parser,
 	CK_ATTRIBUTE critical = { CKA_X_CRITICAL, &vcritical, sizeof (vcritical) };
 	CK_ATTRIBUTE oid = { CKA_OBJECT_ID, (void *)oid_der, p11_oid_length (oid_der) };
 	CK_ATTRIBUTE value = { CKA_VALUE, (void *)ext_der, ext_len };
-	CK_ATTRIBUTE id = { CKA_ID, idv, ID_LENGTH };
 
 	if (ext_der == NULL)
 		value.type = CKA_INVALID;
 
-	return p11_attrs_build (NULL, &id, &klass, &modifiable, &oid, &critical, &value, NULL);
+	return p11_attrs_build (NULL, id, &klass, &modifiable, &oid, &critical, &value, NULL);
 }
 
 static CK_ATTRIBUTE *
 stapled_attrs (p11_parser *parser,
-               CK_BYTE *idv,
+               CK_ATTRIBUTE *id,
                const unsigned char *oid,
                CK_BBOOL critical,
                node_asn *ext)
@@ -237,7 +229,7 @@ stapled_attrs (p11_parser *parser,
 	unsigned char *der;
 	size_t len;
 
-	attrs = extension_attrs (parser, idv, oid, critical, NULL, 0);
+	attrs = extension_attrs (parser, id, oid, critical, NULL, 0);
 	return_val_if_fail (attrs != NULL, NULL);
 
 	der = p11_asn1_encode (ext, &len);
@@ -285,7 +277,7 @@ load_seq_of_oid_str (node_asn *node,
 
 static CK_ATTRIBUTE *
 stapled_eku_attrs (p11_parser *parser,
-                   CK_BYTE *idv,
+                   CK_ATTRIBUTE *id,
                    const unsigned char *oid,
                    CK_BBOOL critical,
                    p11_dict *oid_strs)
@@ -333,7 +325,7 @@ stapled_eku_attrs (p11_parser *parser,
 	}
 
 
-	attrs = stapled_attrs (parser, idv, oid, critical, dest);
+	attrs = stapled_attrs (parser, id, oid, critical, dest);
 	asn1_delete_structure (&dest);
 
 	return attrs;
@@ -342,7 +334,7 @@ stapled_eku_attrs (p11_parser *parser,
 static CK_ATTRIBUTE *
 build_openssl_extensions (p11_parser *parser,
                           CK_ATTRIBUTE *cert,
-                          CK_BYTE *idv,
+                          CK_ATTRIBUTE *id,
                           node_asn *aux,
                           const unsigned char *aux_der,
                           size_t aux_len)
@@ -395,7 +387,7 @@ build_openssl_extensions (p11_parser *parser,
 	 */
 
 	if (trust) {
-		attrs = stapled_eku_attrs (parser, idv, P11_OID_EXTENDED_KEY_USAGE, CK_TRUE, trust);
+		attrs = stapled_eku_attrs (parser, id, P11_OID_EXTENDED_KEY_USAGE, CK_TRUE, trust);
 		return_val_if_fail (attrs != NULL, NULL);
 		sink_object (parser, attrs);
 	}
@@ -409,7 +401,7 @@ build_openssl_extensions (p11_parser *parser,
 	 */
 
 	if (reject && p11_dict_size (reject) > 0) {
-		attrs = stapled_eku_attrs (parser, idv, P11_OID_OPENSSL_REJECT, CK_FALSE, reject);
+		attrs = stapled_eku_attrs (parser, id, P11_OID_OPENSSL_REJECT, CK_FALSE, reject);
 		return_val_if_fail (attrs != NULL, NULL);
 		sink_object (parser, attrs);
 	}
@@ -455,7 +447,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, idv, P11_OID_SUBJECT_KEY_IDENTIFIER, CK_FALSE,
+		attrs = extension_attrs (parser, id, P11_OID_SUBJECT_KEY_IDENTIFIER, CK_FALSE,
 		                         aux_der + start, (end - start) + 1);
 		return_val_if_fail (attrs != NULL, NULL);
 		sink_object (parser, attrs);
@@ -472,6 +464,7 @@ parse_openssl_trusted_certificate (p11_parser *parser,
 {
 	CK_ATTRIBUTE *attrs;
 	CK_BYTE idv[ID_LENGTH];
+	CK_ATTRIBUTE id = { CKA_ID, idv, sizeof (idv) };
 	CK_ATTRIBUTE *value;
 	char *label = NULL;
 	node_asn *cert;
@@ -502,9 +495,10 @@ parse_openssl_trusted_certificate (p11_parser *parser,
 	}
 
 	/* The CKA_ID links related objects */
-	id_generate (parser, idv);
+	if (!p11_x509_calc_keyid (cert, data, cert_len, idv))
+		id.type = CKA_INVALID;
 
-	attrs = certificate_attrs (parser, idv, data, cert_len);
+	attrs = certificate_attrs (parser, &id, data, cert_len);
 	return_val_if_fail (attrs != NULL, P11_PARSE_FAILURE);
 
 	/* Cache the parsed certificate ASN.1 for later use by the builder */
@@ -526,7 +520,7 @@ parse_openssl_trusted_certificate (p11_parser *parser,
 		return_val_if_fail (attrs != NULL, P11_PARSE_FAILURE);
 	}
 
-	attrs = build_openssl_extensions (parser, attrs, idv, aux,
+	attrs = build_openssl_extensions (parser, attrs, &id, aux,
 	                                  data + cert_len, length - cert_len);
 	return_val_if_fail (attrs != NULL, P11_PARSE_FAILURE);
 
diff --git a/trust/tests/files/verisign-v1.der b/trust/tests/files/verisign-v1.der
new file mode 100644
index 0000000..bcd5ebb
--- /dev/null
+++ b/trust/tests/files/verisign-v1.der
diff --git a/trust/tests/test-builder.c b/trust/tests/test-builder.c
index 8ffab88..f879706 100644
--- a/trust/tests/test-builder.c
+++ b/trust/tests/test-builder.c
@@ -152,6 +152,21 @@ test_build_certificate (CuTest *cu)
 		{ CKA_INVALID },
 	};
 
+	CK_ATTRIBUTE expected[] = {
+		{ CKA_CERTIFICATE_TYPE, &x509, sizeof (x509) },
+		{ CKA_CERTIFICATE_CATEGORY, &certificate_authority, sizeof (certificate_authority) },
+		{ CKA_VALUE, (void *)test_cacert3_ca_der, sizeof (test_cacert3_ca_der) },
+		{ CKA_CHECK_VALUE, "\xad\x7c\x3f", 3 },
+		{ CKA_START_DATE, "20110523", 8 },
+		{ CKA_END_DATE, "20210520", 8, },
+		{ CKA_SUBJECT, (void *)test_cacert3_ca_subject, sizeof (test_cacert3_ca_subject) },
+		{ 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_INVALID },
+	};
+
 	CK_ATTRIBUTE *attrs;
 	CK_ATTRIBUTE *merge;
 	CK_RV rv;
@@ -163,7 +178,7 @@ test_build_certificate (CuTest *cu)
 	rv = p11_builder_build (test.builder, test.index, &attrs, merge);
 	CuAssertIntEquals (cu, CKR_OK, rv);
 
-	test_check_cacert3_ca (cu, attrs, "the label");
+	test_check_attrs (cu, expected, attrs);
 	p11_attrs_free (attrs);
 
 	teardown (cu);
@@ -1406,6 +1421,7 @@ test_changed_without_id (CuTest *cu)
 		{ CKA_CERTIFICATE_CATEGORY, &certificate_authority, sizeof (certificate_authority) },
 		{ CKA_VALUE, (void *)test_cacert3_ca_der, sizeof (test_cacert3_ca_der) },
 		{ CKA_TRUSTED, &truev, sizeof (truev) },
+		{ CKA_ID, NULL, 0, },
 		{ CKA_INVALID },
 	};
 
diff --git a/trust/tests/test-module.c b/trust/tests/test-module.c
index 9c633f0..772dc8a 100644
--- a/trust/tests/test-module.c
+++ b/trust/tests/test-module.c
@@ -82,7 +82,7 @@ setup (CuTest *cu)
 	CuAssertTrue (cu, rv == CKR_OK);
 
 	memset (&args, 0, sizeof (args));
-	paths = SRCDIR "/input:" SRCDIR "/files/cacert-ca.der:" SRCDIR "/files/testing-server.der";
+	paths = SRCDIR "/input:" SRCDIR "/files/self-signed-with-ku.der:" SRCDIR "/files/thawte.pem";
 	if (asprintf (&arguments, "paths='%s'", paths) < 0)
 		CuAssertTrue (cu, false && "not reached");
 	args.pReserved = arguments;
@@ -154,8 +154,8 @@ test_get_slot_info (CuTest *cu)
 	/* These are the paths passed in in setup() */
 	const char *paths[] = {
 		SRCDIR "/input",
-		SRCDIR "/files/cacert-ca.der",
-		SRCDIR "/files/testing-server.der"
+		SRCDIR "/files/self-signed-with-ku.der",
+		SRCDIR "/files/thawte.pem"
 	};
 
 	setup (cu);
@@ -191,8 +191,8 @@ test_get_token_info (CuTest *cu)
 	/* These are the paths passed in in setup() */
 	const char *labels[] = {
 		"input",
-		"cacert-ca.der",
-		"testing-server.der"
+		"self-signed-with-ku.der",
+		"thawte.pem"
 	};
 
 	setup (cu);
diff --git a/trust/tests/test-parser.c b/trust/tests/test-parser.c
index 3ad89da..a63d7a5 100644
--- a/trust/tests/test-parser.c
+++ b/trust/tests/test-parser.c
@@ -339,6 +339,36 @@ test_parse_anchor (CuTest *cu)
 	teardown (cu);
 }
 
+static void
+test_parse_thawte (CuTest *cu)
+{
+	CK_ATTRIBUTE *cert;
+	int ret;
+
+	CK_ATTRIBUTE expected[] = {
+		{ CKA_CERTIFICATE_TYPE, &x509, sizeof (x509) },
+		{ CKA_CLASS, &certificate, sizeof (certificate) },
+		{ CKA_MODIFIABLE, &falsev, sizeof (falsev) },
+		{ CKA_TRUSTED, &falsev, sizeof (falsev) },
+		{ CKA_X_DISTRUSTED, &falsev, sizeof (falsev) },
+		{ CKA_INVALID },
+	};
+
+	setup (cu);
+
+	ret = p11_parse_file (test.parser, SRCDIR "/files/thawte.pem",
+	                      P11_PARSE_FLAG_NONE);
+	CuAssertIntEquals (cu, P11_PARSE_SUCCESS, ret);
+
+	/* Should have gotten certificate  */
+	CuAssertIntEquals (cu, 1, p11_index_size (test.index));
+
+	cert = parsed_attrs (certificate_match);
+	test_check_attrs (cu, expected, cert);
+
+	teardown (cu);
+}
+
 /* TODO: A certificate that uses generalTime needs testing */
 
 static void
@@ -393,6 +423,7 @@ main (void)
 	SUITE_ADD_TEST (suite, test_parse_openssl_trusted);
 	SUITE_ADD_TEST (suite, test_parse_openssl_distrusted);
 	SUITE_ADD_TEST (suite, test_parse_anchor);
+	SUITE_ADD_TEST (suite, test_parse_thawte);
 	SUITE_ADD_TEST (suite, test_parse_invalid_file);
 	SUITE_ADD_TEST (suite, test_parse_unrecognized);