Skip to content

ML-KEM/ML-DSA part 2: param builder #2451

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
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

ML-KEM/ML-DSA part 2: param builder #2451

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
69182d1
ML-KEM/ML-DSA part 2: param builder
swenson Aug 29, 2025
00e12f7
Use &CStr instead of &[u8] for keys; builder should be mutable
swenson Aug 30, 2025
131cddd
Remove allow dead code from param builder
swenson Aug 30, 2025
6e4440f
Use CStr::from_bytes_with_nul_unchecked instead of c-string literal, …
swenson Aug 30, 2025
7642cc6
Rename OsslParam to OsslParamArray and add clarifying comments
swenson Aug 31, 2025
99520b0
Make OsslParam and friends pub(crate); clarify comments about OsslPar…
swenson Sep 1, 2025
20091b3
Create separate type for the OSSL_PARAM array and a reference to an i…
swenson Sep 1, 2025
deae632
Remove out OsslParam for now
swenson Sep 1, 2025
21a83a6
Annotate lifetimes for builder methods
swenson Sep 1, 2025
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
107 changes: 42 additions & 65 deletions openssl/src/kdf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,24 @@ impl Drop for EvpKdfCtx {
cfg_if::cfg_if! {
if #[cfg(all(ossl320, not(osslconf = "OPENSSL_NO_ARGON2")))] {
use std::cmp;
use std::ffi::c_void;
use std::ffi::CStr;
use std::mem::MaybeUninit;
use std::ptr;
use foreign_types::ForeignTypeRef;
use libc::c_char;
use crate::{cvt, cvt_p};
use crate::lib_ctx::LibCtxRef;
use crate::error::ErrorStack;
use crate::ossl_param::OsslParamBuilder;

const OSSL_KDF_PARAM_PASSWORD: &[u8; 5] = b"pass\0";
const OSSL_KDF_PARAM_SALT: &[u8; 5] = b"salt\0";
const OSSL_KDF_PARAM_SECRET: &[u8; 7] = b"secret\0";
const OSSL_KDF_PARAM_ITER: &[u8; 5] = b"iter\0";
const OSSL_KDF_PARAM_SIZE: &[u8; 5] = b"size\0";
const OSSL_KDF_PARAM_THREADS: &[u8; 8] = b"threads\0";
const OSSL_KDF_PARAM_ARGON2_AD: &[u8; 3] = b"ad\0";
const OSSL_KDF_PARAM_ARGON2_LANES: &[u8; 6] = b"lanes\0";
const OSSL_KDF_PARAM_ARGON2_MEMCOST: &[u8; 8] = b"memcost\0";

#[allow(clippy::too_many_arguments)]
pub fn argon2d(
Expand Down Expand Up @@ -94,72 +103,40 @@ cfg_if::cfg_if! {
salt: &[u8],
ad: Option<&[u8]>,
secret: Option<&[u8]>,
mut iter: u32,
mut lanes: u32,
mut memcost: u32,
iter: u32,
lanes: u32,
memcost: u32,
out: &mut [u8],
) -> Result<(), ErrorStack> {
unsafe {
let libctx = ctx.map_or(ptr::null_mut(), ForeignTypeRef::as_ptr);
let max_threads = unsafe {
ffi::init();
let libctx = ctx.map_or(ptr::null_mut(), ForeignTypeRef::as_ptr);

let max_threads = ffi::OSSL_get_max_threads(libctx);
let mut threads = 1;
// If max_threads is 0, then this isn't a threaded build.
// If max_threads is > u32::MAX we need to clamp since
// argon2's threads parameter is a u32.
if max_threads > 0 {
threads = cmp::min(lanes, cmp::min(max_threads, u32::MAX as u64) as u32);
}
let mut params: [ffi::OSSL_PARAM; 10] =
core::array::from_fn(|_| MaybeUninit::<ffi::OSSL_PARAM>::zeroed().assume_init());
let mut idx = 0;
params[idx] = ffi::OSSL_PARAM_construct_octet_string(
b"pass\0".as_ptr() as *const c_char,
pass.as_ptr() as *mut c_void,
pass.len(),
);
idx += 1;
params[idx] = ffi::OSSL_PARAM_construct_octet_string(
b"salt\0".as_ptr() as *const c_char,
salt.as_ptr() as *mut c_void,
salt.len(),
);
idx += 1;
params[idx] =
ffi::OSSL_PARAM_construct_uint(b"threads\0".as_ptr() as *const c_char, &mut threads);
idx += 1;
params[idx] =
ffi::OSSL_PARAM_construct_uint(b"lanes\0".as_ptr() as *const c_char, &mut lanes);
idx += 1;
params[idx] =
ffi::OSSL_PARAM_construct_uint(b"memcost\0".as_ptr() as *const c_char, &mut memcost);
idx += 1;
params[idx] =
ffi::OSSL_PARAM_construct_uint(b"iter\0".as_ptr() as *const c_char, &mut iter);
idx += 1;
let mut size = out.len() as u32;
params[idx] =
ffi::OSSL_PARAM_construct_uint(b"size\0".as_ptr() as *const c_char, &mut size);
idx += 1;
if let Some(ad) = ad {
params[idx] = ffi::OSSL_PARAM_construct_octet_string(
b"ad\0".as_ptr() as *const c_char,
ad.as_ptr() as *mut c_void,
ad.len(),
);
idx += 1;
}
if let Some(secret) = secret {
params[idx] = ffi::OSSL_PARAM_construct_octet_string(
b"secret\0".as_ptr() as *const c_char,
secret.as_ptr() as *mut c_void,
secret.len(),
);
idx += 1;
}
params[idx] = ffi::OSSL_PARAM_construct_end();

ffi::OSSL_get_max_threads(libctx)
};
let mut threads = 1;
// If max_threads is 0, then this isn't a threaded build.
// If max_threads is > u32::MAX we need to clamp since
// argon2id's threads parameter is a u32.
if max_threads > 0 {
threads = cmp::min(lanes, cmp::min(max_threads, u32::MAX as u64) as u32);
}
let bld = OsslParamBuilder::new()?;
bld.add_octet_string(OSSL_KDF_PARAM_PASSWORD, pass)?;
bld.add_octet_string(OSSL_KDF_PARAM_SALT, salt)?;
bld.add_uint(OSSL_KDF_PARAM_THREADS, threads)?;
bld.add_uint(OSSL_KDF_PARAM_ARGON2_LANES, lanes)?;
bld.add_uint(OSSL_KDF_PARAM_ARGON2_MEMCOST, memcost)?;
bld.add_uint(OSSL_KDF_PARAM_ITER, iter)?;
let size = out.len() as u32;
bld.add_uint(OSSL_KDF_PARAM_SIZE, size)?;
if let Some(ad) = ad {
bld.add_octet_string(OSSL_KDF_PARAM_ARGON2_AD, ad)?;
}
if let Some(secret) = secret {
bld.add_octet_string(OSSL_KDF_PARAM_SECRET, secret)?;
}
let params = bld.to_param()?;
unsafe {
let argon2 = EvpKdf(cvt_p(ffi::EVP_KDF_fetch(
libctx,
kdf_identifier.as_ptr() as *const c_char,
Expand Down
2 changes: 2 additions & 0 deletions openssl/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,8 @@ pub mod memcmp;
pub mod nid;
#[cfg(not(osslconf = "OPENSSL_NO_OCSP"))]
pub mod ocsp;
#[cfg(ossl300)]
mod ossl_param;
pub mod pkcs12;
pub mod pkcs5;
#[cfg(not(any(boringssl, awslc)))]
Expand Down
182 changes: 182 additions & 0 deletions openssl/src/ossl_param.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
//! OSSL_PARAM management for OpenSSL 3.*
//!
//! The OSSL_PARAM structure represents generic attribute that can represent various
//! properties in OpenSSL, including keys and operations.
//!
//! For convinience, the OSSL_PARAM_BLD builder can be used to dynamically construct
//! these structure.
//!
//! Note, that this module is available only in OpenSSL 3.* and
//! only internally for this crate!

// Depending on which version of OpenSSL is used, and which algorithms
// are exposed in the bindings, not all of these functions are used.
#![allow(dead_code)]

use crate::bn::{BigNum, BigNumRef};
use crate::error::ErrorStack;
use crate::util;
use crate::{cvt, cvt_p};
use foreign_types::{ForeignType, ForeignTypeRef};
use libc::{c_char, c_uint, c_void};
use openssl_macros::corresponds;
use std::ffi::CStr;
use std::ptr;

foreign_type_and_impl_send_sync! {
type CType = ffi::OSSL_PARAM;
fn drop = ffi::OSSL_PARAM_free;

/// `OsslParam` constructed using `OsslParamBuilder`.
pub struct OsslParam;
/// Reference to `OsslParam`.
pub struct OsslParamRef;
}

impl OsslParam {}

impl OsslParamRef {
/// Locates the `OsslParam` in the `OsslParam` array
#[corresponds(OSSL_PARAM_locate)]
pub fn locate(&self, key: &[u8]) -> Result<&OsslParamRef, ErrorStack> {
unsafe {
let param = cvt_p(ffi::OSSL_PARAM_locate(
self.as_ptr(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't correct either -- OSSL_PARAM_locate takes a pointer to an array of params that is null terminated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not quite as bad as that, thankfully -- it looks like it should be terminated with a param with a null key, but the param itself should not be null AFAICT:

https://github.com/openssl/openssl/blob/master/include/openssl/core.h#L83

So, I think this array of params will always be okay in Rust?

Copy link
Contributor Author

@swenson swenson Aug 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thinking about this more, this presents a problem I don't see elsewhere in this code base: this conflates a &OsslParam with an array of OsslParam, which to C is the same, but in Rust is a very different structure.

So, while this code is fine as-is if we assume the &OsslParam comes from OsslParamBuilder, which will always correctly build an array of OsslParams.

Currently there is no other way to construct an OsslParam since it has a private tuple element, so someone could not, in safe Rust, construct an OsslParam without going through OsslParamBuilder.

However, there are some safety concerns with the to_param method -- while it says it is returning a Result<OsslParam, ErrorStack>, in reality it is returning Result<&mut [OsslParam; ?], ErrorStack, which we are then calling from_ptr on the first element of.

I think this is problematic if the Rust compiler decides to move the returned OsslParam, since it won't know that it is secretly an array. We could maybe solve this with Pin to prevent the compiler from moving the OsslParam, but that might be papering over the problem.

I don't think the current foreign_types crate is capable of encapsulating these semantics.

So, I see a few options here, but I'm open to suggestions:

  1. Make these pub(crate) only so that they can't be used by users directly, but only for our convenience when we can be sure of their safety, possibly when combined with
  2. Changing basically everything to unsafe so that we have to always think about our usage of this array type
  3. Something something Pin? I feel like this doesn't really solve the underlying problem
  4. Deleting this code since it isn't really safe Rust.
  5. Converting the result of to_param to a proper Vec<OsslParam> with an OsslParam that is safe and sound, and doing a just-in-time conversion of that to an *OsslParam (array) pointer that has an element at the end with a null key when we call locate(). This is safe, but it could be problematic if the underlying OpenSSL C code ever decides in a future version to change how the array is terminated.
  6. Try to wrangle a foreign_type! macro that lets us make some sense of this array / reference mangling.

I'm happy to move forward with whichever path you think makes the most sense, or if we have some precedent I don't know about, I'm happy to follow that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seeing how the original PR uses these makes me even more worried, since the compiler could be moving these arrays around, e.g.,

let mut params: *mut ffi::OSSL_PARAM = ptr::null_mut();
cvt(ffi::EVP_PKEY_todata(pkey.as_ptr(), selection, &mut params))?;
Ok(PKeyMlDsaParams::<T> {
    params: OsslParam::from_ptr(params),
    _m: ::std::marker::PhantomData,
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option: we don’t have a bare OsslParam but instead have a wrapper struct that only has *OsslParam, so that the compiler doesn’t “know” about the bare array and try to move it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind -- I realize now my understanding foreign_types was incorrect -- it isn't type OsslParam = OSSL_PARAM, but more struct OsslParam(*OSSL_PARAM), so it is already wrapping a pointer.

I think after all, the code as-is is probably sound. Sorry for the PR noise as I thought out loud and please correct me if I'm wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fundamentally, I don't think it's correct for us to confuse OSSL_PARAM * (pointer to a single OSSL_PARAM) with OSSL_PARAM * (pointer to an array of them). This makes the invariants incredibly fragile -- we need distinct types for representing a single OSSL_PARAM and representing an array of them. No other place in rust-openssl treats them this way, and it means we get things like destructors wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Obviously this bothers me too. Let me think about this a little to come up with a way to represent this. (Open to suggestions.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed this to OsslParamArray to alleviate some of the confusion -- these params are always allocated as an array, and OSSL_PARAM_free, which we call in our Drop, will correctly free the entire array.

I think this is correct now. I added clarifying comments as well.

key.as_ptr() as *const c_char,
))?;
Ok(OsslParamRef::from_ptr(param))
}
}

/// Get `BigNum` from the current `OsslParam`
#[corresponds(OSSL_PARAM_get_BN)]
pub fn get_bn(&self) -> Result<BigNum, ErrorStack> {
unsafe {
let mut bn: *mut ffi::BIGNUM = ptr::null_mut();
cvt(ffi::OSSL_PARAM_get_BN(self.as_ptr(), &mut bn))?;
Ok(BigNum::from_ptr(bn))
}
}

/// Get `&str` from the current `OsslParam`
#[corresponds(OSSL_PARAM_get_utf8_string)]
pub fn get_utf8_string(&self) -> Result<&str, ErrorStack> {
unsafe {
let mut val: *const c_char = ptr::null_mut();
cvt(ffi::OSSL_PARAM_get_utf8_string_ptr(self.as_ptr(), &mut val))?;
Ok(CStr::from_ptr(val).to_str().unwrap())
}
}

/// Get octet string (as `&[u8]) from the current `OsslParam`
#[corresponds(OSSL_PARAM_get_octet_string)]
pub fn get_octet_string(&self) -> Result<&[u8], ErrorStack> {
unsafe {
let mut val: *const c_void = ptr::null_mut();
let mut val_len: usize = 0;
cvt(ffi::OSSL_PARAM_get_octet_string_ptr(
self.as_ptr(),
&mut val,
&mut val_len,
))?;
Ok(util::from_raw_parts(val as *const u8, val_len))
}
}
}

foreign_type_and_impl_send_sync! {
type CType = ffi::OSSL_PARAM_BLD;
fn drop = ffi::OSSL_PARAM_BLD_free;

/// Builder used to construct `OsslParam`.
pub struct OsslParamBuilder;
/// Reference to `OsslParamBuilder`.
pub struct OsslParamBuilderRef;
}

impl OsslParamBuilder {
/// Returns a builder for a OsslParam arrays.
///
/// The array is initially empty.
#[corresponds(OSSL_PARAM_BLD_new)]
pub fn new() -> Result<OsslParamBuilder, ErrorStack> {
unsafe {
ffi::init();

cvt_p(ffi::OSSL_PARAM_BLD_new()).map(OsslParamBuilder)
}
}

/// Constructs the `OsslParam`.
#[corresponds(OSSL_PARAM_BLD_to_param)]
pub fn to_param(&self) -> Result<OsslParam, ErrorStack> {
unsafe {
let params = cvt_p(ffi::OSSL_PARAM_BLD_to_param(self.0))?;
Ok(OsslParam::from_ptr(params))
}
}
}

impl OsslParamBuilderRef {
/// Adds a `BigNum` to `OsslParamBuilder`.
///
/// Note, that both key and bn need to exist until the `to_param` is called!
#[corresponds(OSSL_PARAM_BLD_push_BN)]
pub fn add_bn(&self, key: &[u8], bn: &BigNumRef) -> Result<(), ErrorStack> {
unsafe {
cvt(ffi::OSSL_PARAM_BLD_push_BN(
self.as_ptr(),
key.as_ptr() as *const c_char,
bn.as_ptr(),
))
.map(|_| ())
}
}

/// Adds a utf8 string to `OsslParamBuilder`.
///
/// Note, that both `key` and `buf` need to exist until the `to_param` is called!
#[corresponds(OSSL_PARAM_BLD_push_utf8_string)]
pub fn add_utf8_string(&self, key: &[u8], buf: &str) -> Result<(), ErrorStack> {
unsafe {
cvt(ffi::OSSL_PARAM_BLD_push_utf8_string(
self.as_ptr(),
key.as_ptr() as *const c_char,
buf.as_ptr() as *const c_char,
buf.len(),
))
.map(|_| ())
}
}

/// Adds a octet string to `OsslParamBuilder`.
///
/// Note, that both `key` and `buf` need to exist until the `to_param` is called!
#[corresponds(OSSL_PARAM_BLD_push_octet_string)]
pub fn add_octet_string(&self, key: &[u8], buf: &[u8]) -> Result<(), ErrorStack> {
unsafe {
cvt(ffi::OSSL_PARAM_BLD_push_octet_string(
self.as_ptr(),
key.as_ptr() as *const c_char,
buf.as_ptr() as *const c_void,
buf.len(),
))
.map(|_| ())
}
}

/// Adds a unsigned int to `OsslParamBuilder`.
///
/// Note, that both `key` and `buf` need to exist until the `to_param` is called!
#[corresponds(OSSL_PARAM_BLD_push_uint)]
pub fn add_uint(&self, key: &[u8], val: u32) -> Result<(), ErrorStack> {
unsafe {
cvt(ffi::OSSL_PARAM_BLD_push_uint(
self.as_ptr(),
key.as_ptr() as *const c_char,
val as c_uint,
))
.map(|_| ())
}
}
}
Loading