diff --git a/noir-projects/aztec-nr/aztec/src/messages/logs/utils.nr b/noir-projects/aztec-nr/aztec/src/messages/logs/utils.nr index 526acc55b4d3..1dcbf928ea16 100644 --- a/noir-projects/aztec-nr/aztec/src/messages/logs/utils.nr +++ b/noir-projects/aztec-nr/aztec/src/messages/logs/utils.nr @@ -21,7 +21,9 @@ pub(crate) fn compute_discovery_tag(recipient: AztecAddress) -> Field { } mod test { + use crate::oracle::notes::set_sender_for_tags; use crate::protocol::{address::AztecAddress, traits::FromField}; + use crate::test::helpers::test_environment::TestEnvironment; use super::compute_discovery_tag; use std::test::OracleMock; @@ -42,4 +44,21 @@ mod test { let _ = OracleMock::mock("aztec_prv_getNextAppTagAsSender").returns(expected_tag); assert_eq(compute_discovery_tag(recipient), expected_tag); } + + #[test] + unconstrained fn does_not_fail_on_an_invalid_recipient() { + let mut env = TestEnvironment::new(); + + let sender = env.create_light_account(); + + env.private_context(|_context| { + set_sender_for_tags(sender); + + // The recipient is an invalid address + let recipient = AztecAddress { inner: 3 }; + assert(!recipient.is_valid()); + + let _ = compute_discovery_tag(recipient); + }); + } } diff --git a/yarn-project/foundation/src/curves/grumpkin/point.ts b/yarn-project/foundation/src/curves/grumpkin/point.ts index cb486d898188..4254c0012407 100644 --- a/yarn-project/foundation/src/curves/grumpkin/point.ts +++ b/yarn-project/foundation/src/curves/grumpkin/point.ts @@ -65,7 +65,7 @@ export class Point { } /** - * Generate a random Point instance. + * Generate a random Point instance that is on the curve. * * @returns A randomly generated Point instance. */ diff --git a/yarn-project/pxe/src/contract_function_simulator/oracle/private_execution_oracle.ts b/yarn-project/pxe/src/contract_function_simulator/oracle/private_execution_oracle.ts index beb60ab64971..ebeb69b51e36 100644 --- a/yarn-project/pxe/src/contract_function_simulator/oracle/private_execution_oracle.ts +++ b/yarn-project/pxe/src/contract_function_simulator/oracle/private_execution_oracle.ts @@ -209,6 +209,16 @@ export class PrivateExecutionOracle extends UtilityExecutionOracle implements IP recipient, ); + if (!extendedSecret) { + // We'd only fail to compute an extended secret if the recipient is an invalid address. To prevent + // king-of-the-hill attacks, instead of failing we use a random tag. By including a correct-looking tag in the + // log, the transaction shape is preserved and no privacy is leaked, even if the tag is bogus. + this.logger.warn(`Computing a tag for invalid recipient ${recipient} - returning a random tag instead`, { + contractAddress: this.contractAddress, + }); + return new Tag(Fr.random()); + } + const index = await this.#getIndexToUseForSecret(extendedSecret); this.logger.debug( `Incrementing tagging index for sender: ${sender}, recipient: ${recipient}, contract: ${this.contractAddress} to ${index}`, diff --git a/yarn-project/pxe/src/logs/log_service.ts b/yarn-project/pxe/src/logs/log_service.ts index 0f830e018b3e..6df008e1b31c 100644 --- a/yarn-project/pxe/src/logs/log_service.ts +++ b/yarn-project/pxe/src/logs/log_service.ts @@ -176,14 +176,24 @@ export class LogService { ); return Promise.all( - deduplicatedSenders.map(sender => { - return ExtendedDirectionalAppTaggingSecret.compute( + deduplicatedSenders.map(async sender => { + const secret = await ExtendedDirectionalAppTaggingSecret.compute( recipientCompleteAddress, recipientIvsk, sender, contractAddress, recipient, ); + + if (!secret) { + // Note that all senders originate from either the SenderAddressBookStore or the KeyStore. + // TODO(F-512): make sure we actually prevent registering invalid senders. + throw new Error( + `Failed to compute a tagging secret for sender ${sender} - this implies this is an invalid address, which should not happen as they have been previously registered in PXE.`, + ); + } + + return secret; }), ); } diff --git a/yarn-project/stdlib/src/logs/extended_directional_app_tagging_secret.ts b/yarn-project/stdlib/src/logs/extended_directional_app_tagging_secret.ts index 0a083ed053bc..b8c4ae05a96d 100644 --- a/yarn-project/stdlib/src/logs/extended_directional_app_tagging_secret.ts +++ b/yarn-project/stdlib/src/logs/extended_directional_app_tagging_secret.ts @@ -23,14 +23,14 @@ import { computeAddressSecret, computePreaddress } from '../keys/derivation.js'; * doesn't seem to be a good way around this. */ export class ExtendedDirectionalAppTaggingSecret { - private constructor( + constructor( public readonly secret: Fr, public readonly app: AztecAddress, ) {} /** * Derives shared tagging secret and from that, the app address and recipient derives the directional app tagging - * secret. + * secret. Returns undefined if `externalAddress` is an invalid address. * * @param localAddress - The complete address of entity A in the shared tagging secret derivation scheme * @param localIvsk - The incoming viewing secret key of entity A @@ -45,8 +45,12 @@ export class ExtendedDirectionalAppTaggingSecret { externalAddress: AztecAddress, app: AztecAddress, recipient: AztecAddress, - ): Promise { + ): Promise { const taggingSecretPoint = await computeSharedTaggingSecret(localAddress, localIvsk, externalAddress); + if (!taggingSecretPoint) { + return undefined; + } + const appTaggingSecret = await poseidon2Hash([taggingSecretPoint.x, taggingSecretPoint.y, app]); const directionalAppTaggingSecret = await poseidon2Hash([appTaggingSecret, recipient]); @@ -63,18 +67,25 @@ export class ExtendedDirectionalAppTaggingSecret { } } -// Returns shared tagging secret computed with Diffie-Hellman key exchange. +// Returns shared tagging secret computed with Diffie-Hellman key exchange, or undefined if `externalAddress` is an +// invalid address. async function computeSharedTaggingSecret( localAddress: CompleteAddress, localIvsk: Fq, externalAddress: AztecAddress, -): Promise { - const knownPreaddress = await computePreaddress(await localAddress.publicKeys.hash(), localAddress.partialAddress); - // TODO: #8970 - Computation of address point from x coordinate might fail - const externalAddressPoint = await externalAddress.toAddressPoint(); +): Promise { // Given A (local complete address) -> B (external address) and h == preaddress // Compute shared secret as S = (h_A + local_ivsk_A) * Addr_Point_B + const knownPreaddress = await computePreaddress(await localAddress.publicKeys.hash(), localAddress.partialAddress); + + // An invalid address has no corresponding address point + if (!(await externalAddress.isValid())) { + return undefined; + } + + const externalAddressPoint = await externalAddress.toAddressPoint(); + // Beware! h_a + local_ivsk_a (also known as the address secret) can lead to an address point with a negative // y-coordinate, since there's two possible candidates computeAddressSecret takes care of selecting the one that // leads to a positive y-coordinate, which is the only valid address point