Skip to content

PkcsOaepParams mis-aligned #266

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

Jump to bottom
Open
ivozeba opened this issue May 2, 2025 · 6 comments · May be fixed by #272
Open

PkcsOaepParams mis-aligned #266

ivozeba opened this issue May 2, 2025 · 6 comments · May be fixed by #272

Comments

@ivozeba
Copy link

ivozeba commented May 2, 2025 •
edited
Loading

I was looking at why RSA PKCS OAEP fails with data and while debugging on Windows I noticed that CK_RSA_PKCS_OAEP_PARAMS should be 24 bytes, while checking std::mem::size_of::<PkcsOaepParams>() it was returning 32.

The struct is defined as follows:

#[derive(Copy, Debug, Clone)]
#[repr(C)]
pub struct PkcsOaepParams<'a> {
    /// mechanism ID of the message digest algorithm used to calculate the digest of the encoding
    /// parameter
    hash_alg: MechanismType,
    /// mask generation function to use on the encoded block
    mgf: PkcsMgfType,
    /// source of the encoding parameter
    source: cryptoki_sys::CK_RSA_PKCS_OAEP_SOURCE_TYPE,
    /// data used as the input for the encoding parameter source
    source_data: *const c_void,
    /// length of the encoding parameter source input
    source_data_len: Ulong,
    /// marker type to ensure we don't outlive the source_data
    _marker: PhantomData<&'a [u8]>,
}

After a closer look it turns out that without repr(C) it's returning 24, meaning that it's being aligned to the next 8 bytes (green is expected while red is what appears to be happening):

Image

Adding packed (#[repr(C, packed)]) appears to remove the inter-field padding and the rsa_pkcs_oaep_with_data test passes.

I need to test on linux but would welcome any thoughts :).
@ivozeba
Copy link
Author

ivozeba commented May 2, 2025

I tested on softhsm and it still fails, I quickly subbed in bouncyHSM for a quick sanity check and the test passes on both Windows 11 and Linux.

This is sadly a side-effect of what's discussed in #70.

@wiktor-k
Copy link
Collaborator

wiktor-k commented May 5, 2025

This is sadly a side-effect of what's discussed in #70.

Nice detective work! Should we close this ticket as a duplicate of #70? It'd be nice to make it actionable... somehow 🤔

@ivozeba
Copy link
Author

ivozeba commented May 5, 2025

Given the circumstances I think it may be tough to action in it's entirety which I think #70 is attempting to do. From the looks of it (reading that issue as well as scouring SoftHSM), it appears Windows is the only one guaranteed to do struct packing.

Would it be wise to conditionally do it on Windows only? I'd appreciate the sanity check.

If that is sound, the next question would be how to accomplish that.

ivozeba added a commit to ivozeba/rust-cryptoki that referenced this issue May 9, 2025
@ivozeba
parallaxsecond#266: Added conditional attribute to pack the PkcsOaepP…
1d52d25 
…arams struct on windows only.

Signed-off-by: Ivo Zeba <[email protected]>
@ivozeba ivozeba linked a pull request May 9, 2025 that will close this issue
Open
@hug-dev
Copy link
Member

hug-dev commented May 12, 2025

Would it be wise to conditionally do it on Windows only?

From the conclusion of the other issue is seems that we could try packing all C structures on windows only (like you did in the PR but for all structs)?

@wiktor-k
Copy link
Collaborator

From the conclusion of the other issue is seems that we could try packing all C structures on windows only (like you did in the PR but for all structs)?

I think it's a great approach. Too bad we don't have any Windows CI runners :-/

@hug-dev
Copy link
Member

hug-dev commented May 12, 2025

Too bad we don't have any Windows CI runners :-/

ah right! Maybe we can start by asking a kind soul testing all on Windows 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

#266: Added conditional attribute to pack the PkcsOaepParams struct o… ivozeba/rust-cryptoki
3 participants
@wiktor-k @hug-dev @ivozeba