Code review

[jans23]

Code review next week would be good.

[jans23]

Let's postpone this after #93 at least.

[robin-nitrokey]

I’ve reviewed and tested 0.6.0 on a high level. I did not find any significant issues or problems regarding functionality or security in the code. It is mostly easy to read and follow. The structure of the library seems reasonable. Overall, the code has a high quality and looks relatively easy to maintain.

I ran into problems with AES key generation and RSA decryption, but this could also be caused by an old pkcs11-tool (from opensc 0.21). I’ll repeat the tests with a newer version and report the results.

notes

docs

• docs: nethsm/integration
  • podman: use full container ID for compatibility with default configuration? i. e. docker.io/nitrokey/nethsm:testing

    Error: error getting default registries to try: short-name "nitrokey/nethsm:testing" did not resolve to an alias and no unqualified-search registries are defined in "/etc/containers/registries.conf"
    

• docs: nethsm/operation
  • link to API endpoints no longer work, e. g. https://nethsmdemo.nitrokey.com/api_docs/index.html#/default/post_keys_generate vs https://nethsmdemo.nitrokey.com/api_docs/index.html#/default/POST_keys-generate
• docs: nethsm/pkcs11tool
  • AES: error: Unknown key type GENERIC:256 -- maybe my pkcs11-tool is too old?
  • inconsistent key IDs in examples: hex values in example commands do not mach the values from the key generation commands
  • decrypt RSA:
    • error: Unknown PKCS11 mechanism "RSA_PKCS" -- RSA-PKCS?
    • Request error : https://localhost:8443/api/v1/keys/MyKey/decrypt: status code 400
  • sign RSA:
    • error: Unknown PKCS11 mechanism "RSA_PKCS-PSS" -- RSA-PKCS-PSS?
  • decrypt + sign: use pkcs11-tool to retrieve pubkey instead of curl (+ -keyform der)?
• readme:
  • document supported Rust versions (latest stable? some specific version?)
  • NETHSM_PASS variable in example: I think this one is no longer needed?
  • nit: use ${CARGO_TARGET_DIR:-target} instead of target

typos

• api/generation.rs:91: s/GenerateKey/GenerateKeyPair/
• api/mod.rs:70: outdated comment

implementation details

• Some complexity in the API functions comes from the custom error handling with CK_RV. It’s probably not worth to change it now that almost everything is implemented, but a helper macro that makes it possible to write Result<(), CK_RV> functions instead could be more ergonomic. Alternatively, the logic could be moved to safe functions returning Result<_, CK_RV> and the extern "C" functions only perform the mapping between the input and output.
• utils.rs: At least padded_str and version_struct_from_str could be functions instead of macros. Even for lock_session, read_session, lock_mutex I’d rather use functions to make the call sites easier to read, or at least something like let mut session = lock_session!(hSession); to make the variable definition more obvious. That being said, this is probably not worth a big refactoring now.
• data.rs: could also use std::sync::OnceLock instead of lazy_static
• nethsm-sdk-rs could re-export ureq to avoid having to sync versions between nethsm-sdk-rs and nethsm-pkcs11

next steps

• For a more thorough review, all unsafe code could be reviewed systematically to ensure memory safety. In this review I’ve only checked some occurrences (but those were correct).

[nponsard]

Thanks for the review, I've started to implement some changes.

I don't see how you can make padded_str a function, I need it to return slices with static sizes like [u8; 64] or [u8; 32]

[robin-nitrokey]

I think you just need arrays: #137

[nponsard]

Oh I would never have thought about using generics as the size of the array.
Thanks!

[robin-nitrokey]

I’ve also reviewed the unsafe code in 54f7b89:

safety

• I think this function should check if the length is CK_UNAVAILABLE_INFORMATION before accessing the value.
  

  nethsm-pkcs11/pkcs11/src/backend/db/attr.rs

  Lines 23 to 34 in 54f7b89

  	pub fn val_bytes(&self) -> Option<&[u8]> {
  	let val_ptr = unsafe { (*self.0).pValue };
  	if val_ptr.is_null() {
  	return None;
  	}
  	unsafe {
  	Some(std::slice::from_raw_parts(
  	val_ptr as *const u8,
  	self.len() as usize,
  	))
  	}
  	}

