Refine looking up of attributes in arrays

There was a class of bugs for looking up invalid or empty
attributes in the internal PKCS#11 attribute arrays.

 * Refine what p11_attrs_find_valid() treats as valid
 * Rename p11_attrs_is_empty() to p11_attrs_terminator() for clarity

5 files changed, 52 insertions, 60 deletions

diff --git a/trust/builder.c b/trust/builder.c
index 3322157..87e16b2 100644
--- a/trust/builder.c
+++ b/trust/builder.c
@@ -111,8 +111,8 @@ lookup_extension (p11_builder *builder,
 	CK_OBJECT_HANDLE obj;
 	CK_ATTRIBUTE *attrs;
 	unsigned char *ext;
-	CK_ATTRIBUTE *value;
-	CK_ATTRIBUTE *id;
+	void *value;
+	size_t length;
 	node_asn *node;
 
 	CK_ATTRIBUTE match[] = {
@@ -123,33 +123,28 @@ lookup_extension (p11_builder *builder,
 	};
 
 	/* Look for a stapled certificate extension */
-	id = p11_attrs_find (cert, CKA_ID);
-	if (id != NULL && id->ulValueLen > 0) {
-		match[0].pValue = id->pValue;
-		match[0].ulValueLen = id->ulValueLen;
+	match[0].pValue = p11_attrs_find_value (cert, CKA_ID, &length);
+	if (match[0].pValue != NULL) {
+		match[0].ulValueLen = length;
 
 		obj = p11_index_find (index, match);
 		attrs = p11_index_lookup (index, obj);
 		if (attrs != NULL) {
-			value = p11_attrs_find (attrs, CKA_VALUE);
-			return_val_if_fail (value != NULL, NULL);
-
-			*ext_len = value->ulValueLen;
-			ext = memdup (value->pValue, value->ulValueLen);
-			return_val_if_fail (ext != NULL, NULL);
-			return ext;
+			value = p11_attrs_find_value (attrs, CKA_VALUE, ext_len);
+			if (value != NULL) {
+				ext = memdup (value, *ext_len);
+				return_val_if_fail (ext != NULL, NULL);
+				return ext;
+			}
 		}
 	}
 
 	/* Couldn't find a parsed extension, so look in the current certificate */
-	value = p11_attrs_find (cert, CKA_VALUE);
+	value = p11_attrs_find_value (cert, CKA_VALUE, &length);
 	if (value != NULL) {
-		node = decode_or_get_asn1 (builder, "PKIX1.Certificate",
-		                           value->pValue, value->ulValueLen);
+		node = decode_or_get_asn1 (builder, "PKIX1.Certificate", value, length);
 		return_val_if_fail (node != NULL, false);
-
-		return p11_x509_find_extension (node, oid, value->pValue,
-		                                value->ulValueLen, ext_len);
+		return p11_x509_find_extension (node, oid, value, length, ext_len);
 	}
 
 	return NULL;
@@ -498,7 +493,6 @@ certificate_populate (p11_builder *builder,
 {
 	CK_ULONG categoryv = 0UL;
 	CK_ATTRIBUTE *attrs = NULL;
-	CK_ATTRIBUTE *value;
 	node_asn *node = NULL;
 	unsigned char *der = NULL;
 	size_t der_len = 0;
@@ -509,13 +503,9 @@ certificate_populate (p11_builder *builder,
 	attrs = common_populate (builder, index, cert);
 	return_val_if_fail (attrs != NULL, NULL);
 
-	value = p11_attrs_find_valid (cert, CKA_VALUE);
-	if (value != NULL) {
-		der = value->pValue;
-		der_len = value->ulValueLen;
-		node = decode_or_get_asn1 (builder, "PKIX1.Certificate",
-		                           value->pValue, value->ulValueLen);
-	}
+	der = p11_attrs_find_value (cert, CKA_VALUE, &der_len);
+	if (der != NULL)
+		node = decode_or_get_asn1 (builder, "PKIX1.Certificate", der, der_len);
 
 	attrs = certificate_value_attrs (attrs, node, der, der_len);
 	return_val_if_fail (attrs != NULL, NULL);
@@ -666,7 +656,7 @@ attrs_filter_if_unchanged (CK_ATTRIBUTE *attrs,
 	assert (attrs != NULL);
 	assert (merge != NULL);
 
-	for (in = 0, out = 0; !p11_attrs_is_empty (merge + in); in++) {
+	for (in = 0, out = 0; !p11_attrs_terminator (merge + in); in++) {
 		attr = p11_attrs_find (attrs, merge[in].type);
 		if (attr && p11_attr_equal (attr, merge + in)) {
 			free (merge[in].pValue);
@@ -680,7 +670,7 @@ attrs_filter_if_unchanged (CK_ATTRIBUTE *attrs,
 	}
 
 	merge[out].type = CKA_INVALID;
-	assert (p11_attrs_is_empty (merge + out));
+	assert (p11_attrs_terminator (merge + out));
 }
 
 static const char *
@@ -1084,11 +1074,13 @@ replace_nss_trust_object (p11_builder *builder,
 
 	CK_ATTRIBUTE_PTR label;
 	CK_ATTRIBUTE_PTR id;
-	CK_ATTRIBUTE_PTR der;
 	CK_ATTRIBUTE_PTR subject;
 	CK_ATTRIBUTE_PTR issuer;
 	CK_ATTRIBUTE_PTR serial_number;
 
+	void *der;
+	size_t der_len;
+
 	CK_ATTRIBUTE match[] = {
 		{ CKA_CERT_SHA1_HASH, sha1v, sizeof (sha1v) },
 		{ CKA_CLASS, &klassv, sizeof (klassv) },
@@ -1097,10 +1089,10 @@ replace_nss_trust_object (p11_builder *builder,
 	};
 
 	/* Setup the hashes of the DER certificate value */
-	der = p11_attrs_find (cert, CKA_VALUE);
+	der = p11_attrs_find_value (cert, CKA_VALUE, &der_len);
 	return_if_fail (der != NULL);
-	p11_checksum_md5 (md5v, der->pValue, der->ulValueLen, NULL);
-	p11_checksum_sha1 (sha1v, der->pValue, der->ulValueLen, NULL);
+	p11_checksum_md5 (md5v, der, der_len, NULL);
+	p11_checksum_sha1 (sha1v, der, der_len, NULL);
 
 	/* If there is a non-auto-generated NSS trust object, then step away */
 	generated = CK_FALSE;
@@ -1108,20 +1100,20 @@ replace_nss_trust_object (p11_builder *builder,
 		return;
 
 	/* Copy all of the following attributes from certificate */
-	id = p11_attrs_find (cert, CKA_ID);
+	id = p11_attrs_find_valid (cert, CKA_ID);
 	return_if_fail (id != NULL);
-	subject = p11_attrs_find (cert, CKA_SUBJECT);
+	subject = p11_attrs_find_valid (cert, CKA_SUBJECT);
 	if (subject == NULL)
 		subject = &invalid;
-	issuer = p11_attrs_find (cert, CKA_ISSUER);
+	issuer = p11_attrs_find_valid (cert, CKA_ISSUER);
 	if (issuer == NULL)
 		issuer = &invalid;
-	serial_number = p11_attrs_find (cert, CKA_SERIAL_NUMBER);
+	serial_number = p11_attrs_find_valid (cert, CKA_SERIAL_NUMBER);
 	if (serial_number == NULL)
 		serial_number = &invalid;
 
 	/* Try to use the same label */
-	label = p11_attrs_find (cert, CKA_LABEL);
+	label = p11_attrs_find_valid (cert, CKA_LABEL);
 	if (label == NULL)
 		label = &invalid;
 
@@ -1178,14 +1170,14 @@ build_assertions (p11_array *array,
 	CK_ATTRIBUTE *attrs;
 	int i;
 
-	label = p11_attrs_find (cert, CKA_LABEL);
+	label = p11_attrs_find_valid (cert, CKA_LABEL);
 	if (label == NULL)
 		label = &invalid;
 
-	id = p11_attrs_find (cert, CKA_ID);
-	issuer = p11_attrs_find (cert, CKA_ISSUER);
-	serial = p11_attrs_find (cert, CKA_SERIAL_NUMBER);
-	value = p11_attrs_find (cert, CKA_VALUE);
+	id = p11_attrs_find_valid (cert, CKA_ID);
+	issuer = p11_attrs_find_valid (cert, CKA_ISSUER);
+	serial = p11_attrs_find_valid (cert, CKA_SERIAL_NUMBER);
+	value = p11_attrs_find_valid (cert, CKA_VALUE);
 
 	return_if_fail (id != NULL && issuer != NULL && serial != NULL && value != NULL);
 
@@ -1277,7 +1269,7 @@ replace_trust_assertions (p11_builder *builder,
 		{ CKA_INVALID }
 	};
 
-	value = p11_attrs_find (cert, CKA_VALUE);
+	value = p11_attrs_find_valid (cert, CKA_VALUE);
 	return_if_fail (value != NULL);
 
 	built = p11_array_new (NULL);
@@ -1308,7 +1300,7 @@ remove_trust_and_assertions (p11_builder *builder,
 		{ CKA_INVALID }
 	};
 
-	id = p11_attrs_find (attrs, CKA_ID);
+	id = p11_attrs_find_valid (attrs, CKA_ID);
 	return_if_fail (id != NULL);
 
 	/* An empty array of replacements */
@@ -1411,9 +1403,9 @@ replace_compat_for_cert (p11_builder *builder,
 		{ CKA_INVALID }
 	};
 
-	value = p11_attrs_find (attrs, CKA_VALUE);
-	id = p11_attrs_find (attrs, CKA_ID);
-	if (value == NULL || id == NULL || id->ulValueLen == 0)
+	value = p11_attrs_find_valid (attrs, CKA_VALUE);
+	id = p11_attrs_find_valid (attrs, CKA_ID);
+	if (value == NULL || id == NULL)
 		return;
 
 	/*
@@ -1446,8 +1438,8 @@ replace_compat_for_ext (p11_builder *builder,
 	CK_ATTRIBUTE *id;
 	int i;
 
-	id = p11_attrs_find (attrs, CKA_ID);
-	if (id == NULL || id->ulValueLen == 0)
+	id = p11_attrs_find_valid (attrs, CKA_ID);
+	if (id == NULL)
 		return;
 
 	handles = lookup_related (index, CKO_CERTIFICATE, id);
@@ -1477,8 +1469,8 @@ update_related_category (p11_builder *builder,
 		{ CKA_INVALID, },
 	};
 
-	id = p11_attrs_find (attrs, CKA_ID);
-	if (id == NULL || id->ulValueLen == 0)
+	id = p11_attrs_find_valid (attrs, CKA_ID);
+	if (id == NULL)
 		return;
 
 	/* Find all other objects with this handle */
diff --git a/trust/index.c b/trust/index.c
index eb6ca7f..786d840 100644
--- a/trust/index.c
+++ b/trust/index.c
@@ -364,7 +364,7 @@ index_replacev (p11_index *index,
 			continue;
 
 		handled = false;
-		attr = p11_attrs_find_valid (obj->attrs, key);
+		attr = p11_attrs_find (obj->attrs, key);
 
 		/* The match doesn't have the key, so remove it */
 		if (attr != NULL) {
diff --git a/trust/parser.c b/trust/parser.c
index 56d3226..8f37637 100644
--- a/trust/parser.c
+++ b/trust/parser.c
@@ -188,8 +188,8 @@ parse_der_x509_certificate (p11_parser *parser,
 	attrs = certificate_attrs (parser, &id, data, length);
 	return_val_if_fail (attrs != NULL, P11_PARSE_FAILURE);
 
-	value = p11_attrs_find (attrs, CKA_VALUE);
-	assert (value != NULL);
+	value = p11_attrs_find_valid (attrs, CKA_VALUE);
+	return_val_if_fail (value != NULL, P11_PARSE_FAILURE);
 	p11_asn1_cache_take (parser->asn1_cache, cert, "PKIX1.Certificate",
 	                     value->pValue, value->ulValueLen);
 
@@ -504,8 +504,8 @@ parse_openssl_trusted_certificate (p11_parser *parser,
 	return_val_if_fail (attrs != NULL, P11_PARSE_FAILURE);
 
 	/* Cache the parsed certificate ASN.1 for later use by the builder */
-	value = p11_attrs_find (attrs, CKA_VALUE);
-	assert (value != NULL);
+	value = p11_attrs_find_valid (attrs, CKA_VALUE);
+	return_val_if_fail (value != NULL, P11_PARSE_FAILURE);
 	p11_asn1_cache_take (parser->asn1_cache, cert, "PKIX1.Certificate",
 	                     value->pValue, value->ulValueLen);
 
diff --git a/trust/tests/test-data.c b/trust/tests/test-data.c
index 0ddc4c6..b235f33 100644
--- a/trust/tests/test-data.c
+++ b/trust/tests/test-data.c
@@ -116,7 +116,7 @@ test_check_attrs_msg (CuTest *cu,
 {
 	CK_ATTRIBUTE *attr;
 
-	while (!p11_attrs_is_empty (expected)) {
+	while (!p11_attrs_terminator (expected)) {
 		attr = p11_attrs_find (attrs, expected->type);
 		test_check_attr_msg (cu, file, line, expected, attr);
 		expected++;
diff --git a/trust/tests/test-module.c b/trust/tests/test-module.c
index 45af62a..4606a31 100644
--- a/trust/tests/test-module.c
+++ b/trust/tests/test-module.c
@@ -388,7 +388,7 @@ check_trust_object_hashes (CuTest *cu,
 	rv = test.module->C_GetAttributeValue (session, trust, hashes, 2);
 	CuAssertTrue (cu, rv == CKR_OK);
 
-	value = p11_attrs_find (cert, CKA_VALUE);
+	value = p11_attrs_find_valid (cert, CKA_VALUE);
 	CuAssertPtrNotNull (cu, value);
 
 	p11_checksum_md5 (check, value->pValue, value->ulValueLen, NULL);
@@ -410,7 +410,7 @@ check_has_trust_object (CuTest *cu,
 	CK_ATTRIBUTE *attr;
 	CK_ULONG count;
 
-	attr = p11_attrs_find (cert, CKA_ID);
+	attr = p11_attrs_find_valid (cert, CKA_ID);
 	CuAssertPtrNotNull (cu, attr);
 
 	match = p11_attrs_build (NULL, &klass, attr, NULL);