pkcs11: Change how unavailable attributes like CKA_TRUSTED are handled
authorTobias Brunner <tobias@strongswan.org>
Thu, 20 May 2021 14:52:49 +0000 (16:52 +0200)
committerTobias Brunner <tobias@strongswan.org>
Mon, 14 Jun 2021 11:58:48 +0000 (13:58 +0200)
If a PKCS#11 library/token doesn't provide one or more attributes via
C_GetAttributeValue(), we get back CKR_ATTRIBUTE_TYPE_INVALID (similar
for protected attributes where CKR_ATTRIBUTE_SENSITIVE is returned).
This is not an error as the spec demands that all attributes have been
processed with the unavailable attributes having set their length
field to CK_UNAVAILABLE_INFORMATION.

We use this to handle the CKA_TRUSTED attribute, which some tokens
apparently don't support.  We previously used a version check to remove
the attribute from the call but even the latest spec doesn't make the
attribute mandatory (it's just in a list of "common" attributes for
CKO_CERTIFICATE objects, without a default value), so there are current
tokens that don't support it and prevent us from enumerating certificates.

src/libstrongswan/plugins/pkcs11/pkcs11_creds.c
src/libstrongswan/plugins/pkcs11/pkcs11_library.c
src/libstrongswan/plugins/pkcs11/pkcs11_library.h
src/libstrongswan/plugins/pkcs11/pkcs11_manager.c
src/libstrongswan/plugins/pkcs11/pkcs11_private_key.c
src/libstrongswan/plugins/pkcs11/pkcs11_public_key.c

index 76d69d5..74d699a 100644 (file)
@@ -83,21 +83,21 @@ static void find_certificates(private_pkcs11_creds_t *this,
 
        /* store result in a temporary list, avoid recursive operation */
        raw = linked_list_create();
-       /* do not use trusted argument if not supported */
-       if (!(this->lib->get_features(this->lib) & PKCS11_TRUSTED_CERTS))
-       {
-               count--;
-       }
        enumerator = this->lib->create_object_enumerator(this->lib,
                                                                        session, tmpl, countof(tmpl), attr, count);
        while (enumerator->enumerate(enumerator, &object))
        {
-               entry = malloc(sizeof(*entry));
-               entry->value = chunk_clone(
-                                                       chunk_create(attr[0].pValue, attr[0].ulValueLen));
-               entry->label = chunk_clone(
-                                                       chunk_create(attr[1].pValue, attr[1].ulValueLen));
-               entry->trusted = trusted;
+               if (attr[0].ulValueLen == CK_UNAVAILABLE_INFORMATION ||
+                       attr[1].ulValueLen == CK_UNAVAILABLE_INFORMATION)
+               {
+                       continue;
+               }
+               INIT(entry,
+                       .value = chunk_clone(chunk_create(attr[0].pValue, attr[0].ulValueLen)),
+                       .label = chunk_clone(chunk_create(attr[1].pValue, attr[1].ulValueLen)),
+                       /* assume trusted certificates if attribute is not available */
+                       .trusted = attr[2].ulValueLen == CK_UNAVAILABLE_INFORMATION || trusted,
+               );
                raw->insert_last(raw, entry);
        }
        enumerator->destroy(enumerator);
@@ -339,7 +339,8 @@ certificate_t *pkcs11_creds_load(certificate_type_t type, va_list args)
                }
                certs = p11->create_object_enumerator(p11, session,
                                                                        tmpl, countof(tmpl), attr, countof(attr));
-               if (certs->enumerate(certs, &object))
+               if (certs->enumerate(certs, &object) &&
+                       attr[0].ulValueLen != CK_UNAVAILABLE_INFORMATION)
                {
                        data = chunk_clone(chunk_create(attr[0].pValue, attr[0].ulValueLen));
                }
