PIN Handling

[mike-boquard]

This issue is opened to continue the discussion started in #49 in regards to how user PINs are handled by the cryptoki library wrapper.

Currently, when a user wishes to login, they must first call set_pin in the Pkcs11 struct which stores the pin in a HashMap, keyed to the Slot. Then the user may call login on the created Session object.

Any other function that directly handles pins (those that expose the C_InitToken, C_InitPIN and C_SetPIN functionality) wraps the provided PIN within a Secret structure and passes a pointer to the appropriate PKCS#11 function. Upon completion of the function, the Secret structure will drop and zeroize the memory.

My first proposition is to drop the pins HashMap member in the Pkcs11 struct and provide a pins parameter to Session::login

First, if the user wishes to change which user is logged in (say going from CKU_USER to CKU_SO) the must call set_pin to change the pin and then call login. This requires the implementer to track which pin is currently being stored by the library to ensure they are logging in with the correct pin.

Secondly, and this is a personal preference thing, I do not believe the library should be storing the user's passwords. I believe the user should be the one that controls where their sensitive data is stored and be responsible for it, rather than ourselves.

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. For example, if the flag CKF_PROTECTED_AUTHENTICATION_PATH is set in token flags, that indicates there's a separate way of logging into the token (e.g. finger print reader, smart card reader, etc). In this situation, you must pass a NULL_PTR to C_Login to take advantage of that alternative authentication path.

My second proposition is to drop Secret and instead just pass the pointer from the provided str slice to the appropriate C_ function. If this isn't possible, then anytime a pin is provided and needs to be copied somewhere on the stack that is controlled by the library, then use Secret.

[mike-boquard]

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.

[hug-dev]

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.

[mike-boquard]

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?

[ionut-arm]

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

[mike-boquard]

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

[mike-boquard]

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.

[hug-dev]

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.

[mike-boquard]

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).

[hug-dev]

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.

[mike-boquard]

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 😄

[hug-dev]

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 😅