Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ta/pkcs11/include/pkcs11_ta.h
Original file line number Diff line number Diff line change
Expand Up @@ -1188,6 +1188,10 @@ enum pkcs11_attr_id {
0x0600,

/* Vendor specific attributes */
#if defined(CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR)
PKCS11_CKA_OPTEE_INDESTRUCTIBLE = PKCS11_CKA_VENDOR_DEFINED |
0x0001,
#endif

/**
* TEE Internal API requires to have EC public key information
Expand Down
3 changes: 3 additions & 0 deletions ta/pkcs11/src/attributes.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ enum boolprop_attr {
BPA_DESTROYABLE,
BPA_ALWAYS_AUTHENTICATE,
BPA_WRAP_WITH_TRUSTED,
#if defined(CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR)
BPA_INDESTRUCTIBLE,
#endif
};

/*
Expand Down
8 changes: 8 additions & 0 deletions ta/pkcs11/src/object.c
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,14 @@ enum pkcs11_rc entry_destroy_object(struct pkcs11_client *client,
if (!get_bool(object->attributes, PKCS11_CKA_DESTROYABLE))
return PKCS11_CKR_ACTION_PROHIBITED;

#if defined(CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR)
/* Objects with PKCS11_CKA_OPTEE_INDESTRUCTIBLE as true aren't destroyable */
if (get_bool(object->attributes, PKCS11_CKA_OPTEE_INDESTRUCTIBLE)) {
DMSG("Object is indestructible");
return PKCS11_CKR_ACTION_PROHIBITED;
}
#endif

destroy_object(session, object, false);

DMSG("PKCS11 session %"PRIu32": destroy object %#"PRIx32,
Expand Down
40 changes: 40 additions & 0 deletions ta/pkcs11/src/pkcs11_attributes.c
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,9 @@ static uint8_t *pkcs11_object_default_boolprop(uint32_t attribute)
case PKCS11_CKA_WRAP:
case PKCS11_CKA_UNWRAP:
case PKCS11_CKA_EXTRACTABLE:
#if defined(CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR)
case PKCS11_CKA_OPTEE_INDESTRUCTIBLE:
#endif
case PKCS11_CKA_TRUSTED:
return (uint8_t *)&bool_false;
default:
Expand Down Expand Up @@ -1186,6 +1189,9 @@ create_attributes_from_template(struct obj_attrs **out, void *template,
struct obj_attrs *attrs = NULL;
enum pkcs11_rc rc = PKCS11_CKR_OK;
uint8_t local = 0;
#if defined(CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR)
uint8_t indestructible = 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build break: unused variable. This outer indestructible is never read or written — the inner block below (else branch) declares and uses its own bool indestructible. With -Wunused-variable and -Werror (both on in mk/compile.mk), this fails to compile when CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR=y.

#endif
uint8_t always_sensitive = 0;
uint8_t never_extract = 0;
uint8_t extractable = 0;
Expand Down Expand Up @@ -1392,6 +1398,26 @@ create_attributes_from_template(struct obj_attrs **out, void *template,
if (rc)
goto out;

#if defined(CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR)
/* Check if CKA_INDESTRUCTIBLE exists in the template */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build break: declaration-after-statement + shadowing. enum pkcs11_rc rc_check = … is a declaration placed after statements in the function body, and the bool indestructible a few lines down shadows the function-scope indestructible declared at the top. OP-TEE builds the pkcs11 TA with -Wdeclaration-after-statement, -Wshadow, and -Werror, so this won't compile. Suggest declaring rc_check at block start and removing the now-redundant outer variable.

enum pkcs11_rc rc_check = get_attribute_ptr(temp, PKCS11_CKA_OPTEE_INDESTRUCTIBLE, NULL, NULL);

if (rc_check == PKCS11_RV_NOT_FOUND) {
DMSG("CKA_INDESTRUCTIBLE not present in template");
} else {
/* Attribute exists, get its value (TRUE or FALSE) */
bool indestructible = get_bool(temp, PKCS11_CKA_OPTEE_INDESTRUCTIBLE);

DMSG("CKA_INDESTRUCTIBLE present, value: %s", indestructible ? "TRUE" : "FALSE");

/* Add attribute with the actual value from template */
rc = add_attribute(&attrs, PKCS11_CKA_OPTEE_INDESTRUCTIBLE,
&indestructible, sizeof(indestructible));
if (rc)
goto out;
}
#endif

if (get_attribute_ptr(temp, PKCS11_CKA_LOCAL, NULL, NULL) !=
PKCS11_RV_NOT_FOUND) {
rc = PKCS11_CKR_TEMPLATE_INCONSISTENT;
Expand Down Expand Up @@ -1528,6 +1554,16 @@ static enum pkcs11_rc check_attrs_misc_integrity(struct obj_attrs *head)
return PKCS11_CKR_TEMPLATE_INCONSISTENT;
}

#if defined(CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR)
/* Only token objects can be indestructible */
if (get_bool(head, PKCS11_CKA_OPTEE_INDESTRUCTIBLE) &&
!get_bool(head, PKCS11_CKA_TOKEN)) {
DMSG("Can't create a non-token Indestructible object");

return PKCS11_CKR_TEMPLATE_INCONSISTENT;
}
#endif

return PKCS11_CKR_OK;
}

Expand Down Expand Up @@ -2337,6 +2373,10 @@ static bool attribute_is_modifiable(struct pkcs11_session *session,
* direction i.e from TRUE -> FALSE.
*/
return get_bool(obj->attributes, req_attr->id);
#if defined(CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR)
case PKCS11_CKA_OPTEE_INDESTRUCTIBLE:
return false;
#endif
default:
break;
}
Expand Down
1 change: 1 addition & 0 deletions ta/pkcs11/src/pkcs11_attributes.h
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ create_attributes_from_template(struct obj_attrs **out, void *template,
* - create a new object in the current token state
* - use a parent object in the processing
* - use a mechanism with provided configuration
* - use of indestructible attribute only if it is a token object
*/
enum pkcs11_rc check_created_attrs_against_token(struct pkcs11_session *session,
struct obj_attrs *head);
Expand Down
6 changes: 6 additions & 0 deletions ta/pkcs11/src/pkcs11_helpers.c
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ static const struct attr_size attr_ids[] = {
PKCS11_ID_SZ(PKCS11_CKA_DESTROYABLE, 1),
PKCS11_ID_SZ(PKCS11_CKA_ALWAYS_AUTHENTICATE, 1),
PKCS11_ID_SZ(PKCS11_CKA_WRAP_WITH_TRUSTED, 1),
#if defined(CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR)
PKCS11_ID_SZ(PKCS11_CKA_OPTEE_INDESTRUCTIBLE, 1),
#endif
/* Specific PKCS11 TA internal attribute ID */
PKCS11_ID_SZ(PKCS11_CKA_OPTEE_HIDDEN_EC_POINT, 0),
PKCS11_ID_SZ(PKCS11_CKA_UNDEFINED_ID, 0),
Expand Down Expand Up @@ -536,6 +539,9 @@ int pkcs11_attr2boolprop_shift(uint32_t attr)
[BPA_DESTROYABLE] = PKCS11_CKA_DESTROYABLE,
[BPA_ALWAYS_AUTHENTICATE] = PKCS11_CKA_ALWAYS_AUTHENTICATE,
[BPA_WRAP_WITH_TRUSTED] = PKCS11_CKA_WRAP_WITH_TRUSTED,
#if defined(CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR)
[BPA_INDESTRUCTIBLE] = PKCS11_CKA_OPTEE_INDESTRUCTIBLE,
#endif
};
size_t pos = 0;

Expand Down
19 changes: 18 additions & 1 deletion ta/pkcs11/src/pkcs11_token.c
Original file line number Diff line number Diff line change
Expand Up @@ -956,12 +956,29 @@ enum pkcs11_rc entry_ck_token_initialize(uint32_t ptypes, TEE_Param *params)
while (!LIST_EMPTY(&token->object_list)) {
obj = LIST_FIRST(&token->object_list);

#if defined(CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR)
if (!obj->attributes) {
rc = load_persistent_object_attributes(obj);
if (rc)
TEE_Panic(rc);
}

if (get_bool(obj->attributes, PKCS11_CKA_OPTEE_INDESTRUCTIBLE)) {
// Skip deletion for indestructible objects

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Skip deletion for indestructible objects
/* Skip deletion for indestructible objects */

LIST_REMOVE(obj, link);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need a bit more. obj volatile resources should be freed.
I would suggest to use cleanup_volatile_obj_ref() from object.c.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expectation of the implementation is that the INDETSRUCTIBLE objects should persist in the persistent_storage even after reinit. So, we need not call cleanup_volatile_obj_ref().

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the token in re-initialized, the object would still remain loaded in memory.
Calling cleanup_volatile_obj_ref() would release RAM from object resources but the object would still remain in the persistent database and be re-loaded only once it's used.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please confirm whether this leaks: indestructible objects are unlinked from token->object_list via LIST_REMOVE(obj, link) + continue, but the struct pkcs11_object (and its attributes/uuid, plus any loaded attributes handle) is never freed — the normal cleanup_volatile_obj_ref() path is skipped. If so, each C_InitToken on a token holding indestructible objects leaks them.

continue;
}
#endif

/* Try twice otherwise panic! */
if (unregister_persistent_object(token, obj->uuid) &&
unregister_persistent_object(token, obj->uuid))
unregister_persistent_object(token, obj->uuid))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discard this change. Previous indentation was fine.

TEE_Panic(0);

cleanup_persistent_object(obj, token);
#if defined(CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR)
token_invalidate_object_handles(obj);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleanup_persistent_object() frees obj pointer so token_invalidate_object_handles() here will not do anything.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I retained token_invalidate_object_handles(obj) because cleanup_persistent_object() only frees the object memory and does not invalidate the open handles associated with it as I tested some pointers were still remaining after cleanup-persistent_object() call. I was following the sequence observed in the destroy_object definition where both calls are present.

#endif
}

IMSG("PKCS11 token %"PRIu32": initialized", token_id);
Expand Down
3 changes: 3 additions & 0 deletions ta/pkcs11/sub.mk
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ CFG_PKCS11_TA_TOKEN_COUNT ?= 3
# When enabled, embed support for object checksum value computation
CFG_PKCS11_TA_CHECK_VALUE_ATTRIBUTE ?= y

#When enabled, adds the support for pkcs11 Indestructible object attribute/keys.
CFG_PKCS11_TA_INDESTRUCTIBLE_OBJECT_ATTR ?= n

# When enabled, embed support for CKM_RSA_X_509 (a.k.a. Raw RSA) ciphering
# and authentication. The feature can be needed for some TLS v1.2 connections.
# Raw RSA can be unsafe if client uses a weak clear data padding scheme.
Expand Down