Page MenuHomePhabricator

[native][AES] Implement encrypt function on iOS
ClosedPublic

Authored by bartek on Wed, Mar 8, 10:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 11, 7:50 AM
Unknown Object (File)
Sat, Mar 11, 7:49 AM
Unknown Object (File)
Fri, Mar 10, 10:22 PM

Details

Summary
Test Plan
import * as AES from './utils/aes-crypto-module.js';

const key = AES.generateKey();
const plaintextBytes = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]);
const ciphertext = AES.encrypt(key, plaintextBytes);
console.log('Ciphertext', ciphertext);
// ensured that ciphertext is 12+10+16=38 bytes long (ciphertext.length==38)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Wed, Mar 8, 10:39 AM
atul requested changes to this revision.Thu, Mar 9, 12:40 PM
atul added a subscriber: anunay.

Some questions inline. Definitely feel free to re-request review if there's something I'm missing.

Adding @anunay as a non-blocking reviewer just so it's on his radar

native/expo-modules/aes-crypto/ios/AESCryptoModule.swift
4 ↗(On Diff #23540)

Maybe a comment above writing out INITIALIZATION_VECTOR?

"IV" might be a standard acronym in cryptography (eg HMAC), but I wasn't previously familiar with it.

32 ↗(On Diff #23540)

Since we're initializing the destination buffer via:

new Uint8Array(data.length + IV_LENGTH + TAG_LENGTH);

Should we just do == instead of >=? Let me know if there's something I'm missing here.

44–48 ↗(On Diff #23540)

It's a little confusing that we're concatenating iv and tag to ciphertext and calling the destination buffer ciphertext on the JS side. Should we maybe use like seal(...) and sealedData naming instead of encrypt(...) and ciphertext so it's a little less ambiguous?

Maybe a nit/pedantic, but figure we should be as precise as possible with anything cryptography related?

native/utils/aes-crypto-module.js
27 ↗(On Diff #23540)

Are we sure that this array is always going to be the right size buffer? Is it guaranteed that the result from AESCryptoModule.encrypt(...) will strictly be equal to data.length + IV_LENGTH + TAG_LENGTH?

For example, if data.length was size 1, is there no padding added in AES.GCM.seal(...) to increase the size to the the nearest N bytes or whatever?

Might be worth adding an additional check after

guard let ciphertext = encryptionResult.combined

to ensure that ciphertext.length == destination.length?

29 ↗(On Diff #23540)

Mentioned earlier that the encrypt/ciphertext terminology might be a bit confusing.

At a minimum, I think it's confusing that just in this diff ciphertext does not always refer to "the same thing."

Wonder if eg seal(...) and sealedData might be a more precise naming scheme? Since in addition to encryption there's an authentication/integrity tag involved in encrypt() and the ciphertext here contains iv and tag.

This revision now requires changes to proceed.Thu, Mar 9, 12:40 PM
native/expo-modules/aes-crypto/ios/AESCryptoModule.swift
32 ↗(On Diff #23540)

Technically it is important for this array to be just larger to avoid memory errors, but you're right, the == can be used for extra safety.

44–48 ↗(On Diff #23540)

I totally agree that all names related to ciphertext are confusing.
We have three variations across codebase:

  • ciphertext content only (without IV and Tag)
  • ciphertext with tag appended (without IV)
  • combined iv || ciphertext content || tag

I admit I started mixing names because I have no idea how to name them without having extra long/verbose variable names like ciphertextWithTag

native/utils/aes-crypto-module.js
27 ↗(On Diff #23540)

An extra check wouldn't hurt

29 ↗(On Diff #23540)

Replied above

native/expo-modules/aes-crypto/ios/AESCryptoModule.swift
44–48 ↗(On Diff #23540)

Personally would totally err on the side of verbosity for anything having to do with cryptography since any confusion or lack of precision could be "devastating" and a couple extra characters doesn't hurt anyone.

Personally think
ciphertext = just the ciphertext nothing appended or prepended ever
sealedData = iv || ciphertext || tag
ciphertextWithTagSuffix (or something like that) = ciphertext || tag

or something like that might make sense? Could add comments above like sealedData where it's defined to make it super clear what it contains? It's kind of difficult because we're just dealing with a "bunch of bytes" instead of like structs with fields in them.

Regarding all naming issues (applies to the whole stack), I've created a Notion doc that clarifies the names

native/utils/aes-crypto-module.js
27 ↗(On Diff #23540)

For example, if data.length was size 1, is there no padding added in AES.GCM.seal(...) to increase the size to the the nearest N bytes or whatever?

In theory, GCM mode is bit-based and can encrypt even 0 bits: https://crypto.stackexchange.com/a/40732 so this shouldn't happen. Moreover, we will always use multiples of 10KB (padding)

Address code review feedback

Regarding all naming issues (applies to the whole stack), I've created a Notion doc that clarifies the names

Thanks for taking the time to do this, looks much clearer.

native/utils/aes-crypto-module.js
27 ↗(On Diff #23540)

Gotcha, thanks for the explanation + link with further info

This revision is now accepted and ready to land.Mon, Mar 13, 4:56 PM