Queries related to PKCS11 based tests

[anvisriv]

Hi Team

If repos (optee_test,optee_client,optee_os)with tag 4.0.0 is used then,

1. Can you please confirm whether all the PKCS11 related test cases are passing in OPTEE? If not, can you please highlight those testcases and the plan to fix them?
2. Some of the test cases(like 1008,1009.1020) require NULL input to be handled by returning the output size, although the GP specification(for TEE_CipherUpdate, TEE_MACComputeFinal) does not mention this requirement. These test cases are bound to fail where GP APIs are not implemented as the test case expects. Any reason why these test cases were designed in such a way?

[etienne-lms]

Hi @anvisriv,

1. Can you please confirm whether all the PKCS11 related test cases are passing in OPTEE?

All pkcs11 test from xtest are expected to pass.
OP-TEE CI tests check that using qemu_armv8a platform setup.

2. Some of the test cases(like 1008,1009.1020) require NULL input to be handled by returning the output size

Pkcs11 API functions differ from GP TEE API functions in how to get an output buffer size. With Pkcs11, to query an output buffer size, you can either pass a NULL pointer (in which case you get an CKR_OK status with the size output argument set) or you can pass a too small buffer (in which case you get a CKR_BUFFER_TOO_SMALL status and the size output argument set). pkcs11 xtest test are implemented using Pkcs11 cryptoki API functions.

[anvisriv]

Hi @etienne-lms

Pkcs11 API functions differ from GP TEE API functions in how to get an output buffer size. With Pkcs11, to query an output buffer size, you can either pass a NULL pointer (in which case you get an CKR_OK status with the size output argument set) or you can pass a too small buffer (in which case you get a CKR_BUFFER_TOO_SMALL status and the size output argument set). pkcs11 xtest test are implemented using Pkcs11 cryptoki API functions.

Can you please tell me where the output size is calculated for the ‘NULL buffer case with size as 0’ subtest of PKCS11 Test 1008?
Is it calculated inside the PKC11 cryptoki Client library or inside the PKCS11 TA?
Based on the code, it appears that the output size is calculated by the PKCS11 TA using GP TEE API TEE_MACComputeFinal (processing_symm.c#L854) and then returned to the PKCS11 Client library.

[etienne-lms]

The trick is done in the cryptoki API library. When a NULL ref is passed by caller, the API passes a 0 size non-null memref to the TA and expects CKR_BUFFER_TOO_SMALL return code which is converted back into a CKR_OK code. On TA side we always expect a buffer reference.

Regarding C_EncyptFinal() API function, the implementation (ck_signverify_final()) calls ckteec_alloc_shm() with a 0 size when sign_ref is NULL. When TA returns, we handle case CKR_BUFFER_TOO_SMALL && !sign_ref:

	if (sign_ref && sign_len && *sign_len)
		io = ckteec_register_shm(sign_ref, *sign_len,
					 sign ? CKTEEC_SHM_OUT : CKTEEC_SHM_IN);
	else
		io = ckteec_alloc_shm(0, sign ? CKTEEC_SHM_OUT : CKTEEC_SHM_IN);

	if (!io) {
		rv = CKR_HOST_MEMORY;
		goto bail;
	}

	rv = ckteec_invoke_ta(sign ? PKCS11_CMD_SIGN_FINAL :
			      PKCS11_CMD_VERIFY_FINAL, ctrl, NULL, io,
			      &io_size, NULL, NULL);

	if (sign && sign_len && (rv == CKR_OK || rv == CKR_BUFFER_TOO_SMALL))
		*sign_len = io_size;

	if (rv == CKR_BUFFER_TOO_SMALL && io_size && !sign_ref)
		rv = CKR_OK;

The same sequence is implemented in other cryptoki API functions, for example ck_wrap_key() for C_WrapKey().

[anvisriv]

Thanks for the explanation regarding the error codes. Could you also explain how the output_size would be handeled in these cases?

[anvisriv]

Hi @etienne-lms
Gentle Reminder on the pending query.

[etienne-lms]

Could you also explain how the output_size would be handeled in these cases?

The sequence is:

1. Client calls C_SignFinal(session, NULL, &out_size);.
2. Libckteec does not use the NULL reference but instead passes a 0 sized non-null pointer to pkcs11 TA.
3. Pkcs11 TA calls related TEE API function for signing, passing 0 as output buffer size.
4. OP-TEE OS returns TEE_ERROR_SHORT_BUFFER to the TA, passing back the requested output size.
5. Pkcs11 TA returns with error code CK_BUFFER_TO_SMALL, passing the requested output size.
6. Libckteec returns with code CKR_OK with out_size content set to the passed requested output size.
7. Client gets CKR_OK and the output size, as expected and can allocate its buffer and call C_SignFinal() again.

Does that answer your question?

[anvisriv]

Could you also explain how the output_size would be handeled in these cases?

The sequence is:

1. Client calls C_SignFinal(session, NULL, &out_size);.
2. Libckteec does not use the NULL reference but instead passes a 0 sized non-null pointer to pkcs11 TA.
3. Pkcs11 TA calls related TEE API function for signing, passing 0 as output buffer size.
4. OP-TEE OS returns TEE_ERROR_SHORT_BUFFER to the TA, passing back the requested output size.
5. Pkcs11 TA returns with error code CK_BUFFER_TO_SMALL, passing the requested output size.
6. Libckteec returns with code CKR_OK with out_size content set to the passed requested output size.
7. Client gets CKR_OK and the output size, as expected and can allocate its buffer and call C_SignFinal() again.

Does that answer your question?

Hi @etienne-lms
Thanks for the explanation.
What is the expected behavior for step 4? Should OPTEE return the size and error code, or is it the responsibility of the GP TEE API?
If OPTEE OS is responsible for this behavior, then is this behavior specific to OS?

Also, can you please tell which PKCS11 specification(v2.4 or v3.1) does current implementation follow?

[etienne-lms]

What is the expected behavior for step 4? Should OPTEE return the size and error code, or is it the responsibility of the GP TEE API?

It is part of the GP TEE Internal core API that there are functions that are expected to return TEE_ERROR_SHORT_BUFFER when an output buffer is either too small or is referred by a NULL pointer: these 2 methods allow a caller to get the required size of an output buffer.

Also, can you please tell which PKCS11 specification(v2.4 or v3.1) does current implementation follow?

We mainly follow the spec PKCS11 spec v2.4 with some extra support from the spec v3.0, if i'm right.

[anvisriv]

Hi @etienne-lms

Can you please point me in the GP specification where it says it is supposed to return size in these cases?

[etienne-lms]

This is described in section 3.4.4 [outbuf] in TEE Internal Core API specification v1.3.1:

• If the output does not fit in the output buffer, then the implementation SHALL update *size with
  the required number of bytes and SHALL return TEE_ERROR_SHORT_BUFFER. It is
  implementation-dependent whether the output buffer is left untouched or contains part of the
  output. In any case, the TA SHOULD consider that its content is undefined after the function
  returns.

When the function returns TEE_ERROR_SHORT_BUFFER , it SHALL return the size of the output data.

[anvisriv]

This is described in section 3.4.4 [outbuf] in TEE Internal Core API specification v1.3.1:

• If the output does not fit in the output buffer, then the implementation SHALL update *size with
  the required number of bytes and SHALL return TEE_ERROR_SHORT_BUFFER. It is
  implementation-dependent whether the output buffer is left untouched or contains part of the
  output. In any case, the TA SHOULD consider that its content is undefined after the function
  returns.

When the function returns TEE_ERROR_SHORT_BUFFER , it SHALL return the size of the output data.

Hi @etienne-lms

Could you also explain why the size is expected in subtest https://github.com/OP-TEE/optee_test/blob/4.0.0/host/xtest/pkcs11_1000.c#L6001?

As per section 3.4.4,

On entry, *size contains the number of bytes actually allocated in buffer. The buffer with this
number of bytes SHALL be entirely writable by the Trusted Application; otherwise the implementation
SHALL panic the calling Trusted Application instance. In any case, the implementation SHALL NOT
write beyond this limit.

Also,

Note that if the caller sets *size to 0, the function will always return TEE_ERROR_SHORT_BUFFER unless
the actual output data is empty. In this case, the parameter buffer can take any value, e.g. NULL, as it
will not be accessed by the implementation. If *size is set to a non-zero value on entry, then buffer cannot
be NULL because the buffer starting from the NULL address is never writable.

[etienne-lms]

Becareful. https://github.com/OP-TEE/optee_test/blob/4.0.0/host/xtest/pkcs11_1000.c#L6001 relies on the PKCS#11 API, not the GP TEE API.

PKCS#11 API behave differently:
When output buffer reference is NULL, the related PKCS#11 API function is expected to return CKR_OK and provide the output buffer size.
When output buffer reference is not NULL and the size is too smal, the related PKCS#11 API function is expected to return CKR_BUFFER_TOO_SMALL and to provide the output buffer size.

[anvisriv]

Hi @etienne-lms
In this case, is the output size returned from the GP TEE Internal API or does the PKCS11 API handle the case internally? If it is the PKCS11 API, could you please point me to the code?

Also, could you please help with the flow for the below test case?

xtest_pkcs11_test_1018
subtest: https://github.com/OP-TEE/optee_test/blob/4.0.0/host/xtest/pkcs11_1000.c#L5100
The expected Result is CKR_OK
However, the PKCS11 TA will return PKCS11_CKR_ARGUMENTS_BAD from https://github.com/OP-TEE/optee_os/blob/4.0.0/ta/pkcs11/src/processing_digest.c#L199.
Is this analysis correct?

[etienne-lms]

The test instruction you point to calls C_DigestFinal(session, NULL, &disgest_size) hence providing a NULL output buffer reference.

C_DigestFinal() is implemented by libckteec library: it calls ck_digest_final() that allocates an output buffer and invokes the pkcs11 with an output size set to 0 (here).
The function expects return code CKR_BUFFER_TOO_SMALL in which case it converts the return code into CKR_OK (here) and sets the output size to the size provided by the TA (here).
If everything goes as expected, xtest pkcs11_1018 get return code CKR_OK (here) and the requested output buffer size (here).

It the pkcs11 returns PKCS11_CKR_ARGUMENTS_BAD, then something went wrong and this return code to provided to the caller xtest application.

[anvisriv]

Hi @etienne-lms
Thanks for the explanation above.
Can you please help with the below queries as well?

• Is TEE_ALG_RSAES_PKCS1_V1_5 specific to OPTEE OS? If not, where can I find information about it in the GlobalPlatform (GP) specification? I could only find the symbol with Hash algorithm appended to it and not without it.

• Which specification does OPTEE OS libmbedtls follow?

[etienne-lms]

TEE_ALG_RSAES_PKCS1_V1_5 is indeed a GlobalPlatform defined identifier. See "Table C-1: Normative References for Algorithms", page 365 of the GP TEE Internal Core API spec v1.3.1.

Libmbedtls integrated in OP-TEE is based on mainline mbedtls (https://github.com/Mbed-TLS/mbedtls.git). Its revision tag depends on the OP-TEE release tag. Latest OP-TEE, since tag 4.0.0 is based on mbedtls-3.4.0; OP-TEE tags 3.19.0 to 3.22.0 were based on mbedtls-2.28.1; etc... See the Git history of OP-TEE OS directory lib/libmbedtls for more information.

[anvisriv]

Hi @etienne-lms

1. Can you please help with TEE_ALG_RSASSA_PKCS1_V1_5 also? Is it OPTEE specific? If not, where can I find information about it in the GlobalPlatform (GP) specification?

2. According to the GP specification for TEE_AsymetricSignDigest, if digestLen is not equal to the hash size of the algorithm in non-XOF (extendable output function) functions, panic will happen. Regarding the Test1019, where the digest_test_pattern_sha256 is used, shouldn't it be changed to digest_test_pattern_sha512 in the subtest?

[anvisriv]

Hi @etienne-lms
Did you get a chance to check on this?

[etienne-lms]

TEE_ALG_RSASSA_PKCS1_V1_5 is also defined in "Table C-1: Normative References for Algorithms" of the GP TEE Internal Core API spec v1.3.1, see page 364.

2. According to the GP specification ...

You mean the pkcs11 TA should have panicked for that test? We need to cross check this.

[anvisriv]

Hi @etienne-lms

TEE_ALG_RSASSA_PKCS1_V1_5 is also defined in "Table C-1: Normative References for Algorithms" of the GP TEE Internal Core API spec v1.3.1, see page 364.

The TEE_ALG_RSASSA_PKCS1_V1_5 is used in combination with SHA, but unlike TEE_ALG_RSAES_PKCS1_V1_5, there is no standalone symbol defined for it. Could you please verify once more if this symbol is specific to OPTEE?

[etienne-lms]

My bad, I was to quick grepping into the spec document.
Indeed TEE_ALG_RSASSA_PKCS1_V1_5 is an OP-TEE extension to the GP spec. It is defined in lib/libutee/include/tee_api_defines_extensions.h. Explanation can be found in commit 6a2e0a9.

[anvisriv]

Hi @etienne-lms

For ECDSA usecase, TA is determining TEE algorithm based on the key size of ECDSA(here) while the GP specification v1.3 in section B.3 Deprecated Algorithm Identifiers Page 352 says:

ECDSA algorithm identifiers should be tied to the size of the digest, not the key. The key size information is provided with the key material.

Can you please confirm what is the right expectation?

Adding one more query.
What is the ideal configuration for libutils and libmbedtls library to support PKCS11 TA's RSA AES wrap key use case ?

[anvisriv]

Hi @etienne-lms did you get a chance to check?

[anvisriv]

Hi @etienne-lms
In wrap_rsa_aes call flow,
wrap_rsa_aes_key --> mbedtls_nist_kw_setkey --> mbedtls_cipher_setup -> ctx_alloc_func --> (becuase it is AES, call flow should be coming to aes_ctx_alloc --> mbedtls_calloc --> calloc -> raw_calloc --> bgetz --> bget

In bget there is while (b != &poolset->freelist) but i see that this condition will never be true, as in bget_malloc.c , poolset->freelist.ql.flink = &poolset->freelist, consequently always NULL is returned.

Assuming all the BGET configurations are disabled. In the above call flow i didn't find where the poolset's freelist is getting modified to return otherwise from bget.

Can you please guide and clarify the understanding?

[anvisriv]

Hi @etienne-lms

Adding one more question to the list :)
GP specification for TEE_PopulateTransientObject (Section 5.6.4 Table 5-10), in case of TEE_TYPE_RSA_KEYPAIR says,

The following attributes SHALL be provided:
TEE_ATTR_RSA_MODULUS
TEE_ATTR_RSA_PUBLIC_EXPONENT
TEE_ATTR_RSA_PRIVATE_EXPONENT
The CRT parameters are optional. If any of these attributes is
provided, then all of them SHALL be provided:
TEE_ATTR_RSA_PRIME1
TEE_ATTR_RSA_PRIME2
TEE_ATTR_RSA_EXPONENT1
TEE_ATTR_RSA_EXPONENT2
TEE_ATTR_RSA_COEFFICIENT

But, in case of test 1026, the test client is only setting first 5 attributes for unwrapping key. Will it not cause any issue in TEE_PopulateTransientObject in case of Unwrap usecase?

[etienne-lms]

Sorry, I've not looked at your request. I'll try to find a bit of time....

[anvisriv]

Hi @etienne-lms
Can you also share what are the appropriate inputs that needs to be passed when EncryptUpdate and EncryptFinal are used? Also, for DecryptUpdate/DecryptFinal?

[anvisriv]

Hi @etienne-lms

Did you got a chance to check?

[etienne-lms]

Hi @anvisriv, sorry for my late feedback.

/Regarding comment #721 (comment):

Can you also share what are the appropriate inputs that needs to be passed when EncryptUpdate and EncryptFinal are used? Also, for DecryptUpdate/DecryptFinal?

These API functions are described in the PKCS#11 spec (e.g. https://docs.oasis-open.org/pkcs11/pkcs11-base/v3.0/os/pkcs11-base-v3.0-os.html#_Toc29976646 for C_EncryptUpdate()). I strongly recommend to read the spec to properly use the PKCS#11 interface. OP-TEE's implementation of this interface is expected to follow the spec. Do you have questions on our implementation or on the spec itself?

/Regarding comment #721 (comment):

GP specification for TEE_PopulateTransientObject (Section 5.6.4 Table 5-10), in case of TEE_TYPE_RSA_KEYPAIR says,
...
But, in case of test 1026, the test client is only setting first 5 attributes for unwrapping key. Will it not cause any issue in TEE_PopulateTransientObject in case of Unwrap usecase?

Unless I missed something, the pkcs11 TA does consider this constriaint: see https://github.com/OP-TEE/optee_os/blob/4.2.0/ta/pkcs11/src/processing_rsa.c#L562-L593.

/Regarding comment #721 (comment) on bget:

I'm not shure to understand your question. Do you face issues with bget allocator integration? It's been used since a while in OP-TEE core and TAs.
I found this article on Bget (how its OP-TEE integration and miss-use could lead to exploits) maybe it can help you:
https://phi1010.github.io/2020-09-14-bget-exploitation/ and https://phi1010.github.io/2020-11-02-bget-exploitation-2/.

/Regarding comment #721 (comment) on ECDSA and PKCS#11 key wrapping with RSA:

For ECDSA usecase, TA is determining TEE algorithm based on the key size of ECDSA ...

This is still an open point. A pull request was created last summer but stalled before reaching a clean conclusion, see OP-TEE/optee_os#6230. I think the issue is still pending and deserves investigation.

What is the ideal configuration for libutils and libmbedtls library to support PKCS11 TA's ... ?

Pkcs11 TA implementation embeds MbedTLS library and supports key wrapping. There is nothing to configure, but ensure the TA heap is sized enough (CFG_PKCS11_TA_HEAP_SIZE), your platform produces good quality random bytes, and the RSA key used to wrap the AES key is well sized (I think an RSA key of at least 3072bit is recommended for protection of a 128byte AES key, but you should better crosscheck this information).

[anvisriv]

Hi @etienne-lms

Unless I missed something, the pkcs11 TA does consider this constriaint: see https://github.com/OP-TEE/optee_os/blob/4.2.0/ta/pkcs11/src/processing_rsa.c#L562-L593.

Even in the absence of exponent1, exponent2, and coefficient, the count will continue to increment, potentially leading to complications when invoking TEE_PopulateTransientObject. Can you please reverify on your end?

Also, thanks for other references.

[etienne-lms]

Right @anvisriv, there is a bug.
pkcs2tee_load_attr() returns false if the pkcs11 attribute is not found, so count does not increment to 8. However the return code in rc is already PKC11_CKR_OK so even if ``count != 8`, indeed, the function returns with a success.
Nice catch. We need a fix in. Do you want to create a pull request and propose a fix?

[anvisriv]

Hi @etienne-lms

Do you want to create a pull request and propose a fix?

Sure. I will create a pull request.

[etienne-lms]

Hi @anvisriv, I took the liberty of creating a P-R for the RSA attributes in the pkcs11 TA. Please see OP-TEE/optee_os#6815.

[anvisriv]

Hi @etienne-lms
Could you please provide more details on how this change will address the previously discussed issue?
Is it feasible to include a memory leak test case that detects memory leaks in both the PKCS11 library and the Trusted Application (TA)?

[etienne-lms]

Could you please provide more details on how this change will address the previously discussed issue?

This change ensures that rc = PKCS11_CKR_OK only if count == 8 before the break instruction (line 597).

Is it feasible to include a memory leak test case that detects memory leaks in both the PKCS11 library and the Trusted Application (TA)?

I guess it is feasible. Proposals are welcome.
One way to test the TA against heap allocation leakage (is that what you mean by "memory leaks"?) is to run a usecase (some operation) many times. TA heap is quite small (few kBytes, see CFG_PKCS11_TA_HEAP_SIZE) so heap leakage should soon trigger a CKR_DEVICE_MEMORY error.
As for the pkcs11 lib (libckteec) heap leakage, it will be a bit harder I think as Linux applications heap is far bigger.

[jforissier]

Is it feasible to include a memory leak test case that detects memory leaks in both the PKCS11 library and the Trusted Application (TA)?

I guess it is feasible. Proposals are welcome. One way to test the TA against heap allocation leakage (is that what you mean by "memory leaks"?) is to run a usecase (some operation) many times. TA heap is quite small (few kBytes, see CFG_PKCS11_TA_HEAP_SIZE) so heap leakage should soon trigger a CKR_DEVICE_MEMORY error.

Maybe the statistics PTA could be used to query the TA heap size before and after the test ?

[anvisriv]

This change ensures that rc = PKCS11_CKR_OK only if count == 8 before the break instruction (line 597).

Hi @etienne-lms, the count will increase regardless of whether the value is NULL or not NULL, as there is no condition to stop the execution or treat it as an error. Furthermore, the code should handle these three scenarios:

1. The client provides 3 attributes.
2. The client provides 8 attributes.
3. The client supplies a number of attributes that is either less than 3 or more than 3 but less than 8.
   Please let me know if i have missed anything.

I guess it is feasible. Proposals are welcome. One way to test the TA against heap allocation leakage (is that what you mean by "memory leaks"?) is to run a usecase (some operation) many times. TA heap is quite small (few kBytes, see CFG_PKCS11_TA_HEAP_SIZE) so heap leakage should soon trigger a CKR_DEVICE_MEMORY error. As for the pkcs11 lib (libckteec) heap leakage, it will be a bit harder I think as Linux applications heap is far bigger.

How about we consider redefining malloc, calloc, free, and realloc using macro to track memory allocation in libckteec? This approach may require changes in most of the source code of the library.
Or, is it possible to hook up Valgrind?

Maybe the statistics PTA could be used to query the TA heap size before and after the test ?

Sure, will have a look at it for detecting memory leak in TA.

[etienne-lms]

Hi @etienne-lms, the count will increase regardless of whether the value is NULL or not NULL, as there is no condition to stop the execution or treat it as an error. Furthermore, the code should handle these three scenarios:

The client provides 3 attributes.
The client provides 8 attributes.
The client supplies a number of attributes that is either less than 3 or more than 3 but less than 8.
Please let me know if i have missed anything.

Would you be ok to post comments in OP-TEE/optee_os#6815 and discuss the fix on that P-R?

I think the P-R is okay: load_tee_rsa_key_attrs() will succeed only if either the 3 mandatory attributes are found, or if the 8 attributes are found. If the attribute are wrong (e.g. empty) then I expect the TEE operation using the key to fail. But I may have missed something, please fell free to contradict me.

As for the pkcs11 lib (libckteec) heap leakage, it will be a bit harder I think as Linux applications heap is far bigger.

How about we consider redefining malloc, calloc, free, and realloc using macro to track memory allocation in libckteec? This approach may require changes in most of the source code of the library.
Or, is it possible to hook up Valgrind?

I guess using Valgrind could help. Instrumenting allocation from libckteec looks also a good solution. Feel free to propose change if you have some ideas.

[anvisriv]

think the P-R is okay: load_tee_rsa_key_attrs() will succeed only if either the 3 mandatory attributes are found, or if the 8 attributes are found. If the attribute are wrong (e.g. empty) then I expect the TEE operation using the key to fail. But I may have missed something, please fell free to contradict me.

Sure.

I guess using Valgrind could help. Instrumenting allocation from libckteec looks also a good solution. Feel free to propose change if you have some ideas.

Are there any steps available for using Valgrind on PKCS11 TA/lib or any other OPTEE TA/lib?