About the safety of CK_ATTRIBUTE::get_bytes

[hug-dev]

Hi!

First thanks for your library, it is very helpful for us 😃 !

As the title says, I have a question regarding the safety of the CK_ATTRIBUTE::get_bytes function. I ran into a memory safety bug (the classic Segmentation Fault) which I think is related.

On version 0.4.0 the function is:

  pub fn get_bytes(&self) -> Vec<CK_BYTE> {
    let slice = unsafe { slice::from_raw_parts(self.pValue as CK_BYTE_PTR, self.ulValueLen as usize) };
    Vec::from(slice).clone()
  }

and is safe. However, I think that in certain cases it can still exhibit an unsafe behaviour, specially because it assumes the values of the fields of the attributes to be valid (without checking them).

One of those cases I found is when using the Ctx::get_attribute_value function. As it says in the code, the function will return the Ok variant even if some attributes given in the templates were invalid or missing from the object. In those cases, as per PKCS 11 specification (C_GetAttributeValue function), it will modify the ulValueLen field of the attribute to CK_UNAVAILABLE_INFORMATION which is a really large number:

pub const CK_UNAVAILABLE_INFORMATION: CK_ULONG = !0;

After that, a subsequent call to get_bytes will create a very big vector (all the memory space actually?).

If the attribute method with_bytes was called before with a byte array of address P then the get_bytes function will return a vector of address P and size !0.

Maybe the get_attribute_value function should actually return an Err variant when that happens or, if we do not want to fail the function for all the attribute if only one failed, it could return a Result of a vector of Result.

[mheese]

obviously buggy behavior :)

The question though is on how we should fix it. I still believe that the Ok variant for get_attribute_value is correct, because it will deliver on all the attributes it could find, and it is a very nice way of retrieving all attributes with just one call. However, that it modifies the ulValueLen to CK_UNAVAILABLE_INFORMATION is very unfortunate of course :) and will make get_bytes unsafe effectively

I'm very open for a PR that addresses this.

Unfortunately, I can't spare as much time on developing this as I could when I started to work on this. My original goal for this library was:

• a low-level module that directly calls PKCS11 as is
• a high-level module which would make it more user friendly to work with it (in particular when it comes to multiple sessions, etc.)
• and the final two applications that I actually had in mind: a pkcs11 CLI which one can actually use without studying the source code of the application (and I've written that in golang before which I could not open source unfortunately); and a provider of pkcs11 for embedded systems to expose pkcs11 functionality from an embedded system as an HSM.

Any input in which direction you would like this library to develop would be useful as well.

[mheese]

Maybe the get_attribute_value function should actually return an Err variant when that happens or, if we do not want to fail the function for all the attribute if only one failed, it could return a Result of a vector of Result.

My gut feeling tells me that this might be the right approach: Result of a vector of Result. Or cleaning the Result to only return the successful ones.

[hug-dev]

Thanks for your feedback, I will try to get a PR started for this 😃 I also thought that maybe the get_attribute_value function could just check if the ulValueLen field is correct or not (as in is CK_UNAVAILABLE_INFORMATION or not) and not generate a Vec if that is the case. Because I think that it is valid for an attribute to have such field (it is used by the user to check the length of an attribute value with C_GetAttributeValue as well).
We can develop the discussion more on the PR.

Concerning the library, it is very useful for us as it is but it might be worth to audit the use of unsafe blocks so that this library is not only a PKCS 11 wrapper but a safe one. Of course any abstractions on top of that would be helpful, if it makes the code easier to read/understand!

Concerning our use-case, we are mainly using this library as part of PARSEC which is a service interfacing with different cryptographic backends (and PKCS 11 is one of them) with the same API (PSA Crypto API over protobuf). Clients can connect to the service through a Unix socket and send crypto requests. Client libraries can be written in any language and should provide the most easy-to-use abstraction with secure defaults. For example, a client library written in Rust (here the test client) could be used like this to create a RSA key, sign a message and export the public key:

use parsec_client_test::TestClient;
​
let mut client = TestClient::new();
let key_name = String::from("🔑 Bob's Key 🔑");
let message = String::from("Bob wrote this message.");
​
client.create_rsa_sign_key(key_name.clone()).unwrap();
let signature = client.sign(key_name.clone(), message.into_bytes()).unwrap();
let public_key = client.export_public_key(key_name).unwrap();

And we also have started a Go client.

[hug-dev]

cc parallaxsecond/parsec#66

[mheese]

@hug-dev I assume you got busy and did not start writing a PR? (no worries, I know how it is)

[hug-dev]

Thanks for pinging me! I actually proposed a solution in a PR linked above 😃