Attachment 107606 Details for Bug 84761 – trust: Certificate CKA_ID is SubjectKeyIdentifier if possible

[patch] trust: Certificate CKA_ID is SubjectKeyIdentifier if possible

trust-Certificate-CKAID-is-SubjectKeyIdentifier-if.patch (text/plain), 16.43 KB, created by Stef Walter on 2014-10-09 09:55:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Stef Walter

Created: 2014-10-09 09:55:00 UTC

Size: 16.43 KB

patch

obsolete

>From c11d5933fc08935e07f8109a186cf5f8c5ef17a2 Mon Sep 17 00:00:00 2001
>From: Stef Walter <stefw@redhat.com>
>Date: Thu, 9 Oct 2014 08:15:29 +0200
>Subject: [PATCH] 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
>---
> trust/builder.c      | 55 ++++++++++++++++++++++++++++++++++++++++++----------
> trust/parser.c       | 37 ++++++++++-------------------------
> trust/test-builder.c |  2 +-
> trust/test-parser.c  |  2 --
> trust/test-trust.c   |  2 ++
> trust/x509.c         | 32 +++++++++++++++++++++++++-----
> trust/x509.h         |  7 ++++++-
> 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,
>-- 
>1.9.3

Actions: View | Splinter Review

Attachments on bug 84761: 107606

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.