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

feat: Add gcm algorithm for cross platform encryption/decryption #33

Merged
merged 35 commits into from
Feb 25, 2019

Conversation

Andrew-Lees11
Copy link
Contributor

Description

This pull request adds a new algorithm case called gcm.
This will use SHA1 for signing/verifying.
For encryption/decryption it will use:

  • EVP_aes_128_gcm
  • 16 Byte 0 IV
  • the RSA key modulus and publicExponent in an ASN1 sequence as AAD

This makes it compatible with the Apple SecKeyCreateEncryptedData/SecKeyCreateDecryptedData.
As a result you will be able to encrypt on an apple device and decrypt on linux using this algorithm.

The travis builds will fail until a new patch of openSSL 1 and 2 with EVP_CIPHER_CTX_init_wrapper are released

Motivation and Context

This is a fix for issue #32 to allow cross platform RSA encryption.

How Has This Been Tested?

The gcm algorithm has been added to the current tests.
Two new tests have also been added using previously encrypted data from linux and mac to test cross platform support.

Copy link

@kilnerm kilnerm left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me.

Would be nice to remove the white space where possible.

// Apple use a 16 byte all 0 IV. This is allowed since a random key is generated for each encryption.
let iv = [UInt8](repeating: 0, count: 16)

// TODO: change EVP_aes_128_gcm to EVP_aes_256_gcm for rsaKey > 4096 bits to match apple.
Copy link

Choose a reason for hiding this comment

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

Are we doing this in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added in the code to choose algorithm based on key size so that this supports RSA keys >= 4096 bytes. I have also added a test to cover this case.

@@ -80,7 +80,7 @@ class CryptorRSATests: XCTestCase {

let path = CryptorRSATests.getFilePath(for: "public", ofType: "der")
XCTAssertNotNil(path)
Copy link

Choose a reason for hiding this comment

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

Can this white space (and the rest of the similar lines) just be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BlueRSA uses a mix of both tabs and spaces through the project (Even switching within the same function) which caused lots of white space changes. I have removed most of those and will pick up the last few here.
I won't change the sha algorithms in the tests since they have varying amounts of tabs and then spaces within the same line and i can't bring myself to replicate this.

@@ -222,7 +222,9 @@ public class CryptorRSA {
}

#if os(Linux)

if algorithm == .gcm {
Copy link
Contributor

Choose a reason for hiding this comment

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

This special-casing of GCM seems a bit hacky... Would it be better to switch algorithm { } and call a function depending on the result? In other words, have encryptedSHA(with: key) as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The switch would be:

switch algorithm {
    case .gcm:
         return try encryptedGCM(with: key)
    default:
         return try encryptedSHA(with: key)
}

Which seems a bit strange for a switch statement.
I could make the SHA a separate function and do an if else statement.

Alternatively we could just make encryptedGCM public as it's own function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would list all the SHA cases explicitly rather than using default: but yes this is what I'm suggesting. This function is called encrypted() - now that it does more than just SHA I don't see why all the SHA code belongs in it?

@Andrew-Lees11 Andrew-Lees11 merged commit dc6f4fa into master Feb 25, 2019
@djones6 djones6 deleted the issue.GCM branch February 26, 2019 14:12
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