feat!: return Option from to_address_point and point_from_x_coord (#8… (#17134)

# Motivation
Some AztecAddresses and x coordinates may not correspond to valid points
on the curve. Previously, `to_address_point` `point_from_x_coord` and
`point_from_x_coord_and_sign` assumed the x coordinate were always
valid, which could lead to runtime errors or potential security issues.

Now, both functions return an `Option`, returning `None` if the x
coordinate does not correspond to a valid point. This makes handling
invalid addresses explicit and safe.

# Key points
- `sqrt` moved from `aztec-nr/aztec/src/utils/field.nr` to
`noir-protocol-circuits`
- Deleted redundant `field.nr` from `aztec-nr`
- `AztecAddress::to_address_point` now returns `Option<AddressPoint>`
- `point_from_x_coord` now returns `Option<Point>`
- `point_from_x_coord_and_sign` now returns `Option<Point>` as well
- Existing internal call sites temporarily use `.unwrap()` to maintain
behaviour
- Added tests in `point.nr`, `field.nr`, and `aztec_address.nr` to
verify Optionals in all relevant functions:
  - Valid x (sqrt exists)
  - Invalid x (sqrt fails, returns `None`)

# Breaking change
Any code calling `to_address_point`, `point_from_x_coord`, or
`point_from_x_coord_and_sign` must now handle `Option` instead of
assuming a valid point.

# Addresses
Closes #8970

Author: wildjos

cspell.json

@@ -323,6 +323,7 @@
     "tldr",
     "tload",
     "tmpfs",
+    "tonelli",
     "toplevel",
     "tparam",
     "transferables",

noir-projects/aztec-nr/aztec/src/keys/ecdh_shared_secret.nr

@@ -16,15 +16,27 @@ pub fn derive_ecdh_shared_secret(secret: Scalar, public_key: Point) -> Point {
     shared_secret
 }
 
-/// Computes a standard ecdh shared secret using the address public key of the given address:
-/// [ephemeral_secret] * recipient_address_public_key = shared_secret.
-/// The intention is that the _creator_ of a shared secret would call this function,
-/// given the address of their intended recipient.
+/// Computes a standard ECDH shared secret using the public key corresponding to an AztecAddress:
+///
+/// # Formula
+/// `[ephemeral_secret] * recipient_address_public_key = shared_secret`
+///
+/// # Usage
+/// The intention is that the _creator_ of a shared secret calls this function,
+/// providing the address of their intended recipient.
+///
+/// # Note
+/// The function returns `Option<Point>` because the recipient address might be invalid
+/// (i.e., not correspond to a point on the curve). Callers must handle the `None` case.
+/// This is unlike `derive_ecdh_shared_secret`, which always returns a `Point` because it
+/// operates on guaranteed-valid inputs.
 pub fn derive_ecdh_shared_secret_using_aztec_address(
     ephemeral_secret: Scalar,
     recipient_address: AztecAddress,
-) -> Point {
-    derive_ecdh_shared_secret(ephemeral_secret, recipient_address.to_address_point().inner)
+) -> Option<Point> {
+    recipient_address.to_address_point().map(|addr_point| {
+        derive_ecdh_shared_secret(ephemeral_secret, addr_point.inner)
+    })
 }
 
 #[test]
@@ -80,12 +92,12 @@ unconstrained fn test_shared_secret_computation_from_address_in_both_directions(
     // If needed, we negate the pk's so that they yield valid address points.
     // (We could also have negated the secrets, but there's no negate method for
     // EmbeddedCurvesScalar).
-    pk_a = if (AztecAddress::from_field(pk_a.x).to_address_point().inner == pk_a) {
+    pk_a = if (AztecAddress::from_field(pk_a.x).to_address_point().unwrap().inner == pk_a) {
         pk_a
     } else {
         pk_a.neg()
     };
-    pk_b = if (address_b.to_address_point().inner == pk_b) {
+    pk_b = if (address_b.to_address_point().unwrap().inner == pk_b) {
         pk_b
     } else {
         pk_b.neg()
@@ -94,5 +106,5 @@ unconstrained fn test_shared_secret_computation_from_address_in_both_directions(
     let shared_secret = derive_ecdh_shared_secret_using_aztec_address(secret_a, address_b);
     let shared_secret_alt = derive_ecdh_shared_secret(secret_b, pk_a);
 
-    assert_eq(shared_secret, shared_secret_alt);
+    assert_eq(shared_secret.unwrap(), shared_secret_alt);
 }

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

@@ -173,8 +173,10 @@ impl LogEncryption for AES128 {
         let eph_pk_sign_byte: u8 = get_sign_of_point(eph_pk) as u8;
 
         // (not to be confused with the tagging shared secret)
+        // TODO (#17158): Currently we unwrap the Option returned by derive_ecdh_shared_secret_using_aztec_address.
+        // We need to handle the case where the ephemeral public key is invalid to prevent potential DoS vectors.
         let ciphertext_shared_secret =
-            derive_ecdh_shared_secret_using_aztec_address(eph_sk, recipient);
+            derive_ecdh_shared_secret_using_aztec_address(eph_sk, recipient).unwrap();
         // TODO: also use this shared secret for deriving note randomness.
 
         // *****************************************************************************
@@ -338,7 +340,8 @@ impl LogEncryption for AES128 {
         let eph_pk = point_from_x_coord_and_sign(eph_pk_x, eph_pk_sign_bool);
 
         // Derive shared secret
-        let ciphertext_shared_secret = get_shared_secret(recipient, eph_pk);
+        // TODO(#17158): handle invalid ephemeral keys when decrypting to prevent DoS vectors
+        let ciphertext_shared_secret = get_shared_secret(recipient, eph_pk.unwrap());
 
         // Derive symmetric keys:
         let pairs = derive_aes_symmetric_key_and_iv_from_ecdh_shared_secret_using_poseidon2_unsafe::<2>(
@@ -426,7 +429,8 @@ mod test {
                 EmbeddedCurveScalar::from_field(eph_sk),
                 recipient,
             );
-            let _ = OracleMock::mock("utilityGetSharedSecret").returns(shared_secret);
+
+            let _ = OracleMock::mock("utilityGetSharedSecret").returns(shared_secret.unwrap());
 
             // Decrypt the log
             let decrypted = AES128::decrypt_log(encrypted_log, recipient);

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

@@ -34,7 +34,7 @@ mod test {
         let env = TestEnvironment::new();
 
         env.utility_context(|_| {
-            let ciphertext_shared_secret = point_from_x_coord(1);
+            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>(
                 ciphertext_shared_secret,
@@ -91,7 +91,7 @@ mod test {
             // 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.
-            let ciphertext_shared_secret = point_from_x_coord(1);
+            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>(
                 ciphertext_shared_secret,

noir-projects/aztec-nr/aztec/src/utils/field.nr

@@ -1,7 +1,6 @@
 pub mod array;
 pub mod comparison;
 pub mod conversion;
-pub mod field;
 pub mod point;
 pub mod random;
 pub mod to_bytes;