PIN Handling about rust-cryptoki HOT 13 CLOSED

Thanks for the issue!

I found the original PR where we changed to store the PIN (here). I thought there was a specific reason why we did that but it appears it was only for convenience, to not have to enter it everytime.

However, I fully agree with your points:

• calling set_pin multiple times if you change user type feels a bit clunky
• the logic could be simplified if the pins field was dropped
• no double set_pin method in the interface
• not storing the password in the library is actually safer in a security point of view

So I would be in favour to remove that and add the PIN value as a method parameter!

Thirdly, and this is one of those weird PKCS#11 corner cases, there are situations where you may not want to provide a password to C_Login.

I didn't know that but it's a good point as well!

My second proposition is to drop Secret and instead just pass the pointer from the provided str slice to the appropriate C_ function.

Totally agree if it's possible! Another reason why we clone the PIN on the stack (and then have to zeroize it later) is because we convert it to a CString. I thought that this format was required in order to pass it to PKCS11. If it just works with &str, we maybe actually don't need to do that? I have to admit I have never tried!

To just add one more thing, I think we should wrap any provided attributes used in object creation (e.g. Session::create_object) in Secret to ensure we do erase any sensitive data the user provides.

Agree that generally we should zeroize sensitive memory that we allocate but without necessarely having to force the users to use those funky types. They can be responsible of zeroizing their own memory.

from rust-cryptoki.

That type of behavior (opening a 'logged in' session for the lift of the application) is, as far as I've seen, the pretty typical thing that other P11 applications, like CA's, do. It's a way of P11 putting the onus of security on the user application.

Also, don't kick yourself about not knowing this - during development on a previous project, we completely missed the session object coherency requirement until we shipped... that was a little embarrassing when our customer called and asked why we weren't spec compliant 😓 But hey, live and learn 😄

from rust-cryptoki.

Another reason why we clone the PIN on the stack (and then have to zeroize it later) is because we convert it to a CString. I thought that this format was required in order to pass it to PKCS11. If it just works with &str, we maybe actually don't need to do that?

According to PKCS#11, the PIN may be any valid UTF-8 character, and str slices are always valid UTF-8 so I believe that it should be ok to just pass it in as is. 🤞

I'll take a stab at this.

As a side note, I know one of the main projects that uses this is parsec. I'd imagine the changes I'm making here (and others that just recently got merged into main) will probably break the PKCS#11 'interface' in parsec. I don't mind jumping in there and fixing those, but I'd imagine that project is on a stricter release schedule. What is the plan on integrating updates from this crate into parsec?

from rust-cryptoki.

Thanks for the link. I had a read and I understand that it would definitely make the logic easier if a "login" session is kept open as long as Parsec lives.

I think we thought of that as a possibility but were worried about the security there: Parsec might stay up for a long time (as long as the system is up actually which might be very long) and during all that time all of the sessions will be logged in.

Now reading this page, those are only sessions open by the Parsec application (and not others) so it might be fine.

from rust-cryptoki.

To just add one more thing, I think we should wrap any provided attributes used in object creation (e.g. Session::create_object) in Secret to ensure we do erase any sensitive data the user provides. An example would be creating an AES key using Session::create_object. In this situation, the operator is required to provide the value of the AES key in the CKA_VALUE attribute.

from rust-cryptoki.

Just one note here, though it's mostly a FYI and not directly an argument here (for reasons that I'll mention):

The TPM Software Stack (or more precisely the Enhanced System API layer) does something somewhat similar. Objects can have authorization values in TPM world, which are used to "authorize" commands (via MACs) sent to the TPM, so normally if you work with a low-level API you have to use the authorization values all the time. The ESAPI layer takes care of that for you - you tell it the value (which it stores), and then you can use the object without worrying about the authorization stuff. I think we might've been aiming for a similar "feel" to the API.

