-
Notifications
You must be signed in to change notification settings - Fork 157
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
Conversation
src/lib.rs
Outdated
extern crate digest; | ||
extern crate sha1; | ||
extern crate sha2; | ||
extern crate sha3; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
Thank you for implementing this! I don't know when I will find time for a more detailed review, but left some immediate comments |
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. |
5842b75
to
9f1464c
Compare
I forced pushed since I leaked some email addresses... Also for the last commit, I :
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). |
Thank you, I will try and take this to push the rest of the integration through as soon as I find some time. |
Is there anything I could help with to move this over the line? |
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". |
Test assertion is manual and done with openssl
Might change the way oaep options are passed, current version is not convenient keys not fully implemented for oaep
Modify oaep methods signatures
Thanks for this PR, it's working great so far in my local code! One annoyance: Using
|
I have integrated this into the existing api in #43 |
closing in favor of #43 |
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: