Passing CK_P11PROV_IMPORTED_HANDLE while creating mock public key

[kshitizvars]

This commit adds CK_P11PROV_IMPORTED_HANDLE argument while creating mock public key session object.

Before this patch, when we run TLS1.3 connection, below issue was reported by openssl:-

tls_parse_ctos_key_share:unable to find ecdh
parameters:ssl/statem/extensions_srvr.c:684

It is because of returning CK_INVALID_HANDLE instead of obj->handle.

Checklist

• Code modified for feature

Reviewer's checklist:

• Any issues marked for closing are addressed
• There is a test suite reasonably covering new functionality or modifications
• This feature/change has adequate documentation added
• Code conform to coding style that today cannot yet be enforced via the check style test
• Commits have short titles and sensible commit messages
• Coverity Scan has run if needed (code PR) and no new defects were found

[simo5]

LGTM, but please fix style

[simo5]

SoI have been thinking about this overnight, and I am surprised we do not see this issue in the current tests we have. Is there any change we can make to the test suite to catch this error, so we ensure it does not regress ?

[simo5]

@kshitizvars tabs vs spaces style failure, I strongly suggest to use make check-style-fix and commit --amend the code it modifies.

[kshitizvars]

SoI have been thinking about this overnight, and I am surprised we do not see this issue in the current tests we have. Is there any change we can make to the test suite to catch this error, so we ensure it does not regress ?

Hi @simo5 ,

Could you please point out the test? Have you tested full TLS1.3 connection?

[simo5]

SoI have been thinking about this overnight, and I am surprised we do not see this issue in the current tests we have. Is there any change we can make to the test suite to catch this error, so we ensure it does not regress ?

Hi @simo5 ,

Could you please point out the test? Have you tested full TLS1.3 connection?

Yes if you look at tests/ttls you will see that the last test verifies a full tls 1.3 connection forcing all operations on the token

[simo5]

@kshitizvars can you tell me what pkcs11-provider function tries to resolve this object?
In my tests I never see a need to turn this object into an actual session key, as p11prov_obj_get_ec_public_raw() should be able to extract the value we imported in the mock object directly. So I would like to know what operation fails before merging.
The aim is to avoid needlessly creating session objects on the token, as we have no way to remove them until the session is closed or the program terminates.

[kshitizvars]

@simo5 p11prov_ecdh_derive API calls p11prov_obj_get_handle which further checks the obj->handle and returns obj->handle which was still CK_INVALID_HANDLE and hence cause issue.

[simo5]

Wait,
p11prov_obj_get_handle() is called on the private key, not the peer key.
@kshitizvars are you trying to do ECDH exchange with a private key imported from OpenSSL?

That's unusual given OpenSSL should be asking the provider to generate the key and therefore you should have a valid handle. Is your provider incapable of generating ECC keys, and relies on OpenSSL to generate them and import them back ?

I mean, we can support this scenario I guess, but I want to make sure this is intentional.

[kshitizvars]

Hi @simo5
Sorry for wrong explanation earlier, first of all we are not trying to ECDH exchange with private key, we are doing it with public key only, as per TLS protocol.

Below is the correct explanation:-

On debugging, it was found that tls_parse_ctos_key_share API inside ssl/statem/extensions_srvr.c calls ssl_generate_param_group -> EVP_PKEY_paramgen -> EVP_PKEY_generate ->evp_keymgmt_util_gen -> evp_keymgmt_gen -> p11prov_ec_gen -> mock_pub_ec_key -> p11prov_obj_new

And before this patch, p11prov_obj_new returns obj as NULL, due to which below conditions gets failed inside OpenSSL in tls_parse_ctos_key_share:-

if ((s->s3.peer_tmp = ssl_generate_param_group(s, group_id)) == NULL) {
 684             SSLfatal(s, SSL_AD_INTERNAL_ERROR,
 685                    SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS);
 686             return 0;
 687         
}

[simo5]

I am sorry but I know that part of ECDH works because we have explicit tests for it,
can you tell me how to reproduce the problem you see?

I assume there is a corner case the CI tests do not cover?

Or provide a log file that shows the issue so I can attempt to write a test to reproduce the problem in CI ?

I understand you have an issue, I am not contesting that at all, and will gladly merge this PR, but my problem is to have a test so we do not regress in future. Currently this code change has no tests...

[kshitizvars]

Steps used:-

Openssl.conf related change:-

[openssl_init]
providers = provider_sect
alg_section = algorithm_sect
 
# List of providers to load
[provider_sect]
default = default_sect
pkcs11 = pkcs11_sect
 
[default_sect]
activate = 1
[pkcs11_sect]
module = /usr/lib/ossl-modules/pkcs11.so
pkcs11-module-path = /usr/lib/libckteec.so.0
pkcs11-module-cache-keys = false
pkcs11-module-quirks = no-operation-state
pkcs11-module-block-operations = digest
activate = 1
 
[algorithm_sect]
default_properties = ?provider=pkcs11

Commands used:-

#Server side command:-
$ openssl s_server -key "pkcs11:model=OP-TEE%20TA;manufacturer=Linaro;serial=0000000000000001;token=token0;id=%01;object=ecc-key-256;type=private?pin-value=1234" -cert server.crt -accept 443
 
 
Run openssl s_client from another machine:-
#Client side command:-
$ openssl s_client -connect <server ip>:443 -tls1_3 -ciphersuites 'TLS_AES_256_GCM_SHA384'

Debug logs:- debug_with_bad_commit.txt

[simo5]

The debug log says you are testing on version 0.3, is that right ?

I can't reproduce with the code on main.

[kshitizvars]

No, its on latest code base only, logs related to 0.3 version are coming because I have compiled code using yocto and used devtool for changing source code to point at latest pkcs11-provider code. Btw, I am currently on below commit:-

commit d6510d6dee7ecb78dbc0287ff14cd2a6bbcb3e2e (HEAD -> public_key_import, main-repo/main, main-repo/main)
Author: Simo Sorce <[email protected]>
Date:   Tue Oct 8 09:48:32 2024 -0400

    Fix TLS Script

    Not sure "how" this fixes the tests, but without these changes we get an
    error in the last TLS tests with latest code changes, and this error
    goes away when modifying the expect script to be more verbose.

    Signed-off-by: Simo Sorce <[email protected]>

[simo5]

And before this patch, p11prov_obj_new returns obj as NULL, due to which below conditions gets failed inside OpenSSL in tls_parse_ctos_key_share:-

I think you tested with an old version of pkcs11-provider before we started supporting mock objects for ECDH, if p11prov_obj_new() were to return NULL (which it did before that was fixed) then you would never get to a pint where you have an object with CK_HANDLE_INVALID at all ...

So I am confused here. We test fully tls1.3 connections in tests/ttls and ECDH in
tests/tecdh and neither of those test seem to have an issue with the mock_ec_pub_key codepath on three different tokens.

How hard is it to get your token in CI? Does it require special HW or special Software ?
If it requires special stuff I wonder if you would be able to host a runner so we get these tests running there and I can help diagnose issues directly.

[kshitizvars]

Hi @simo5

After your patch:- 4f67dfd#diff-27cab8114f01afc3d42583bae767a29fedc048b745ad01757090e4235a22ade0

whenever p11prov_obj_new is called with handle as CK_P11PROV_IMPORTED_HANDLE , object get returned, without adding it to pool. See below code:-

275     if (handle == CK_P11PROV_IMPORTED_HANDLE) {
 276         /* mock object, return w/o adding to pool */
 277         return obj;
 278     }
 279

and, when handle value is other than CK_P11PROV_IMPORTED_HANDLE, objects get stored into pool and inside p11prov_slot_get_obj_pool, we are calling p11prov_slot_get_obj_pool which do below work:-

478     for (int s = 0; s < sctx->num; s++) {
479         if (sctx->slots[s]->id == id) {
480             slot = sctx->slots[s];
481             break;
482         }
483     }
484
485     if (!slot) {
486         ret = CKR_SLOT_ID_INVALID;

And, as slot id is currently CK_UNAVAILABLE_INFORMATION, hence p11prov_slot_get_obj_pool returns with return value 3 which is CKR_SLOT_ID_INVALID, and hence p11prov_obj_new returns NULL.

Moreover, I was going through your tests

pkcs11-provider/tests/ttls

Line 111 in c7fb177

#Try again forcing all operations on the token

, it seems like, you haven't ran below test:-
run_test "$ECPRIURI" "$ECCRTURI" "" "-tls1_3 -ciphersuites 'TLS_AES_256_GCM_SHA384' -groups secp256r1".

Point is to run TLS1.3 with ciphersuite as TLS_AES_256_GCM_SHA384 and group as secp256r1

Because, I am getting issue in above scenario only. You can try running this test.

[simo5]

Ok we are adding tests and more fixes to this are in #453 so we'll merge this and defer the needed tests to that PR.