parallaxsecond/rust-psa-crypto

Rust wrapper around the PSA Cryptography API

rust-psa-crypto's Issues

Handling key handle closing is noisy and error prone

As discussed in other PRs, having to deal with cleanup operations after a normal operation and returning any errors in the correct order is noisy and error prone.

E.g.:

let export_res = Status::from(unsafe {
        psa_crypto_sys::psa_export_public_key(
            handle,
            data.as_mut_ptr(),
            data.len(),
            &mut data_length,
        )
    })
    .to_result();
    key.close_handle(handle)?;
    export_res?;
    Ok(data_length)

(not sure if the errors are returned in the 'correct' order here!)

Add a test on the CI compiling Mbed TLS and linking dynamically the crate to Mbed Crypto

We currently always build Mbed Crypto from source but we should add a test where it is built independantly and linked to with the environment variables.

Delete const from pub const fn key_derivation

I can't build psa-crypto with version 1.45.2 of Rust because:

`match` is not allowed in a `const fn`

I can build it if I delete the const from pub const fn key_derivation here:

https://github.com/parallaxsecond/rust-psa-crypto/blob/master/psa-crypto/src/types/key_derivation.rs#L109

I don't think the const currently does anything useful so probably this change would be generally helpful.

'export_key_pair_test' panicked at 'called `Result::unwrap()` on an `Err` value: StorageFailure'

I've got this test failure for rust-1.45.0 on Fedora Rawhide:

---- export_key_pair_test stdout ----
thread 'export_key_pair_test' panicked at 'called `Result::unwrap()` on an `Err` value: StorageFailure', tests/mod.rs:155:22
stack backtrace:
   0:     0x559ebc0118a5 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h83fd459720a1b780
   1:     0x559ebc0358dc - core::fmt::write::h0fb2c8a7c74dc706
   2:     0x559ebbf8c0a5 - std::io::Write::write_fmt::hb203e303045fd2e8
   3:     0x559ebc00bcb1 - std::io::impls::<impl std::io::Write for alloc::boxed::Box<W>>::write_fmt::hbcb41256641e1664
   4:     0x559ebc0140d0 - std::panicking::default_hook::{{closure}}::hbb12a969b90f30ab
   5:     0x559ebc013dca - std::panicking::default_hook::hd46a6f05cd95ae1c
   6:     0x559ebc014707 - std::panicking::rust_panic_with_hook::hfaf36f07e96e1a70
   7:     0x559ebc01430b - rust_begin_unwind
   8:     0x559ebc034281 - core::panicking::panic_fmt::ha79c22c8889dbf6b
   9:     0x559ebc0340a3 - core::option::expect_none_failed::he9f78d166510670b
  10:     0x559ebbf8a659 - core::result::Result<T,E>::unwrap::h3e1a442c611a1234
                               at /builddir/build/BUILD/rustc-1.45.0-src/src/libcore/result.rs:1005
  11:     0x559ebbf8a659 - mod::test_tools::TestClient::import::h2b975496b53e491a
                               at tests/mod.rs:155
  12:     0x559ebbf8a659 - mod::export_key_pair_test::hc1fe2c966e9ab52a
                               at tests/mod.rs:124
  13:     0x559ebbf8a659 - mod::export_key_pair_test::{{closure}}::hfebe911071c9ccf8
                               at tests/mod.rs:82
  14:     0x559ebbf8a659 - core::ops::function::FnOnce::call_once::hb7c8e6f4e27a34b4
                               at /builddir/build/BUILD/rustc-1.45.0-src/src/libcore/ops/function.rs:232
  15:     0x559ebbfb33b2 - test::run_test::run_test_inner::{{closure}}::h8ead2fffa18ec440
  16:     0x559ebbf8b566 - std::sys_common::backtrace::__rust_begin_short_backtrace::h0514d4ec185c1c32
  17:     0x559ebbf906e5 - core::ops::function::FnOnce::call_once{{vtable.shim}}::h6d24acaff99ea573
  18:     0x559ebc01a9da - std::sys::unix::thread::Thread::new::thread_start::h4354789cb8f1f413
  19:     0x7f601626d53a - start_thread
  20:     0x7f601617e283 - __GI___clone
  21:                0x0 - <unknown>

Exposing native types for key encodings

