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

Replace the rust-crypto crate to other crates #8

Merged
merged 5 commits into from Feb 7, 2020

Conversation

@somniumism
Copy link
Contributor

somniumism commented Jan 30, 2020

The rust-crypto@2.3.6 crate that we are using now is unaudited and has not received a commit since 2016. We are still unknown whether there are any vulnerabilities offhand. But if there are ones, they will never receive fixes. Thus, we think that it is necessary to completely replace the rust-crypto crate to other crypto crates.

I replaced hash in the rust-crypto crate to other crates(sha-1, sha2, sha3, ripemd160 and digest), and blake2 in the rust-crypto crate to blake2 crate. And I'm working on replacing aes and block mode to other crates(block-modes, aes, aes-soft).

I think there are unnecessary and dirty codes, so I need your review to modify them.

@somniumism somniumism requested review from sgkim126 and HoOngEe Jan 30, 2020
@HoOngEe HoOngEe changed the title [wip] Replace the rust-crypto crate to other crates [WIP] Replace the rust-crypto crate to other crates Jan 30, 2020
@sgkim126

This comment has been minimized.

Copy link
Contributor

sgkim126 commented Jan 31, 2020

The first and second commit looks good to be merged.
How about splitting the PR?

src/blake.rs Outdated Show resolved Hide resolved
@somniumism

This comment has been minimized.

Copy link
Contributor Author

somniumism commented Jan 31, 2020

@sgkim126 For the time being, I'm waiting for @HoOngEe's review. And I think it's fine to split the PR. By the way, if there is a special reason for doing it, could you tell me the reason?

@sgkim126

This comment has been minimized.

Copy link
Contributor

sgkim126 commented Jan 31, 2020

The 4th commit seems to take more time to be implemented.
If you can implement it before long, I think splitting is not necessary.

@somniumism

This comment has been minimized.

Copy link
Contributor Author

somniumism commented Jan 31, 2020

I'll try to complete the 4th commit within today. If I can't finish it by today, I'll split it up.

@HoOngEe

This comment has been minimized.

Copy link

HoOngEe commented Jan 31, 2020

It looks good to me up to the 3rd commit. I'll review the fourth when you finish it.

@somniumism somniumism force-pushed the somniumism:Change branch 2 times, most recently from eea3b54 to bd22dd3 Feb 3, 2020
@somniumism

This comment has been minimized.

Copy link
Contributor Author

somniumism commented Feb 3, 2020

@sgkim126 @HoOngEe I replaced crates related to aes-256-cbc to other crates. I'm waiting for your review. : )

There are still two parts left to be fixed: 1) ctr mode, and 2) error handling.
To solve 1) ctr mode, I used the ctr crate. However, there seems to be no way to make encryption and decoding by dividing them into two functions. This crate is designed to allow decryption only if the object cipher = Aes128Ctr::new(&key, &nonce) is shared. The object can be created in the encryption process, and when divided into two functions, it is difficult to convey the object to the decryption function. Unfortunately, existing crates may still need to be used. How about it?

And I don't know how to solve 2) error handling. To be exact, I don't understand our error handling structure, and roles of ScryptError, ring::error::Unspecified and SymmetricCipherError. So, I need your advice.

If the third and forth commit looks good to be merged too, I'll split it up.

@somniumism somniumism requested a review from sgkim126 Feb 3, 2020
Copy link
Contributor

sgkim126 left a comment

It seems that there is no test to check whether the encrypted result has been changed.
Please add tests like

assert_eq!(H128::from("46fb7408d4f285228f4af516ea25851b"), result);
.

src/aes.rs Outdated Show resolved Hide resolved
src/aes.rs Outdated Show resolved Hide resolved
Copy link

HoOngEe left a comment

I checked the functions in ctr crate. We can implement encryption and decryption using apply_keystream method in SyncStreamCipher trait. However I still cannot understand why ctr does not provide StreamCipher methods. I think it still remains unimplemented.

@somniumism somniumism force-pushed the somniumism:Change branch from bd22dd3 to da90429 Feb 5, 2020
src/scrypt.rs Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
src/aes.rs Show resolved Hide resolved
@somniumism

This comment has been minimized.

Copy link
Contributor Author

somniumism commented Feb 5, 2020

@sgkim126 @HoOngEe I replaced crates related to the aes-128-ctr mode to other crates, and I made a commit combined two commits, replacing the 256-cbc mode and replacing the 128-ctr mode. I need your review : )

I'm working on replacing scrypt mode to other crates, and I have a problem. The function scrypt()'s output format is Result<(), InvalidOutputLen>, so it seems it needs error handling. but I don't know how to solve it.

@somniumism somniumism force-pushed the somniumism:Change branch 2 times, most recently from 1cfbe71 to f7c182e Feb 5, 2020
@somniumism somniumism changed the title [WIP] Replace the rust-crypto crate to other crates Replace the rust-crypto crate to other crates Feb 5, 2020
@somniumism

This comment has been minimized.

Copy link
Contributor Author

somniumism commented Feb 5, 2020

I replaced all modules in the rust-crypto crate to other crates. I removed completely the rust-crypto crate from Crago.toml. Therefore, from now on, we don't use the rust-crypto crate.

@sgkim126 @HoOngEe Could you review this PR?

@HoOngEe HoOngEe self-requested a review Feb 5, 2020
src/scrypt.rs Outdated Show resolved Hide resolved
@somniumism somniumism force-pushed the somniumism:Change branch from f7c182e to 195991d Feb 5, 2020
src/scrypt.rs Show resolved Hide resolved
@somniumism somniumism force-pushed the somniumism:Change branch from 195991d to 7df30e1 Feb 6, 2020
@somniumism somniumism force-pushed the somniumism:Change branch from 7df30e1 to a687270 Feb 6, 2020
src/scrypt.rs Show resolved Hide resolved
@HoOngEe HoOngEe self-requested a review Feb 6, 2020
@HoOngEe
HoOngEe approved these changes Feb 6, 2020
Copy link

HoOngEe left a comment

LGTM

@sgkim126 sgkim126 merged commit 7730690 into CodeChain-io:master Feb 7, 2020
3 checks passed
3 checks passed
unit-test (macOS-latest)
Details
unit-test (ubuntu-latest)
Details
staic-analysis
Details
@somniumism somniumism deleted the somniumism:Change branch Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.