• I dislike that this does not update the length. I think it would be easier to use if set_len would be dropped as a public method, set_value_bytes would update the length accordingly and there was a third function set_value_unavailable that sets the length to CK_UNAVAILABLE_INFORMATION.
  

  nethsm-pkcs11/pkcs11/src/backend/db/attr.rs

  Lines 54 to 66 in 54f7b89

  	pub fn set_val_bytes(&mut self, bytes: &[u8]) -> Result<(), Error> {
  	unsafe {
  	if (*self.0).pValue.is_null() {
  	return Err(Error::NullPtrDeref);
  	}
  	if bytes.len() > (*self.0).ulValueLen as usize {
  	return Err(Error::BufTooSmall);
  	}
  	std::ptr::copy_nonoverlapping(bytes.as_ptr(), (*self.0).pValue as *mut u8, bytes.len());
  	}
  	Ok(())
  	}
  	}

• Shouldn’t this check whether the pointer is null? Generally, I think these raw wrapper classes would be easier to use correctly if the constructors would perform the null checks.
  

  nethsm-pkcs11/pkcs11/src/backend/db/attr.rs

  Lines 82 to 87 in 54f7b89

  	pub fn attr_wrapper(&self, index: usize) -> Option<CkRawAttr> {
  	if index >= self.count {
  	return None;
  	}
  	Some(unsafe { CkRawAttr::from_raw_ptr_unchecked(self.ptr.add(index)) })
  	}

clarifications

• Maybe use std::ptr::write instead even if it not necessary for a ulong?
  

  nethsm-pkcs11/pkcs11/src/api/session.rs

  Line 43 in 54f7b89

  *phSession = session;

• If CkRawMechanism::params is unsafe, shouldn’t this be unsafe too?
  

  nethsm-pkcs11/pkcs11/src/backend/db/attr.rs

  Lines 36 to 42 in 54f7b89

  	pub fn read_value<T>(&self) -> Option<T> {
  	let val_ptr = unsafe { (*self.0).pValue };
  	if val_ptr.is_null() {
  	return None;
  	}
  	unsafe { Some(std::ptr::read(val_ptr as *const T)) }
  	}

• The spec says: pulObjectCount points to the location that receives the actual number of object handles returned. This sets it to the actual number of objects found, not to the actual number of objects returned.
  

  nethsm-pkcs11/pkcs11/src/api/object.rs

  Line 64 in 54f7b89

  std::ptr::write(pulObjectCount, objects.len() as CK_ULONG);

  
  objects already respects the maximum number, but this is still inconsistent with:
  

  nethsm-pkcs11/pkcs11/src/api/object.rs

  Line 62 in 54f7b89

  objects.len().min(ulMaxObjectCount as usize),

documentation

• Why not call std::process::exit or panic!? Please change or add a comment.
  

  nethsm-pkcs11/pkcs11/src/data.rs

  Lines 20 to 23 in 54f7b89

  	Err(e) => {
  	eprintln!("Error initializing configuration: {:?}", e);
  	unsafe { libc::exit(1) }
  	}

• Please add a comment with the safety conditions.
  

  nethsm-pkcs11/pkcs11/src/backend/mechanism.rs

  Lines 26 to 28 in 54f7b89

  	pub unsafe fn from_raw_ptr_unchecked(ptr: *mut cryptoki_sys::CK_MECHANISM) -> Self {
  	Self { ptr }
  	}

• Same as above.
  

  nethsm-pkcs11/pkcs11/src/backend/db/attr.rs

  Lines 15 to 17 in 54f7b89

  	pub unsafe fn from_raw_ptr_unchecked(ptr: cryptoki_sys::CK_ATTRIBUTE_PTR) -> Self {
  	Self(ptr)
  	}

[nponsard]

• If CkRawMechanism::params is unsafe, shouldn’t this be unsafe too?

Both of these functions were written by Amazon engineers. I don't know what motivated them to set one unsafe and the other not.
Anyway I think that with the null checks and length checks in read_value the function can be marked as safe.

[robin-nitrokey]

I think the point is that the function call is only valid if the pointer actually points to a T. If you pass a different type, it would unsound. So i think both should be unsafe.

[nponsard]

Okay I made it unsafe in the PR