Add support of key check value CKA_CHECK_VALUE to PKCS11 TA

[maroueneboubakri]

An attempt to support CKA_CHECK_VALUE attribute in PKCS 11 TA. For AES, the value of this attribute is derived from the key object by taking the first three bytes of the ECB encryption of a single block of null (0x00) bytes, using the default cipher associated with the key type of the secret key object. This is useful to detect that a key object has not tampered with.

For now, we try to calculate the value when a symmetric key is randomly generated. Still need to add for (Create, Unwarp, Derive and Copy) in incoming commits.

Related issues: #6453, #6431

[maroueneboubakri]

Added commit ta: pkcs11: pkcs11_attributes.c: calculate KCV on key derive/unwarp db6b79c

[etienne-lms]

By the way, could you add a test case in optee_test pkcs11_1000.c? Maybe start with a basic test to read the attribute, destroy the attribute empty, and set the attribute to a valid and an invalid value.

Also I wonder if there should be a CFG_PKCS11_TA_CHECK_VALUE=y|n config switch to enable or not this feature. @vesajaaskelainen, what do you think?

[maroueneboubakri]

Addressed comments above.
To add CFG_PKCS11_TA_CHECK_VALUE=y|n flag and a test case in optee_test pkcs11_1000.c

[etienne-lms]

The implementation lacks C_CopyObject and C_DeriveObject cases (I don't think there are other cases but I should cross-check).

If you prefer to go step by step, I think we definitely need a config switch ,default disabled, so that the attribute is not half supported as the specs says that if supported, it must be fully supported.

[etienne-lms]

Should not remove the attribute if present.
If present and of size 0, then PKCS11_CKA_CHECK_VALUE should be that one.
If present and of non-0 size, we should compute the value and succeed only if the provided value matches the expected value.

[maroueneboubakri]

@etienne-lms could you clarify which "that one" you are referring to you mean the one calculated from the resulting key ?
For the second case, the specs says that if provided the value provided should be taken even if it conflicts with the expected value.
Could you please precesie ?

[etienne-lms]

Sure.
Extracts from the PKCS#11 Base Specification, section 4.10 Secret key objects:

• The attribute is optional, but if supported, regardless of how the key object is created or derived, the value of the attribute is always supplied.
• If a value is supplied in the application template (allowed but never necessary) then, if supported, it MUST match what the library calculates it to be or the library returns a CKR_ATTRIBUTE_VALUE_INVALID.
• The generation of the KCV may be prevented by the application supplying the attribute in the template as a no-value (0 length) entry.
• The application can query the value at any time like any other attribute using C_GetAttributeValue.
• C_SetAttributeValue may be used to destroy the attribute, by supplying no-value.

[etienne-lms]

Sorry i was wrong in one point: it seems at object creation, the client template cannot prevent generation of CKA_CHECK_VALUE attribute : ".. it MUST match what the library calculates.". To destroy it, the client must later use C_SetAttributeValue and set a "no-value" (that is a zero-sized value) for the attribute.

[etienne-lms]

Ditto about not removing the attribute if present.

[maroueneboubakri]

This is a new randomly generated key. I guess the statement should be simply removed ?

[etienne-lms]

The template passed to C_GenerateKey can set CKA_CHECK_VALUE attribute. In which case we should not omit it but ensure it complies with a valid value, that is either a zero-sized value or the value the pkcs11 TA also computes. Other values for this attribute should error with return code CKR_ATTRIBUTE_VALUE_INVALID.

[maroueneboubakri]

@etienne-lms Stills unclear, For randomly generated key we can't beforehand determine the KCV to be provided in the template. So if we provide a zero-sized value, should we set the value computed by the TA ? The spec says:The generation of the KCV may be prevented by the application supplying the attribute in the template as a no-value (0 length) entry. Confused.
My understatding, the KCV will be always computed, if the client provides zero-sized value then it will be removed from attribute list remove_empty_attribute() as done before , when invalid value we return CKR_ATTRIBUTE_VALUE_INVALID. Is my understanding correct ?

[maroueneboubakri]

The implementation lacks C_CopyObject and C_DeriveObject cases (I don't think there are other cases but I should cross-check).

@etienne-lms My understatding is that set_key_data() serves fro key unwrapping and derivation ? while copy object nothing to do since all the attribues will be copied ?

[etienne-lms]

The implementation lacks C_CopyObject and C_DeriveObject cases (I don't think there are other cases but I should cross-check).

@etienne-lms My understatding is that set_key_data() serves fro key unwrapping and derivation ? while copy object nothing to do since all the attribues will be copied ?

You right for C_DeriveKey, it ends in calls entry_processing_key() that calls set_key_data(). C_UnwrapKey also ends in entry_processing_key().
C_CreateObject is not yet covered in your P-R. The template provided could set CKA_CHECK_VALUE attribute.
C_CopyObject can provide a template that modifies the copied object attributes, so need to handle CKA_CHECK_VALUE.
C_GenerateKey also provides a template that could define CKA_CHECK_VALUE: unlikely to have to right value, I admit :). in such case, the operation should fail with CKR_ATTRIBUTE_VALUE_INVALID.

[maroueneboubakri]

Updated the commit "ta: pkcs11: processing.c: calculate KCV on key generation" to comply with the specifications.

[maroueneboubakri]

Update: Moving the key check value handeling to a single function check_attr_checksum(). This function is called to set the attribute on C_GenerateKey(), C_CreateObject(), C_UnwrapKey(), C_DeriveKey(), C_CopyObject() and C_SetAttributeValue()

Commits will be squashed later, for now just to make sure the control flow is okay.

[etienne-lms]

Thanks for the update. The implementation looks quite nice. I have few comments (see below) but I think the main needed pieces are in place.

Since the spec says the attribute is optional, I really think a CFG_PCSK11_TA_CHECK_VALUE_ATTRIBUTE=y|n would be nice. Note this can be discussed in a separated P-R.

[maroueneboubakri]

Addressed most of the comments. Stills need to implement compute_check_value_with_sha1() add flag CFG_PCSK11_TA_CHECK_VALUE_ATTRIBUTE. Investigating issue with struct obj_attrs allocation prior to adding the KCV attribute. @etienne-lms

[maroueneboubakri]

Update: introduced CFG_PKCS11_TA_CHECK_VALUE_ATTRIBUTE flag to enable or not setting CKA_CHECK_VALUE attribute.

[maroueneboubakri]

All comments have been addressed.

[maroueneboubakri]

Comments addressed

[etienne-lms]

I think all commits should be squashed in a single one, inlcuding the change in ta/pkcs11/sub.mk that describes CFG_PKCS11_TA_CHECK_VALUE_ATTRIBUTE.

I'll make another review round on the changes but they look good to me.

[maroueneboubakri]

Commits squashed into one single commit.

[etienne-lms]

Thanks for the update.
The commit message is far too verbose. You don't need to describe everything that is done in the change, readers can look at the modification. The commit message should rather briefly describe what is changed and why.
Suggestion:

Add PKCS11_CKA_CHECK_VALUE as an optional attribute of symmetric
key and certificate objects . As per the PKCS#11 specification, key
check value attribute is optional therefore add pkcs11 TA configuration
switch CFG_PKCS11_TA_CHECK_VALUE_ATTRIBUTE to embed or not the support.

When supported, as per the spec, the attribute can be either the
legitimate value recomputed by the PKCS#11 token or a zero-sized value
called a no-value for when client does not want the attribute to set
in an object.

This change adds the support for the pcks11 TA commands related to
Cryptoki API functions C_GenerateKey(), C_CreateObject(), C_CopyObject(),
C_SetAttributeValue(), C_UnwrapKey() and C_DeriveKey(). TA command
related to C_FindOjects() support the attribute without any change.

[maroueneboubakri]

Comments addressed

[maroueneboubakri]

@etienne-lms any more comments or tags to apply ?

[etienne-lms]

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Could you provide few tests in a new test case in xtest pkcs11_1000.c to test this? By using CFG_PKCS11_TA_CHECK_VALUE_ATTRIBUTE=y in Qemu based CI test config, we would run regression test covering the feature.

[etienne-lms]

@vesajaaskelainen, maybe are you interested in looking at this change.

[maroueneboubakri]

@etienne-lms I have a concern here, as per your recommandation set_check_value_attr() has been moved before check_attrs_against_modification() to santize CKA_CHECK_VALUE against modification, however, at this stage the object is object attributes are required to compute the KCV such as the class and key type. Here, the template and object are both needed to compute/test the KCV. My question, should this call moved elsewhere where all attributes are consolidated/merged together ?

[maroueneboubakri]

Could you provide few tests in a new test case in xtest pkcs11_1000.c to test this?

PR which adds test cases OP-TEE/optee_test#719

[maroueneboubakri]

Change: For C_SetAttributeValue() -> entry_set_attribute_value() function set_check_value_attr() operates on object attributes not on the provided template. Object attributes such as the class, key type and key value are required to compute the KCV.

[etienne-lms]

One last comment otherwise looks good to me. Let's see it tested.
Last minute, I think the certificate part lacks the attribute definition.

[maroueneboubakri]

Here is the test result:

# xtest -t pkcs11 1029
Test ID: 1029
Run test suite with level=0

TEE test application started over default TEE instance
######################################################
#
# pkcs11
#
######################################################
 
* pkcs11_1029 PKCS11: Test support for object checksum value computation
o pkcs11_1029.1 Compute KCV on C_GenerateKey()
  pkcs11_1029.1 OK
o pkcs11_1029.2 Set KCV on C_CreateObject()
  pkcs11_1029.2 OK
o pkcs11_1029.3 Compute KCV on C_CopyObject()
  pkcs11_1029.3 OK
o pkcs11_1029.4 Compute KCV on C_DeriveKey()
  pkcs11_1029.4 OK
o pkcs11_1029.5 Compute KCV on C_UnwrapKey()
  pkcs11_1029.5 OK
o pkcs11_1029.6 Destroy KCV using C_SetAttributeValue()
  pkcs11_1029.6 OK
  pkcs11_1029 OK
+-----------------------------------------------------
Result of testsuite pkcs11 filtered by "1029":
pkcs11_1029 OK
+-----------------------------------------------------
25 subtests of which 0 failed
1 test case of which 0 failed
29 test cases were skipped
TEE test application done!

[maroueneboubakri]

Comments addressed and tag applied.

[etienne-lms]

Change: For C_SetAttributeValue() -> entry_set_attribute_value() function set_check_value_attr() operates on object attributes not on the provided template. Object attributes such as the class, key type and key value are required to compute the KCV.

I've fetched your test (OP-TEE/optee_test#719) and added few others tests (see etienne-lms/optee_test@f6f4cb5 temp link).

I see an issue with C_SetAttribute.
When client provides an invalid KCV value, the object is changed before the error is reported. The object should not change. We must review integration in C_SetAttributeValue.

[maroueneboubakri]

Change: For C_SetAttributeValue() -> entry_set_attribute_value() function set_check_value_attr() operates on object attributes not on the provided template. Object attributes such as the class, key type and key value are required to compute the KCV.

I've fetched your test (OP-TEE/optee_test#719) and added few others tests (see etienne-lms/optee_test@f6f4cb5 temp link).

I see an issue with C_SetAttribute. When client provides an invalid KCV value, the object is changed before the error is reported. The object should not change. We must review integration in C_SetAttributeValue.

@etienne-lms this has been asked in my comment above, for C_SetAttributeValue() both the template attributes and object attributes are required to make the computation and the check. My question was somehow if there is any way where these are merged together to proceed with check value computation and it seems that there is ony a way after the checks are done.

[etienne-lms]

C_SetAttributeValue is specific among the other cryptoki service is that it modifies an existing object. I'll propose a way, where this sequence preserves the object original data (attributes) if the service fails at any point. The idea is to copy the object attributes in a temporary reference.

[maroueneboubakri]

C_SetAttributeValue is specific among the other cryptoki service is that it modifies an existing object. I'll propose a way, where this sequence preserves the object original data (attributes) if the service fails at any point. The idea is to copy the object attributes in a temporary reference.

Thanks @etienne-lms, I'll update accordingly.

[etienne-lms]

See #6550.