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

crypto-common: test for weak keys / IVs #1739

Closed
wants to merge 1 commit into from

Conversation

baloo
Copy link
Member

@baloo baloo commented Jan 29, 2025

Fixes #1738

Example usage: RustCrypto/block-ciphers#465

@newpavlov
Copy link
Member

Maybe it's better to move it to the KeyInit/KeyIvInit traits? It will mean a bit of code duplication, but it looks like a more logical place for this method.

@baloo
Copy link
Member Author

baloo commented Jan 30, 2025

I'm not sure I understand. Didn't I place it in KeyInit?
Or you want me to also duplicate it in KeyIvInit?

Are they cases where the IV on its own is problematic? (I'm only aware of it being an issue if the (key, IV) pair gets reused)

@newpavlov
Copy link
Member

newpavlov commented Jan 30, 2025

Oh, sorry, for some reason I thought it was added to KeySizeUser.

Yes, it's worth to add the same method to KeyIvInit and modify its IvInnerInit-based blanket impl accordingly.

@baloo baloo changed the title crypto-common: test for weak keys crypto-common: test for weak keys / IVs Jan 30, 2025
@baloo baloo marked this pull request as draft January 30, 2025 23:07
@baloo baloo marked this pull request as ready for review January 30, 2025 23:14
@baloo
Copy link
Member Author

baloo commented Jan 30, 2025

I'm not sure I see your point about InnerIvInit. I've changed the various generate function to check for weak keys, and otherwise re-run.

But the blanket implementations should inherit from the default implementations in the traits (KeyInit/KeyIvInit) and wouldn't need to be changed.

@newpavlov
Copy link
Member

newpavlov commented Jan 31, 2025

I meant that in the impl<T> KeyInit for T where T: InnerInit, T::Inner: KeyInit { ... } blanket impl you should use T::Inner::weak_key_test to implement T::weak_key_test.

I am not sure whether we should use the weak key test in the generate functions, since probability of generating such key is astronomically small (for existing implementors). @tarcieri WDYT?

@tarcieri
Copy link
Member

tarcieri commented Jan 31, 2025

I agree a keygen producing uniformly random keys should never encounter a weak key in practice.

A more interesting place to put it might be in a fallible init function, which could be separate from the existing ones. Not sure what to call something like that though... try_new_with_key_check? (blech, too verbose)

@newpavlov
Copy link
Member

fn new_checked(key: &Key<Self>) -> Result<Self, WeakKeyError>?

@tarcieri
Copy link
Member

Sure, sounds good

/// Check if an IV might be considered weak
fn weak_iv_test(_iv: &Iv<Self>) -> Result<(), WeakKeyError> {
Ok(())
}
Copy link
Member

@newpavlov newpavlov Jan 31, 2025

Choose a reason for hiding this comment

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

I don't think we need this method. Plus, we can add it later in a backward-compatible way if it's needed for something after all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can I ask you take over?
I'm not sure I can follow what you're asking or how you want the traits to be laid out.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I can do it a bit later. Though this PR is almost ready in my opinion. You just need to remove KeyIvInit::weak_iv_test and keep KeyIvInit::weak_key_test.

Copy link
Member

Choose a reason for hiding this comment

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

You also could allow edits by maintainers (there should be a checkbox on the right) and I can make the necessary edits.

Copy link
Member Author

Choose a reason for hiding this comment

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

Restored the branch, you now have access (somehow I thought it was on by default? but it appears not)

I didn't mean to be rude, I'm just not sure I can follow the entire logic of the Inner trait and how they are meant to integrate together, and I thought that would have been easier for you to just write the code.

Copy link
Member

Choose a reason for hiding this comment

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

No problem. I already created #1742 based on this PR.

@baloo baloo closed this Jan 31, 2025
@baloo baloo deleted the baloo/weak-key branch January 31, 2025 23:49
@baloo baloo restored the baloo/weak-key branch February 1, 2025 00:22
@baloo baloo deleted the baloo/weak-key branch February 1, 2025 01:42
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.

Trait for weak keys
3 participants