The following code compiles without error or warning: <div class="highlight highli

RSA OAEP interface is unsound about rust-cryptoki HOT 6 CLOSED

parallaxsecond commented on June 14, 2024

RSA OAEP interface is unsound

from rust-cryptoki.

Comments (6)

ionut-arm commented on June 14, 2024 1

Hey, thanks for reporting this! I agree, it's definitely a problem, don't know how we hadn't noticed it before.

First things first I'd mark the struct as #[deprecated]

Don't know if that's necessary, really - if we break/change the interface we can just bump the version number appropriately.

I also agree about the assessment - ideally I would've liked us to take ownership of that data and generate our own pointer from it when necessary, but that might be inefficient. However, if we do go for a reference, the new lifetime parameter isn't the only problem with that approach - we would then be inconsistent with a lot of other mechanisms which take ownership of buffers. Perhaps we could switch to references everywhere, or at least in places with "unbounded" buffers.

The Mechanism struct is probably a low-impact place to have lifetimes because I wouldn't expect them to be persisted between calls/operations - but perhaps I'm wrong on that. Overall, I'd lean towards using a reference.

Thoughts?

from rust-cryptoki.

wiktor-k commented on June 14, 2024

Okay, this is definitely a problem. First things first I'd mark the struct as #[deprecated] linking to this issue to give people using it a heads-up that it's going to change.

Yeah, the interface looks half-low-level half-high level. I'm wondering how much code will need to change for the "lifetime parameter" scenario although I'm OK with the "own source data" too.

@hug-dev, @ionut-arm, @gowthamsk-arm what do you think?

from rust-cryptoki.

jhagborgftx commented on June 14, 2024

On Thu, 2022-11-24 at 09:44 -0800, Ionuț Mihalcea wrote: I also agree about the assessment - ideally I would've liked us to take ownership of that data and generate our own pointer from it when necessary, but that might be inefficient. However, if we do go for a reference, the new lifetime parameter isn't the only problem with that approach - we would then be inconsistent with a lot of other mechanisms which take ownership of buffers. Perhaps we could switch to references everywhere, or at least in places with "unbounded" buffers.

The only data we actaully own already are small, fixed-sized structs: - PkcsPssParams - PkcsOaepParams - Ecdh1DeriveParams I think these should stay owned! There's nothing to be gained by keeping references to these structs. However, PkcsOaepParams and Ecdh1DeriveParams both contain raw pointers to variable-length data. The closest safe approximation to this is a reference to a slice. For the sake of consistency, we may as well discuss how we'd handle some other (not yet supported) mechanisms: * AES-GCM takes a struct with pointers to IV and AAD. These can both be arbitrarily long (up to 2^32 - 1 bytes). It would make sense for these to be slices as well. * AES-CCM takes a similar struct with variable-length nonce and AAD. Here though, the nonce can only be up to 13 bytes long, so owning vs. borrowing is just a question of ergonomics, not efficiency. I'd still borrow a slice for consistency and simplicity. * {AES,DES3}-{CBC,OFB} takes a block-sized IV as a parameter directly, not via a parameter struct. I think just passing an owned [u8; N] around is fine here. I like enforcing the length at the type level and &[u8; N] is a little odd with no benefit (since N is small). * Key derivation by encryption mechanisms all seem to take their fixed-sized IV's embedded directly in the structs, *not* via reference. So owned [u8; N] seems the way to go here. With this in mind, I'd propose the following rule: A `Mechanism` owns its parameter, whether that's a structure or a fixed-sized buffer. If that structure contains pointers and lengths, replace them with references to slices. If that structure has embedded arrays, keep that as [u8; N]. This is consistent with all currenlty supported mechanisms. This assumes no mechanisms take variable-length buffers as their parameter directly. A quick scan through < https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/os/pkcs11-curr-v3.0-os.html

seems to confirm that is the case. If one does pop up, I wouldn't

feel bad about making an exception for it.

The Mechanism struct is probably a low-impact place to have lifetimes because I wouldn't expect them to be persisted between calls/operations - but perhaps I'm wrong on that. Overall, I'd lean towards using a reference.

That's definately true for the code I'm working on -- mechanisms only live for one or two operations each, always within a single function. In any case, if someone is persisting `Mechanism`s today, they're either fine with using a `'static` lifetime, or are already doing some reasoning about raw pointers that could be helped by the lifetime.

…

-- James Hagborg

from rust-cryptoki.

ionut-arm commented on June 14, 2024

Hey, thanks for the reply!

AES-GCM takes a struct with pointers to IV and AAD. These can both
be arbitrarily long (up to 2^32 - 1 bytes). It would make sense for
these to be slices as well.

I don't think that's true about IV in version 2.40 of the spec:

ulIvLen length of initialization vector in bytes. The length of the initialization vector can be any number between 1 and 256. 96-bit (12 byte) IV values can be processed more efficiently, so that length is recommended for situations in which efficiency is critical.

Though looking at v3, I can see where you got the value from (see comment below about versioning). It seems fairly reasonable to treat IVs as fairly limited in size and thus take full ownership of them, whereas for AAD or other large, variable-length structures we could just take a reference and use that.

One thing I noticed is that you seem to be referring to version 3 of the spec:

A quick scan through https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/os/pkcs11-curr-v3.0-os.html

While we use version 2.40. A peek through our READMEs made me realize we don't explicitly advertise this! So we definitely need to make it more clear. Did you want to move to v3.0 of the spec? It seems like a fairly big jump, not sure how to handle it easily - perhaps gating it behind a Cargo feature - but that's a separate discussion.

Anyway, your approach in the open PR seems pretty good to me, happy to go down that route.

from rust-cryptoki.

jhagborgftx commented on June 14, 2024

On Mon, 2022-12-05 at 03:54 -0800, Ionuț Mihalcea wrote: Hey, thanks for the reply! > AES-GCM takes a struct with pointers to IV and AAD. These can both > be arbitrarily long (up to 2^32 - 1 bytes). It would make sense for > these to be slices as well. > I don't think that's true about IV in version 2.40 of the spec: > ulIvLen length of initialization vector in bytes. The length of the > initialization vector can be any number between 1 and 256. 96-bit > (12 byte) IV values can be processed more efficiently, so that > length is recommended for situations in which efficiency is > critical. > Though looking at v3, I can see where you got the value from (see comment below about versioning). It seems fairly reasonable to treat IVs as fairly limited in size and thus take full ownership of them, whereas for AAD or other large, variable-length structures we could just take a reference and use that.

That works, but I still think we should use a slice because * The C struct contains a pointer and length, so we can easily make our parameter struct #[repr(C)] and use it directly. With a Vec it's slightly more complex, since we need to produce a second struct on the fly, and then *also* take a pointer to that struct. This is possible, but means we can no longer have a From<&Mechanism> instance for CK_MECHANISM. * It's consistent with every other place (so far) where there is variable-length data in a mechanism parameter. * Some day in the future, we will want to use v3, so we'll need to switch to a slice anyway. After implementing a few more mechanisms, I'm really liking the "match what the C struct does" approach of either using &[u8] or [u8; N] everywhere.

One thing I noticed is that you seem to be referring to version 3 of the spec: > A quick scan through > https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/os/pkcs11-curr-v3.0-os.html > While we use version 2.40. A peek through our READMEs made me realize we don't explicitly advertise this! So we definitely need to make it more clear. Did you want to move to v3.0 of the spec? It seems like a fairly big jump, not sure how to handle it easily - perhaps gating it behind a Cargo feature - but that's a separate discussion.

My bad! I'm not too familiar with what's changed between them, I've just been using the 3.0 spec for reference, assuming it was essentially 2.4 + features - depreciations.

from rust-cryptoki.

jhagborgftx commented on June 14, 2024

Ok, after a bit more thinking, I think there is another issue with these two mechanisms. We define a #[repr(C)] struct manually, which means we get the padding wrong on Windows! We would avoid this if we used #[repr(transparent)] over the corresponding structs in cryptoki- sys. That's the approach I'm using for AEAD, and I'll update this PR to matche.

from rust-cryptoki.

RSA OAEP interface is unsound about rust-cryptoki HOT 6 CLOSED

Comments (6)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent