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

13 files changed, 158 insertions, 163 deletions

diff --git a/common/attrs.c b/common/attrs.c
index 0b8032c..a438264 100644
--- a/common/attrs.c
+++ b/common/attrs.c
@@ -52,7 +52,7 @@
 #define ELEMS(x) (sizeof (x) / sizeof (x[0]))
 
 bool
-p11_attrs_is_empty (const CK_ATTRIBUTE *attrs)
+p11_attrs_terminator (const CK_ATTRIBUTE *attrs)
 {
 	return (attrs == NULL || attrs->type == CKA_INVALID);
 }
@@ -65,7 +65,7 @@ p11_attrs_count (const CK_ATTRIBUTE *attrs)
 	if (attrs == NULL)
 		return 0UL;
 
-	for (count = 0; !p11_attrs_is_empty (attrs); count++, attrs++);
+	for (count = 0; !p11_attrs_terminator (attrs); count++, attrs++);
 
 	return count;
 }
@@ -79,7 +79,7 @@ p11_attrs_free (void *attrs)
 	if (!attrs)
 		return;
 
-	for (i = 0; !p11_attrs_is_empty (ats + i); i++)
+	for (i = 0; !p11_attrs_terminator (ats + i); i++)
 		free (ats[i].pValue);
 	free (ats);
 }
@@ -147,7 +147,7 @@ attrs_build (CK_ATTRIBUTE *attrs,
 
 	/* Mark this as the end */
 	(attrs + at)->type = CKA_INVALID;
-	assert (p11_attrs_is_empty (attrs + at));
+	assert (p11_attrs_terminator (attrs + at));
 	return attrs;
 }
 
@@ -248,7 +248,7 @@ p11_attrs_find (CK_ATTRIBUTE *attrs,
 {
 	CK_ULONG i;
 
-	for (i = 0; !p11_attrs_is_empty (attrs + i); i++) {
+	for (i = 0; !p11_attrs_terminator (attrs + i); i++) {
 		if (attrs[i].type == type)
 			return attrs + i;
 	}
@@ -278,7 +278,7 @@ p11_attrs_find_bool (CK_ATTRIBUTE *attrs,
 {
 	CK_ULONG i;
 
-	for (i = 0; !p11_attrs_is_empty (attrs + i); i++) {
+	for (i = 0; !p11_attrs_terminator (attrs + i); i++) {
 		if (attrs[i].type == type &&
 		    attrs[i].ulValueLen == sizeof (CK_BBOOL) &&
 		    attrs[i].pValue != NULL) {
@@ -317,7 +317,7 @@ p11_attrs_find_ulong (CK_ATTRIBUTE *attrs,
 {
 	CK_ULONG i;
 
-	for (i = 0; !p11_attrs_is_empty (attrs + i); i++) {
+	for (i = 0; !p11_attrs_terminator (attrs + i); i++) {
 		if (attrs[i].type == type &&
 		    attrs[i].ulValueLen == sizeof (CK_ULONG) &&
 		    attrs[i].pValue != NULL) {
@@ -329,24 +329,25 @@ p11_attrs_find_ulong (CK_ATTRIBUTE *attrs,
 	return false;
 }
 
-bool
-p11_attrs_findn_ulong (CK_ATTRIBUTE *attrs,
-                       CK_ULONG count,
-                       CK_ATTRIBUTE_TYPE type,
-                       CK_ULONG *value)
+void *
+p11_attrs_find_value (CK_ATTRIBUTE *attrs,
+                      CK_ATTRIBUTE_TYPE type,
+                      size_t *length)
 {
 	CK_ULONG i;
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; !p11_attrs_terminator (attrs + i); i++) {
 		if (attrs[i].type == type &&
-		    attrs[i].ulValueLen == sizeof (CK_ULONG) &&
+		    attrs[i].ulValueLen != 0 &&
+		    attrs[i].ulValueLen != (CK_ULONG)-1 &&
 		    attrs[i].pValue != NULL) {
-			*value = *((CK_ULONG *)attrs[i].pValue);
-			return true;
+			if (length)
+				*length = attrs[i].ulValueLen;
+			return attrs[i].pValue;
 		}
 	}
 
-	return false;
+	return NULL;
 }
 
 CK_ATTRIBUTE *
@@ -355,24 +356,10 @@ p11_attrs_find_valid (CK_ATTRIBUTE *attrs,
 {
 	CK_ULONG i;
 
-	for (i = 0; !p11_attrs_is_empty (attrs + i); i++) {
-		if (attrs[i].type == type &&
-		    attrs[i].ulValueLen != (CK_ULONG)-1)
-			return attrs + i;
-	}
-
-	return NULL;
-}
-
-CK_ATTRIBUTE *
-p11_attrs_findn_valid (CK_ATTRIBUTE *attrs,
-                       CK_ULONG count,
-                       CK_ATTRIBUTE_TYPE type)
-{
-	CK_ULONG i;
-
-	for (i = 0; i < count; i++) {
+	for (i = 0; !p11_attrs_terminator (attrs + i); i++) {
 		if (attrs[i].type == type &&
+		    attrs[i].pValue != NULL &&
+		    attrs[i].ulValueLen != 0 &&
 		    attrs[i].ulValueLen != (CK_ULONG)-1)
 			return attrs + i;
 	}
@@ -380,7 +367,6 @@ p11_attrs_findn_valid (CK_ATTRIBUTE *attrs,
 	return NULL;
 }
 
-
 bool
 p11_attrs_remove (CK_ATTRIBUTE *attrs,
                   CK_ATTRIBUTE_TYPE type)
@@ -410,7 +396,7 @@ p11_attrs_purge (CK_ATTRIBUTE *attrs)
 {
 	int in, out;
 
-	for (in = 0, out = 0; !p11_attrs_is_empty (attrs + in); in++) {
+	for (in = 0, out = 0; !p11_attrs_terminator (attrs + in); in++) {
 		if (attrs[in].ulValueLen == (CK_ULONG)-1) {
 			free (attrs[in].pValue);
 			attrs[in].pValue = NULL;
@@ -423,7 +409,7 @@ p11_attrs_purge (CK_ATTRIBUTE *attrs)
 	}
 
 	attrs[out].type = CKA_INVALID;
-	assert (p11_attrs_is_empty (attrs + out));
+	assert (p11_attrs_terminator (attrs + out));
 
 }
 
@@ -433,7 +419,7 @@ p11_attrs_match (const CK_ATTRIBUTE *attrs,
 {
 	CK_ATTRIBUTE *attr;
 
-	for (; !p11_attrs_is_empty (match); match++) {
+	for (; !p11_attrs_terminator (match); match++) {
 		attr = p11_attrs_find ((CK_ATTRIBUTE *)attrs, match->type);
 		if (!attr)
 			return false;
diff --git a/common/attrs.h b/common/attrs.h
index 619403d..f6eb950 100644
--- a/common/attrs.h
+++ b/common/attrs.h
@@ -61,7 +61,7 @@ CK_ATTRIBUTE *      p11_attrs_merge         (CK_ATTRIBUTE *attrs,
 
 void                p11_attrs_purge         (CK_ATTRIBUTE *attrs);
 
-bool                p11_attrs_is_empty      (const CK_ATTRIBUTE *attrs);
+bool                p11_attrs_terminator    (const CK_ATTRIBUTE *attrs);
 
 CK_ULONG            p11_attrs_count         (const CK_ATTRIBUTE *attrs);
 
@@ -87,18 +87,13 @@ bool                p11_attrs_find_ulong    (CK_ATTRIBUTE *attrs,
                                              CK_ATTRIBUTE_TYPE type,
                                              CK_ULONG *value);
 
-bool                p11_attrs_findn_ulong   (CK_ATTRIBUTE *attrs,
-                                             CK_ULONG count,
+void *              p11_attrs_find_value    (CK_ATTRIBUTE *attrs,
                                              CK_ATTRIBUTE_TYPE type,
-                                             CK_ULONG *value);
+                                             size_t *length);
 
 CK_ATTRIBUTE *      p11_attrs_find_valid    (CK_ATTRIBUTE *attrs,
                                              CK_ATTRIBUTE_TYPE type);
 
-CK_ATTRIBUTE *      p11_attrs_findn_valid   (CK_ATTRIBUTE *attrs,
-                                             CK_ULONG count,
-                                             CK_ATTRIBUTE_TYPE type);
-
 bool                p11_attrs_remove        (CK_ATTRIBUTE *attrs,
                                              CK_ATTRIBUTE_TYPE type);
 
diff --git a/common/mock.c b/common/mock.c
index 411c6eb..3ba3085 100644
--- a/common/mock.c
+++ b/common/mock.c
@@ -124,24 +124,6 @@ free_session (void *data)
 	free (sess);
 }
 
-static bool
-find_boolean_attribute (CK_ATTRIBUTE *attrs,
-                        CK_ATTRIBUTE_TYPE type,
-                        CK_BBOOL *value)
-{
-	CK_ATTRIBUTE *attr;
-
-	attr = p11_attrs_find (attrs, type);
-	if (attr != NULL &&
-	    attr->pValue != NULL &&
-	    attr->ulValueLen == sizeof (CK_BBOOL)) {
-		*value = *((CK_BBOOL *)attr->pValue);
-		return true;
-	}
-
-	return false;
-}
-
 static CK_RV
 lookup_object (Session *sess,
                CK_OBJECT_HANDLE object,
@@ -164,7 +146,7 @@ lookup_object (Session *sess,
 
 	if (!*attrs)
 		return CKR_OBJECT_HANDLE_INVALID;
-	else if (!logged_in && find_boolean_attribute (*attrs, CKA_PRIVATE, &priv) && priv)
+	else if (!logged_in && p11_attrs_find_bool (*attrs, CKA_PRIVATE, &priv) && priv)
 		return CKR_USER_NOT_LOGGED_IN;
 
 	return CKR_OK;
@@ -1183,7 +1165,7 @@ mock_C_CreateObject (CK_SESSION_HANDLE session,
 
 	attrs = p11_attrs_buildn (NULL, template, count);
 
-	if (find_boolean_attribute (attrs, CKA_PRIVATE, &priv) && priv) {
+	if (p11_attrs_find_bool (attrs, CKA_PRIVATE, &priv) && priv) {
 		if (!logged_in) {
 			p11_attrs_free (attrs);
 			return CKR_USER_NOT_LOGGED_IN;
@@ -1191,7 +1173,7 @@ mock_C_CreateObject (CK_SESSION_HANDLE session,
 	}
 
 	*object = ++unique_identifier;
-	if (find_boolean_attribute (attrs, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (attrs, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*object), attrs);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*object), attrs);
@@ -1232,7 +1214,7 @@ mock_C_CopyObject (CK_SESSION_HANDLE session,
 	if (rv != CKR_OK)
 		return rv;
 
-	if (find_boolean_attribute (attrs, CKA_PRIVATE, &priv) && priv) {
+	if (p11_attrs_find_bool (attrs, CKA_PRIVATE, &priv) && priv) {
 		if (!logged_in)
 			return CKR_USER_NOT_LOGGED_IN;
 	}
@@ -1240,7 +1222,7 @@ mock_C_CopyObject (CK_SESSION_HANDLE session,
 	attrs = p11_attrs_buildn (p11_attrs_dup (attrs), template, count);
 
 	*new_object = ++unique_identifier;
-	if (find_boolean_attribute (attrs, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (attrs, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*new_object), attrs);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*new_object), attrs);
@@ -1310,7 +1292,7 @@ mock_C_GetObjectSize (CK_SESSION_HANDLE session,
 		return rv;
 
 	*size = 0;
-	for (i = 0; !p11_attrs_is_empty (attrs + i); i++) {
+	for (i = 0; !p11_attrs_terminator (attrs + i); i++) {
 		if (attrs[i].ulValueLen != (CK_ULONG)-1)
 			*size += attrs[i].ulValueLen;
 	}
@@ -2118,7 +2100,7 @@ prefix_mechanism_init (CK_SESSION_HANDLE session,
 	if (rv != CKR_OK)
 		return rv;
 
-	value = p11_attrs_find (attrs, CKA_VALUE);
+	value = p11_attrs_find_valid (attrs, CKA_VALUE);
 	if (value == NULL)
 		return CKR_KEY_TYPE_INCONSISTENT;
 
@@ -2697,7 +2679,7 @@ mock_C_GenerateKey (CK_SESSION_HANDLE session,
 	attrs = p11_attrs_buildn (attrs, &value, 1);
 
 	*key = ++unique_identifier;
-	if (find_boolean_attribute (attrs, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (attrs, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*key), attrs);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*key), attrs);
@@ -2757,7 +2739,7 @@ mock_C_GenerateKeyPair (CK_SESSION_HANDLE session,
 	attrs = p11_attrs_buildn (attrs, &value, 1);
 
 	*public_key = ++unique_identifier;
-	if (find_boolean_attribute (attrs, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (attrs, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*public_key), attrs);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*public_key), attrs);
@@ -2766,7 +2748,7 @@ mock_C_GenerateKeyPair (CK_SESSION_HANDLE session,
 	attrs = p11_attrs_buildn (attrs, &value, 1);
 
 	*private_key = ++unique_identifier;
-	if (find_boolean_attribute (attrs, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (attrs, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*private_key), attrs);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*private_key), attrs);
@@ -2830,7 +2812,7 @@ mock_C_WrapKey (CK_SESSION_HANDLE session,
 		return CKR_MECHANISM_PARAM_INVALID;
 	}
 
-	attr = p11_attrs_find (attrs, CKA_VALUE);
+	attr = p11_attrs_find_valid (attrs, CKA_VALUE);
 	if (attr == NULL)
 		return CKR_WRAPPED_KEY_INVALID;
 
@@ -2914,7 +2896,7 @@ mock_C_UnwrapKey (CK_SESSION_HANDLE session,
 	attrs = p11_attrs_buildn (attrs, &value, 1);
 
 	*key = ++unique_identifier;
-	if (find_boolean_attribute (attrs, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (attrs, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*key), attrs);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*key), attrs);
@@ -2981,7 +2963,7 @@ mock_C_DeriveKey (CK_SESSION_HANDLE session,
 	copy = p11_attrs_buildn (copy, &value, 1);
 
 	*key = ++unique_identifier;
-	if (find_boolean_attribute (copy, CKA_TOKEN, &token) && token)
+	if (p11_attrs_find_bool (copy, CKA_TOKEN, &token) && token)
 		p11_dict_set (the_objects, handle_to_pointer (*key), copy);
 	else
 		p11_dict_set (sess->objects, handle_to_pointer (*key), copy);
diff --git a/common/tests/test-attrs.c b/common/tests/test-attrs.c
index 49350be..f1e6d91 100644
--- a/common/tests/test-attrs.c
+++ b/common/tests/test-attrs.c
@@ -43,6 +43,21 @@
 #include "debug.h"
 
 static void
+test_terminator (CuTest *tc)
+{
+	CK_ATTRIBUTE attrs[] = {
+		{ CKA_LABEL, "label", 5 },
+		{ CKA_LABEL, NULL, 0 },
+		{ CKA_INVALID },
+	};
+
+	CuAssertIntEquals (tc, true, p11_attrs_terminator (attrs + 2));
+	CuAssertIntEquals (tc, true, p11_attrs_terminator (NULL));
+	CuAssertIntEquals (tc, false, p11_attrs_terminator (attrs));
+	CuAssertIntEquals (tc, false, p11_attrs_terminator (attrs + 1));
+}
+
+static void
 test_count (CuTest *tc)
 {
 	CK_BBOOL vtrue = CK_TRUE;
@@ -647,22 +662,53 @@ test_find_ulong (CuTest *tc)
 }
 
 static void
+test_find_value (CuTest *tc)
+{
+	void *value;
+	size_t length;
+
+	CK_ATTRIBUTE attrs[] = {
+		{ CKA_LABEL, "", (CK_ULONG)-1 },
+		{ CKA_LABEL, NULL, 5 },
+		{ CKA_LABEL, "", 0 },
+		{ CKA_LABEL, "test", 4 },
+		{ CKA_VALUE, NULL, 0 },
+		{ CKA_INVALID },
+	};
+
+	value = p11_attrs_find_value (attrs, CKA_LABEL, &length);
+	CuAssertPtrEquals (tc, attrs[3].pValue, value);
+	CuAssertIntEquals (tc, 4, length);
+
+	value = p11_attrs_find_value (attrs, CKA_LABEL, NULL);
+	CuAssertPtrEquals (tc, attrs[3].pValue, value);
+
+	value = p11_attrs_find_value (attrs, CKA_VALUE, &length);
+	CuAssertPtrEquals (tc, NULL, value);
+
+	value = p11_attrs_find_value (attrs, CKA_TOKEN, &length);
+	CuAssertPtrEquals (tc, NULL, value);
+}
+
+static void
 test_find_valid (CuTest *tc)
 {
 	CK_ATTRIBUTE *attr;
 
 	CK_ATTRIBUTE attrs[] = {
 		{ CKA_LABEL, "", (CK_ULONG)-1 },
+		{ CKA_LABEL, NULL, 5 },
+		{ CKA_LABEL, "", 0 },
 		{ CKA_LABEL, "test", 4 },
-		{ CKA_VALUE, NULL, 0 },
+		{ CKA_VALUE, "value", 5 },
 		{ CKA_INVALID },
 	};
 
 	attr = p11_attrs_find_valid (attrs, CKA_LABEL);
-	CuAssertPtrEquals (tc, attrs + 1, attr);
+	CuAssertPtrEquals (tc, attrs + 3, attr);
 
 	attr = p11_attrs_find_valid (attrs, CKA_VALUE);
-	CuAssertPtrEquals (tc, attrs + 2, attr);
+	CuAssertPtrEquals (tc, attrs + 4, attr);
 
 	attr = p11_attrs_find_valid (attrs, CKA_TOKEN);
 	CuAssertPtrEquals (tc, NULL, attr);
@@ -682,6 +728,7 @@ main (void)
 	SUITE_ADD_TEST (suite, test_hash);
 	SUITE_ADD_TEST (suite, test_to_string);
 
+	SUITE_ADD_TEST (suite, test_terminator);
 	SUITE_ADD_TEST (suite, test_count);
 	SUITE_ADD_TEST (suite, test_build_one);
 	SUITE_ADD_TEST (suite, test_build_two);
@@ -702,6 +749,7 @@ main (void)
 	SUITE_ADD_TEST (suite, test_findn);
 	SUITE_ADD_TEST (suite, test_find_bool);
 	SUITE_ADD_TEST (suite, test_find_ulong);
+	SUITE_ADD_TEST (suite, test_find_value);
 	SUITE_ADD_TEST (suite, test_find_valid);
 	SUITE_ADD_TEST (suite, test_remove);
 
diff --git a/tools/extract-info.c b/tools/extract-info.c
index da84bbe..1c81e07 100644
--- a/tools/extract-info.c
+++ b/tools/extract-info.c
@@ -117,19 +117,13 @@ extract_purposes (p11_extract_info *ex)
 	                     sizeof (P11_OID_EXTENDED_KEY_USAGE) };
 	const unsigned char *ext = NULL;
 	unsigned char *alloc = NULL;
-	CK_ATTRIBUTE *value;
 	CK_ATTRIBUTE *attrs;
 	size_t ext_len;
 
 	if (ex->stapled) {
 		attrs = p11_dict_get (ex->stapled, &oid);
-		if (attrs != NULL) {
-			value = p11_attrs_find (attrs, CKA_VALUE);
-			if (value) {
-				ext = value->pValue;
-				ext_len = value->ulValueLen;
-			}
-		}
+		if (attrs != NULL)
+			ext = p11_attrs_find_value (attrs, CKA_VALUE, &ext_len);
 	}
 
 	if (ext == NULL && ex->cert_asn) {
@@ -203,6 +197,7 @@ extract_certificate (P11KitIter *iter,
 {
 	char message[ASN1_MAX_ERROR_DESCRIPTION_SIZE];
 	CK_ATTRIBUTE *attr;
+
 	CK_ULONG type;
 
 	/* Don't even bother with not X.509 certificates */
@@ -280,14 +275,10 @@ extract_info (P11KitIter *iter,
 		return false;
 	}
 
-	attr = p11_attrs_find (ex->attrs, CKA_CLASS);
-
 	/* No class attribute, very strange, just skip */
-	if (!attr || !attr->pValue || attr->ulValueLen != sizeof (CK_OBJECT_CLASS))
+	if (!p11_attrs_find_ulong (ex->attrs, CKA_CLASS, &ex->klass))
 		return false;
 
-	ex->klass = *((CK_ULONG *)attr->pValue);
-
 	/* If a certificate then  */
 	if (ex->klass != CKO_CERTIFICATE) {
 		p11_message ("skipping non-certificate object");
@@ -297,7 +288,7 @@ extract_info (P11KitIter *iter,
 	if (!extract_certificate (iter, ex))
 		return false;
 
-	attr = p11_attrs_find (ex->attrs, CKA_ID);
+	attr = p11_attrs_find_valid (ex->attrs, CKA_ID);
 	if (attr) {
 		ex->stapled = load_stapled_extensions (p11_kit_iter_get_module (iter),
 		                                       p11_kit_iter_get_slot (iter),
@@ -412,7 +403,7 @@ extract_label (p11_extract_info *extract)
 	CK_ATTRIBUTE *attr;
 
 	/* Look for a label and just use that */
-	attr = p11_attrs_find (extract->attrs, CKA_LABEL);
+	attr = p11_attrs_find_valid (extract->attrs, CKA_LABEL);
 	if (attr && attr->pValue && attr->ulValueLen)
 		return strndup (attr->pValue, attr->ulValueLen);
 
diff --git a/tools/extract-jks.c b/tools/extract-jks.c
index 6d40da0..d75735c 100644
--- a/tools/extract-jks.c
+++ b/tools/extract-jks.c
@@ -265,7 +265,7 @@ prepare_jks_buffer (P11KitIter *iter,
 		add_msb_int (buffer, trusted_cert);
 
 		/* The alias */
-		label = p11_attrs_find (ex->attrs, CKA_LABEL);
+		label = p11_attrs_find_valid (ex->attrs, CKA_LABEL);
 		if (!add_alias (buffer, aliases, label)) {
 			p11_message ("could not generate a certificate alias name");
 			p11_dict_free (aliases);
diff --git a/tools/extract-openssl.c b/tools/extract-openssl.c
index 13a1e05..bc7427a 100644
--- a/tools/extract-openssl.c
+++ b/tools/extract-openssl.c
@@ -107,16 +107,16 @@ load_usage_ext (p11_extract_info *ex,
 {
 	CK_ATTRIBUTE attr = { CKA_OBJECT_ID, (void *)ext_oid,
 	                      p11_oid_length (ext_oid) };
-	CK_ATTRIBUTE *value;
+	void *value;
+	size_t length;
 
-	value = p11_attrs_find_valid (p11_dict_get (ex->stapled, &attr), CKA_VALUE);
+	value = p11_attrs_find_value (p11_dict_get (ex->stapled, &attr), CKA_VALUE, &length);
 	if (value == NULL) {
 		*oids = NULL;
 		return true;
 	}
 
-	*oids = p11_x509_parse_extended_key_usage (ex->asn1_defs, value->pValue,
-	                                           value->ulValueLen);
+	*oids = p11_x509_parse_extended_key_usage (ex->asn1_defs, value, length);
 	return_val_if_fail (*oids != NULL, false);
 
 	return true;
diff --git a/tools/tests/test-extract.c b/tools/tests/test-extract.c
index c7382cd..29ee986 100644
--- a/tools/tests/test-extract.c
+++ b/tools/tests/test-extract.c
@@ -218,7 +218,8 @@ static CK_ATTRIBUTE extension_eku_invalid[] = {
 static void
 test_info_simple_certificate (CuTest *tc)
 {
-	CK_ATTRIBUTE *value;
+	void *value;
+	size_t length;
 	CK_RV rv;
 
 	setup (tc);
@@ -237,9 +238,9 @@ test_info_simple_certificate (CuTest *tc)
 
 	CuAssertIntEquals (tc, CKO_CERTIFICATE, test.ex.klass);
 	CuAssertPtrNotNull (tc, test.ex.attrs);
-	value = p11_attrs_find_valid (test.ex.attrs, CKA_VALUE);
+	value = p11_attrs_find_value (test.ex.attrs, CKA_VALUE, &length);
 	CuAssertPtrNotNull (tc, value);
-	CuAssertTrue (tc, memcmp (value->pValue, test_cacert3_ca_der, value->ulValueLen) == 0);
+	CuAssertTrue (tc, memcmp (value, test_cacert3_ca_der, length) == 0);
 	CuAssertPtrNotNull (tc, test.ex.cert_der);
 	CuAssertTrue (tc, memcmp (test.ex.cert_der, test_cacert3_ca_der, test.ex.cert_len) == 0);
 	CuAssertPtrNotNull (tc, test.ex.cert_asn);
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);