wolfssl: Move shared secret calculation to get_shared_secret()

The ECDH implementation gets a bit simpler since we removed the
ecp_x_coordinate_only option a while ago.

Also added calls to verify public keys.

src/libstrongswan/plugins/wolfssl/wolfssl_diffie_hellman.c

@@ -68,6 +68,11 @@ struct private_wolfssl_diffie_hellman_t {
 	 */
 	chunk_t pub;
 
+	/**
+	 * Public key provided by peer
+	 */
+	chunk_t other;
+
 	/**
 	 * Shared secret
 	 */
@@ -84,9 +89,19 @@ METHOD(key_exchange_t, get_public_key, bool,
 METHOD(key_exchange_t, get_shared_secret, bool,
 	private_wolfssl_diffie_hellman_t *this, chunk_t *secret)
 {
+	word32 len;
+
 	if (!this->shared_secret.len)
 	{
-		return FALSE;
+		this->shared_secret = chunk_alloc(this->len);
+		if (wc_DhAgree(&this->dh, this->shared_secret.ptr, &len, this->priv.ptr,
+					   this->priv.len, this->other.ptr, this->other.len) != 0)
+		{
+			DBG1(DBG_LIB, "DH shared secret computation failed");
+			chunk_free(&this->shared_secret);
+			return FALSE;
+		}
+		this->shared_secret.len = len;
 	}
 	*secret = chunk_copy_pad(chunk_alloc(this->len), this->shared_secret, 0x00);
 	return TRUE;
@@ -95,23 +110,17 @@ METHOD(key_exchange_t, get_shared_secret, bool,
 METHOD(key_exchange_t, set_public_key, bool,
 	private_wolfssl_diffie_hellman_t *this, chunk_t value)
 {
-	word32 len;
-
 	if (!key_exchange_verify_pubkey(this->group, value))
 	{
 		return FALSE;
 	}
-
+	if (wc_DhCheckPubKey(&this->dh, value.ptr, value.len) != 0)
-	chunk_clear(&this->shared_secret);
-	this->shared_secret = chunk_alloc(this->len);
-	if (wc_DhAgree(&this->dh, this->shared_secret.ptr, &len, this->priv.ptr,
-				   this->priv.len, value.ptr, value.len) != 0)
 	{
-		DBG1(DBG_LIB, "DH shared secret computation failed");
+		DBG1(DBG_LIB, "DH public key value invalid");
-		chunk_free(&this->shared_secret);
 		return FALSE;
 	}
-	this->shared_secret.len = len;
+	chunk_clear(&this->other);
+	this->other = chunk_clone(value);
 	return TRUE;
 }
 
@@ -153,6 +162,7 @@ METHOD(key_exchange_t, destroy, void,
 	wc_FreeDhKey(&this->dh);
 	chunk_clear(&this->pub);
 	chunk_clear(&this->priv);
+	chunk_clear(&this->other);
 	chunk_clear(&this->shared_secret);
 	free(this);
 }

src/libstrongswan/plugins/wolfssl/wolfssl_ec_diffie_hellman.c

@@ -62,29 +62,17 @@ struct private_wolfssl_ec_diffie_hellman_t {
 	 */
 	ecc_key key;
 
+	/**
+	 * Public key provided by peer
+	 */
+	ecc_key pubkey;
+
 	/**
 	 * Shared secret
 	 */
 	chunk_t shared_secret;
 };
 
-/**
- * Convert a chunk to an ecc_point (which must already exist). The x and y
- * coordinates of the point have to be concatenated in the chunk.
- */
-static bool chunk2ecp(chunk_t chunk, ecc_point *point)
-{
-	if (!wolfssl_mp_split(chunk, point->x, point->y))
-	{
-		return FALSE;
-	}
-	if (mp_set(point->z, 1) != 0)
-	{
-		return FALSE;
-	}
-	return TRUE;
-}
-
 /**
  * Convert an ec_point to a chunk by concatenating the x and y coordinates of
  * the point. This function allocates memory for the chunk.
@@ -138,69 +126,30 @@ static bool wolfssl_ecc_multiply(const ecc_set_type *ecc_set, mp_int *scalar,
 	return ret == 0;
 }
 
-/**
- * Compute the shared secret.
- *
- * We cannot use the function wc_ecc_shared_secret() because that returns only
- * the x coordinate of the shared secret point (which is defined, for instance,
- * in 'NIST SP 800-56A').
- * However, we need both coordinates as RFC 4753 says: "The Diffie-Hellman
- *   public value is obtained by concatenating the x and y values. The format
- *   of the Diffie-Hellman shared secret value is the same as that of the
- *   Diffie-Hellman public value."
- */
-static bool compute_shared_key(private_wolfssl_ec_diffie_hellman_t *this,
-							   ecc_point *pub_key, chunk_t *shared_secret)
-{
-	ecc_point* secret;
-	bool success = FALSE;
-
-	if ((secret = wc_ecc_new_point()) == NULL)
-	{
-		return FALSE;
-	}
-
-	if (wolfssl_ecc_multiply(this->key.dp, &this->key.k, pub_key, secret))
-	{
-		success = ecp2chunk(this->keysize, secret, shared_secret, TRUE);
-	}
-
-	wc_ecc_del_point(secret);
-	return success;
-}
-
 METHOD(key_exchange_t, set_public_key, bool,
 	private_wolfssl_ec_diffie_hellman_t *this, chunk_t value)
 {
-	ecc_point *pub_key;
+	chunk_t uncomp;
 
 	if (!key_exchange_verify_pubkey(this->group, value))
 	{
 		return FALSE;
 	}
 
-	if ((pub_key = wc_ecc_new_point()) == NULL)
+	/* prepend 0x04 to indicate uncompressed point format */
-	{
+	uncomp = chunk_cata("cc", chunk_from_chars(0x04), value);
-		return FALSE;
+	if (wc_ecc_import_x963_ex(uncomp.ptr, uncomp.len, &this->pubkey,
-	}
+							  this->curve_id) != 0)
-
-	if (!chunk2ecp(value, pub_key))
 	{
 		DBG1(DBG_LIB, "ECDH public value is malformed");
-		wc_ecc_del_point(pub_key);
 		return FALSE;
 	}
 
-	chunk_clear(&this->shared_secret);
+	if (wc_ecc_check_key(&this->pubkey) != 0)
-
-	if (!compute_shared_key(this, pub_key, &this->shared_secret))
 	{
-		DBG1(DBG_LIB, "ECDH shared secret computation failed");
+		DBG1(DBG_LIB, "ECDH public value is invalid");
-		chunk_clear(&this->shared_secret);
-		wc_ecc_del_point(pub_key);
 		return FALSE;
 	}
-	wc_ecc_del_point(pub_key);
 	return TRUE;
 }
 
@@ -248,10 +197,51 @@ METHOD(key_exchange_t, set_private_key, bool,
 	return success;
 }
 
+/**
+ * Derive the shared secret
+ */
+static bool compute_shared_key(private_wolfssl_ec_diffie_hellman_t *this)
+{
+	word32 len;
+#ifdef ECC_TIMING_RESISTANT
+	WC_RNG rng;
+
+	if (wc_InitRng(&rng) != 0)
+	{
+		return FALSE;
+	}
+	if (wc_ecc_set_rng(&this->key, &rng) != 0)
+	{
+		wc_FreeRng(&rng);
+		return FALSE;
+	}
+#endif
+
+	this->shared_secret = chunk_alloc(this->keysize);
+	len = this->shared_secret.len;
+
+	if (wc_ecc_shared_secret(&this->key, &this->pubkey, this->shared_secret.ptr,
+							 &len) != 0)
+	{
+		DBG1(DBG_LIB, "ECDH shared secret computation failed");
+		chunk_clear(&this->shared_secret);
+#ifdef ECC_TIMING_RESISTANT
+		wc_FreeRng(&rng);
+#endif
+		return FALSE;
+	}
+	this->shared_secret.len = len;
+#ifdef ECC_TIMING_RESISTANT
+	wc_FreeRng(&rng);
+#endif
+	return TRUE;
+}
+
 METHOD(key_exchange_t, get_shared_secret, bool,
 	private_wolfssl_ec_diffie_hellman_t *this, chunk_t *secret)
 {
-	if (!this->shared_secret.len)
+	if (!this->shared_secret.len &&
+		!compute_shared_key(this))
 	{
 		return FALSE;
 	}
@@ -269,6 +259,7 @@ METHOD(key_exchange_t, destroy, void,
 	private_wolfssl_ec_diffie_hellman_t *this)
 {
 	wc_ecc_free(&this->key);
+	wc_ecc_free(&this->pubkey);
 	chunk_clear(&this->shared_secret);
 	free(this);
 }
@@ -295,10 +286,10 @@ wolfssl_ec_diffie_hellman_t *wolfssl_ec_diffie_hellman_create(key_exchange_metho
 		.group = group,
 	);
 
-	if (wc_ecc_init(&this->key) != 0)
+	if (wc_ecc_init(&this->key) != 0 || wc_ecc_init(&this->pubkey) != 0)
 	{
 		DBG1(DBG_LIB, "key init failed, ecdh create failed");
-		free(this);
+		destroy(this);
 		return NULL;
 	}
 

src/libstrongswan/plugins/wolfssl/wolfssl_x_diffie_hellman.c

@@ -68,6 +68,18 @@ struct private_diffie_hellman_t {
 #endif
 	} key;
 
+	/**
+	 * Public key provided by peer
+	 */
+	union {
+#ifdef HAVE_CURVE25519
+		curve25519_key key25519;
+#endif
+#ifdef HAVE_CURVE448
+		curve448_key key448;
+#endif
+	} pub;
+
 	/**
 	 * Shared secret
 	 */
@@ -76,47 +88,42 @@ struct private_diffie_hellman_t {
 
 #ifdef HAVE_CURVE25519
 
+METHOD(key_exchange_t, get_shared_secret_25519, bool,
+	private_diffie_hellman_t *this, chunk_t *secret)
+{
+	word32 len = CURVE25519_KEYSIZE;
+
+	if (!this->shared_secret.len)
+	{
+		this->shared_secret = chunk_alloc(len);
+		if (wc_curve25519_shared_secret_ex(&this->key.key25519, &this->pub.key25519,
+					this->shared_secret.ptr, &len, EC25519_LITTLE_ENDIAN) != 0)
+		{
+			DBG1(DBG_LIB, "%N shared secret computation failed",
+				 key_exchange_method_names, this->group);
+			chunk_clear(&this->shared_secret);
+			return FALSE;
+		}
+	}
+	*secret = chunk_clone(this->shared_secret);
+	return TRUE;
+}
+
 METHOD(key_exchange_t, set_public_key_25519, bool,
 	private_diffie_hellman_t *this, chunk_t value)
 {
-	word32 len = CURVE25519_KEYSIZE;
-	curve25519_key pub;
-	int ret;
-
 	if (!key_exchange_verify_pubkey(this->group, value))
 	{
 		return FALSE;
 	}
 
-	ret = wc_curve25519_init(&pub);
+	if (wc_curve25519_import_public_ex(value.ptr, value.len, &this->pub.key25519,
-	if (ret != 0)
+									   EC25519_LITTLE_ENDIAN) != 0)
-	{
-		DBG1(DBG_LIB, "%N public key initialization failed",
-			 key_exchange_method_names, this->group);
-		return FALSE;
-	}
-
-	ret = wc_curve25519_import_public_ex(value.ptr, value.len, &pub,
-										 EC25519_LITTLE_ENDIAN);
-	if (ret != 0)
 	{
 		DBG1(DBG_LIB, "%N public value is malformed",
 			 key_exchange_method_names, this->group);
 		return FALSE;
 	}
-
-	chunk_clear(&this->shared_secret);
-	this->shared_secret = chunk_alloc(len);
-	if (wc_curve25519_shared_secret_ex(&this->key.key25519, &pub,
-					this->shared_secret.ptr, &len, EC25519_LITTLE_ENDIAN) != 0)
-	{
-		DBG1(DBG_LIB, "%N shared secret computation failed",
-			 key_exchange_method_names, this->group);
-		chunk_clear(&this->shared_secret);
-		wc_curve25519_free(&pub);
-		return FALSE;
-	}
-	wc_curve25519_free(&pub);
 	return TRUE;
 }
 
@@ -168,47 +175,42 @@ METHOD(key_exchange_t, set_private_key_25519, bool,
 
 #ifdef HAVE_CURVE448
 
+METHOD(key_exchange_t, get_shared_secret_448, bool,
+	private_diffie_hellman_t *this, chunk_t *secret)
+{
+	word32 len = CURVE448_KEY_SIZE;
+
+	if (!this->shared_secret.len)
+	{
+		this->shared_secret = chunk_alloc(len);
+		if (wc_curve448_shared_secret_ex(&this->key.key448, &this->pub.key448,
+					this->shared_secret.ptr, &len, EC448_LITTLE_ENDIAN) != 0)
+		{
+			DBG1(DBG_LIB, "%N shared secret computation failed",
+				 key_exchange_method_names, this->group);
+			chunk_clear(&this->shared_secret);
+			return FALSE;
+		}
+	}
+	*secret = chunk_clone(this->shared_secret);
+	return TRUE;
+}
+
 METHOD(key_exchange_t, set_public_key_448, bool,
 	private_diffie_hellman_t *this, chunk_t value)
 {
-	word32 len = CURVE448_KEY_SIZE;
-	curve448_key pub;
-	int ret;
-
 	if (!key_exchange_verify_pubkey(this->group, value))
 	{
 		return FALSE;
 	}
 
-	ret = wc_curve448_init(&pub);
+	if (wc_curve448_import_public_ex(value.ptr, value.len, &this->pub.key448,
-	if (ret != 0)
+									 EC448_LITTLE_ENDIAN) != 0)
-	{
-		DBG1(DBG_LIB, "%N public key initialization failed",
-			 key_exchange_method_names, this->group);
-		return FALSE;
-	}
-
-	ret = wc_curve448_import_public_ex(value.ptr, value.len, &pub,
-									   EC448_LITTLE_ENDIAN);
-	if (ret != 0)
 	{
 		DBG1(DBG_LIB, "%N public value is malformed",
 			 key_exchange_method_names, this->group);
 		return FALSE;
 	}
-
-	chunk_clear(&this->shared_secret);
-	this->shared_secret = chunk_alloc(len);
-	if (wc_curve448_shared_secret_ex(&this->key.key448, &pub,
-					this->shared_secret.ptr, &len, EC448_LITTLE_ENDIAN) != 0)
-	{
-		DBG1(DBG_LIB, "%N shared secret computation failed",
-			 key_exchange_method_names, this->group);
-		chunk_clear(&this->shared_secret);
-		wc_curve448_free(&pub);
-		return FALSE;
-	}
-	wc_curve448_free(&pub);
 	return TRUE;
 }
 
@@ -258,17 +260,6 @@ METHOD(key_exchange_t, set_private_key_448, bool,
 
 #endif /* HAVE_CURVE448 */
 
-METHOD(key_exchange_t, get_shared_secret, bool,
-	private_diffie_hellman_t *this, chunk_t *secret)
-{
-	if (!this->shared_secret.len)
-	{
-		return FALSE;
-	}
-	*secret = chunk_clone(this->shared_secret);
-	return TRUE;
-}
-
 METHOD(key_exchange_t, get_method, key_exchange_method_t,
 	private_diffie_hellman_t *this)
 {
@@ -282,12 +273,14 @@ METHOD(key_exchange_t, destroy, void,
 	{
 #ifdef HAVE_CURVE25519
 		wc_curve25519_free(&this->key.key25519);
+		wc_curve25519_free(&this->pub.key25519);
 #endif
 	}
 	else if (this->group == CURVE_448)
 	{
 #ifdef HAVE_CURVE448
 		wc_curve448_free(&this->key.key448);
+		wc_curve448_free(&this->pub.key448);
 #endif
 	}
 	chunk_clear(&this->shared_secret);
@@ -305,7 +298,6 @@ key_exchange_t *wolfssl_x_diffie_hellman_create(key_exchange_method_t group)
 
 	INIT(this,
 		.public = {
-			.get_shared_secret = _get_shared_secret,
 			.get_method = _get_method,
 			.destroy = _destroy,
 		},
@@ -322,14 +314,16 @@ key_exchange_t *wolfssl_x_diffie_hellman_create(key_exchange_method_t group)
 	if (group == CURVE_25519)
 	{
 #ifdef HAVE_CURVE25519
+		this->public.get_shared_secret = _get_shared_secret_25519;
 		this->public.set_public_key = _set_public_key_25519;
 		this->public.get_public_key = _get_public_key_25519;
 		this->public.set_private_key = _set_private_key_25519;
 
-		if (wc_curve25519_init(&this->key.key25519) != 0)
+		if (wc_curve25519_init(&this->key.key25519) != 0 ||
+			wc_curve25519_init(&this->pub.key25519) != 0)
 		{
 			DBG1(DBG_LIB, "initializing key failed");
-			free(this);
+			destroy(this);
 			return NULL;
 		}
 		ret = wc_curve25519_make_key(&rng, CURVE25519_KEYSIZE,
@@ -339,14 +333,16 @@ key_exchange_t *wolfssl_x_diffie_hellman_create(key_exchange_method_t group)
 	else if (group == CURVE_448)
 	{
 #ifdef HAVE_CURVE448
+		this->public.get_shared_secret = _get_shared_secret_448;
 		this->public.set_public_key = _set_public_key_448;
 		this->public.get_public_key = _get_public_key_448;
 		this->public.set_private_key = _set_private_key_448;
 
-		if (wc_curve448_init(&this->key.key448) != 0)
+		if (wc_curve448_init(&this->key.key448) != 0 ||
+			wc_curve448_init(&this->pub.key448) != 0)
 		{
 			DBG1(DBG_LIB, "initializing key failed");
-			free(this);
+			destroy(this);
 			return NULL;
 		}
 		ret = wc_curve448_make_key(&rng, CURVE448_KEY_SIZE, &this->key.key448);