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

Key generation should not use token GenerateRandom() #280

Closed
fabled opened this issue Sep 2, 2023 · 6 comments · Fixed by #401
Closed

Key generation should not use token GenerateRandom() #280

fabled opened this issue Sep 2, 2023 · 6 comments · Fixed by #401
Assignees

Comments

@fabled
Copy link
Collaborator

fabled commented Sep 2, 2023

When generating a key on token, and if ID is not specified, pkcs11-provider falls back to generating random bytes in https://github.com/latchset/pkcs11-provider/blob/main/src/keymgmt.c#L375-L382.

This is problematic because a token may support key generation, might not implement the pkcs#11 random generator interface. Using the token random generator may also be unexpectedly slow.

While object ID is a mandatory object attribute, it is not a required attribute in key generation template. Omitting it from the template is valid, and usually the tokens will generate deterministic ID by hashing the public key components (or possibly just randomizing it for secret keys). The token generated ID can be read back from the generated key object.

@beldmit
Copy link
Collaborator

beldmit commented Sep 2, 2023

IRL it may be a problem - if attribute is omitted and automatically generated instead, it will be necessary to use 3rdparty tools to find out the URI of freshly generated object

@fabled
Copy link
Collaborator Author

fabled commented Sep 2, 2023

IRL it may be a problem - if attribute is omitted and automatically generated instead, it will be necessary to use 3rdparty tools to find out the URI of freshly generated object

I do not follow you. The caller can still specify the ID if it wants in the id attribute. And should do so if it wants to include the id in the URI. There's also a separate issue about specifying the (potentially full) URI. See #279.

This issue is about if the application intentionally does not set ID. In this case pkcs11-provider automatically generates one using the token's GenerateRandom which might not be implemented (and thus fail the key generation). The token is also required to generate the ID if the attribute is missing from the template. In this case pkcs11-provder can get the ID from the returned object handle.

And you are not required to put the ID in the URI either.

@beldmit
Copy link
Collaborator

beldmit commented Sep 2, 2023

I agree that call to GenerateRandom may cause side effects and, if it is possible to check whether it is implemented or fallback to openssl RNG, it would be better (didn't look into the code here).

@simo5
Copy link
Member

simo5 commented Sep 5, 2023

@fabled Tokens are required to generate CKA_UNIQUE_ID but not CKA_ID (default empty in spec).

But CKA_ID is the only field of the two that i accessible via PKCS#11 URIs.

I think I can simply omit the CKA_ID, or, perhaps retrieve the CKA_UNIQUE_ID, and set the CKA_ID to match that attribute if GenerateRandom fails.

Key Generation is a very slow process, I do not care if GenerateRandom slows it down a little bit more, so that is not really a consideration unless you find a token where GenerateRandom is so slow to be comparable to the whole key Generation operation.

@fabled
Copy link
Collaborator Author

fabled commented Sep 5, 2023

@fabled Tokens are required to generate CKA_UNIQUE_ID but not CKA_ID (default empty in spec).

Ah, my confusion on this.

But CKA_ID is the only field of the two that i accessible via PKCS#11 URIs.

I think I can simply omit the CKA_ID, or, perhaps retrieve the CKA_UNIQUE_ID, and set the CKA_ID to match that attribute if GenerateRandom fails.

Key Generation is a very slow process, I do not care if GenerateRandom slows it down a little bit more, so that is not really a consideration unless you find a token where GenerateRandom is so slow to be comparable to the whole key Generation operation.

This is fair assumption for RSA. But EC key generation is actually fairly fast. The generation speed can be easily dominated by the communication latency/speed. So this can have significant performance impact.

But more importantly It is possible that the PKCS#11 library does not implement RNG API.

Given the above. Would it be an option to just use OpenSSL or system random function generate the ID?

@simo5
Copy link
Member

simo5 commented Sep 5, 2023

Given the above. Would it be an option to just use OpenSSL or system random function generate the ID?

I am open to any scheme that makes sense, I am not wedded to GenerateRandom and a fully random ID.

The idea of using a random id is that collision are extremely unlikely, and I can calculate it before key generation is perfomed, so that we do not require multiple roundtrip.

I think it would also be perfectly fair to stop generating it, but that would not be great because some fallback functions like the one that searches froma matching public object when the private object does not have public attributes relies on it.

@Jakuje Jakuje self-assigned this Apr 2, 2024
simo5 added a commit to simo5/pkcs11-provider that referenced this issue May 30, 2024
Let *that* cycle back into the provider and call C_GenerateRandom()
if that's how the properties end up wiring things.

Fixes latchset#280

Signed-off-by: Simo Sorce <[email protected]>
@simo5 simo5 mentioned this issue May 30, 2024
10 tasks
simo5 added a commit to simo5/pkcs11-provider that referenced this issue May 30, 2024
Let *that* cycle back into the provider and call C_GenerateRandom()
if that's how the properties end up wiring things.

Fixes latchset#280

Signed-off-by: Simo Sorce <[email protected]>
simo5 added a commit to simo5/pkcs11-provider that referenced this issue May 30, 2024
Let *that* cycle back into the provider and call C_GenerateRandom()
if that's how the properties end up wiring things.

Fixes latchset#280

Signed-off-by: Simo Sorce <[email protected]>
simo5 added a commit to simo5/pkcs11-provider that referenced this issue May 31, 2024
Let *that* cycle back into the provider and call C_GenerateRandom()
if that's how the properties end up wiring things.

Fixes latchset#280

Signed-off-by: Simo Sorce <[email protected]>
simo5 added a commit that referenced this issue May 31, 2024
Let *that* cycle back into the provider and call C_GenerateRandom()
if that's how the properties end up wiring things.

Fixes #280

Signed-off-by: Simo Sorce <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants