Skip to content
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.

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

Add OAEP Encryption & Decryption #18

Closed
wants to merge 9 commits into from
Closed

Conversation

lucdew
Copy link
Contributor

@lucdew lucdew commented Apr 3, 2019

Add OAEP encryption and decryption to RSA.
Go's crypto module was a source of inspiration.

Since I am a Rustlang newbie, it probably cannot be merged as it is.

Here are important changes:

  1. An oaep module has been added and made public (see 3 for the reason)
  2. Hashes enum now has a digest function for most of enum's elements. Thus the import of sha-1,sha2 and sha3 crates
  3. Keys now supports oaep encrypt/decrypt but only with default options (options cannot be changed) which are a sha1 digest and empty label
  4. OAEP encode/decode options are the hash function (well Hashes instance) and a label. The hash function for the label and OAEP mask generation function cannot be chosen independently

src/lib.rs Outdated
extern crate digest;
extern crate sha1;
extern crate sha2;
extern crate sha3;
Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be need for this with the 2018 edition.

mgf1_xor(seed, &h, db);

{
let mut m = BigUint::from_bytes_be(&em);
Copy link
Contributor

@phayes phayes Apr 3, 2019

Choose a reason for hiding this comment

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

Can you comment on if there's a side-channel vulnerability here? Is BigUint::from_bytes_be constant-time? If it's not, could it leak sensitive info via timing side-channel?

Copy link
Member

Choose a reason for hiding this comment

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

I am pretty certain from_bytes_be is not constant time, but I think we do this in other places as well, so likely needs some more analysis as to what can leak, and how to fix it.

@@ -21,6 +21,10 @@ rand = "0.6.5"
byteorder = "1.3.1"
failure = "0.1.5"
subtle = "2.0.0"
sha-1 = "0.8.1"
Copy link
Member

Choose a reason for hiding this comment

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

I would really like to avoid these dependencies, especially as I want to change signing to be abstract over Digest. See RustCrypto/signatures#7 for some details on the plans there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I agree that depending on every hashing crates is not a good idea. The digest implementation should be passed. I tried that in the first place but failed, fighting the Rust compiler due to my limited Rust experience, I don't remember the exact reason...
I'll see what I can do (see my other comment on my commitment)

@dignifiedquire
Copy link
Member

Thank you for implementing this! I don't know when I will find time for a more detailed review, but left some immediate comments

@lucdew
Copy link
Contributor Author

lucdew commented Apr 3, 2019

Anyway I am not sure I will have time to work on it anymore. so if you don't have any news from me within a week feel free to close/discard the pull request.

@lucdew lucdew force-pushed the oaep branch 2 times, most recently from 5842b75 to 9f1464c Compare April 4, 2019 12:01
@lucdew
Copy link
Contributor Author

lucdew commented Apr 4, 2019

I forced pushed since I leaked some email addresses...

Also for the last commit, I :

  • removed the sha-1,sha2,sha3 dependencies (moved them to dev dependencies)

  • updated oaep to take a DynDigest borrow as argument

  • reverted changes in key and hash.
    key encrypt/decrypt methods signatures need to updated to support OAEP

Therefore in the current state it can only be used through the oaep module directly. Maybe it could be used as a starting point for OAEP support. Anyway, I'll stop working on it. I'll use my fork meanwhile.

I was interested to have a Rust only RSA implementation with OAEP to build a A WASM POC (I know WebCryptoAPI has RSA OAEP support).

@dignifiedquire
Copy link
Member

Thank you, I will try and take this to push the rest of the integration through as soon as I find some time.

@str4d str4d mentioned this pull request Oct 13, 2019
@str4d
Copy link
Contributor

str4d commented Oct 22, 2019

Is there anything I could help with to move this over the line?

@dignifiedquire
Copy link
Member

@str4d figuring out how to move the current api to sth that supports this and #26 is the main blocker

@str4d str4d mentioned this pull request Jan 2, 2020
@mehcode
Copy link

mehcode commented Jan 12, 2020

As a data point, I've copied this PR into SQLx and it works great.

I know we're waiting on the API refactor / redesign but I just wanted to give some context of "this works great in the field".

@TimNN
Copy link

TimNN commented Feb 16, 2020

Thanks for this PR, it's working great so far in my local code!

One annoyance: Using decrypt without a random number generator cumbersone:

  1. I have to add an explicit dependency on rand to my project.
  2. I have to create a let dummy: Option<&mut rand::rngs::StdRng> = None; for the first argument of decrypt, because Rust currently doesn't allow explicitly specifying type arguments if impl Trait is present in argument position.

@dignifiedquire
Copy link
Member

I have integrated this into the existing api in #43

@dignifiedquire
Copy link
Member

closing in favor of #43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants