Safety of types containing raw pointers and methods using them

[hug-dev]

Hello again 👋

We are working in Parsec (starting with this PR) of increasing our coverage of what PKCS11 operations/types we support. Doing that we wanted to implement conversions between our types and the PKCS11 ones but more generally work with higher-level, more idiomatic, Rust types.

We started for example to implement CkMechanism as an abstraction over CK_MECHANISM and conversion to go from one to the other.

However we realized we were doing something wrong and putting in CK_MECHANISM.pParameter a pointer that would no longer be valid at the moment it would be dereferenced in encrypt_init. But more importantly, this happened using safe Rust and without any error from the compiler.

This led us to think that our abstraction should instead contain a reference on the mechanism parameter, reference to which there could be a Rust lifetime associated with it.

But the problem is the same: when encrypt_init is called, the mechanism.pParameter parameter will be dereferenced and the compiler has no way to know if it is safe or not, as it goes inside an unsafe zone.

I believe this is the same for any type containing raw pointer that will be dereferenced by one of the PKCS11 method (CK_ATTRIBUTE and CK_MECHANISM come to mind).

To solve this problem (and by the way this also simplifies the fix for #15), I propose the following:

1. put as unsafe any method on Ctx that will dereference a raw pointer
2. build abstraction equivalents on all structures containing raw pointers to instead contain pure Rust references and lifetime specifiers
3. Have equivalents of the unsafe methods taking the abstraction structures. In those structures make the conversion between the abstraction and the C-like structures because we are ensured that is safe (the reference must be valid) and call the underlying method.

More than safety, the Rust abstraction would make it easier I think for clients to make valid operations and use them more idiomatically.

For example we could imagime for the CK_MECHANISM abstraction:

enum CkMechanism<&'a> {
    CkmRsaPkcs,
    CkmRsaPkcsPss(&'a CkRsaPkcsPssParams),
    CkmRsaPkcsOaep(&'a CkRsaPkcsOaepParams),
    CkmSha1,
    CkmSha256,
    CkmSha384,
    CkmSha512,
    // etc...
}

And CK_ATTRIBUTE:

enum CkAttribute<&'a> {
    CkaClass(CkObjectClass),
    CkaModulus(&'a [u8]),
    // etc...
}

enum CkObjectClass {
    CkoData,
    CkoCertificate,
    // etc...
}

It would be quite tedious to do everything (but I don't think it has to happen in one go) but the results would be great I am sure! For example all the methods on CkAttribute could all be always safe!

[hug-dev]

Actually I think it would be nicer to have those abstraction structures take full ownership of the data they contain but have methods on them, taking a reference, to produce C type form

[mheese]

@hug-dev yes, I ran into this myself before; I guess I should have put a warning once I saw it myself. Mea culpa.

Why I never considered this to be a "big problem" was because this API layer was never really meant to be used at all publicly; it was always meant as a first one-to-one translation of C to Rust - assuming that people who use pkcs11 know what they are doing anyway.

I always had the goal to write a higher level API which would fix these problems (or hide them tbh), and expose exactly the higher level rust paradigms in that similar manner that you are mentioning. I actually have a local branch where I started, however, as it is (as you pointed out) a lot of work / leg work to do it, I never finished that.

Nonetheless, you are pointing out the real problem here: the library is making something safe to use which it actually isn't, and therefore shouldn't even be declared as such.

Let me think about this over the weekend, there is actually some general refactoring involved that should be done. And btw I'll most likely simply merge the linked fix for ARM as it is definitely a problem.

If you need a general resolution and/or direction on this earlier, please let me know.

[hug-dev]

Awesome! Interesting to know your thinking on this, the history of this crate and great that we share a similar vision.

We merged some abstraction that we needed in Parsec here. We are happy to change to something similar that would be implemented inside this crate.

This will be a lot of work but some developpers of Parsec will definitely be happy to help designing or developping!

Independantly of that, feel free to merge the Arm fixes and we also posted #42.

[hug-dev]

This will be a lot of work but some developpers of Parsec will definitely be happy to help designing or developping!

Actually, now that I am thinking about it, it would be awesome if @joechrisellis could test his PR on the PKCS 11 API and try to dynamically load the SoftHSM library 🚀

If that works fine, it could be the basis of a pkcs11-sys crate that works for multiple architecture 😃 If @mheese is ok with the principle of course.

[mheese]

If that works fine, it could be the basis of a pkcs11-sys crate that works for multiple architecture

Yes, that sounds like the right way to do this 👍

@joechrisellis would you mind testing that? :)

[joechrisellis]

Hi!

Apologies for the late response here; I have been away for a week.

Yes, this definitely looks like something I can test. I'll start looking into this as soon as I can, and will report back here. 👍

[joechrisellis]

Hello again!

I've just gotten around to taking a look at this, apologies for the lag! I tried generating dynamic bindings for the PKCS11 headers (the one defined here) and got the following result: here.

It's a pretty beefy file at almost 5000 lines long, but if you grep for impl Pkcs11, you'll see the dynamic library part. It seems to have generated the right bindings but I haven't tried this with SoftHSM -- will give that a go next and report back.

Cheers! ⚡

[joechrisellis]

The code snippet below seems to be working with the bindings generated above + the SoftHSM dylib.

#![allow(non_upper_case_globals)]
#![allow(non_camel_case_types)]
#![allow(non_snake_case)]
include!(concat!(env!("OUT_DIR"), "/bindings.rs"));

use std::mem::MaybeUninit;

fn main() {
    unsafe {
        let lib = Pkcs11::new("/usr/local/lib/softhsm/libsofthsm2.so").unwrap();
        let mut list = MaybeUninit::uninit();
        let list = match lib.C_GetFunctionList(&mut list.as_mut_ptr()).unwrap() as u32 {
            CKR_OK => *list.as_ptr(),
            _ => panic!("oops"),
        };
        println!("{:?}", list);
    }
}

@mheese @hug-dev -- would appreciate your insight on if and how to move forward here! Good to know that this works, though. 🙂

[hug-dev]

That looks fantastic, thanks @joechrisellis 🚀
I was thinking of the following, feel free to criticize as those would be big changes:

• move src and Cargo.toml into the pkcs11 folder
• create a pkcs11-sys folder with a new library crate
• create a top-level Cargo.toml to have a workspace of two crates, pkcs11 and pkcs11-sys

pkcs11-sys would expose all the PKCS11 Rust types created with bindgen for different targets. I propose now to target Linux on Aarch64, Arm32 and x86. Depending on the --target option this crate would select the appropriate file containing all the bindings + the dynamic loading code.

Inside pkcs11, remove functions.rs and types.rs and use directly the pkcs11-sys crate wherever those types are needed. Also modify the Ctx structure methods (mostly new) to use the new bindgen-generated loading function.

That would be a first step to keep exactly the same API surfaced (so no breaking change) but separating into two crates.
I believe the step after that would be to add abstractions in the pkcs11 crate but I think all can be done without any breaking change for now but just by adding new structs/methods.

To address the immediate safety issues, it might be needed to add unsafe on all methods though.

[joechrisellis]

Hi @mheese!

I have implemented @hug-dev's suggestions above on a local branch -- looks like everything is working, and tests are passing.

However, there has been some teething problems with introducing bindgen into the mix. Unfortunately, I think this will create an API break. Specifically, in the existing implementation, builders exist for types such as CK_ATTRIBUTE. This is possible because CK_ATTRIBUTE is defined locally. In the *-sys crate implementation, though, CK_ATTRIBUTE is defined in the *-sys crate, and AFAIK it is bad practice to include impl blocks inside your *-sys crates (they should contain type definitions only). The way that we can get around this is by re-defining the CK_ATTRIBUTE struct (plus a few others) as CkAttribute with #[repr(C)] inside the non-*-sys crate. This can be cast to the CK_ATTRIBUTE struct defined in the *-sys crate, and since it's declared locally, we can create implementations/builder functions for it.

So, the breaking change here is that CK_ATTRIBUTE (as it was defined) now becomes CkAttribute. A similar name conversion also has to happen for CK_INFO and CK_INITIALIZE_ARGS. The renamed structs are functionally identical to the existing structs, just under a different name.

I've tried various things to try to preserve the existing naming but haven't been able to come up with anything that both works and doesn't result in some ugly consequences.

Would really appreciate your thoughts on this, especially if you have any suggestions on how to move forward. 😄

[mheese]

@joechrisellis @hug-dev cool! I'll try to look into this next week. I'm planning on making some progress on this library next week in general. This is one of the biggest problems at the moment imho. And I'm truly sorry that I'm only looking into this basically a year later. As this is not my day-to-day job, it's something I'm only making progress on when I have some cycles to spare (although it'd be nice if it could become that at some point, but I don't see how right now).

[mheese]

@hug-dev closing all issues right now together with a deprecation notice that I'm trying to squeeze in everywhere. For others reading this issue, please switch to the cryptoki crate https://github.com/parallaxsecond/rust-cryptoki

[hug-dev]

Thanks a lot for your work with this crate and providing PKCS11 for everyone using Rust so far! Appreciate the redirect. You will always be welcome in the cryptoki repo if you ever wish to hack on Rust with PKCS11!

[mheese]

@hug-dev if you want to, I would like to move ownership of the pkcs11 crate to either you guys, or @tarcieri from the @RustCrypto org might have interest otherwise. What do you think?

[hug-dev]

Thanks for proposing! Let me ask on our community slack channel to see what people think.