The reason that sort of approach doesn't work here is because the use case seems quite different, I guess :) It might be a bit of a change in Parsec to have to provide the pin every time, but it shouldn't be a big deal

from rust-cryptoki.

It might be a bit of a change in Parsec to have to provide the pin every time, but it shouldn't be a big deal

Is this because Parsec doesn't open and maintain a session for an extended period of time? (I've only really glanced through the project so I'm not sure all of its use cases). If you open one session on a PKCS#11 compliant token and log into it, and keep that session open, all other sessions opened on that same token (within the same process) share that same login 'state'.

For example:

Thread 1: Opens R/W session on slot 2, logs in as CKU_USER
Thread 2: Opens R/O session on slot 2 -> Already logged in as CKU_USER

from rust-cryptoki.

And I appreciate hearing other points of view on how other projects utilize the P11 api... my perspective, as I've said before, is from a spec implementer which is definitely a different point of view than someone who utilizes said spec.

from rust-cryptoki.

It might be a bit of a change in Parsec to have to provide the pin every time, but it shouldn't be a big deal

I think it should be fine, it should be only one line change here, we would need to store the pin in the Provider structure.

Is this because Parsec doesn't open and maintain a session for an extended period of time?

Everytime Parsec receives a request to be performed on the PKCS11 provider, it creates a new session (in its own thread) via the new_session method linked above.

all other sessions opened on that same token (within the same process) share that same login 'state'

Yes, I remember that we were very surprised to notice that peculiarity when we started working with PKCS11 😅
There is a feature in cryptoki to manage that actually: the crate remembers what sessions are logged on which slot and ignore the login/logout calls if another session is already logged in.
It means that in the Parsec side it is then fine to call login/logout at the start/end of every thread.
It is a bit opinionated and I am happy to discuss that particular feature.

I'd imagine the changes I'm making here (and others that just recently got merged into main) will probably break the PKCS#11 'interface' in parsec.

I think generally for cryptoki and the other crates we maintain we should not care about Parsec when developping but should do what's best for the crate 👍 We want this crate to be successfull and usefull by itself 😃

If you are curious (but you are not required to!!), the PKCS11 Parsec code is in here.

from rust-cryptoki.

There is a feature in cryptoki to manage that actually: the crate remembers what sessions are logged on which slot and ignore the login/logout calls if another session is already logged in.
It means that in the Parsec side it is then fine to call login/logout at the start/end of every thread.

Yeah I noticed that. I actually am in the process of removing it 😰

My thinking is that you should rely on the token to maintain it's state and not try to 'help' it. There's always a risk of the token and the library getting "out-of-sync" when both are trying to maintain the same state.

Here's the page in the P11 spec that talks about session coherency. Definitely read through section 2.6.8. It is a bit verbose but does a good job explaining how sessions are supposed to be handled on a token, as well as how session objects are handled. Session objects created on a session with CKA_TOKEN set to CK_FALSE, but once created are accessible to all sessions opened by the same process on the same token.

Parsec should probably just open a session once it starts up, log into it, and just maintain that session throughout the life of the process. Than when any request comes in, it can simply open a new session and then either perform the operation or check the state using the get_session_info and check the state value to ensure it's logged in (which it should be).

from rust-cryptoki.

That type of behavior (opening a 'logged in' session for the lift of the application) is, as far as I've seen, the pretty typical thing that other P11 applications, like CA's, do.

If that's the common thing to do then I am fine to adopt hat in Parsec too! We can always add mitigations to open/close the "logged in" only when it's needed.
But yeah again, what we have to do in Parsec should not influence this crate! So I am happy to go forward with that 😄

Also, don't kick yourself about not knowing this - during development on a previous project, we completely missed the session object coherency requirement until we shipped... that was a little embarrassing when our customer called and asked why we weren't spec compliant 😓 But hey, live and learn 😄

oops 😅

from rust-cryptoki.

I guess we can close this now?

from rust-cryptoki.

Yup, I'm good with it

from rust-cryptoki.