Skip to content
This repository was archived by the owner on Oct 17, 2022. It is now read-only.

crypto: zeroize private key in BLS12377#850

Merged
joyqvq merged 1 commit intomainfrom
zeroize-bls
Aug 26, 2022
Merged

crypto: zeroize private key in BLS12377#850
joyqvq merged 1 commit intomainfrom
zeroize-bls

Conversation

@joyqvq
Copy link
Contributor

@joyqvq joyqvq commented Aug 25, 2022

No description provided.

}

impl zeroize::Zeroize for BLS12377PrivateKey {
fn zeroize(&mut self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about the privkey? In BLS12_381 we zeroize that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

talked to pun about this today, in BLS12-381 it is automatically dropped without explicitly drop here. in fact i commented out https://github.com/MystenLabs/fastcrypto/blob/main/src/bls12381.rs#L618 and all tests still passed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a comment here, this is done by the library under the hood, the test suggested this. in addition, the SecertKey in BLS12377 library prohibits a mutable reference, there is actually no easy to explicitly call zeroize on the object.

Copy link
Contributor

@kchalkias kchalkias left a comment

Choose a reason for hiding this comment

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

LGTM


impl zeroize::Zeroize for BLS12377PrivateKey {
fn zeroize(&mut self) {
// PrivateKey.zeroize here is not necessary here because the underlying implicitly zeroizes.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: using the word here twice

}
let sk_memory: &[u8] =
unsafe { ::std::slice::from_raw_parts(bytes_ptr, CELO_BLS_PRIVATE_KEY_LENGTH) };
// Assert that this is equal to sk_bytes before deletion
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer adding period (.) on every comment

@joyqvq joyqvq enabled auto-merge (squash) August 26, 2022 16:04
@joyqvq joyqvq merged commit 58e6954 into main Aug 26, 2022
@joyqvq joyqvq deleted the zeroize-bls branch August 26, 2022 16:09
mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants