chore: address extra decryption oracle pr comments

noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr

@@ -1,34 +1,28 @@
 /// Decrypts a ciphertext, using AES128.
 ///
-/// Returns a padded plaintext, of the same size as the input ciphertext.
-/// Note that between 1-16 bytes at the end of the returned plaintext will be pkcs#7 padding.
-/// It's up to the calling function to identify and remove that padding.
-/// See the tests below for an example of how.
+/// Returns a BoundedVec containing the plaintext.
+///
 /// It's up to the calling function to determine whether decryption succeeded or failed.
 /// See the tests below for an example of how.
-unconstrained fn aes128_decrypt_oracle_wrapper<let N: u32>(
-    ciphertext: [u8; N],
-    iv: [u8; 16],
-    sym_key: [u8; 16],
-) -> [u8; N] {
-    aes128_decrypt_oracle(ciphertext, iv, sym_key)
-}
-
 #[oracle(aes128Decrypt)]
-unconstrained fn aes128_decrypt_oracle<let N: u32>(
+pub unconstrained fn aes128_decrypt_oracle<let N: u32>(
     ciphertext: [u8; N],
     iv: [u8; 16],
     sym_key: [u8; 16],
-) -> [u8; N] {}
+) -> BoundedVec<u8, N> {}
 
 mod test {
     use crate::{
         encrypted_logs::encrypt::aes128::derive_aes_symmetric_key_and_iv_from_ecdh_shared_secret_using_sha256,
-        utils::point::point_from_x_coord,
+        utils::{array::subarray::subarray, point::point_from_x_coord},
     };
-    use super::aes128_decrypt_oracle_wrapper;
+    use super::aes128_decrypt_oracle;
     use std::aes128::aes128_encrypt;
 
+    global TEST_PLAINTEXT_LENGTH: u32 = 10;
+    global TEST_CIPHERTEXT_LENGTH: u32 = 16;
+    global TEST_PADDING_LENGTH: u32 = TEST_CIPHERTEXT_LENGTH - TEST_PLAINTEXT_LENGTH;
+
     #[test]
     unconstrained fn aes_encrypt_then_decrypt() {
         let ciphertext_shared_secret = point_from_x_coord(1);
@@ -37,34 +31,41 @@ mod test {
             ciphertext_shared_secret,
         );
 
-        let plaintext: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
-
-        let ciphertext = aes128_encrypt(plaintext, iv, sym_key);
+        let plaintext: [u8; TEST_PLAINTEXT_LENGTH] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
 
-        let received_plaintext = aes128_decrypt_oracle_wrapper(ciphertext, iv, sym_key);
-        let padding_length = received_plaintext[received_plaintext.len() - 1] as u32;
+        let ciphertext: [u8; TEST_CIPHERTEXT_LENGTH] = aes128_encrypt(plaintext, iv, sym_key);
 
-        // A BoundedVec could also be used.
-        let mut received_plaintext_with_padding_removed = std::collections::vec::Vec::new();
-        for i in 0..received_plaintext.len() - padding_length {
-            received_plaintext_with_padding_removed.push(received_plaintext[i]);
-        }
+        let received_plaintext = aes128_decrypt_oracle(ciphertext, iv, sym_key);
 
-        assert_eq(received_plaintext_with_padding_removed.slice, plaintext.as_slice());
+        assert_eq(received_plaintext.len(), TEST_PLAINTEXT_LENGTH);
+        assert_eq(received_plaintext.max_len(), TEST_CIPHERTEXT_LENGTH);
+        assert_eq(
+            subarray::<_, _, TEST_PLAINTEXT_LENGTH>(received_plaintext.storage(), 0),
+            plaintext,
+        );
+        assert_eq(
+            subarray::<_, _, TEST_PADDING_LENGTH>(
+                received_plaintext.storage(),
+                TEST_PLAINTEXT_LENGTH,
+            ),
+            [0 as u8; TEST_PADDING_LENGTH],
+        );
     }
 
-    global TEST_PLAINTEXT_LENGTH: u32 = 10;
     global TEST_MAC_LENGTH: u32 = 32;
 
     #[test(should_fail_with = "mac does not match")]
     unconstrained fn aes_encrypt_then_decrypt_with_bad_sym_key_is_caught() {
         // The AES decryption oracle will not fail for any ciphertext; it will always
         // return some data. As for whether the decryption was successful, it's up
         // to the app to check this in a custom way.
+        //
         // E.g. if it's a note that's been encrypted, then upon decryption, the app
         // can check to see if the note hash exists onchain. If it doesn't exist
         // onchain, then that's a strong indicator that decryption has failed.
-        // E.g. for non-note messages, the plaintext could include a MAC. We
+        //
+        // E.g. for non-note messages, the plaintext could include a MAC
+        // (https://en.wikipedia.org/wiki/Message_authentication_code). We
         // demonstrate what this could look like in this test.
         //
         // We compute a MAC and we include that MAC in the plaintext. We then encrypt
@@ -86,7 +87,10 @@ mod test {
 
         // We append the mac to the plaintext. It doesn't necessarily have to be 32 bytes;
         // that's quite an extreme length. 16 bytes or 8 bytes might be sufficient, and would
-        // save on data broadcasting costs.
+        // save on data broadcasting costs. The shorter the mac, the more possibility
+        // of false positive decryptions (decryption seemingly succeeding, but the
+        // decrypted plaintext being garbage).
+        // Some projects use the `iv` (all 16 bytes or the first 8 bytes) as a mac.
         let mut plaintext_with_mac = [0 as u8; TEST_PLAINTEXT_LENGTH + TEST_MAC_LENGTH];
         for i in 0..TEST_PLAINTEXT_LENGTH {
             plaintext_with_mac[i] = plaintext[i];
@@ -109,12 +113,10 @@ mod test {
         let mut bad_sym_key = sym_key;
         bad_sym_key[0] = 0;
 
-        let received_plaintext = aes128_decrypt_oracle_wrapper(ciphertext, iv, bad_sym_key);
+        let received_plaintext = aes128_decrypt_oracle(ciphertext, iv, bad_sym_key);
 
-        let mut extracted_mac_as_bytes = [0 as u8; TEST_MAC_LENGTH];
-        for i in 0..TEST_MAC_LENGTH {
-            extracted_mac_as_bytes[i] = received_plaintext[TEST_PLAINTEXT_LENGTH + i];
-        }
+        let extracted_mac_as_bytes: [u8; TEST_MAC_LENGTH] =
+            subarray(received_plaintext.storage(), TEST_PLAINTEXT_LENGTH);
 
         // We expect this assertion to fail, because we used a bad sym key.
         assert_eq(mac_as_bytes, extracted_mac_as_bytes, "mac does not match");

yarn-project/simulator/src/acvm/oracle/typed_oracle.ts

@@ -264,6 +264,6 @@ export abstract class TypedOracle {
   }
 
   aes128Decrypt(_ciphertext: Buffer, _iv: Buffer, _symKey: Buffer): Promise<Buffer> {
-    throw new OracleMethodNotAvailableError('aes128Decrypt');
+    return Promise.reject(new OracleMethodNotAvailableError('aes128Decrypt'));
   }
 }

yarn-project/simulator/src/client/view_data_oracle.ts

@@ -368,11 +368,7 @@ export class ViewDataOracle extends TypedOracle {
 
   // TODO(#11849): consider replacing this oracle with a pure Noir implementation of aes decryption.
   public override aes128Decrypt(ciphertext: Buffer, iv: Buffer, symKey: Buffer): Promise<Buffer> {
-    // Noir can't predict the amount of padding that gets trimmed,
-    // but it needs to know the length of the returned value.
-    // So we tell Noir that the length is the (predictable) length
-    // of the padded plaintext, we return that padded plaintext, and have Noir interpret the padding to do the trimming.
     const aes128 = new Aes128();
-    return aes128.decryptBufferCBCKeepPadding(ciphertext, iv, symKey);
+    return aes128.decryptBufferCBC(ciphertext, iv, symKey);
   }
 }

yarn-project/txe/src/txe_service/txe_service.ts

@@ -25,6 +25,8 @@ import {
   type ForeignCallArray,
   type ForeignCallSingle,
   addressFromSingle,
+  arrayToBoundedVec,
+  bufferToU8Array,
   fromArray,
   fromSingle,
   fromUintArray,
@@ -587,14 +589,9 @@ export class TXEService {
     const ivBuffer = fromUintArray(iv, 8);
     const symKeyBuffer = fromUintArray(symKey, 8);
 
-    // TODO(AD): this is a hack to shut up linter - this shouldn't be async!
-    const paddedPlaintext = await Promise.resolve(
-      this.typedOracle.aes128Decrypt(ciphertextBuffer, ivBuffer, symKeyBuffer),
-    );
+    const plaintextBuffer = await this.typedOracle.aes128Decrypt(ciphertextBuffer, ivBuffer, symKeyBuffer);
 
-    // We convert each byte of the buffer to its own Field, so that the Noir
-    // function correctly receives [u8; N].
-    return toForeignCallResult([toArray(Array.from(paddedPlaintext).map(byte => new Fr(byte)))]);
+    return toForeignCallResult(arrayToBoundedVec(bufferToU8Array(plaintextBuffer), ciphertextBuffer.length));
   }
 
   // AVM opcodes

yarn-project/txe/src/util/encoding.ts

@@ -32,22 +32,48 @@ export function fromArray(obj: ForeignCallArray) {
  * @param uintBitSize If it's an array of Noir u8's, put `8`, etc.
  * @returns
  */
-export function fromUintArray(obj: ForeignCallArray, uintBitSize: number) {
+export function fromUintArray(obj: ForeignCallArray, uintBitSize: number): Buffer {
   if (uintBitSize % 8 !== 0) {
     throw new Error(`u${uintBitSize} is not a supported type in Noir`);
   }
   const uintByteSize = uintBitSize / 8;
   return Buffer.concat(obj.map(str => hexToBuffer(str).slice(-uintByteSize)));
 }
 
-export function toSingle(obj: Fr | AztecAddress) {
+export function toSingle(obj: Fr | AztecAddress): ForeignCallSingle {
   return obj.toString().slice(2);
 }
 
-export function toArray(objs: Fr[]) {
+export function toArray(objs: Fr[]): ForeignCallArray {
   return objs.map(obj => obj.toString());
 }
 
+export function bufferToU8Array(buffer: Buffer): ForeignCallArray {
+  return toArray(Array.from(buffer).map(byte => new Fr(byte)));
+}
+
+/**
+ * Converts a ForeignCallArray into a tuple which represents a nr BoundedVec.
+ * If the input array is shorter than the maxLen, it pads the result with zeros,
+ * so that nr can correctly coerce this result into a BoundedVec.
+ * @param array
+ * @param maxLen - the max length of the BoundedVec.
+ * @returns a tuple representing a BoundedVec.
+ */
+export function arrayToBoundedVec(array: ForeignCallArray, maxLen: number): [ForeignCallArray, ForeignCallSingle] {
+  if (array.length > maxLen) {
+    throw new Error(`Array of length ${array.length} larger than maxLen ${maxLen}`);
+  }
+  const lengthDiff = maxLen - array.length;
+  // We pad the array to the maxLen of the BoundedVec.
+  const zeroPaddingArray = toArray(Array(lengthDiff).fill(new Fr(0)));
+
+  // These variable names match with the BoundedVec members in nr:
+  const storage = array.concat(zeroPaddingArray);
+  const len = toSingle(new Fr(array.length));
+  return [storage, len];
+}
+
 export function toForeignCallResult(obj: (ForeignCallSingle | ForeignCallArray)[]) {
   return { values: obj };
 }