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

Mark PKCS#11 private keys as unmodifiable #1018

Open
ximon18 opened this issue Feb 28, 2023 · 15 comments · Fixed by #1020
Open

Mark PKCS#11 private keys as unmodifiable #1018

ximon18 opened this issue Feb 28, 2023 · 15 comments · Fixed by #1020
Assignees
Labels
enhancement New feature or request hsm Relates to adding HSM support to Krill

Comments

@ximon18
Copy link
Member

ximon18 commented Feb 28, 2023

Per best practice, e.g. from http://secgroup.dais.unive.it/wp-content/uploads/2010/10/Reponse-by-SafeNet.pdf:

  1. Follow best practices when setting sensitive key attributes: If you have secret or private keys
    that are particularly sensitive and you want to prevent them from being wrapped off, they can be
    generated with their template attributes: CKA_SENSITIVE and CKA_PRIVATE set to True and
    CKA_EXTRACTABLE and CKA_MODIFIABLE both set to False. This way, the keys are only
    accessible by a user who is logged in, and key values cannot be read by anyone. Also, the keys
    cannot be wrapped off and the attribute values cannot be changed at some later time to invalidate
    the original settings

Currently when Krill creates an RSA key pair using the PKCS#11 C_GenerateKeyPair() function it sets key attributes CKA_SENSITIVE and CKA_PRIVATE to CK_TRUE ✔️ and CKA_EXTRACTABLE to CK_FALSE ✔️ as per the guidance above, but leaves CKA_MODIFIABLE to the default value (which is CK_TRUE according to section 4 of the PKCS#11 v2.40 specification) ❌.

@ximon18 ximon18 added enhancement New feature or request hsm Relates to adding HSM support to Krill labels Feb 28, 2023
@ties
Copy link

ties commented Feb 28, 2023

To fully restrict keys, CKA_COPYABLE should probably also be CK_FALSE

@ximon18
Copy link
Member Author

ximon18 commented Mar 2, 2023

@ties: I've re-read the PKCS#11 v2.40 specification wherever it mentions copying of keys but I can't find a situation that would be a security concern because we set CKA_SENSITIVE to CK_TRUE and CKA_EXTRACTABLE to CK_FALSE and CKA_MODIFIABLE to CK_FALSE (the latter with this PR). The specification doesn't allow for changing these attributes from more secure to less secure, only vice versa. Once CKA_COPYABLE is set to CK_FALSE it cannot be changed back to CK_TRUE.

I'm not sure in what scenario a copy within the HSM that cannot change these attributes would be wanted, maybe an operator wants to keep a copy of private keys to ensure that if the original is deleted or modified that this can be detected?

I'm unsure about preventing copying as this would be prevent an operation that was previously permitted for HSM operators without it being clear which benefit it actually brings.

The article referenced above says nothing about copying. I found another two articles, the first says nothing about copying, the second however does seem to advise against it but it's unclear to me if that's really to do with weaknesses in implementations rather than with the specification, and there's nothing we can do about insecure implementations, some workshop slides also don't mention copying.

@rijswijk, @halderen & @Koenvh1: Do any of you have any thoughts on this?

@timbru
Copy link
Contributor

timbru commented Mar 8, 2023

@ties @rijswijk @halderen @Koenvh1

My concern with restrictions on the keys is that - even though it may be best practice - it is currently not possible to perform a key rollover for an RPKI TA key, or more importantly for the BPKI ID keys used in the provisioning and publication protocols.

Therefore, there may be something to be said for being less restrictive wrt extracting keys out of the HSM in case a user needs to migrate to another vendor. Or in case this setting is needed to migrate to a new HSM by the same vendor.

If users actively choose to add restriction per best practice that is of course fine. Perhaps this is a matter of clear documentation so that the user can decide what they want?

What do you think?

@ximon18
Copy link
Member Author

ximon18 commented Mar 8, 2023

Also, I'm really not sure about how this influences, if at all, other means of accessing the keys, e.g. via HSM vendor specific tooling. Maybe it is always possible to get the keys out using vendor specific tooling irrespective of what flags were set via the PKCS#11 library interface?

@ties
Copy link

ties commented Mar 8, 2023

My concern with restrictions on the keys is that - even though it may be best practice - it is currently not possible to perform a key rollover for an RPKI TA key, or more importantly for the BPKI ID keys used in the provisioning and publication protocols.

Therefore, there may be something to be said for being less restrictive wrt extracting keys out of the HSM in case a user needs to migrate to another vendor. Or in case this setting is needed to migrate to a new HSM by the same vendor.

If users actively choose to add restriction per best practice that is of course fine. Perhaps this is a matter of clear documentation so that the user can decide what they want?

What do you think?

For the HSMs I have experience with (Entrust), the operations possible on a key are the intersection of the settings for the main HSM keys protecting other keys ("security world") allows and what is allowed for a key.

Depending on those settings, it may even not be possible to migrate settings between HSMs, or not between versions of HSMs (newer "security world"). These interactions are complicated, and too strict key attributes may make it impossible to migrate to a newer HSM, because to migrate between HSMs you may need to store keys under new main HSM keys ("import keys in a new security world").

I'm not an expert in these HSM attributes but I would only set restrictive attributes based on attracks it mitigates under the applicable threat model. What I am quite confident about is that when modifiable+copyable are set, potentially strict attributes can be removed, i.e. downgrading is possible. However, as far as I know, HSM security settings ("security world" for our vendor) can not be downgraded to become more permissive.

@Koenvh1
Copy link

Koenvh1 commented Mar 8, 2023

A current outstanding issue is the fact that rolling over a key is rather difficult. Whilst I agree that the best practice is, well, best practice, it also might cause a lock-in to potentially outdated HSMs (or with the only migration path being a newer HSM by the same vendor). I am however not sure to what extent that is currently possible given that CKA_EXTRACTABLE is FALSE (which is I believe not affected by MODIFIABLE), nor do I have the operational experience to back that up.

That said, there have been HSMs in the past where it was possible to extract the private key as long as CKA_MODIFIABLE was set to TRUE (CVE-2015-5464), so if the former is not a concern then I would be in favour. I mainly want to prevent organisations being forced to used outdated or unsupported HSMs. I believe @timbru 's proposal of making it a user setting is a good idea, but that would require very clear documentation on both what it does and what the consequences are - and then the discussion is of course what the default will be (if any).

@ties
Copy link

ties commented Mar 8, 2023

A current outstanding issue is the fact that rolling over a key is rather difficult. Whilst I agree that the best practice is, well, best practice, it also might cause a lock-in to potentially outdated HSMs (or with the only migration path being a newer HSM by the same vendor).

In general migrations between HSM vendors are tricky, but possible, if both vendors cooperate. At least that is what is shared in sales processes.

It depends on settings how hard this is, the exact FIPS mode use matters here.

I am however not sure to what extent that is currently possible given that CKA_EXTRACTABLE is FALSE (which is I believe not affected by MODIFIABLE), nor do I have the operational experience to back that up.

Probably best to ask a vendor instead of to speculate. I do not think PKCS attributes give as much guarantees as the HSM itself does (PKCS is just a library wrapping a lower level API).

It depends on the ACL in that lower level API what is really possible.

@ximon18
Copy link
Member Author

ximon18 commented Mar 9, 2023

So, how about:

  • Changing the default PKCS#11 key attribute set to sets CKA_EXTRACTABLE to CK_TRUE by default instead of the current CKA_FALSE.
  • Leaving CKA_COPYABLE set to CK_TRUE by default as it is now.
  • Adding a new PKCS#11 signer setting called secure that defaults to false and when set to true causes CKA_EXTRACTABLE and CKA_COPYABLE to both be set to CK_FALSE? (with documentation stating roughly that this may make it harder or impossible, depending on the token vendor, to extract the keys e.g. for migration).
  • Mention in the release announcement that the default security on PKCS#11 keys is being weakened deliberately.

?

@timbru
Copy link
Contributor

timbru commented Mar 9, 2023

I am starting to wonder whether it would be better to not have defaults for some attributes, but instead require that the user makes an active choice. We can then of course still have an example best practice configuration in our documentation and example config files.

It's not the most user friendly perhaps, but then again HSMs are not for the faint of heart.

@ties
Copy link

ties commented Mar 10, 2023

I would also be hesitant to call one of the configurations more secure or not.

I think using an authorized token slot definitely is slightly more secure and a default that makes sense.

When it comes to PKCS11 attributes, I do not know what to choose. I do not know when you would want extractable and what the impact is - if any - when interacting with fips modes.

@timbru a good thing about explicit choices: a vendor can advise you on what is sane (and works)

@ximon18
Copy link
Member Author

ximon18 commented Mar 10, 2023

If we are uncertain about the right settings to use and about where we can get more information or if there is not even a single right answer, then I can also make it such that rather than just the recently added (and not yet released) pubkey_access setting for controlling when and how the CKA_PRIVATE key attribute is used for the public key, that instead the complete set of boolean flags to use for public and private key generation can be overridden, though would need to invent a config syntax for it.

@ximon18
Copy link
Member Author

ximon18 commented Mar 21, 2023

After discussion with @timbru we've concluded that we should require operators to specify their desired key attributes and not make any assumptions in Krill. Exact details to be worked out, I'll update the existing with a PR with a proposal and we can discuss further.

@halderen
Copy link
Member

Perhaps a bit late, but my 2cts;
CKA_SENSITIVE to TRUE, otherwise the secret key is just too easily available. CKA_EXTRACTABLE configurable. For migration purposes, even withing the same security domain where this would be still safe as the key cannot exit the security domain. However a sane default is for this to be False. For a security point of view, CKA_COPYABLE doesn't matter. I think it is mainly used for two applications that want to use the same key, but need to access it under a different Id/Label. I've not seen this ever. And Copying isn't always implemented. I would just not specify this attribute, to allow PKCS#11 implementations to do non-standard stuff (ie ignore or set to false).

These have been the choices even prior my appearance for OpenDNSSEC.

@ximon18
Copy link
Member Author

ximon18 commented Apr 4, 2023

Thanks @halderen!

ximon18 added a commit that referenced this issue Apr 4, 2023
@ximon18
Copy link
Member Author

ximon18 commented Apr 4, 2023

After discussion with @timbru we've concluded that we should require operators to specify their desired key attributes and not make any assumptions in Krill. Exact details to be worked out, I'll update the existing with a PR with a proposal and we can discuss further.

I have updated PR #1020 with a mechanism for overriding some of the default PKCS#11 key attributes Krill passes to the PKCS#11 driver when creating public/private key pairs. This also affects the method an operator can use to work around #1019. See this updated configuration file section and these new tests for more information.

Feedback welcome!

ximon18 added a commit that referenced this issue Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hsm Relates to adding HSM support to Krill
Development

Successfully merging a pull request may close this issue.

5 participants