Page MenuHomePhabricator

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

Authored by bartek on Mar 8 2023, 10:09 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 21 2024, 9:52 PM
Unknown Object (File)
Feb 21 2024, 2:45 AM
Unknown Object (File)
Feb 21 2024, 2:45 AM
Unknown Object (File)
Feb 21 2024, 2:44 AM
Unknown Object (File)
Feb 15 2024, 1:15 AM
Unknown Object (File)
Feb 15 2024, 1:15 AM
Unknown Object (File)
Feb 15 2024, 1:15 AM
Unknown Object (File)
Feb 15 2024, 1:15 AM
Subscribers

Details

Summary
Test Plan

Placed this e.g. in root component render function

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

const key = AES.generateKey();
console.log('Key', key);
const plaintextBytes = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]);
const ciphertext = AES.encrypt(key, plaintextBytes);
console.log('Ciphertext', ciphertext);
const decrypted = AES.decrypt(key, ciphertext);
console.log('Decrypted', decrypted); // should be [1,2,3...9,10]

should display sth like:

LOG  Key [82, 226, 211, 141, 62, 105, 93, 161, 238, 193, 163, 46, 151, 205, 133, 163, 175, 15, 110, 16, 139, 86, 150, 10, 26, 30, 179, 245, 221, 215, 19, 13]
LOG  Ciphertext [74, 52, 31, 132, 80, 213, 246, 255, 188, 231, 18, 27, 222, 224, 91, 53, 204, 198, 109, 53, 40, 194, 198, 149, 181, 34, 131, 102, 228, 246, 171, 190, 89, 232, 36, 74, 173, 211]
LOG  Decrypted [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]

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.Mar 8 2023, 10:41 AM
atul requested changes to this revision.Mar 9 2023, 2:47 PM

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

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

Already mentioned in D7005, but it's kind of confusing how ciphertext can mean multiple things. I'm guessing here ciphertext is iv || ciphertext || tag concatenation vs. "just the ciphertext"?

Think some sort of seal(...)/sealedData (where sealedData = iv || ciphertext || tag concatenation) naming convention would make the distinction clearer?

55 ↗(On Diff #23541)

It looks like this check handles one specific possible invalid ciphertext case.

Not sure we need this, I'm fairly confident we can trust AES.GCM.SealedBox(...) to throw if ciphertext is invalid (this case + others)?

Let me know if there's something I'm missing

56 ↗(On Diff #23541)

Is there a reason we're doing a >= check instead of ==? Seems like elsewhere we expect the sizes to be exact?

62–68 ↗(On Diff #23541)

This looks correct, but I'm hesitant about deconstructing cipherText "manually" based on offsets. This case is pretty simple, but still seems like a brittle approach.

Is there a reason we can't use the

init<D>(combined: D) throws where D : DataProtocol

initializer instead?

According to https://developer.apple.com/documentation/cryptokit/aes/gcm/sealedbox/init(combined:):

Creates a sealed box from the combined bytes of an authentication tag, nonce, and encrypted data.

That sounds like exactly what we need? Is there something preventing us from using that initializer?

70 ↗(On Diff #23541)

Personal style preference would be to pull nonce out to a separate line.

Totally arbitrary preference, feel free to go with whatever you prefer

This revision now requires changes to proceed.Mar 9 2023, 2:47 PM
native/expo-modules/aes-crypto/ios/AESCryptoModule.swift
53 ↗(On Diff #23541)

Done

55 ↗(On Diff #23541)

If this check fails, we don't even need to create SealedBox because it's evident that the given input is too short. Anyway, I'll move this check to JS because we cannot even create a buffer with a negative length

62–68 ↗(On Diff #23541)

Yes, this looks like what we want. I must have missed this initializer. I'll try and check if it works

Address code review feedback

Nice, glad we could cut code and make it simpler with the combined initializer

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