I was wondering if it makes sense to expose, potentially behind a feature flag, types that can be used for (de)serializing the various key types supported by PSA crypto. So, something like what we implement in Parsec, but for all types of keys.

This would help Parsec as well, and any users of the psa-crypto crate who want proper support for the "whole" PSA crypto interface.

Investigate interfacing to a defined subset of PSA Crypto

The PSA Crypto API has some implementation-defined parts. For example, output size macros and various types need to be defined by the implementor.
The consequence of that are:

• the header file of the PSA Crypto implementation needs to be given as an input of this crate to generate the bindings
• bindings can not be pre-generated
• dynamic loading is impossible

A solution to investigate would be for us to define a subset PSA Crypto API where:

• all functions are defined but return PSA_ERROR_NOT_SUPPORTED if not implementend
• the key types and algorithms are limited to only the ones defined within the API (no extension)
• the output macro are implemented (they can be with the constraint above)
• the types are implemented, probably with void * pointer

We could implement all this in a header file, in this repo that we produce bindings from.

On the PSA Crypto implementors side, there will be a need to write a wrapper implementing our subset with the available functions.
In an ideal world, we could imagine a compilation option on the implementor project to produce a .a or .so which implements the subset instead of the original API. Then people will need to use that with Parsec.

psa-crypto-sys test fail on i686

One of the tests fails when run on i686:

failures:
---- psa_crypto_binding::bindgen_test_layout_max_align_t stdout ----
thread 'psa_crypto_binding::bindgen_test_layout_max_align_t' panicked at 'assertion failed: `(left == right)`
  left: `16`,
 right: `24`: Size of: max_align_t', /builddir/build/BUILD/psa-crypto-sys-0.3.0/target/release/build/psa-crypto-sys-59266dd2c6e5c562/out/shim_bindings.rs:3:22153
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
    psa_crypto_binding::bindgen_test_layout_max_align_t
test result: FAILED. 52 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

Implement Display for Algorithm

It is useful for user facing applications that query the algorithm of a key.

Create the Rust representation of PSA Crypto operations

The PSA methods, written in Rust, can very largely be inspired by the ones created in the Parsec Rust Client replacing the key_name as a String with the key ID.

The following principle was adopted:

• use Vec<u8> whenever a buffer is needed
• use a Rust abstraction instead of the bindgen type
• restrict the algorithm argument to the only possible values for the specific operation. For example psa_sign_hash and psa_verify_hash directly take an AsymmetricSignature type and not the high-level Algorithm

Have a feature only using the PSA interface

For the Secure Element Driver use-case, PSA Crypto is only needed for its interface (the include files in include/psa): the types, the values, the macros and static inline methods.
It should not need linking with the actual PSA Crypto implementation but only need its include files.

For that use-case, and similar ones, we should have a special feature on psa-crypto and psa-crypto-sys which make them expose all the PSA Crypto interface, without the operations.

MBEDTLS_INCLUDE_DIR env var must be passed for that feature as part of the interface is implementation-defined.

I am not sure fo the name of the feature. If the use-case is only for the SE driver, it could be the se-driver feature. Or interface.

Design a good abstraction for key IDs

We need to find a nice way to represent the psa_key_id_t type in Rust.

Separate `HKDF-Extract` and `HKDF-Expand`

The PSA API v1.1.1 defines HKDF-Extract and HKDF-Expand, and these functions are available in mbedtls-3.0.0^0 (the version used in this wrapper).

Are there plans to include these 2 functions as part of the wrapper? Would a PR be welcome?

Be able to dynamically link with any PSA Crypto C shared library

Ideally this crate should not be responsible for compiling the PSA Crypto implementation (such as Mbed Crypto) but the user should do it and instantiate this wrapper with a path to the .so.

The dynamic loading can be done libloading.

Conversion to `Status` seems wrong

Currently the conversion From<psa_crypto_sys::psa_status_t> for Status converts any unsupported integer to a GenericError - I think that's not entirely correct as it aliases an error occuring in the crate with one relayed by the linked library.

The correct thing to do would be to have TryFrom and return a crate-specific error for anything outside of the recognised codes, and the correct enum variant for the correct ones. However I do get that TryFrom is not actually available without std so maybe we should have a method implemented straight on Status when no-std is on and the normal TryFrom when std is available.

Compile issue with clang 13

Openembedded is now using gcc 11 and I am seeing the following build error.