index 6c0dda3..ae0a843 100644 (file)
@@ -624,6 +624,8 @@ typedef struct {
        pkcs11_library_t *lib;
        /* attributes to retrieve */
        CK_ATTRIBUTE_PTR attr;
+       /* copy of the original attributes provided by the caller */
+       CK_ATTRIBUTE_PTR orig_attr;
        /* number of attributes */
        CK_ULONG count;
        /* object handle in case of a single object */
@@ -633,17 +635,36 @@ typedef struct {
 } object_enumerator_t;
 
 /**
- * Free contents of attributes in a list
+ * Keep a copy of the original attribute values so we can restore them while
+ * enumerating e.g. if an attribute was unavailable for a particular object.
+ */
+static void init_attrs(object_enumerator_t *this)
+{
+       int i;
+
+       this->orig_attr = calloc(this->count, sizeof(CK_ATTRIBUTE));
+       for (i = 0; i < this->count; i++)
+       {
+               this->orig_attr[i] = this->attr[i];
+       }
+}
+
+/**
+ * Free contents of allocated attributes and reset them to their original
+ * values.
  */
 static void free_attrs(object_enumerator_t *this)
 {
        CK_ATTRIBUTE_PTR attr;
+       int i;
 
        while (this->freelist->remove_last(this->freelist, (void**)&attr) == SUCCESS)
        {
                free(attr->pValue);
-               attr->pValue = NULL;
-               attr->ulValueLen = 0;
+       }
+       for (i = 0; i < this->count; i++)
+       {
+               this->attr[i] = this->orig_attr[i];
        }
 }
 
@@ -687,7 +708,9 @@ static bool get_attributes(object_enumerator_t *this, CK_OBJECT_HANDLE object)
        /* get length of objects first */
        rv = this->lib->f->C_GetAttributeValue(this->session, object,
                                                                                   this->attr, this->count);
-       if (rv != CKR_OK)
+       if (rv != CKR_OK &&
+               rv != CKR_ATTRIBUTE_SENSITIVE &&
+               rv != CKR_ATTRIBUTE_TYPE_INVALID)
        {
                DBG1(DBG_CFG, "C_GetAttributeValue(NULL) error: %N", ck_rv_names, rv);
                return FALSE;
@@ -695,8 +718,12 @@ static bool get_attributes(object_enumerator_t *this, CK_OBJECT_HANDLE object)
        /* allocate required chunks */
        for (i = 0; i < this->count; i++)
        {
-               if (this->attr[i].pValue == NULL &&
-                       this->attr[i].ulValueLen != 0 && this->attr[i].ulValueLen != -1)
+               if (this->attr[i].ulValueLen == CK_UNAVAILABLE_INFORMATION)
+               {       /* reset this unavailable attribute before the next call */
+                       this->attr[i] = this->orig_attr[i];
+               }
+               else if (this->attr[i].pValue == NULL &&
+                                this->attr[i].ulValueLen != 0)
                {
                        this->attr[i].pValue = malloc(this->attr[i].ulValueLen);
                        this->freelist->insert_last(this->freelist, &this->attr[i]);
@@ -705,9 +732,10 @@ static bool get_attributes(object_enumerator_t *this, CK_OBJECT_HANDLE object)
        /* get the data */
        rv = this->lib->f->C_GetAttributeValue(this->session, object,
                                                                                   this->attr, this->count);
-       if (rv != CKR_OK)
+       if (rv != CKR_OK &&
+               rv != CKR_ATTRIBUTE_SENSITIVE &&
+               rv != CKR_ATTRIBUTE_TYPE_INVALID)
        {
-               free_attrs(this);
                DBG1(DBG_CFG, "C_GetAttributeValue() error: %N", ck_rv_names, rv);
                return FALSE;
        }
@@ -774,6 +802,7 @@ METHOD(enumerator_t, object_destroy, void,
        }
        free_attrs(this);
        this->freelist->destroy(this->freelist);
+       free(this->orig_attr);
        free(this);
 }
 
@@ -804,6 +833,7 @@ METHOD(pkcs11_library_t, create_object_enumerator, enumerator_t*,
                .count = acount,
                .freelist = linked_list_create(),
        );
+       init_attrs(enumerator);
        return &enumerator->public;
 }
 
@@ -826,6 +856,7 @@ METHOD(pkcs11_library_t, create_object_attr_enumerator, enumerator_t*,
                .object = object,
                .freelist = linked_list_create(),
        );
+       init_attrs(enumerator);
        return &enumerator->public;
 }
 
@@ -1033,7 +1064,6 @@ static void check_features(private_pkcs11_library_t *this, CK_INFO *info)
 {
        if (has_version(info, 2, 20))
        {
-               this->features |= PKCS11_TRUSTED_CERTS;
                this->features |= PKCS11_ALWAYS_AUTH_KEYS;
        }
 }
index 4038b7e..f8aa0af 100644 (file)
@@ -37,10 +37,8 @@ typedef struct pkcs11_library_t pkcs11_library_t;
  * Optional PKCS#11 features some libraries support, some not
  */
 enum pkcs11_feature_t {
-       /** CKA_TRUSTED attribute supported for certificate objects */
-       PKCS11_TRUSTED_CERTS = (1<<0),
        /** CKA_ALWAYS_AUTHENTICATE attribute supported for private keys */
-       PKCS11_ALWAYS_AUTH_KEYS = (1<<1),
+       PKCS11_ALWAYS_AUTH_KEYS = (1<<0),
 };
 
 /**
@@ -73,6 +71,8 @@ struct pkcs11_library_t {
         * An optional attribute array is automatically filled in with the
         * objects associated attributes. If the value of an output attribute
         * is NULL, the value gets allocated/freed during enumeration.
+        * For attributes that are unavailable, the length field will be set to
+        * CK_UNAVAILABLE_INFORMATION.
         *
         * @param session       session to use
         * @param tmpl          search template
@@ -92,6 +92,8 @@ struct pkcs11_library_t {
         * The given attribute array is automatically filled in with the
         * associated attributes. If the value of an output attribute is NULL,
         * the required memory gets allocated/freed during enumeration.
+        * For attributes that are unavailable, the length field will be set to
+        * CK_UNAVAILABLE_INFORMATION.
         *
         * @param session       session to use
         * @param object        object handle
index c7dfe69..a1400ad 100644 (file)
@@ -389,4 +389,3 @@ pkcs11_manager_t *pkcs11_manager_create(pkcs11_manager_token_event_t cb,
 
        return &this->public;
 }
-
index 6b8be62..820ac3f 100644 (file)
@@ -627,13 +627,17 @@ static pkcs11_library_t* find_lib_and_keyid_by_skid(chunk_t keyid_chunk,
                                                                                          attr, countof(attr));
                while (certs->enumerate(certs, &object))
                {
-                       INIT(entry,
-                               .value = chunk_clone(
-                                                       chunk_create(attr[0].pValue, attr[0].ulValueLen)),
-                               .ckaid = chunk_clone(
-                                                       chunk_create(attr[1].pValue, attr[1].ulValueLen)),
-                       );
-                       raw->insert_last(raw, entry);
+                       if (attr[0].ulValueLen != CK_UNAVAILABLE_INFORMATION &&
+                               attr[1].ulValueLen != CK_UNAVAILABLE_INFORMATION)
+                       {
+                               INIT(entry,
+                                       .value = chunk_clone(
+                                                               chunk_create(attr[0].pValue, attr[0].ulValueLen)),
+                                       .ckaid = chunk_clone(
+                                                               chunk_create(attr[1].pValue, attr[1].ulValueLen)),
+                               );
+                               raw->insert_last(raw, entry);
+                       }
                }
                certs->destroy(certs);
 
@@ -708,7 +712,8 @@ static bool find_key(private_pkcs11_private_key_t *this, chunk_t keyid)
        }
        enumerator = this->lib->create_object_enumerator(this->lib,
                                                        this->session, tmpl, countof(tmpl), attr, count);
-       if (enumerator->enumerate(enumerator, &object))
+       if (enumerator->enumerate(enumerator, &object) &&
+               attr[0].ulValueLen != CK_UNAVAILABLE_INFORMATION)
        {
                this->type = KEY_RSA;
                switch (type)
@@ -717,7 +722,10 @@ static bool find_key(private_pkcs11_private_key_t *this, chunk_t keyid)
                                this->type = KEY_ECDSA;
                                /* fall-through */
                        case CKK_RSA:
-                               this->reauth = reauth;
+                               if (attr[1].ulValueLen != CK_UNAVAILABLE_INFORMATION)
+                               {
+                                       this->reauth = reauth;
+                               }
                                this->object = object;
                                found = TRUE;
                                break;
@@ -803,7 +811,8 @@ static public_key_t* find_pubkey_in_certs(private_pkcs11_private_key_t *this,
 
        enumerator = this->lib->create_object_enumerator(this->lib, this->session,
                                                                        tmpl, countof(tmpl), attr, countof(attr));
-       if (enumerator->enumerate(enumerator, &object))
+       if (enumerator->enumerate(enumerator, &object) &&
+               attr[0].ulValueLen != CK_UNAVAILABLE_INFORMATION)
        {
                data = chunk_clone(chunk_create(attr[0].pValue, attr[0].ulValueLen));
        }
index 5c7a9a1..cbebc63 100644 (file)
@@ -888,7 +888,8 @@ static private_pkcs11_public_key_t *find_key_by_keyid(pkcs11_library_t *p11,
 
        enumerator = p11->create_object_enumerator(p11, session, tmpl, count, attr,
                                                                                           countof(attr));
-       if (enumerator->enumerate(enumerator, &object))
+       if (enumerator->enumerate(enumerator, &object) &&
+               attr[0].ulValueLen != CK_UNAVAILABLE_INFORMATION)
        {
                switch (type)
                {