feat!: make AES128 decrypt oracle return Option

noir-projects/aztec-nr/aztec/src/messages/encryption/aes128.nr

@@ -20,7 +20,7 @@ use crate::{
             get_arr_of_size__message_bytes__from_PT, get_arr_of_size__message_bytes_padding__from_PT,
         },
     },
-    oracle::{aes128_decrypt::aes128_decrypt, random::random, shared_secret::get_shared_secret},
+    oracle::{aes128_decrypt::try_aes128_decrypt, random::random, shared_secret::get_shared_secret},
     utils::{
         array,
         conversion::{
@@ -424,27 +424,22 @@ impl MessageEncryption for AES128 {
                     let header_ciphertext_bvec =
                         BoundedVec::<u8, HEADER_CIPHERTEXT_SIZE_IN_BYTES>::from_array(header_ciphertext);
 
-                    // Decrypt header
-                    let header_plaintext = aes128_decrypt(header_ciphertext_bvec, header_iv, header_sym_key);
-
+                    try_aes128_decrypt(header_ciphertext_bvec, header_iv, header_sym_key)
                     // Extract ciphertext length from header (2 bytes, big-endian)
-                    extract_ciphertext_length(header_plaintext)
+                    .and_then(|header_plaintext| extract_ciphertext_length(header_plaintext))
                         .filter(|ciphertext_length| ciphertext_length <= MESSAGE_PLAINTEXT_SIZE_IN_BYTES)
-                        .and_then(|ciphertext_length| {
+                        .map(|ciphertext_length| {
                             // Extract and decrypt main ciphertext
                             let ciphertext_start = header_start + HEADER_CIPHERTEXT_SIZE_IN_BYTES;
                             let ciphertext_with_padding: [u8; MESSAGE_PLAINTEXT_SIZE_IN_BYTES] =
                                 array::subarray(ciphertext_without_eph_pk_x.storage(), ciphertext_start);
-                            let ciphertext: BoundedVec<u8, MESSAGE_PLAINTEXT_SIZE_IN_BYTES> =
-                                BoundedVec::from_parts(ciphertext_with_padding, ciphertext_length);
-
-                            // Decrypt main ciphertext and return it
-                            let plaintext_bytes = aes128_decrypt(ciphertext, body_iv, body_sym_key);
-
-                            // Convert bytes back to fields (32 bytes per field). Returns None if the actual bytes are
-                            // not valid.
-                            try_fields_from_bytes(plaintext_bytes)
+                            BoundedVec::from_parts(ciphertext_with_padding, ciphertext_length)
                         })
+                        // Decrypt main ciphertext and return it
+                        .and_then(|ciphertext| try_aes128_decrypt(ciphertext, body_iv, body_sym_key))
+                        // Convert bytes back to fields (32 bytes per field). Returns None if the actual bytes are
+                        // not valid.
+                        .and_then(|plaintext_bytes| try_fields_from_bytes(plaintext_bytes))
                 })
             })
     }
@@ -690,7 +685,7 @@ mod test {
             let ciphertext = BoundedVec::from_array(AES128::encrypt(plaintext, recipient));
 
             let empty_header = BoundedVec::<u8, HEADER_CIPHERTEXT_SIZE_IN_BYTES>::new();
-            let _ = OracleMock::mock("aztec_utl_aes128Decrypt").returns(empty_header).times(1);
+            let _ = OracleMock::mock("aztec_utl_tryAes128Decrypt").returns(Option::some(empty_header)).times(1);
 
             assert(AES128::decrypt(ciphertext, recipient).is_none());
         });
@@ -710,7 +705,7 @@ mod test {
             let bad_header = BoundedVec::<u8, HEADER_CIPHERTEXT_SIZE_IN_BYTES>::from_array(encode_header(
                 MESSAGE_PLAINTEXT_SIZE_IN_BYTES + 1,
             ));
-            let _ = OracleMock::mock("aztec_utl_aes128Decrypt").returns(bad_header).times(1);
+            let _ = OracleMock::mock("aztec_utl_tryAes128Decrypt").returns(Option::some(bad_header)).times(1);
 
             assert(AES128::decrypt(ciphertext, recipient).is_none());
         });

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

@@ -1,30 +1,30 @@
 use crate::utils::array::assert_bounded_vec_trimmed;
 
-#[oracle(aztec_utl_aes128Decrypt)]
-unconstrained fn aes128_decrypt_oracle<let N: u32>(
+#[oracle(aztec_utl_tryAes128Decrypt)]
+unconstrained fn try_aes128_decrypt_oracle<let N: u32>(
     ciphertext: BoundedVec<u8, N>,
     iv: [u8; 16],
     sym_key: [u8; 16],
-) -> BoundedVec<u8, N> {}
+) -> Option<BoundedVec<u8, N>> {}
 
-/// Decrypts a ciphertext, using AES128.
+/// Attempts to decrypt a ciphertext using AES128.
 ///
-/// 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.
+/// Returns `Option::some(plaintext)` on success, or `Option::none()` if decryption fails (e.g. due to malformed
+/// ciphertext). Note that decryption with the wrong key will still return `Some` with garbage data, it's up to
+/// the calling function to verify correctness (e.g. via a MAC check).
 ///
 /// Note that we accept ciphertext as a BoundedVec, not as an array. This is because this function is typically used
-/// when processing logs and at that point we don't have a comptime information about the length of the ciphertext as
+/// when processing logs and at that point we don't have comptime information about the length of the ciphertext as
 /// the log is not specific to any individual note.