parsec-service/0.7.0-r0/cargo_home/bitbake/psa-crypto-sys-0.8.0/vendor/library/psa_crypto.c:4590:12: error: parameter 'output_size' set but not used [-Werror,-Wunused-but-set-parameter]
| size_t output_size,
| ^
| 1 error generated.

Looking @ Mbedlts, it appears the offending function has been removed via commit:

commit 6d05173359fb53eb2a893e6834a221a4262233da
Author: Ronald Cron [email protected]
Date: Thu Oct 1 14:10:20 2020 +0200

psa: Add mbedtls_psa_cipher_xyz() APIs

Signed-off-by: Ronald Cron <[email protected]>

I think there is a mismatch somewhere.

Implement macros to have size of output buffers

The macros like PSA_SIGN_OUTPUT_SIZE and PSA_EXPORT_PUBLIC_KEY_OUTPUT_SIZE need to have their abstraction in this crate in order to use the export or sign operations correctly.

warning: `extern` block uses type `[u64; 4]`, which is not FFI-safe

In the output from the CI tests there are about ten warnings similar to this one:

   Compiling psa-crypto-sys v0.10.0 (/home/runner/work/rust-psa-crypto/rust-psa-crypto/psa-crypto-sys)
warning: `extern` block uses type `[u64; 4]`, which is not FFI-safe
 --> /home/runner/work/rust-psa-crypto/rust-psa-crypto/target/aarch64-unknown-linux-gnu/debug/build/psa-crypto-sys-0bf6a434edbfaaab/out/shim_bindings.rs:3:154378
  |
3 | ...at : * const :: std :: os :: raw :: c_char , __arg : [u64 ; 4usize]) -> :: std :: os :: raw :: c_int ; } extern "C" { pub fn vprintf (...
  |                                                         ^^^^^^^^^^^^^^ not FFI-safe
  |
  = note: `#[warn(improper_ctypes)]` on by default
  = help: consider passing a pointer to the array
  = note: passing raw arrays by value is not FFI-safe

The warnings happen when building for AArch64 and apparently relate to va_list. They are probably harmless so we don't have to do anything about it, but maybe we'll want to turn warnings into errors one day. Possible actions:

1. Do nothing.
2. If the functions are not used, tell bindgen not to generate bindings for those functions: is there a better way of doing that than listing all the functions somewhere in build.rs?
3. Disable that particular warning for shim_bindings.rs.

Include the LICENSE file in the crate

The LICENSE file should be included in the crate.

Update Lifetime structure

Version 1.0.1 of PSA Crypto introduced new subtypes for psa_key_lifetime_t: key persistence (psa_key_persistence_t) and key location (psa_key_location_t).

These new types need to be abstracted in the Rust wrapper and their utility macros need to be accessible as well.

Warning: Parsec uses the Lifetime type in its mappings. Changing it means that it will break the deserialisation of older mappings. Ultimately this is a Parsec problem and does not concern this crate. But worth noting. I put it as an agenda item for 09/02/2021 to discuss.

Fix the UsageFlags structure

We added a clarification in the book in parallaxsecond/parsec-book#110 regarding sign/verify_hash/message.

The following part is specifically important about sign/verify_hash:

This flag automatically sets sign_message: if an application sets the flag sign_hash when creating a key, then the key always has the permissions conveyed by sign_message. For a key pair, this concerns the private key.

I see multiple ways to fix this:

1. Make the fields of UsageFlags private (or just the sign/verify ones?) with setters and getters on the different usages. set_sign_hash would set both sign_hash and sign_message to true.
2. Just add verify/sign_message methods that return true if either sign/verify_message or sign/verify_hash are true.

I would prefer 1 I think but that is a breaking change. That's probably fine. 2 would make the behaviour correct if you use the new method but we would have to check that we use that everywhere. It's not breaking though.

What's the purpose of the "interface" feature?

I'm attempting to us rust-psa-crypto to enable PSA Crypto bindings to a native PSA Crypto library in Web Assembly binaries compiled from Rust.

It looks like I want to remove compilation of shim.c, and replace it with a rust source file that has "extern C" for each of the API entrypoints.

However, I then found the "interface" feature, which sounds like what I'd want, but doesn't look like it in the code (but I'm still looking).

Anyway, I don't want to go down a stupid, work-heavy path, if you guys have already built in a mechanism that makes it easy.

Move the Mbed TLS/Mbed Crypto specific code in its own crate

The psa-crypto crate should ideally be a generic wrapper around the PSA Crypto API. Currently we are also offering in that crate the building and linking with Mbed Crypto (in Mbed TLS), one of its implementation.

We should maybe try to separate those two things with maybe a mbedcrypto crate that contains the finding, building and linking with Mbed Crypto. It could also contain the bindings for Mbed Crypto so will remove the need of bindgen!

Design discussion of the psa-crypto crate

I assume that the psa-crypto-sys crate would re-exports all of of the public items of PSA Crypto API, with their expected names (functions, types, macros, constants).

I propose the following design for the psa-crypto crate:

• in src/lib.rs: create the PsaCrypto struct and the static ONCE: AtomicBool;. A new method creates an instance, sets the ONCE bool to true, initialises the library or returns an error if already created
• types/key.rs containing Attributes (psa_key_attributes_t), Lifetime, Id, Type, Usage and Policy (an Algorithm and an Usage). Most of the types copied from here.
• types/algorithm.rs containing the Algorithm structure.
• types/status.rs containing the Status enumeration with all psa_status_t. Create a Result typedef with Status as the error type
• operations/ containing the files defining all methods on PsaCrypto: key_management.rs, message_digest.rs, mac.rs, ciphers.rs, aead.rs, key_derivation.rs, asym_signature.rs, asym_encryption.rs, key_agreement.rs and other_crypto.rs.

Each one of the types files would implement conversions to and from the corresponding FFI type.

Create the function signatures applying the following systematic method:

• remove the psa_ prefix and the _t suffix and use camel-case
• make all functions return a Result
• instead of the psa_..._t type, replace with the Rust native type. If they are small enough, it might be simpler to implement Copy on them and pass them directly instead of by reference
• when taking an input buffer (const uint8_t *toto and size_t toto_length) replace with a byte slice: &[u8]
• when returning a value (like a key ID), put it in the Ok variant of the Result
• when taking an output buffer (uint8_t *toto, size_t toto_size and size_t *toto_length) replace with a mutable byte slice &mut [u8] and return a usize. The reason for not using a Vec<u8> is that it would require std and would not be applicable on embedded targets. We could produce higher-level function with a cfg flag for when std is available, which use Vec for more simplicity.

Safety

We have to provide a safe interface. Using only Rust types and assuming the safety of PSA Crypto when used correctly we can achieve memory safety easily.
Thread safety is more tricky and can add more challenges. Section 5.4 lists the constraints needed to achieve thread-safety:

1. There is no overlap between an output parameter of one call and an input or output parameter of another call. Overlap between input parameters is permitted.
2. If a call destroys a key, then no other call must destroy or use that key. Using, in this context, includes all functions of multi-part operations which have used the key as an input in a previous function.
3. Concurrent calls that use the same key are permitted.
4. Concurrent calls must not use the same operation object.

Rust safety guarantees, I think, prevent us from 1 and 4. 3 is not really a constraint.
2 is the trickiest and I am not sure how to handle it. If it is undefined behaviour to destroy a key more than once, we need to make sure that destroy is called only on KeyId of keys that exist. For persistent keys: how to retrieve, at instantiation, the keys that are already created?
I think the most simple way to approach this for now would be to put the destroy function as unsafe.

Warn when the `vendor` directory is not populated

Just after freshly cloning the repo, the vendor directory needs to be populated with the git submodule. If one tries to compile, a cryptic error message is returned:

error: failed to run custom build command for `psa-crypto-sys v0.8.0 (/root/rust-psa-crypto/psa-crypto-sys)`

Caused by:
  process didn't exit successfully: `/root/rust-psa-crypto/target/debug/build/psa-crypto-sys-f27d8926d3ac59c9/build-script-build` (exit status: 1)
  --- stdout
  Did not find environment variables, building MbedTLS!
  cargo:rerun-if-changed=src/c/shim.c
  cargo:rerun-if-changed=src/c/shim.h

  --- stderr
  Error: Custom { kind: Other, error: "configuring mbedtls failed" }

We should try to improve the error message and also note in the README which command is needed.

psa_crypto_sys is not no_std

Starting to look into how this could be used on RIOT OS (that since its 2023.10 release has PSA support on its C side), I've noticed that psa_crypto_sys is no_std.

Is that an oversight (if so, it may be convenient to test that things are buildable on no_std, eg. by including a build with --target , similar to the CI commits of wasmi-labs/wasmi#387); if not: is there a plan for how this is to be used on embedded? (Indeed it might be that for RIOT's purposes the psa_crypto_sys crate is patched to go through a different header discovery route, though I'd hope that this can be done upstream by just providing an environment variable).

Update mbedtls to 3.5.2

There's a CVE (CVE-2024-23775) up to and including 3.5.1 which is a buffer overflow in mbedtls_x509_set_extension():
https://mbed-tls.readthedocs.io/en/latest/security-advisories/mbedtls-security-advisory-2024-01-2/

SecpR1 bit length 284?

In https://github.com/parallaxsecond/rust-psa-crypto/blob/7695a58a0a27fc6b8d2fd49a77f149fe987a3afb/psa-crypto/src/types/key.rs#L923C43-L923C46 the allowed SecpR1 bit lengths include 256, 284 and 521. Shouldn't the 284 be 384?

Support latest LTS (2.28.0 ) release of mbedts

The #88 commit moved to mbedtls 3.0 but it seems MbedTLS has made 2.28.x the new LTS release.

It would be good to be able to support that in the psa-crypto and allow distros to build against their supported version of MBedTLS so that when there's CVEs in MBedTLS the psa-crypto pieces aren't affected and can just consume the distro updates directly.

Update the Mbed TLS submodule to 2.25.0

This is mainly to update the types exported by the new SE driver interface. Will also fix some of the key handling issues with alignments of Mbed Crypto to 1.0.0

Always close key handle if key is persistent

Currently handles for persistent keys are kept open after generating and importing, which causes mbedtls to run out of key slots.

If a key is persistent, its handle should be closed after each generate and import operation.

Only set the handle field if the key is volatile.

Cipher: IV must not be set in case ECB mode of operation is used

In cipher.rs
psa_crypto_sys::psa_cipher_set_iv is called without checking if the operation requires setting IV or not.
In case ECB operations this function psa_crypto_sys::psa_cipher_set_iv should not be called.

Available approaches:

• Check operation mode and call the function if iv is required.
• Pass the IV as an Option and here the responsibility of the caller is to determine if the iv is required or not.
  if the user misused it then the function will propagate the error.

Changes to the C library do not trigger a rebuild

If the C code in psa-crypto-sys/vendor/ is changed then cargo does not rebuild it.

A work-around is to run touch psa-crypto-sys/build.rs when the C code might have changed.

I think it would be better if build.rs found every file (and perhaps every directory) under vendor/ (there's a crate called walkdir) and emitted cargo:rerun-if-changed=PATH for each one. (Or you could try to more selective, but persuading cmake to tell you which files really matter would probably be too much work.)

Create abstractions of key attributes and algorithms

Rust structures representing PSA key attributes and algorithms were created in parsec_interface. They should probably live in this crate and Parsec interface consuming them.

See PSA algorithms and PSA key attributes.

Discuss type/module naming

I'm a bit weary of some of our naming conventions. We adopt the "no prefix/sufix" format for not including the module name in type or function names.

For example, we have psa_crypto::types::key which has types related to key attributes within it. I'd say this module (and its contents) could lead to confusion and/or conflicts for a couple of reasons:

• the module doesn't actually contain a Key type or anything of the sort - the closest would probably be Id which is an opaque handler for key stored elsewhere, so from that point of view the module name isn't accurate
• at least some of the types within the module have very vague names (mostly talking about Id and Type here); I agree that not all types should be fully named (e.g. KeyPolicy), but the overall effect of not using more specific names falls on users of the crate; if you know/see the use psa_crypto::types::key::Type then you'll find it easy to understand something like Type::from(...); but if you're in the middle of a file and stumble across that, you have to look it up and then remember what that Type type represents; this puts a burden on the users to either rename the type on import, or keep it in mind while coding - the more types we do this with, the worse it gets; (also, imagine if all libraries did this and there were 3 Types and 2Ids)

I've only looked through the standard library to get a feel for how we could tackle this and most of the types there seem to exclude prefixes or suffixes most of the time, but for example std::thread contains both Thread and ThreadId. My suggestion would be to prefix or suffix the names that are way too vague, leaving the other ones to follow convention.

Take a look at the original PR that added the module_name_repetitions lint and especially at Martin Carton's comments: rust-lang/rust-clippy#994