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

Authored by bartek on Mar 8 2023, 10:06 AM.

Details

Reviewers

marcin
atul

Commits

rCOMM5789ca85981d: [native][AES] Implement encrypt function on iOS

Summary

This diff implements the encrypt() function on iOS.

Its desired behavior is described here: https://linear.app/comm/issue/ENG-388#comment-76a38890

Docs: https://developer.apple.com/documentation/cryptokit/aes/gcm/seal(_:using:nonce:)
Inspired by: https://dev.to/craftzdog/how-to-encrypt-decrypt-with-aes-gcm-using-cryptokit-in-swift-24h1

Depends on D7004

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 created this revision.Mar 8 2023, 10:06 AM

bartek held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptMar 8 2023, 10:06 AM

bartek added a child revision: D7006: [native][AES] Implement decrypt function on iOS.Mar 8 2023, 10:09 AM

bartek mentioned this in D7007: [native][AES] Implement encrypt function on Android.Mar 8 2023, 10:13 AM

Harbormaster completed remote builds in B17189: Diff 23540.Mar 8 2023, 10:21 AM

bartek published this revision for review.Mar 8 2023, 10:39 AM

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.Mar 9 2023, 12:40 PM

atul mentioned this in D7006: [native][AES] Implement decrypt function on iOS.Mar 9 2023, 2:47 PM

bartek added inline comments.Mar 10 2023, 1:00 AM

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

atul added inline comments.Mar 10 2023, 11:40 AM

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)