-pub unconstrained fn aes128_decrypt<let N: u32>(
+pub unconstrained fn try_aes128_decrypt<let N: u32>(
     ciphertext: BoundedVec<u8, N>,
     iv: [u8; 16],
     sym_key: [u8; 16],
-) -> BoundedVec<u8, N> {
-    let result = aes128_decrypt_oracle(ciphertext, iv, sym_key);
-    assert_bounded_vec_trimmed(result);
-    result
+) -> Option<BoundedVec<u8, N>> {
+    try_aes128_decrypt_oracle(ciphertext, iv, sym_key).map(|result: BoundedVec<u8, N>| {
+        assert_bounded_vec_trimmed(result);
+        result
+    })
 }
 
 mod test {
@@ -33,7 +33,7 @@ mod test {
         utils::{array::subarray::subarray, point::point_from_x_coord},
     };
     use crate::test::helpers::test_environment::TestEnvironment;
-    use super::aes128_decrypt;
+    use super::try_aes128_decrypt;
     use poseidon::poseidon2::Poseidon2;
     use std::aes128::aes128_encrypt;
 
@@ -56,13 +56,9 @@ mod test {
 
             let ciphertext: [u8; TEST_CIPHERTEXT_LENGTH] = aes128_encrypt(plaintext, iv, sym_key);
 
-            // We need to convert the array to a BoundedVec because the oracle expects a BoundedVec as it's designed to
-            // work with logs with unknown length at compile time. This would not be necessary here as the header
-            // ciphertext length is fixed. But we do it anyway to not have to have duplicate oracles.
             let ciphertext_bvec = BoundedVec::<u8, TEST_CIPHERTEXT_LENGTH>::from_array(ciphertext);
 
-            let received_plaintext = aes128_decrypt(ciphertext_bvec, iv, sym_key);
-
+            let received_plaintext = try_aes128_decrypt(ciphertext_bvec, iv, sym_key).unwrap();
             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);
@@ -80,21 +76,17 @@ mod test {
         let env = TestEnvironment::new();
 
         env.utility_context(|_| {
-            // 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.
+            // The AES decryption oracle will not fail for any valid ciphertext; it will always return
+            // `Some` with some data. Whether the decryption was successful is up to the app to check 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. if it's a note that's been encrypted, upon decryption the app can check whether the
+            // note hash exists onchain. If it doesn't, that's a strong indicator that decryption failed.
             //
             // 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 this plaintext to get a
-            // ciphertext. We broadcast the [ciphertext, mac] tuple. The eventual decryptor will expect the mac in the
-            // decrypted plaintext to match the mac that was broadcast. If not, the recipient knows that decryption has
-            // failed.
+            // (https://en.wikipedia.org/wiki/Message_authentication_code). We demonstrate this approach in
+            // this test: we compute a MAC, include it in the plaintext, encrypt, and then verify that
+            // decryption with a bad key produces a MAC mismatch.
             let ciphertext_shared_secret = point_from_x_coord(1).unwrap();
 
             let (sym_key, iv) = derive_aes_symmetric_key_and_iv_from_ecdh_shared_secret_using_poseidon2_unsafe::<1>(
@@ -135,12 +127,12 @@ mod test {
             // We need to convert the array to a BoundedVec because the oracle expects a BoundedVec as it's designed to
             // work with logs of unknown length.
             let ciphertext_bvec = BoundedVec::<u8, 48>::from_array(ciphertext);
-            let received_plaintext = aes128_decrypt(ciphertext_bvec, iv, bad_sym_key);
+            // Decryption with wrong key still returns Some (with garbage).
+            let received_plaintext = try_aes128_decrypt(ciphertext_bvec, iv, bad_sym_key).unwrap();
 
             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");
         });
     }

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

@@ -4,7 +4,7 @@
 ///
 /// @dev Whenever a contract function or Noir test is run, the `aztec_utl_assertCompatibleOracleVersion` oracle is
 /// called and if the oracle version is incompatible an error is thrown.
-pub global ORACLE_VERSION: Field = 16;
+pub global ORACLE_VERSION: Field = 17;
 
 /// Asserts that the version of the oracle is compatible with the version expected by the contract.
 pub fn assert_compatible_oracle_version() {

yarn-project/pxe/src/contract_function_simulator/oracle/legacy_oracle_mappings.ts

@@ -74,13 +74,6 @@ export function buildLegacyOracleCallbacks(oracle: Oracle): ACIRCallback {
     ): Promise<ACVMField[]> => oracle.aztec_utl_copyCapsule(contractAddress, srcSlot, dstSlot, numEntries),
     utilityDeleteCapsule: (contractAddress: ACVMField[], slot: ACVMField[]): Promise<ACVMField[]> =>
       oracle.aztec_utl_deleteCapsule(contractAddress, slot),
-    utilityAes128Decrypt: (
-      ciphertextBVecStorage: ACVMField[],
-      ciphertextLength: ACVMField[],
-      iv: ACVMField[],
-      symKey: ACVMField[],
-    ): Promise<(ACVMField | ACVMField[])[]> =>
-      oracle.aztec_utl_aes128Decrypt(ciphertextBVecStorage, ciphertextLength, iv, symKey),
     utilityGetSharedSecret: (
       address: ACVMField[],
       ephPKField0: ACVMField[],

yarn-project/pxe/src/contract_function_simulator/oracle/oracle.ts

@@ -604,9 +604,8 @@ export class Oracle {
     return [];
   }
 
-  // TODO(F-452): Return Option and wrap in try/catch so BB exceptions don't crash PXE.
   // eslint-disable-next-line camelcase
-  async aztec_utl_aes128Decrypt(
+  async aztec_utl_tryAes128Decrypt(
     ciphertextBVecStorage: ACVMField[],
     [ciphertextLength]: ACVMField[],
     iv: ACVMField[],
@@ -616,8 +615,15 @@ export class Oracle {
     const ivBuffer = fromUintArray(iv, 8);
     const symKeyBuffer = fromUintArray(symKey, 8);
 
-    const plaintext = await this.handlerAsUtility().aes128Decrypt(ciphertext, ivBuffer, symKeyBuffer);
-    return bufferToBoundedVec(plaintext, ciphertextBVecStorage.length);
+    // Noir Option<BoundedVec> is encoded as [is_some: Field, storage: Field[], length: Field].
+    try {
+      const plaintext = await this.handlerAsUtility().aes128Decrypt(ciphertext, ivBuffer, symKeyBuffer);
+      const [storage, length] = bufferToBoundedVec(plaintext, ciphertextBVecStorage.length);
+      return [toACVMField(1), storage, length];
+    } catch {
+      const zeroStorage = Array(ciphertextBVecStorage.length).fill(toACVMField(0));
+      return [toACVMField(0), zeroStorage, toACVMField(0)];
+    }
   }
 
   // eslint-disable-next-line camelcase

yarn-project/pxe/src/oracle_version.ts

@@ -4,9 +4,9 @@
 ///
 /// @dev Whenever a contract function or Noir test is run, the `aztec_utl_assertCompatibleOracleVersion` oracle is called
 /// and if the oracle version is incompatible an error is thrown.
-export const ORACLE_VERSION = 16;
+export const ORACLE_VERSION = 17;
 
 /// This hash is computed as by hashing the Oracle interface and it is used to detect when the Oracle interface changes,
 /// which in turn implies that you need to update the ORACLE_VERSION constant in this file and in
 /// `noir-projects/aztec-nr/aztec/src/oracle/version.nr`.
-export const ORACLE_INTERFACE_HASH = '73ccb2a24bc9fe7514108be9ff98d7ca8734bc316fb7c1ec4329d1d32f412a55';
+export const ORACLE_INTERFACE_HASH = 'f9e965a21bd027693cb6b66dbdf87a3b7411d5ec1c8e0124b3a8852eaac574df';

yarn-project/txe/src/rpc_translator.ts

@@ -878,9 +878,8 @@ export class RPCTranslator {
   // The compiler didn't throw an error, so it took me a while to learn of the existence of this file, and that I need
   // to implement this function here. Isn't there a way to programmatically identify that this is missing, given the
   // existence of a txe_oracle method?
-  // TODO(F-452): Return Option and wrap in try/catch so BB exceptions don't crash TXE.
   // eslint-disable-next-line camelcase
-  async aztec_utl_aes128Decrypt(
+  async aztec_utl_tryAes128Decrypt(
     foreignCiphertextBVecStorage: ForeignCallArray,
     foreignCiphertextLength: ForeignCallSingle,
     foreignIv: ForeignCallArray,
@@ -890,11 +889,18 @@ export class RPCTranslator {
     const iv = fromUintArray(foreignIv, 8);
     const symKey = fromUintArray(foreignSymKey, 8);
 
-    const plaintextBuffer = await this.handlerAsUtility().aes128Decrypt(ciphertext, iv, symKey);
-
-    return toForeignCallResult(
-      arrayToBoundedVec(bufferToU8Array(plaintextBuffer), foreignCiphertextBVecStorage.length),
-    );
+    // Noir Option<BoundedVec> is encoded as [is_some: Field, storage: Field[], length: Field].
+    try {
+      const plaintextBuffer = await this.handlerAsUtility().aes128Decrypt(ciphertext, iv, symKey);
+      const [storage, length] = arrayToBoundedVec(
+        bufferToU8Array(plaintextBuffer),
+        foreignCiphertextBVecStorage.length,
+      );
+      return toForeignCallResult([toSingle(new Fr(1)), storage, length]);
+    } catch {
+      const zeroStorage = toArray(Array(foreignCiphertextBVecStorage.length).fill(new Fr(0)));
+      return toForeignCallResult([toSingle(new Fr(0)), zeroStorage, toSingle(new Fr(0))]);
+    }
   }
 
   // eslint-disable-next-line camelcase