x.crypto.curve25519: fix possible double free, add consistencies check, cleanup dead test code (#24762)

Author: blackshirt

vlib/x/crypto/curve25519/curve25519.v

@@ -101,6 +101,12 @@ pub fn (mut pv PrivateKey) x25519(point []u8) ![]u8 {
 	if point.len != point_size {
 		return error('bad point size, should be 32')
 	}
+	// We reject and disallow zero-bytes point to be passed
+	// and check it here as a quick exit before heavy math
+	// calculation on `x25519` call
+	if is_zero(point) {
+		return error('x25519: get zeros point')
+	}
 	// even technically its possible, but we limit to unallow it
 	if subtle.constant_time_compare(pv.key, point) == 1 {
 		return error('pv.key identical with point')
@@ -121,6 +127,12 @@ pub fn (pv PrivateKey) bytes() ![]u8 {
 // free releases underlying key. Dont use the key after calling .free
 @[unsafe]
 pub fn (mut pv PrivateKey) free() {
+	// when private key has been marked as done (freed),
+	// calling free on already freed key would lead to undefined behavior.
+	// so, we check it
+	if pv.done {
+		return
+	}
 	unsafe { pv.key.free() }
 	// sets flag to finish
 	pv.done = true
@@ -143,7 +155,7 @@ pub fn PublicKey.new_from_bytes(bytes []u8) !&PublicKey {
 	// in curve25519 is generally not needed for Diffie-Hellman key exchange.
 	// See https://cr.yp.to/ecdh.html#validate
 	// But there are availables suggestion to do validation on them spreads on the internet, likes
-	// bytes return the clone of the bytes of the underlying PublicKey
+	// - blacklisting the known bad public keys
 	// - check the shared value and to raise exception if it is zero.
 	// - You can also bind the exchanged public keys to the shared keys, i.e.,
 	//   instead of using H(abG) as the shared keys, you should use H(aG || bG || abG)
@@ -209,6 +221,7 @@ fn (rd RawDerivator) derive(sec []u8, opt DeriveOpts) ![]u8 {
 // 6.  Diffie-Hellman with Curve25519
 // See https://datatracker.ietf.org/doc/html/rfc7748#section-6
 pub fn derive_shared_secret(mut local PrivateKey, remote PublicKey, opt SharedOpts) ![]u8 {
+	// TODO: should this check be relaxed ?
 	// check for safety
 	local_pubkey := local.public_key()!
 	if local_pubkey.equal(remote) {
@@ -220,6 +233,8 @@ pub fn derive_shared_secret(mut local PrivateKey, remote PublicKey, opt SharedOp
 	// Both now share shared = X25519(local_privkey, remote_pubkey) = X25519(remote_privkey, local_pubkey)
 	// as a shared secret, which is then used as a key or input to a key derivation function.
 	sec := local.x25519(remote.key)!
+	// Internally, x25519 has builtin check for zeros result
+	// but only for non base point branch on x25519_generic routine
 	if is_zero(sec) {
 		return error('zeroes shared secret')
 	}
@@ -247,6 +262,11 @@ pub fn derive_shared_secret(mut local PrivateKey, remote PublicKey, opt SharedOp
 // be either `base_point` or the output of another `x25519` call.
 @[direct_array_access]
 pub fn x25519(mut scalar []u8, point []u8) ![]u8 {
+	// likes the previous comment, we add zeroes point check here
+	// and reject if it happen.
+	if is_zero(point) || is_zero(scalar) {
+		return error('x25519: unallowed zeros/scalar point')
+	}
 	mut dst := []u8{len: 32}
 	// we do bytes clamping here, to make sure scalar was ready to use
 	clamp(mut scalar)!
@@ -264,8 +284,8 @@ fn x25519_generic(mut dst []u8, mut scalar []u8, point []u8) ![]u8 {
 			return error('dst: get base_point')
 		}
 	} else {
-		// base := point.clone()
 		scalar_mult(mut dst, mut scalar, point)!
+		// check for zeros point result
 		if is_zero_point(dst) {
 			return error('bad input point: low order point')
 		}

vlib/x/crypto/curve25519/curve25519_test.v

@@ -99,9 +99,10 @@ struct LowOrderPoint {
 }
 
 const loworder_points = [
+	// zeros point
 	LowOrderPoint{[u8(0x00), 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-		0x00, 0x00, 0x00, 0x00, 0x00], error('bad input point: low order point')},
+		0x00, 0x00, 0x00, 0x00, 0x00], error('x25519: unallowed zeros/scalar point')},
 	LowOrderPoint{[u8(0x01), 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 		0x00, 0x00, 0x00, 0x00, 0x00], error('bad input point: low order point')},
@@ -354,55 +355,3 @@ const tests_vectors = [
 			0x61, 0x96, 0x1e, 0xfc, 0xb]
 	},
 ]
-
-// Please be aware, this is long running times test, its loop until 1.000.000 times.
-// so, please be patient. See the detail of the type 2 test from rfc
-// see at https://datatracker.ietf.org/doc/html/rfc7748#section-5.2
-//
-// Currently, this test was disabled due to its takes long time to complete,
-// please uncomment it if you need run the test.
-// On my test machine, its pass successfully in 2225590.282 ms
-// ```v
-// ... (previous line)
-// ...
-// start i: 999995
-// start i: 999996
-// start i: 999997
-// start i: 999998
-// start i: 999999
-//
-//     OK   2225590.282 ms     3 asserts | curve25519.test_x25519_after_iteration_from_rfc_vector_type2()
-//     Summary for running V tests in "curve25519_test.v": 3 passed, 3 total. Elapsed time: 2225590 ms.
-// ```
-/*
-fn test_x25519_after_iteration_from_rfc_vector_type2() ! {
-	iteration1 := hex.decode('422c8e7a6227d7bca1350b3e2bb7279f7897b87bb6854b783c60e80311ae3079')!
-	iteration1000 := hex.decode('684cf59ba83309552800ef566f2f4d3c1c3887c49360e3875f2eb94d99532c51') !
-	iteration1000000 := hex.decode('7c3911e0ab2586fd864497297e575e6f3bc601c0883c30df5f4dd2d24f665424') !
-
-	// Initially, set k and u to be the following values
-	key := '0900000000000000000000000000000000000000000000000000000000000000'
-	mut k := hex.decode(key)!
-	mut u := k.clone()
-	mut r := []u8{len: 32}
-	
-	// For each iteration, set k to be the result of calling the function and u
-	// to be the old value of k.  The final result is the value left in k.
-	//
-	for i in 0..1000000 {
-		println("start i: $i")
-		tmp_k := k.clone()
-		r = x25519(mut k, u) !
-		unsafe {u = tmp_k}
-		unsafe {k = r}
-		if i == 0 {
-			assert k == iteration1
-		} else if i == 999 {
-			assert k == iteration1000
-		} else if i == 999999 {
-			assert k == iteration1000000
-		}
-		
-	}
-}
-*/