mheese/rust-pkcs11

Rust PKCS#11 Library

rust-pkcs11's Issues

Mutability compile errors

Running cargo test (both against Rust 1.40.0 and 1.32.0) got me lots of errors mostly of this form:

   --> src/tests.rs:734:49
    |
734 |       let res = ctx.get_attribute_value(sh, oh, &template);
    |                                                 ^^^^^^^^^ types differ in mutability
    |
    = note: expected type `&mut std::vec::Vec<types::CK_ATTRIBUTE>`
               found type `&std::vec::Vec<types::CK_ATTRIBUTE>`

Unsound transmute_copy in `attr_ck_long`, `get_date`, `attr_ck_ulong`

---- tests::attr_ck_long stdout ----
CK_ATTRIBUTE { attrType: "0x402", pValue: [214, 255, 255, 255, 255, 255, 255, 255], ulValueLen: 8 }
[src/types.rs:838] std::mem::size_of::<CK_LONG>() = 8
[src/types.rs:839] std::mem::size_of_val(&*self.pValue) = 1
thread 'tests::attr_ck_long' panicked at 'cannot transmute_copy if U is larger than T', /home/jess/src/rust/library/co
re/src/mem/mod.rs:1043:5
stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::mem::transmute_copy
             at /home/jess/src/rust/library/core/src/mem/mod.rs:1043:5
   3: pkcs11::types::CK_ATTRIBUTE::get_ck_long
             at ./src/types.rs:842:21
   4: pkcs11::tests::attr_ck_long
             at ./src/tests.rs:544:24
   5: pkcs11::tests::attr_ck_long::{{closure}}
             at ./src/tests.rs:540:1
   6: core::ops::function::FnOnce::call_once
             at /home/jess/src/rust/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

---- tests::attr_date stdout ----
CK_ATTRIBUTE { attrType: "0x3", pValue: [0, 0, 0, 0, 0, 0, 0, 0], ulValueLen: 8 }
thread 'tests::attr_date' panicked at 'cannot transmute_copy if U is larger than T', /home/jess/src/rust/library/core/
src/mem/mod.rs:1043:5
stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::mem::transmute_copy
             at /home/jess/src/rust/library/core/src/mem/mod.rs:1043:5
   3: pkcs11::types::CK_ATTRIBUTE::get_date
             at ./src/types.rs:922:21
   4: pkcs11::tests::attr_date
             at ./src/tests.rs:579:15
   5: pkcs11::tests::attr_date::{{closure}}
             at ./src/tests.rs:575:1
   6: core::ops::function::FnOnce::call_once
             at /home/jess/src/rust/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

---- tests::attr_ck_ulong stdout ----
CK_ATTRIBUTE { attrType: "0x402", pValue: [42, 0, 0, 0, 0, 0, 0, 0], ulValueLen: 8 }
thread 'tests::attr_ck_ulong' panicked at 'cannot transmute_copy if U is larger than T', /home/jess/src/rust/library/c
ore/src/mem/mod.rs:1043:5
stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::mem::transmute_copy
             at /home/jess/src/rust/library/core/src/mem/mod.rs:1043:5
   3: pkcs11::types::CK_ATTRIBUTE::get_ck_ulong
             at ./src/types.rs:817:21
   4: pkcs11::tests::attr_ck_ulong
             at ./src/tests.rs:534:25
   5: pkcs11::tests::attr_ck_ulong::{{closure}}
             at ./src/tests.rs:530:1
   6: core::ops::function::FnOnce::call_once
             at /home/jess/src/rust/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Runtime checks for transmute_copy's invariant that the output type is no larger than the input type are being added in rust-lang/rust#98839 , and this crate looks to be one that will get affected by that.

Looks like the transmute_copy should instead be a pointer cast and a read.

Crash on initialize

This is my first Rust program! Be gentle.

extern crate pkcs11;

fn main() {
    let mut ctx = pkcs11::Ctx::new("eToken").unwrap();
    ctx.initialize(None).expect("Couldn't initialize dongle libary");
}

Crashes with an invalid memory read.
Drivers from here - I'm using 10.3. Thankfully you don't need a dongle attached, simply loading the library is enough to break things.

Change `Ctx::generate_random` to take the buffer to be filled instead of generating the buffer from a given size

Ctx::generate_random takes in a randomLength: CK_ULONG parameter and uses this to generate a corresponding buffer of the given size, fills it with randomized data, and returns it to the caller. I propose instead letting the caller pass in the buffer directly to the function as a mutable reference, which the function then fills with randomized data.

I prefer this approach because a common use case for this function is to create a custom CryptoRng struct from the rand crate. To implement this trait includes implementing these functions:

• fn fill_bytes(&mut self, dest: &mut [u8])
• fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error>

Both functions already include the buffer to be filled. Currently, implementing these functions using Ctx::generate_random results in an unnecessary allocation of the same size as the dest buffer, which then needs to be copied over.

My main concern with this proposed change is that C_GenerateRandom can only fill a buffer up to CK_ULONG size, which is smaller than usize, the maximum buffer size. Therefore, the buffer size may need to be checked before calling C_GenerateRandom.

Unknown error code from `Ctx::new()` on ARMv7 when calling the PKCS#11 `C_GetFunctionList()` function.

See:

• ximon18/keyls#4
• NLnetLabs/krill#752

This issue was observed with a YubiHSM2 Nano connected to a Raspberry Pi 4b which is an ARMv7 (arm7l) architecture platform. The issue occurred both when building the code locally on the Pi and when cross compiling from an x86_64 host.

The error message produced by the pkcs11 crate was:

PKCS#11: unknown (0xb6ab784000000000)

The issue occurs in the pkcs11 v0.5.0 Rust crate code when func() is invoked within Ctx::new() in the code shown below (link to actual pkcs11 crate code):

let func: libloading::Symbol<
    unsafe extern "C" fn(CK_FUNCTION_LIST_PTR_PTR) -> CK_RV,
> = lib.get(b"C_GetFunctionList")?;
match func(list.as_mut_ptr()) {
    CKR_OK => (),
    err => return Err(Error::Pkcs11(err)),
}

The same code works fine on x86_64 targets. pkcs11-tool had no issues on ARM?7 using the same PKCS#11 library.

The cryptoki create doesn't appear to have the same problem as switching to that crate resolved the issue for the simple https://github.com/ximon18/keyls tool.

Support for partial implementations

Hi!

Currently the library seems to offer support only for PKCS11 implementations that support all functions described in the spec. However, the official documentation indicates that, to be compliant with PKCS11, only a subset of the functionality within the main spec must be implemented - the rest is optional. I can see from the README (and from using SoftHSM ourselves!) that there are implementations that do this, but others do not and interacting with these would be impossible because of how the Ctx constructor works.

Our suggestion was that instead of erroring out of the constructor if any of the methods is unavailable, an Option or Result can be kept and the error returned when the corresponding Ctx method is called. Does that sound sensible from your perspective?

`ctx.open_session` against Luna Network HSM crashed with SIGSEGV

It doesn’t seem rust-pkcs11 can be used to open a pkcs11 session to the Thales eLab, i.e. Luna Network HSM without ending up in (signal: 11, SIGSEGV: invalid memory reference).

Sample unit test:

extern crate pkcs11;

use std::env;
use std::path::PathBuf;

use pkcs11::Ctx;
use pkcs11::types::CKF_RW_SESSION;
use pkcs11::types::CKF_SERIAL_SESSION;

fn pkcs11_module_name() -> PathBuf {
    let default_path =
        option_env!("PKCS11_HSM_MODULE").unwrap_or("/usr/safenet/lunaclient/lib/libCryptoki2_64.so");
    let path = env::var_os("PKCS11_HSM_MODULE").unwrap_or_else(|| default_path.into());
    let path_buf = PathBuf::from(path);

    if !path_buf.exists() {
        panic!(
      "Could not find HSM at `{}`. Set the `PKCS11_HSM_MODULE` environment variable to \
       its location.",
      path_buf.display());
    }

    path_buf
}

#[test]
#[serial]
fn ctx_open_session() {
    let ctx = Ctx::new_and_initialize(pkcs11_module_name()).unwrap();
    let slot = 0;
    let res = ctx.open_session(slot, CKF_RW_SESSION | CKF_SERIAL_SESSION, None, None);
    assert!(
        res.is_ok(),
        "failed to call C_OpenSession({}, CKF_SERIAL_SESSION, None, None): {}",
        slot,
        res.unwrap_err()
    );
    let sh = res.unwrap();
    println!("Opened Session on Slot {}: CK_SESSION_HANDLE {}", slot, sh);
}

Test run and failure

# Centos 7
$ cargo test -- --nocapture
Opened Session on Slot 0: CK_SESSION_HANDLE 1
error: test failed, to rerun pass '--bin xks-proxy'

Caused by:
  process didn't exit successfully: `/local/centos/ThalesElab/rust/target/debug/deps/my_project-ede7135472e10bbe --nocapture` (signal: 11, SIGSEGV: invalid memory reference)

Note

1. I can open the session in pure C without issue.
2. Switching between rust stable vs nightly build doesn't seem to make any difference.
3. Upgrading dependency libloading to the latest v0.7.2 doesn't seem to make any difference.

Commit "Fix Windows x64 compatiblity" breaks MacOS

Hi,

on MacOS I do get a "(signal: 11, SIGSEGV: invalid memory reference)" Segmentation fault when i call ctx.initialize(None)
Commit 74057c9 works fine, so it has to be the "Fix Windows x64 compatiblity" commit.

Any idea why?

About the safety of `CK_ATTRIBUTE::get_bytes`

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.

How to use it with a different SHM

Hi,

I have a HSM which can be set up a PKCS11 provider. I was wondering would this library run out of box with the provided PKCS11(after configuring pkcs11 location) or would we need some modifications?

Any pointers would be helpful.

Thanks

Disabling of compiler optimizations needed to prevent non-null `pReserved` when invoking `C_Initialize()`.

Environment

Reproducible with Rust pkcs11 crate version v0.5.0 on:

• SoftHSM 2.6.1-2 on Ubuntu 21.10 installed with apt install softhsm, or
• SoftHSM 2.6.1 on Alpine Linux 3.12 installed with RUN apk add softhsm from a Dockerfile

Steps to reproduce

Calling Ctx::Initialize(Some(args)) where args is either CK_C_INITIALIZE_ARGS::default() or:

let mut args = CK_C_INITIALIZE_ARGS::new();
args.CreateMutex = None;
args.DestroyMutex = None;
args.LockMutex = None;
args.UnlockMutex = None;
args.flags = CKF_OS_LOCKING_OK;
ctx.initialize(Some(args)

(note that CK_C_INITIALIZE_ARGS::new() sets args.pReserved = ptr::null_mut() and that CK_C_INITIALIZE_ARGS::default() calls CK_C_INITIALIZE_ARGS::new())

Works in debug builds

When built with cargo build my application works without error.

Fails in release builds

When built with cargo build --release my application fails with error CKR_ARGUMENTS_BAD and in /var/log/ via Syslog logging from SoftHSM I see error message SoftHSM.cpp(436): pReserved must be set to NULL_PTR.

Disabling compiler optimizations helps

Adding this to Cargo.toml and rebuilding my application with cargo build --release "solves" the problem:

[profile.release.package.pkcs11]
opt-level = 0

Ctx::new should take a &Path

It's treated as a path, so it'd be nice to have the type reflect that.

latest code not available in https://crates.io/

Can you help with the latest code in https://crates.io/

CK_ATTRIBUTE get_biginteger mixes up endianness

Cryptoki base spec defines "Big integer (...) a string of CK_BYTEs representing an unsigned integer of arbitrary size, most-significant byte first (e.g., the integer 32768 is represented as the 2-byte string 0x80 0x00)".

Meanwhile, this library parses as little endian:

rust-pkcs11/src/types.rs

Lines 853 to 858 in 1323d5f

	pub fn get_biginteger(&self) -> Result<BigUint, Error> {
	self.available_value()?;
	let slice =
	unsafe { slice::from_raw_parts(self.pValue as CK_BYTE_PTR, self.ulValueLen as usize) };
	Ok(BigUint::from_bytes_le(slice))
	}

Report as unmaintained?

Hey!

I wanted to check whether you're confortable with us marking pkcs11 as unmaintained in the Rustsec advisory database?

possible segmentation fault

Hi there, due to public new function available for type, there is an potential segmentation fault chance without any unsafe involved.

fn main() {
    let a = pkcs11::types::CK_ATTRIBUTE::new(123);
    a.get_bool(); // get_bool can also be replaced with any member functions contains dereference operation without check pointer is null 
}

cargo run

Segmentation fault

Safety of types containing raw pointers and methods using them

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!