From 6747330c9c1c90336e1ce89c71cf6e8356923213 Mon Sep 17 00:00:00 2001 From: iAmMichaelConnor Date: Tue, 18 Feb 2025 10:20:54 +0000 Subject: [PATCH 1/5] address pr comments --- .../aztec/src/oracle/aes128_decrypt.nr | 24 +++++++++++++++++-- yarn-project/simulator/src/client/index.ts | 1 - .../simulator/src/client/view_data_oracle.ts | 10 ++++---- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr b/noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr index 5ca72abf49c8..e42eff8dcef1 100644 --- a/noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr +++ b/noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr @@ -2,8 +2,22 @@ /// /// 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. +/// +/// A reminder on pkcs#7 padding: +/// PKCS#7 padding standard says to pad a plaintext to the next multiple of 16 bytes +/// with repeated bytes of the length of the padding. +/// +/// If the plaintext is 0 bytes: Pad with 0x10 (16) sixteen times. +/// If the plaintext is 1 byte: Pad with 0x0f (15) fifteen times. +/// If the plaintext is 2 bytes: Pad with 0x0e (14) fourteen times. +/// ... +/// If the plaintext is 15 bytes: Pad with 0x01 one time. +/// +/// So the final byte always tells you how long the padding is. +/// /// It's up to the calling function to identify and remove that padding. /// See the tests below for an example of how. +/// /// 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( @@ -61,10 +75,13 @@ mod test { // 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 +103,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]; diff --git a/yarn-project/simulator/src/client/index.ts b/yarn-project/simulator/src/client/index.ts index a18122af8431..d7369c35b2e7 100644 --- a/yarn-project/simulator/src/client/index.ts +++ b/yarn-project/simulator/src/client/index.ts @@ -1,5 +1,4 @@ export { AcirSimulator } from './simulator.js'; -export { ViewDataOracle } from './view_data_oracle.js'; export { type DBOracle, ContractClassNotFoundError, ContractNotFoundError } from './db_oracle.js'; export * from './pick_notes.js'; export { ExecutionNoteCache } from './execution_note_cache.js'; diff --git a/yarn-project/simulator/src/client/view_data_oracle.ts b/yarn-project/simulator/src/client/view_data_oracle.ts index 190dd5132e1b..5d6fa0f2a857 100644 --- a/yarn-project/simulator/src/client/view_data_oracle.ts +++ b/yarn-project/simulator/src/client/view_data_oracle.ts @@ -364,10 +364,12 @@ 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 { - // 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. + // We do not trim the padding in typescript-land. + // In Noir-land, when this oracle is called, all the caller knows is the + // ciphertext length. Without performing decryption, the caller cannot know the + // true plaintext length. Therefore, the array length of the return type of this + // oracle must be that of the ciphertext, which is equal to the _padded_ plaintext. + // It's then up to the Noir code to remove the pkcs#7 padding itself. const aes128 = new Aes128(); return aes128.decryptBufferCBCKeepPadding(ciphertext, iv, symKey); } From 5c24c2bf6d1c81d017dc6a214c0e84c57aa9fd0b Mon Sep 17 00:00:00 2001 From: iAmMichaelConnor Date: Tue, 18 Feb 2025 10:30:21 +0000 Subject: [PATCH 2/5] revert import change because it broke txe --- yarn-project/simulator/src/client/index.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/yarn-project/simulator/src/client/index.ts b/yarn-project/simulator/src/client/index.ts index d7369c35b2e7..a18122af8431 100644 --- a/yarn-project/simulator/src/client/index.ts +++ b/yarn-project/simulator/src/client/index.ts @@ -1,4 +1,5 @@ export { AcirSimulator } from './simulator.js'; +export { ViewDataOracle } from './view_data_oracle.js'; export { type DBOracle, ContractClassNotFoundError, ContractNotFoundError } from './db_oracle.js'; export * from './pick_notes.js'; export { ExecutionNoteCache } from './execution_note_cache.js'; From 3d1da4b74b35fcf29c4630e763b8737b65c2802d Mon Sep 17 00:00:00 2001 From: iAmMichaelConnor Date: Thu, 20 Feb 2025 19:18:21 +0000 Subject: [PATCH 3/5] merge --- .../aztec/src/oracle/aes128_decrypt.nr | 56 +++++++++---------- .../simulator/src/acvm/oracle/typed_oracle.ts | 2 +- .../simulator/src/client/view_data_oracle.ts | 8 +-- .../txe/src/txe_service/txe_service.ts | 14 ++--- yarn-project/txe/src/util/encoding.ts | 32 ++++++++++- 5 files changed, 67 insertions(+), 45 deletions(-) diff --git a/noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr b/noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr index e42eff8dcef1..c58eb31667d3 100644 --- a/noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr +++ b/noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr @@ -20,29 +20,25 @@ /// /// 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( - 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( +pub unconstrained fn aes128_decrypt_oracle( ciphertext: [u8; N], iv: [u8; 16], sym_key: [u8; 16], -) -> [u8; N] {} +) -> BoundedVec {} 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, subbvec::subbvec}, 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); @@ -51,23 +47,27 @@ 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")] @@ -129,12 +129,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"); diff --git a/yarn-project/simulator/src/acvm/oracle/typed_oracle.ts b/yarn-project/simulator/src/acvm/oracle/typed_oracle.ts index 5fda45816c98..5c154a518e78 100644 --- a/yarn-project/simulator/src/acvm/oracle/typed_oracle.ts +++ b/yarn-project/simulator/src/acvm/oracle/typed_oracle.ts @@ -259,6 +259,6 @@ export abstract class TypedOracle { } aes128Decrypt(_ciphertext: Buffer, _iv: Buffer, _symKey: Buffer): Promise { - throw new OracleMethodNotAvailableError('aes128Decrypt'); + return Promise.reject(new OracleMethodNotAvailableError('aes128Decrypt')); } } diff --git a/yarn-project/simulator/src/client/view_data_oracle.ts b/yarn-project/simulator/src/client/view_data_oracle.ts index 7c0db20eb75e..e6059670c80f 100644 --- a/yarn-project/simulator/src/client/view_data_oracle.ts +++ b/yarn-project/simulator/src/client/view_data_oracle.ts @@ -363,13 +363,11 @@ 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 { - // We do not trim the padding in typescript-land. // In Noir-land, when this oracle is called, all the caller knows is the // ciphertext length. Without performing decryption, the caller cannot know the - // true plaintext length. Therefore, the array length of the return type of this - // oracle must be that of the ciphertext, which is equal to the _padded_ plaintext. - // It's then up to the Noir code to remove the pkcs#7 padding itself. + // true plaintext length. Therefore, we return a BoundedVec, which conveys the + // actual length of the plaintext. const aes128 = new Aes128(); - return aes128.decryptBufferCBCKeepPadding(ciphertext, iv, symKey); + return aes128.decryptBufferCBC(ciphertext, iv, symKey); } } diff --git a/yarn-project/txe/src/txe_service/txe_service.ts b/yarn-project/txe/src/txe_service/txe_service.ts index b3f469ceafa2..5c5bdfdcd787 100644 --- a/yarn-project/txe/src/txe_service/txe_service.ts +++ b/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,12 @@ 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); + + const result = toForeignCallResult(arrayToBoundedVec(bufferToU8Array(plaintextBuffer), ciphertextBuffer.length)); + this.logger.debug('result', result); - // 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 result; } // AVM opcodes diff --git a/yarn-project/txe/src/util/encoding.ts b/yarn-project/txe/src/util/encoding.ts index 152470395067..f7b6868774c0 100644 --- a/yarn-project/txe/src/util/encoding.ts +++ b/yarn-project/txe/src/util/encoding.ts @@ -32,7 +32,7 @@ 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`); } @@ -40,14 +40,40 @@ export function fromUintArray(obj: ForeignCallArray, uintBitSize: number) { 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 }; } From 3429ed0145b273bb52e551f458cb02bd1d2ea492 Mon Sep 17 00:00:00 2001 From: iAmMichaelConnor Date: Thu, 20 Feb 2025 19:27:35 +0000 Subject: [PATCH 4/5] tidy --- .../aztec/src/oracle/aes128_decrypt.nr | 20 ++----------------- 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr b/noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr index c58eb31667d3..3b86686e752d 100644 --- a/noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr +++ b/noir-projects/aztec-nr/aztec/src/oracle/aes128_decrypt.nr @@ -1,22 +1,6 @@ /// 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. -/// -/// A reminder on pkcs#7 padding: -/// PKCS#7 padding standard says to pad a plaintext to the next multiple of 16 bytes -/// with repeated bytes of the length of the padding. -/// -/// If the plaintext is 0 bytes: Pad with 0x10 (16) sixteen times. -/// If the plaintext is 1 byte: Pad with 0x0f (15) fifteen times. -/// If the plaintext is 2 bytes: Pad with 0x0e (14) fourteen times. -/// ... -/// If the plaintext is 15 bytes: Pad with 0x01 one time. -/// -/// So the final byte always tells you how long the padding is. -/// -/// 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. @@ -30,7 +14,7 @@ pub unconstrained fn aes128_decrypt_oracle( mod test { use crate::{ encrypted_logs::encrypt::aes128::derive_aes_symmetric_key_and_iv_from_ecdh_shared_secret_using_sha256, - utils::{array::{subarray::subarray, subbvec::subbvec}, point::point_from_x_coord}, + utils::{array::subarray::subarray, point::point_from_x_coord}, }; use super::aes128_decrypt_oracle; use std::aes128::aes128_encrypt; From 5cc453be57fdfcd56233ef4e702850f2a65b879f Mon Sep 17 00:00:00 2001 From: iAmMichaelConnor Date: Fri, 21 Feb 2025 10:33:04 +0000 Subject: [PATCH 5/5] whoops --- yarn-project/simulator/src/client/view_data_oracle.ts | 4 ---- yarn-project/txe/src/txe_service/txe_service.ts | 5 +---- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/yarn-project/simulator/src/client/view_data_oracle.ts b/yarn-project/simulator/src/client/view_data_oracle.ts index e6059670c80f..0debd9fd856e 100644 --- a/yarn-project/simulator/src/client/view_data_oracle.ts +++ b/yarn-project/simulator/src/client/view_data_oracle.ts @@ -363,10 +363,6 @@ 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 { - // In Noir-land, when this oracle is called, all the caller knows is the - // ciphertext length. Without performing decryption, the caller cannot know the - // true plaintext length. Therefore, we return a BoundedVec, which conveys the - // actual length of the plaintext. const aes128 = new Aes128(); return aes128.decryptBufferCBC(ciphertext, iv, symKey); } diff --git a/yarn-project/txe/src/txe_service/txe_service.ts b/yarn-project/txe/src/txe_service/txe_service.ts index 5c5bdfdcd787..791a210a10e9 100644 --- a/yarn-project/txe/src/txe_service/txe_service.ts +++ b/yarn-project/txe/src/txe_service/txe_service.ts @@ -591,10 +591,7 @@ export class TXEService { const plaintextBuffer = await this.typedOracle.aes128Decrypt(ciphertextBuffer, ivBuffer, symKeyBuffer); - const result = toForeignCallResult(arrayToBoundedVec(bufferToU8Array(plaintextBuffer), ciphertextBuffer.length)); - this.logger.debug('result', result); - - return result; + return toForeignCallResult(arrayToBoundedVec(bufferToU8Array(plaintextBuffer), ciphertextBuffer.length)); } // AVM opcodes