Skip to content

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

Open
evnik wants to merge 3 commits into
krzyzanowskim:mainfrom
evnik:randomIV
Open

evnik wants to merge 3 commits into
krzyzanowskim:mainfrom
evnik:randomIV

Conversation

@evnik

@evnik evnik commented Oct 4, 2021

Copy link
Copy Markdown
Contributor

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

evnik commented Oct 20, 2021

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

pronebird commented Jun 3, 2024 •

Copy link
Copy Markdown

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

This branch has not been deployed

No deployments
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