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

Improve AES encryption logic #37

Merged
merged 3 commits into from Apr 24, 2016
Merged

Improve AES encryption logic #37

merged 3 commits into from Apr 24, 2016

Conversation

henrinormak
Copy link
Owner

Two big changes, making sure initialisation vector is not null, indeed using random bytes that are stored along with the encrypted AES key.

Second, make sure the AES key size is determined correctly, also leaving room for the included IV in the first block.

Two big changes, making sure initialisation vector is not null, indeed using random bytes that are stored along with the encrypted AES key.

Second, make sure the AES key size is determined correctly, also leaving room for the included IV in the first block.
@henrinormak
Copy link
Owner Author

This is not backwards compatible, meaning it will require bumping version to 1.0.0 to signify the fact. Too bad, as the reason for it is a 🐛, but it is the correct thing to do.

@soyersoyer
Copy link

If you check decryptedMetadataLength, and it is less than the keysize+ivsize, you can use a null IV.
It mades it partial backward compatible (it will work when the rsa key size is the default 2048bit or bigger)

The keySize function is not completely correct, because the available size depends on the padding too.
But I don't think somebody ever want to use it with a key which is less than 2048 bit.

But if you don't want any backward compatibility please consider changing the rsa padding to OAEP, because it is recommended for new applications.

@henrinormak
Copy link
Owner Author

That sounds like a reasonable plan (I mean taking the chance and switching over to OAEP), I don't particularly like the idea of having special cases for backwards compatibility (especially if it doesn't cover all of them). I will look into changing the padding (as far as I managed to read into it, it should be just about switching two enums in encrypt/decrypt).

@henrinormak
Copy link
Owner Author

Crazy typo in the last commit message, mind is totally somewhere else. Anyhow, I will likely just merge this as is and then bump version to 1.0.0

EDIT: Related to keySize, I will look into what the padding would amount to and make sure that is taken into account, sounds like something that should be done.

Take padding into account
@henrinormak
Copy link
Owner Author

I adjusted the keySize calculation to take the padding into account. Could you go over it once again @soyersoyer? Thanks a bunch!

@soyersoyer
Copy link

👍

@henrinormak henrinormak merged commit 7c29716 into master Apr 24, 2016
@henrinormak henrinormak deleted the improved-aes-encryption branch April 24, 2016 18:54
@henrinormak
Copy link
Owner Author

Submitted 1.0.0 to CocoaPods and released on GitHub. Thanks a bunch for your help!

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.

None yet

2 participants