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

Use BlockCipher.blockSize as a result length in randomIV helper #893

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

evnik
Copy link
Contributor

@evnik evnik commented Oct 4, 2021

Checklist:

  • Correct file headers (see CONTRIBUTING.md) (not needed).
  • Formatted with SwiftFormat.
  • Tests added.

Changes proposed in this pull request:

Use BlockCipher.blockSize as a result length in randomIV helper. This should help to avoid two mistakes when randomIV helper is used:

  • Passing of invalid array length for IV generation.
  • Usage of randomIV helper for a key generation. Because it uses Swift.SystemRandomNumberGenerator, depending on the platform the result might be not cryptographically secure to be used as a key. Security.SecRandomCopyBytes should be preferred to generate a key on Apple platforms.

This should help to avoid two mistakes when `randomIV` helper is used:
* Passing of invalid array length for IV generation
* Usage of this method for key generation
@evnik evnik requested a review from krzyzanowskim as a code owner October 4, 2021 06:48
@evnik
Copy link
Contributor Author

evnik commented Oct 20, 2021

@krzyzanowskim any feedback for this?

public extension Cryptors where Self: BlockCipher {
/// Generates array of random bytes. `blockSize` is used as length of the result array.
/// Convenience helper that uses `Swift.SystemRandomNumberGenerator`.
static func randomIV() -> [UInt8] {
Copy link
Owner

Choose a reason for hiding this comment

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

can it be randomIV(_ count: Int = Self.blockSize) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can, but then it defeats the purpose of these changes. I'd like to make this helper hard to use in a wrong way.

Copy link
Owner

Choose a reason for hiding this comment

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

It's not, really. It keep the backward compatibility, while use the blockSize default now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I've returned randomIV(_ count: Int) helper marking it as deprecated, so existing code won't be broken after upgrade to the new version

Repository owner deleted a comment Apr 9, 2023
@pronebird
Copy link

pronebird commented Jun 3, 2024

The proposal to use SecRandomCopyBytes() seems to be sane, but I don't think it's implemented in this PR.

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.

3 participants