fix cfg(fuzzing) roundtrip of uncompressed keys

sanket1729 · sanket1729 · commit 4b06f20f5e50 · 2021-07-20T13:47:42.000-07:00
diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs
@@ -680,6 +680,15 @@ impl<T> CPtr for [T] {
 
 #[cfg(fuzzing)]
 mod fuzz_dummy {
+    /// Serialization logic:
+    /// If pk is created from sk keypair:
+    ///     - It is serialized with prefix 0x02: sk || [0xaa;32]
+    /// If pk is created from from slice:
+    ///     - 0x02||pk_bytes -> pk_bytes||[0xaa;32]
+    ///     - 0x03||pk_bytes -> pk_bytes||[0xbb;32]
+    ///     - 0x04||pk_bytes -> pk_bytes
+    /// This leaves the room for collision between compressed and uncompressed pks,
+    /// but as such collisions should be improbable. 2/2^128
     use super::*;
     use core::sync::atomic::{AtomicUsize, Ordering};
 
@@ -770,26 +779,16 @@ mod fuzz_dummy {
         assert_eq!(cx_flags & required_flags, required_flags);
     }
 
-    /// Checks that pk != 0xffff...ffff and pk[1..32] == pk[33..64]
+    /// Checks that pk is valid
     unsafe fn test_pk_validate(cx: *const Context,
                                pk: *const PublicKey) -> c_int {
         check_context_flags(cx, 0);
-        if (*pk).0[1..32] != (*pk).0[33..64] ||
-           ((*pk).0[32] != 0 && (*pk).0[32] != 0xff) ||
-           secp256k1_ec_seckey_verify(cx, (*pk).0[0..32].as_ptr()) == 0 {
+        if secp256k1_ec_seckey_verify(cx, (*pk).0[0..32].as_ptr()) == 0 {
             0
         } else {
             1
         }
     }
-    unsafe fn test_cleanup_pk(pk: *mut PublicKey) {
-        (*pk).0[32..].copy_from_slice(&(*pk).0[..32]);
-        if (*pk).0[32] <= 0x7f {
-            (*pk).0[32] = 0;
-        } else {
-            (*pk).0[32] = 0xff;
-        }
-    }
 
     // Pubkeys
     pub unsafe fn secp256k1_ec_pubkey_parse(cx: *const Context, pk: *mut PublicKey,
@@ -802,11 +801,10 @@ mod fuzz_dummy {
                     0
                 } else {
                     ptr::copy(input.offset(1), (*pk).0[0..32].as_mut_ptr(), 32);
-                    ptr::copy(input.offset(2), (*pk).0[33..64].as_mut_ptr(), 31);
                     if *input == 3 {
-                        (*pk).0[32] = 0xff;
+                        ptr::write_bytes((*pk).0[32..64].as_mut_ptr(), 0xbb, 32);
                     } else {
-                        (*pk).0[32] = 0;
+                        ptr::write_bytes((*pk).0[32..64].as_mut_ptr(), 0xaa, 32);
                     }
                     test_pk_validate(cx, pk)
                 }
@@ -816,7 +814,6 @@ mod fuzz_dummy {
                     0
                 } else {
                     ptr::copy(input.offset(1), (*pk).0.as_mut_ptr(), 64);
-                    test_cleanup_pk(pk);
                     test_pk_validate(cx, pk)
                 }
             },
@@ -833,10 +830,10 @@ mod fuzz_dummy {
         assert_eq!(test_pk_validate(cx, pk), 1);
         if compressed == SECP256K1_SER_COMPRESSED {
             assert_eq!(*out_len, 33);
-            if (*pk).0[32] <= 0x7f {
-                *output = 2;
-            } else {
+            if &(*pk).0[32..64] == &[0xbb; 32] {
                 *output = 3;
+            } else {
+                *output = 2;
             }
             ptr::copy((*pk).0.as_ptr(), output.offset(1), 32);
         } else if compressed == SECP256K1_SER_UNCOMPRESSED {
@@ -856,7 +853,7 @@ mod fuzz_dummy {
         check_context_flags(cx, SECP256K1_START_SIGN);
         if secp256k1_ec_seckey_verify(cx, sk) != 1 { return 0; }
         ptr::copy(sk, (*pk).0[0..32].as_mut_ptr(), 32);
-        test_cleanup_pk(pk);
+        ptr::write_bytes((*pk).0[32..64].as_mut_ptr(), 0xaa, 32);
         assert_eq!(test_pk_validate(cx, pk), 1);
         1
     }
@@ -866,7 +863,6 @@ mod fuzz_dummy {
         check_context_flags(cx, 0);
         assert_eq!(test_pk_validate(cx, pk), 1);
         if secp256k1_ec_seckey_negate(cx, (*pk).0[..32].as_mut_ptr()) != 1 { return 0; }
-        test_cleanup_pk(pk);
         assert_eq!(test_pk_validate(cx, pk), 1);
         1
     }
@@ -879,7 +875,6 @@ mod fuzz_dummy {
         check_context_flags(cx, SECP256K1_START_VERIFY);
         assert_eq!(test_pk_validate(cx, pk), 1);
         if secp256k1_ec_seckey_tweak_add(cx, (*pk).0[..32].as_mut_ptr(), tweak) != 1 { return 0; }
-        test_cleanup_pk(pk);
         assert_eq!(test_pk_validate(cx, pk), 1);
         1
     }
@@ -892,7 +887,6 @@ mod fuzz_dummy {
         check_context_flags(cx, 0);
         assert_eq!(test_pk_validate(cx, pk), 1);
         if secp256k1_ec_seckey_tweak_mul(cx, (*pk).0[..32].as_mut_ptr(), tweak) != 1 { return 0; }
-        test_cleanup_pk(pk);
         assert_eq!(test_pk_validate(cx, pk), 1);
         1
     }
@@ -911,7 +905,6 @@ mod fuzz_dummy {
                 return 0;
             }
         }
-        test_cleanup_pk(out);
         assert_eq!(test_pk_validate(cx, out), 1);
         1
     }
@@ -1059,8 +1052,7 @@ mod fuzz_dummy {
         check_context_flags(cx, 0);
         let inslice = slice::from_raw_parts(input32, 32);
         (*pubkey).0[..32].copy_from_slice(inslice);
-        (*pubkey).0[32..].copy_from_slice(inslice);
-        test_cleanup_pk(pubkey as *mut PublicKey);
+        ptr::write_bytes((*pubkey).0[32..64].as_mut_ptr(), 0xaa, 32);
         test_pk_validate(cx, pubkey as *mut PublicKey)
     }
 
diff --git a/src/key.rs b/src/key.rs
@@ -714,10 +714,20 @@ mod test {
             PublicKey::from_str("0218845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166").unwrap(),
             pk
         );
+        #[cfg(fuzzing)]
+        assert_eq!(
+            PublicKey::from_str("04\
+                18845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166\
+                aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+            ).unwrap(),
+            pk
+        );
+        println!("{:02x?}", pk.serialize_uncompressed());
+        #[cfg(not(fuzzing))]
         assert_eq!(
             PublicKey::from_str("04\
                 18845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166\
-                84B84DB303A340CD7D6823EE88174747D12A67D2F8F2F9BA40846EE5EE7A44F6"
+                84b84db303a340cd7d6823ee88174747d12a67d2f8f2f9ba40846ee5ee7a44f6"
             ).unwrap(),
             pk
         );
diff --git a/src/lib.rs b/src/lib.rs
@@ -92,7 +92,7 @@
 //!     0xd5, 0x44, 0x53, 0xcf, 0x6e, 0x82, 0xb4, 0x50,
 //! ]).expect("messages must be 32 bytes and are expected to be hashes");
 //!
-//! let sig = Signature::from_compact(&[
+//! let sig2 = Signature::from_compact(&[
 //!     0xdc, 0x4d, 0xc2, 0x64, 0xa9, 0xfe, 0xf1, 0x7a,
 //!     0x3f, 0x25, 0x34, 0x49, 0xcf, 0x8c, 0x39, 0x7a,
 //!     0xb6, 0xf1, 0x6f, 0xb3, 0xd6, 0x3d, 0x86, 0x94,
@@ -103,8 +103,8 @@
 //!     0xc9, 0x42, 0x8f, 0xca, 0x69, 0xc1, 0x32, 0xa2,
 //! ]).expect("compact signatures are 64 bytes; DER signatures are 68-72 bytes");
 //!
-//! # #[cfg(not(fuzzing))]
-//! assert!(secp.verify(&message, &sig, &public_key).is_ok());
+//! #[cfg(not(fuzzing))]
+//! assert!(secp.verify(&message, &sig2, &public_key).is_ok());
 //! ```
 //!
 //! Observe that the same code using, say [`signing_only`](struct.Secp256k1.html#method.signing_only)

Original file line number	Diff line number	Diff line change
`@@ -680,6 +680,15 @@ impl<T> CPtr for [T] {`
`680`	`680`
`681`	`681`	`#[cfg(fuzzing)]`
`682`	`682`	`mod fuzz_dummy {`
	`683`	`+ /// Serialization logic:`
	`684`	`+ /// If pk is created from sk keypair:`
	`685`	`+ /// - It is serialized with prefix 0x02: sk \|\| [0xaa;32]`
	`686`	`+ /// If pk is created from from slice:`
	`687`	`+ /// - 0x02\|\|pk_bytes -> pk_bytes\|\|[0xaa;32]`
	`688`	`+ /// - 0x03\|\|pk_bytes -> pk_bytes\|\|[0xbb;32]`
	`689`	`+ /// - 0x04\|\|pk_bytes -> pk_bytes`
	`690`	`+ /// This leaves the room for collision between compressed and uncompressed pks,`
	`691`	`+ /// but as such collisions should be improbable. 2/2^128`
`683`	`692`	`use super::*;`
`684`	`693`	`use core::sync::atomic::{AtomicUsize, Ordering};`
`685`	`694`
`@@ -770,26 +779,16 @@ mod fuzz_dummy {`
`770`	`779`	`assert_eq!(cx_flags & required_flags, required_flags);`
`771`	`780`	`}`
`772`	`781`
`773`		`- /// Checks that pk != 0xffff...ffff and pk[1..32] == pk[33..64]`
	`782`	`+ /// Checks that pk is valid`
`774`	`783`	`unsafe fn test_pk_validate(cx: *const Context,`
`775`	`784`	`pk: *const PublicKey) -> c_int {`
`776`	`785`	`check_context_flags(cx, 0);`
`777`		`- if (pk).0[1..32] != (pk).0[33..64] \|\|`
`778`		`- ((pk).0[32] != 0 && (pk).0[32] != 0xff) \|\|`
`779`		`- secp256k1_ec_seckey_verify(cx, (*pk).0[0..32].as_ptr()) == 0 {`
	`786`	`+ if secp256k1_ec_seckey_verify(cx, (*pk).0[0..32].as_ptr()) == 0 {`
`780`	`787`	`0`
`781`	`788`	`} else {`
`782`	`789`	`1`
`783`	`790`	`}`
`784`	`791`	`}`
`785`		`- unsafe fn test_cleanup_pk(pk: *mut PublicKey) {`
`786`		`- (pk).0[32..].copy_from_slice(&(pk).0[..32]);`
`787`		`- if (*pk).0[32] <= 0x7f {`
`788`		`- (*pk).0[32] = 0;`
`789`		`- } else {`
`790`		`- (*pk).0[32] = 0xff;`
`791`		`- }`
`792`		`- }`
`793`	`792`
`794`	`793`	`// Pubkeys`
`795`	`794`	`pub unsafe fn secp256k1_ec_pubkey_parse(cx: const Context, pk: mut PublicKey,`
`@@ -802,11 +801,10 @@ mod fuzz_dummy {`
`802`	`801`	`0`
`803`	`802`	`} else {`
`804`	`803`	`ptr::copy(input.offset(1), (*pk).0[0..32].as_mut_ptr(), 32);`
`805`		`- ptr::copy(input.offset(2), (*pk).0[33..64].as_mut_ptr(), 31);`
`806`	`804`	`if *input == 3 {`
`807`		`- (*pk).0[32] = 0xff;`
	`805`	`+ ptr::write_bytes((*pk).0[32..64].as_mut_ptr(), 0xbb, 32);`
`808`	`806`	`} else {`
`809`		`- (*pk).0[32] = 0;`
	`807`	`+ ptr::write_bytes((*pk).0[32..64].as_mut_ptr(), 0xaa, 32);`
`810`	`808`	`}`
`811`	`809`	`test_pk_validate(cx, pk)`
`812`	`810`	`}`
`@@ -816,7 +814,6 @@ mod fuzz_dummy {`
`816`	`814`	`0`
`817`	`815`	`} else {`
`818`	`816`	`ptr::copy(input.offset(1), (*pk).0.as_mut_ptr(), 64);`
`819`		`- test_cleanup_pk(pk);`
`820`	`817`	`test_pk_validate(cx, pk)`
`821`	`818`	`}`
`822`	`819`	`},`
`@@ -833,10 +830,10 @@ mod fuzz_dummy {`
`833`	`830`	`assert_eq!(test_pk_validate(cx, pk), 1);`
`834`	`831`	`if compressed == SECP256K1_SER_COMPRESSED {`
`835`	`832`	`assert_eq!(*out_len, 33);`
`836`		`- if (*pk).0[32] <= 0x7f {`
`837`		`- *output = 2;`
`838`		`- } else {`
	`833`	`+ if &(*pk).0[32..64] == &[0xbb; 32] {`
`839`	`834`	`*output = 3;`
	`835`	`+ } else {`
	`836`	`+ *output = 2;`
`840`	`837`	`}`
`841`	`838`	`ptr::copy((*pk).0.as_ptr(), output.offset(1), 32);`
`842`	`839`	`} else if compressed == SECP256K1_SER_UNCOMPRESSED {`
`@@ -856,7 +853,7 @@ mod fuzz_dummy {`
`856`	`853`	`check_context_flags(cx, SECP256K1_START_SIGN);`
`857`	`854`	`if secp256k1_ec_seckey_verify(cx, sk) != 1 { return 0; }`
`858`	`855`	`ptr::copy(sk, (*pk).0[0..32].as_mut_ptr(), 32);`
`859`		`- test_cleanup_pk(pk);`
	`856`	`+ ptr::write_bytes((*pk).0[32..64].as_mut_ptr(), 0xaa, 32);`
`860`	`857`	`assert_eq!(test_pk_validate(cx, pk), 1);`
`861`	`858`	`1`
`862`	`859`	`}`
`@@ -866,7 +863,6 @@ mod fuzz_dummy {`
`866`	`863`	`check_context_flags(cx, 0);`
`867`	`864`	`assert_eq!(test_pk_validate(cx, pk), 1);`
`868`	`865`	`if secp256k1_ec_seckey_negate(cx, (*pk).0[..32].as_mut_ptr()) != 1 { return 0; }`
`869`		`- test_cleanup_pk(pk);`
`870`	`866`	`assert_eq!(test_pk_validate(cx, pk), 1);`
`871`	`867`	`1`
`872`	`868`	`}`
`@@ -879,7 +875,6 @@ mod fuzz_dummy {`
`879`	`875`	`check_context_flags(cx, SECP256K1_START_VERIFY);`
`880`	`876`	`assert_eq!(test_pk_validate(cx, pk), 1);`
`881`	`877`	`if secp256k1_ec_seckey_tweak_add(cx, (*pk).0[..32].as_mut_ptr(), tweak) != 1 { return 0; }`
`882`		`- test_cleanup_pk(pk);`
`883`	`878`	`assert_eq!(test_pk_validate(cx, pk), 1);`
`884`	`879`	`1`
`885`	`880`	`}`
`@@ -892,7 +887,6 @@ mod fuzz_dummy {`
`892`	`887`	`check_context_flags(cx, 0);`
`893`	`888`	`assert_eq!(test_pk_validate(cx, pk), 1);`
`894`	`889`	`if secp256k1_ec_seckey_tweak_mul(cx, (*pk).0[..32].as_mut_ptr(), tweak) != 1 { return 0; }`
`895`		`- test_cleanup_pk(pk);`
`896`	`890`	`assert_eq!(test_pk_validate(cx, pk), 1);`
`897`	`891`	`1`
`898`	`892`	`}`
`@@ -911,7 +905,6 @@ mod fuzz_dummy {`
`911`	`905`	`return 0;`
`912`	`906`	`}`
`913`	`907`	`}`
`914`		`- test_cleanup_pk(out);`
`915`	`908`	`assert_eq!(test_pk_validate(cx, out), 1);`
`916`	`909`	`1`
`917`	`910`	`}`
`@@ -1059,8 +1052,7 @@ mod fuzz_dummy {`
`1059`	`1052`	`check_context_flags(cx, 0);`
`1060`	`1053`	`let inslice = slice::from_raw_parts(input32, 32);`
`1061`	`1054`	`(*pubkey).0[..32].copy_from_slice(inslice);`
`1062`		`- (*pubkey).0[32..].copy_from_slice(inslice);`
`1063`		`- test_cleanup_pk(pubkey as *mut PublicKey);`
	`1055`	`+ ptr::write_bytes((*pubkey).0[32..64].as_mut_ptr(), 0xaa, 32);`
`1064`	`1056`	`test_pk_validate(cx, pubkey as *mut PublicKey)`
`1065`	`1057`	`}`
`1066`	`1058`