Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkcs11: keep EC_POINT in attributes for private object creation #5166

Conversation

StevenCai-Gallagher
Copy link

EC_POINT attribute should be stored in both private and public object.
Remove the empty EC_POINT attribute when TA try to generate EC keys.

Signed-off-by: Steven Cai steven.cai@gallagher.com

When using imported EC keypair to sign a digest. It will be failed in c_signinit, which
unable to find the EC_POINT attribute in the private key object. The root cause of that
is EC_POINT doesn't be stored in the private key object when it's created. This PR
tries to add the EC_POINT attribute from input templates if it's available. And remove
the empty EC_POINT attribute when it tries to generate an EC key.

issue: #5165

EC_POINT attribute should be stored in both private and public object.
Remove the empty EC_POINT attribute when TA try to generate EC keys.

Signed-off-by: Steven Cai <steven.cai@gallagher.com>
@vesajaaskelainen
Copy link
Contributor

As far I can tell EC_POINT is not supposed to be stored in private key.

From PKCS#2.40:
2.3.5 Elliptic curve key pair generation

The mechanism contributes the CKA_CLASS, CKA_KEY_TYPE, and CKA_EC_POINT attributes to the new public key and the CKA_CLASS, CKA_KEY_TYPE, CKA_EC_PARAMS or CKA_ECDSA_PARAMS and CKA_VALUE attributes to the new private key.

Also object specs do not specify it being so:

@github-actions
Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Mar 16, 2022
@github-actions github-actions bot closed this Mar 21, 2022
@jforissier jforissier reopened this Jun 19, 2023
@ldts
Copy link
Contributor

ldts commented Jun 19, 2023

um, does this request comply with the PKCS#11 standard?

I am asking because other useful changes along the lines of this one - i.e: importing pre-provisioned keys from secure elements into the pkcs11 secure storage - have been proposed and rejected for not complying.

@github-actions github-actions bot removed the Stale label Jun 20, 2023
@vesajaaskelainen
Copy link
Contributor

um, does this request comply with the PKCS#11 standard?

I am asking because other useful changes along the lines of this one - i.e: importing pre-provisioned keys from secure elements into the pkcs11 secure storage - have been proposed and rejected for not complying.

Problem with that is that you leave door open to operate with the same key from other environments bypassing PKCS#11 TA's security policies who can access/operate with the keys.

In here the one imports EC Public & Private Key from clear text forms. In that operation one transfers the ownership to the PKCS#11 TA and then the key material is owned. If you happen to have copy of such material elsewhere that is not a problem for PKCS#11 TA as such.

@cdo82
Copy link

cdo82 commented Jun 28, 2023

For the moment the PKCS11 specification didn't allow EC_POINT for EC Private key.
But The behavior of PKCS11 TA need to be coherent.
When PKCS11 TA generate an EC keypair, the EC_POINT is set for public AND private object.
https://github.com/OP-TEE/optee_os/blob/83a3d56a2c82888c86dd03bf6851cde815936020/ta/pkcs11/src/processing_ec.c#L551C1-L556C1
https://github.com/OP-TEE/optee_os/blob/83a3d56a2c82888c86dd03bf6851cde815936020/ta/pkcs11/src/processing_ec.c#L376C1-L403C1

If a people want import EC Private keys (for exemple from another TA to PKCS11 TA using PKCS11 API), this key will be not usable due to PKCS11 TA implementation.

@vesajaaskelainen
Copy link
Contributor

vesajaaskelainen commented Jun 28, 2023

For the moment the PKCS11 specification didn't allow EC_POINT for EC Private key. But The behavior of PKCS11 TA need to be coherent. When PKCS11 TA generate an EC keypair, the EC_POINT is set for public AND private object. https://github.com/OP-TEE/optee_os/blob/83a3d56a2c82888c86dd03bf6851cde815936020/ta/pkcs11/src/processing_ec.c#L551C1-L556C1 https://github.com/OP-TEE/optee_os/blob/83a3d56a2c82888c86dd03bf6851cde815936020/ta/pkcs11/src/processing_ec.c#L376C1-L403C1

If a people want import EC Private keys (for exemple from another TA to PKCS11 TA using PKCS11 API), this key will be not usable due to PKCS11 TA implementation.

Yes and this is a problem that needs to be fixed too.

The problem here is that TEE Internal API requires one to have both public and private properties when doing operations.

Kinda the right thing would be to create hidden property for public key part when private key is imported or when it is generated -- but we have now fleet of devices with the current behavior so right thing probably would be to make it generate/import the public key properties to private key object but with new hidden attribute. And then first look for that and if not found then look for "older" way.

When one generates new key pair:

  • private key object:
    • PKCS11_CKA_OPTEE_EC_POINT_HIDDEN: public key
    • PKCS11_CKA_VALUE: private key
  • public key object:
    • PKCS11_CKA_EC_POINT: public key

When one imports private key:

  • private key object:
    • PKCS11_CKA_OPTEE_EC_POINT_HIDDEN:
    • PKCS11_CKA_VALUE: private key

Now when key is opened then:

  • private key object:
    • read PKCS11_CKA_OPTEE_EC_POINT_HIDDEN
      • backup read: PKCS11_CKA_EC_POINT
    • read PKCS11_CKA_VALUE

@etienne-lms comments on above? -- I could try to bake the code for that.

@github-actions
Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Jul 29, 2023
@vesajaaskelainen
Copy link
Contributor

I have above mentioned changes in development that should replace this PR.

@vesajaaskelainen
Copy link
Contributor

PR #6204 fixes the problem. Please check it out.

@github-actions github-actions bot removed the Stale label Jul 30, 2023
@github-actions
Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Aug 30, 2023
@github-actions github-actions bot closed this Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants