Skip to content

Allocation-free operations #349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bal-e wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allocation-free operations #349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 140 additions & 15 deletions cryptoki/src/session/signing_macing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,29 +3,38 @@
//! Signing and authentication functions

use crate::context::Function;
#[cfg(doc)]
use crate::error::RvError;
use crate::error::{Result, Rv};
use crate::mechanism::Mechanism;
use crate::object::ObjectHandle;
use crate::session::Session;
use cryptoki_sys::*;
use std::convert::TryInto;

/// # Generating and Verifying Signatures
Comment thread
wiktor-k marked this conversation as resolved.
///
/// Several functions are provided for signing data and verifying signatures.
/// This includes message authentication codes (MACs). The signed data can be
/// provided in one-shot and streaming modes.
impl Session {
/// Sign data in single-part
/// Sign data in one-shot mode.
///
/// `data` should be a byte sequence representing the input message. It will
/// be signed using the specified key, and the resulting signature will be
/// returned in a `Vec`.
///
/// Use [`Self::sign_into()`] if (an upper bound for) the size of the
/// signature is known, to avoid the heap allocation of `Vec`. Use
/// [`Self::sign_init()`] etc. if the input data is being streamed (i.e. it
/// is not all immediately available).
pub fn sign(&self, mechanism: &Mechanism, key: ObjectHandle, data: &[u8]) -> Result<Vec<u8>> {
let mut mechanism: CK_MECHANISM = mechanism.into();
let mut signature_len = 0;

unsafe {
Rv::from(get_pkcs11!(self.client(), C_SignInit)(
self.handle(),
&mut mechanism as CK_MECHANISM_PTR,
key.handle(),
))
.into_result(Function::SignInit)?;
}
// Initialize the signing operation.
self.sign_init(mechanism, key)?;

// Get the output buffer length
// Get the output buffer length.
unsafe {
Rv::from(get_pkcs11!(self.client(), C_Sign)(
self.handle(),
Expand All @@ -37,9 +46,10 @@ impl Session {
.into_result(Function::Sign)?;
}

// Allocate the output buffer.
let mut signature = vec![0; signature_len.try_into()?];

//TODO: we should add a new error instead of those unwrap!
// Perform the actual signing.
unsafe {
Rv::from(get_pkcs11!(self.client(), C_Sign)(
self.handle(),
Expand All @@ -51,11 +61,68 @@ impl Session {
.into_result(Function::Sign)?;
}

// Limit the output buffer to the size of the generated signature.
signature.truncate(signature_len.try_into()?);

Ok(signature)
}

/// Sign data into the given buffer.
///
/// `data` should be a byte sequence representing the input message. It will
/// be signed using the specified key, and the resulting signature will be
/// written to the buffer `sig`.
///
/// `sig` should be large enough to store the signature. The number of
/// filled bytes will be returned, such that `sig[0..size]` contains the
/// prepared signature. `sig[size..]` might be modified.
///
/// Use [`Self::sign()`] if (an upper bound for) the size of the signature
Comment thread
wiktor-k marked this conversation as resolved.
/// is not known. Use [`Self::sign_init()`] etc. if the input data is being
/// streamed (i.e. it is not all immediately available).
///
/// ## Errors
///
/// Returns [`RvError::BufferTooSmall`] if the generated signature does not
/// fit in `sig`. `sig` might be modified. The size of the actual signature
/// is **not** returned. This method should only be used if the caller knows
/// an upper bound for the signature size.
pub fn sign_into(
&self,
mechanism: &Mechanism,
key: ObjectHandle,
data: &[u8],
sig: &mut [u8],
) -> Result<usize> {
// The size of the signature buffer, into which 'C_Sign' will write the
// size of the generated signature.
let sig_buf_len = sig.len().try_into()?;
let mut sig_len = sig_buf_len;

// Initialize the signing operation.
self.sign_init(mechanism, key)?;

// Perform the actual signing.
unsafe {
Rv::from(get_pkcs11!(self.client(), C_Sign)(
self.handle(),
data.as_ptr() as *mut u8,
data.len().try_into()?,
sig.as_mut_ptr(),
&mut sig_len,
))
.into_result(Function::Sign)?;
}

assert!(
sig_len <= sig_buf_len,
"'C_Sign' succeeded but increased 'sig_len', possibly indicating out-of-bounds accesses"
);
Comment on lines +117 to +120
Copy link
Copy Markdown
Collaborator

@Jakuje Jakuje Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not use assert!() throughout the code, just in tests. It is either something that can not happen and then the check should not be there or it can happen and we should report proper error, regardless how much unlikely it would be or how much the pkcs11 module would have to be broken.

Copy link
Copy Markdown
Contributor Author

@bal-e bal-e Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially had a TODO comment here discussing whether an assert was appropriate. We could add some kind of ImplementationError variant to crate::error::Error, maybe that would be helpful for other functions too. @hug-dev, what do you think?

Copy link
Copy Markdown
Member

@hug-dev hug-dev Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that PKCS11 implementation should increase the sig_len value while returning CK_OK (as per the specs). If they do then I would say that it's an error in the implementation but maybe not something that we should check in runtime.

So I would also be in favor of removing those asserts!

Copy link
Copy Markdown
Contributor Author

@bal-e bal-e Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is that user code will assume this property to be upheld. It would be better for cryptoki to forcibly crash than to introduce a buffer overflow in user code (e.g. if the user does &sig[..sig_len], which loses bounds checks in release mode).

Copy link
Copy Markdown
Collaborator

@wiktor-k wiktor-k Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK Rust still inserts bounds checks in release mode (source https://news.ycombinator.com/item?id=39261604) unless the optimizer can deduct that it's safe to remove (eg. in for loops).

Maybe yet another possibility would be to use debug_assert? 🤔

Copy link
Copy Markdown
Collaborator

@wiktor-k wiktor-k Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think returning an error is more appropriate than assert in any production code though.

Yeah, asserts in production code are especially tricky. For really rare occasions even stdlib has panics but maybe we can "have a cookie and eat it too" here by introducing some error variant with just a string description that'd be a catch-all for all these assert-like bugs? 🤔

Copy link
Copy Markdown
Member

@hug-dev hug-dev Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is that user code will assume this property to be upheld.

I think that is fair to assume (we can even write a warning notice about that in the readme). If we can't trust that the PKCS11 library implements the spec correctly then there is not much we can do 😢

Copy link
Copy Markdown
Contributor Author

@bal-e bal-e Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, asserts in production code are especially tricky.

I would normally agree, but I think "we think UB has occurred, but we don't want to panic, maybe everything is fine" is concerning. I don't know what the ideal solution is. I'm happy to implement whatever you (the maintainers) think should happen.

Copy link
Copy Markdown
Collaborator

@wiktor-k wiktor-k Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to implement whatever you (the maintainers) think should happen.

That's great, we just have a little split brain problem here so it may take a while until we have a consensus:)

Personally I'm okay with an assert of a very unlikely scenario. I'm also fine with a new Error enum variant if that makes @Jakuje calm.

The "no check at all" scenario is the worse one IMO.

Copy link
Copy Markdown
Collaborator

@Jakuje Jakuje Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree so don't let me hold the PR back as it is a great improvement! I agree that this case really means the underlying token is broken so maybe the assert is adequate. I just know how annoying is when somewhere deep in the dependency chain of your built package you get assert and there is nothing you can do but rebuild.


// NOTE: As checked above, 'sig_len <= sig_buf_len <= usize::MAX'.
Ok(sig_len as usize)
}

/// Starts new multi-part signing operation
pub fn sign_init(&self, mechanism: &Mechanism, key: ObjectHandle) -> Result<()> {
let mut mechanism: CK_MECHANISM = mechanism.into();
Expand Down Expand Up @@ -87,12 +154,20 @@ impl Session {
Ok(())
}

/// Finalizes ongoing multi-part signing operation,
/// returning the signature
/// Complete an ongoing streaming signing operation.
///
/// This must be preceded by [`Self::sign_init()`] and zero or more calls
/// to [`Self::sign_update()`]. This method will terminate the multi-part
/// signing operation. The resulting signature will be returned in a `Vec`.
///
/// Use [`Self::sign_final_into()`] if (an upper bound for) the size of
/// the signature is known, to avoid the heap allocation of `Vec`. Use
/// [`Self::sign()`] if the input data is entirely available in a single
/// buffer (i.e. does not have to be streamed).
pub fn sign_final(&self) -> Result<Vec<u8>> {
let mut signature_len = 0;

// Get the output buffer length
// Get the output buffer length.
unsafe {
Rv::from(get_pkcs11!(self.client(), C_SignFinal)(
self.handle(),
Expand All @@ -102,8 +177,10 @@ impl Session {
.into_result(Function::SignFinal)?;
}

// Allocate the output buffer.
let mut signature = vec![0; signature_len.try_into()?];

// Perform the actual signing.
unsafe {
Rv::from(get_pkcs11!(self.client(), C_SignFinal)(
self.handle(),
Expand All @@ -113,11 +190,59 @@ impl Session {
.into_result(Function::SignFinal)?;
}

// Limit the output buffer to the size of the generated signature.
signature.truncate(signature_len.try_into()?);

Ok(signature)
}

/// Complete an ongoing multi-part signing operation, writing the signature
/// into the given buffer.
///
/// This must be preceded by [`Self::sign_init()`] and zero or more calls
/// to [`Self::sign_update()`]. This method will terminate the multi-part
/// signing operation, and write the signature to the buffer `sig`.
///
/// `sig` should be large enough to store the signature. The number of
/// filled bytes will be returned, such that `sig[0..size]` contains the
/// prepared signature. `sig[size..]` might be modified.
///
/// Use [`Self::sign_final()`] if (an upper bound for) the size of the
/// signature is not known. Use [`Self::sign_into()`] if the input data
/// is entirely available in a single buffer (i.e. does not have to be
/// streamed).
///
/// ## Errors
///
/// Returns [`RvError::BufferTooSmall`] if the generated signature does not
/// fit in `sig`. `sig` might be modified. The size of the actual signature
/// is **not** returned. This method should only be used if the caller knows
/// an upper bound for the signature size.
pub fn sign_final_into(&self, sig: &mut [u8]) -> Result<usize> {
// The size of the signature buffer, into which 'C_SignFinal' will write
// the size of the generated signature.
let sig_buf_len = sig.len().try_into()?;
let mut sig_len = sig_buf_len;

// Perform the underlying finalization.
unsafe {
Rv::from(get_pkcs11!(self.client(), C_SignFinal)(
self.handle(),
sig.as_mut_ptr(),
&mut sig_len,
))
.into_result(Function::SignFinal)?;
}

assert!(
sig_len <= sig_buf_len,
"'C_SignFinal' succeeded but increased 'sig_len', possibly indicating out-of-bounds accesses"
);

// NOTE: As checked above, 'sig_len <= sig_buf_len <= usize::MAX'.
Ok(sig_len as usize)
}

/// Verify data in single-part
pub fn verify(
&self,
Expand Down
35 changes: 35 additions & 0 deletions cryptoki/tests/basic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@ fn sign_verify() -> TestResult {
// verify the signature
session.verify(&Mechanism::RsaPkcs, public, &data, &signature)?;

// sign into a user-provided buffer with it
let mut signature = [0u8; 2048 / 8];
let sig_len = session.sign_into(&Mechanism::RsaPkcs, private, &data, &mut signature)?;

// verify the signature
session.verify(&Mechanism::RsaPkcs, public, &data, &signature[..sig_len])?;

// delete keys
session.destroy_object(public)?;
session.destroy_object(private)?;
Expand Down Expand Up @@ -122,6 +129,12 @@ fn sign_verify_eddsa() -> TestResult {

session.verify(&Mechanism::Eddsa(params), public, &data, &signature)?;

let mut signature = [0u8; 64];
let sig_len = session.sign_into(&Mechanism::Eddsa(params), private, &data, &mut signature)?;
assert_eq!(sig_len, 64);

session.verify(&Mechanism::Eddsa(params), public, &data, &signature)?;

session.destroy_object(public)?;
session.destroy_object(private)?;

Expand Down Expand Up @@ -177,6 +190,13 @@ fn sign_verify_eddsa_with_ed25519_schemes() -> TestResult {
let signature = session.sign(&Mechanism::Eddsa(params), private, &data)?;

session.verify(&Mechanism::Eddsa(params), public, &data, &signature)?;

let mut signature = [0u8; 64];
let sig_len =
session.sign_into(&Mechanism::Eddsa(params), private, &data, &mut signature)?;
assert_eq!(sig_len, 64);

session.verify(&Mechanism::Eddsa(params), public, &data, &signature)?;
}

session.destroy_object(public)?;
Expand Down Expand Up @@ -285,6 +305,21 @@ fn sign_verify_multipart() -> TestResult {
}
session.verify_final(&signature)?;

// Sign data into a user-provided buffer
session.sign_init(&Mechanism::Sha256RsaPkcs, priv_key)?;
for part in data.chunks(3) {
session.sign_update(part)?;
}
let mut signature = [0u8; 2048 / 8];
let sig_len = session.sign_final_into(&mut signature)?;

// Verify signature from the user-provided buffer
session.verify_init(&Mechanism::Sha256RsaPkcs, pub_key)?;
for part in data.chunks(3) {
session.verify_update(part)?;
}
session.verify_final(&signature[..sig_len])?;

// Delete keys
session.destroy_object(pub_key)?;
session.destroy_object(priv_key)?;
Expand Down
Loading