NLnetLabs / krill

RPKI Certificate Authority and Publication Server written in Rust
https://nlnetlabs.nl/projects/routing/krill/
Mozilla Public License 2.0
294 stars 42 forks source link

Mark PKCS#11 private keys as unmodifiable #1018

Open ximon18 opened 1 year ago

ximon18 commented 1 year ago

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 :heavy_check_mark: and CKA_EXTRACTABLE to CK_FALSE :heavy_check_mark: 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) :x:.

ties commented 1 year ago

To fully restrict keys, CKA_COPYABLE should probably also be CK_FALSE

ximon18 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

So, how about:

?

timbru commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Thanks @halderen!

ximon18 commented 1 year ago

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!