diffie-hellman: Add a bool return value to set_other_public_value()

scripts/dh_speed.c

@@ -90,7 +90,7 @@ static void run_test(diffie_hellman_group_t group, int rounds)
 	for (round = 0; round < rounds; round++)
 	{
 		assert(l[round]->get_my_public_value(l[round], &chunk));
-		r->set_other_public_value(r, chunk);
+		assert(r->set_other_public_value(r, chunk));
 		chunk_free(&chunk);
 	}
 
@@ -98,7 +98,7 @@ static void run_test(diffie_hellman_group_t group, int rounds)
 	start_timing(&timing);
 	for (round = 0; round < rounds; round++)
 	{
-		l[round]->set_other_public_value(l[round], chunk);
+		assert(l[round]->set_other_public_value(l[round], chunk));
 	}
 	printf(" | S = B^a/s: %8.1f\n", rounds / end_timing(&timing));
 	chunk_free(&chunk);
@@ -144,4 +144,3 @@ int main(int argc, char *argv[])
 	}
 	return 0;
 }
-

src/charon-tkm/src/tkm/tkm_diffie_hellman.c

@@ -70,16 +70,14 @@ METHOD(diffie_hellman_t, get_shared_secret, bool,
 }
 
 
-METHOD(diffie_hellman_t, set_other_public_value, void,
+METHOD(diffie_hellman_t, set_other_public_value, bool,
 	private_tkm_diffie_hellman_t *this, chunk_t value)
 {
-	// TODO: unvoid this function
-
 	dh_pubvalue_type othervalue;
 	othervalue.size = value.len;
 	memcpy(&othervalue.data, value.ptr, value.len);
 
-	ike_dh_generate_key(this->context_id, othervalue);
+	return ike_dh_generate_key(this->context_id, othervalue) == TKM_OK;
 }
 
 METHOD(diffie_hellman_t, get_dh_group, diffie_hellman_group_t,

src/charon-tkm/tests/keymat_tests.c

@@ -54,7 +54,7 @@ START_TEST(test_derive_ike_keys)
 	/* Use the same pubvalue for both sides */
 	chunk_t pubvalue;
 	ck_assert(dh->dh.get_my_public_value(&dh->dh, &pubvalue));
-	dh->dh.set_other_public_value(&dh->dh, pubvalue);
+	ck_assert(dh->dh.set_other_public_value(&dh->dh, pubvalue));
 
 	fail_unless(keymat->keymat_v2.derive_ike_keys(&keymat->keymat_v2, proposal,
 				&dh->dh, nonce, nonce, ike_sa_id, PRF_UNDEFINED, chunk_empty),

src/libcharon/plugins/load_tester/load_tester_diffie_hellman.c

@@ -22,9 +22,10 @@ METHOD(diffie_hellman_t, get_my_public_value, bool,
 	return TRUE;
 }
 
-METHOD(diffie_hellman_t, set_other_public_value, void,
+METHOD(diffie_hellman_t, set_other_public_value, bool,
 	load_tester_diffie_hellman_t *this, chunk_t value)
 {
+	return TRUE;
 }
 
 METHOD(diffie_hellman_t, get_shared_secret, bool,

src/libcharon/sa/ikev1/phase1.c

@@ -745,7 +745,11 @@ METHOD(phase1_t, get_nonce_ke, bool,
 		return FALSE;
 	}
 	this->dh_value = chunk_clone(ke_payload->get_key_exchange_data(ke_payload));
-	this->dh->set_other_public_value(this->dh, this->dh_value);
+	if (!this->dh->set_other_public_value(this->dh, this->dh_value))
+	{
+		DBG1(DBG_IKE, "unable to apply received KE value");
+		return FALSE;
+	}
 
 	nonce_payload = (nonce_payload_t*)message->get_payload(message, PLV1_NONCE);
 	if (!nonce_payload)

src/libcharon/sa/ikev1/tasks/quick_mode.c

@@ -493,8 +493,12 @@ static bool get_ke(private_quick_mode_t *this, message_t *message)
 		DBG1(DBG_IKE, "KE payload missing");
 		return FALSE;
 	}
-	this->dh->set_other_public_value(this->dh,
+	if (this->dh->set_other_public_value(this->dh,
-								ke_payload->get_key_exchange_data(ke_payload));
+								ke_payload->get_key_exchange_data(ke_payload)))
+	{
+		DBG1(DBG_IKE, "unable to apply received KE value");
+		return FALSE;
+	}
 	return TRUE;
 }
 

src/libcharon/sa/ikev2/tasks/child_create.c

@@ -104,6 +104,11 @@ struct private_child_create_t {
 	 */
 	diffie_hellman_t *dh;
 
+	/**
+	 * Applying DH public value failed?
+	 */
+	bool dh_failed;
+
 	/**
 	 * group used for DH exchange
 	 */
@@ -893,7 +898,7 @@ static void process_payloads(private_child_create_t *this, message_t *message)
 				}
 				if (this->dh)
 				{
-					this->dh->set_other_public_value(this->dh,
+					this->dh_failed = !this->dh->set_other_public_value(this->dh,
 								ke_payload->get_key_exchange_data(ke_payload));
 				}
 				break;
@@ -1185,12 +1190,19 @@ METHOD(task_t, build_r, status_t,
 		case IKE_SA_INIT:
 			return get_nonce(message, &this->my_nonce);
 		case CREATE_CHILD_SA:
-			if (generate_nonce(this) != SUCCESS)
+			if (generate_nonce(this) != SUCCESS )
 			{
 				message->add_notify(message, FALSE, NO_PROPOSAL_CHOSEN,
 									chunk_empty);
 				return SUCCESS;
 			}
+			if (this->dh_failed)
+			{
+				DBG1(DBG_IKE, "applying DH public value failed");
+				message->add_notify(message, FALSE, NO_PROPOSAL_CHOSEN,
+									chunk_empty);
+				return SUCCESS;
+			}
 			no_dh = FALSE;
 			break;
 		case IKE_AUTH:
@@ -1480,6 +1492,13 @@ METHOD(task_t, process_i, status_t,
 		return delete_failed_sa(this);
 	}
 
+	if (this->dh_failed)
+	{
+		DBG1(DBG_IKE, "applying DH public value failed");
+		handle_child_sa_failure(this, message);
+		return delete_failed_sa(this);
+	}
+
 	if (select_and_install(this, no_dh, ike_auth) == SUCCESS)
 	{
 		if (!this->rekey)
@@ -1557,6 +1576,7 @@ METHOD(task_t, migrate, void,
 	DESTROY_IF(this->child_sa);
 	DESTROY_IF(this->proposal);
 	DESTROY_IF(this->dh);
+	this->dh_failed = FALSE;
 	if (this->proposals)
 	{
 		this->proposals->destroy_offset(this->proposals, offsetof(proposal_t, destroy));

src/libcharon/sa/ikev2/tasks/ike_init.c

@@ -69,6 +69,11 @@ struct private_ike_init_t {
 	 */
 	diffie_hellman_t *dh;
 
+	/**
+	 * Applying DH public value failed?
+	 */
+	bool dh_failed;
+
 	/**
 	 * Keymat derivation (from IKE_SA)
 	 */
@@ -384,7 +389,7 @@ static void process_payloads(private_ike_init_t *this, message_t *message)
 		}
 		if (this->dh)
 		{
-			this->dh->set_other_public_value(this->dh,
+			this->dh_failed = !this->dh->set_other_public_value(this->dh,
 								ke_payload->get_key_exchange_data(ke_payload));
 		}
 	}
@@ -576,6 +581,13 @@ METHOD(task_t, build_r, status_t,
 		return FAILED;
 	}
 
+	if (this->dh_failed)
+	{
+		DBG1(DBG_IKE, "applying DH public value failed");
+		message->add_notify(message, TRUE, NO_PROPOSAL_CHOSEN, chunk_empty);
+		return FAILED;
+	}
+
 	if (!derive_keys(this, this->other_nonce, this->my_nonce))
 	{
 		DBG1(DBG_IKE, "key derivation failed");
@@ -701,6 +713,12 @@ METHOD(task_t, process_i, status_t,
 		return FAILED;
 	}
 
+	if (this->dh_failed)
+	{
+		DBG1(DBG_IKE, "applying DH public value failed");
+		return FAILED;
+	}
+
 	if (!derive_keys(this, this->my_nonce, this->other_nonce))
 	{
 		DBG1(DBG_IKE, "key derivation failed");
@@ -724,6 +742,7 @@ METHOD(task_t, migrate, void,
 	this->ike_sa = ike_sa;
 	this->keymat = (keymat_v2_t*)ike_sa->get_keymat(ike_sa);
 	this->proposal = NULL;
+	this->dh_failed = FALSE;
 	if (this->dh && this->dh->get_dh_group(this->dh) != this->dh_group)
 	{	/* reset DH value only if group changed (INVALID_KE_PAYLOAD) */
 		this->dh->destroy(this->dh);

src/libimcv/pts/pts.c

@@ -238,7 +238,10 @@ METHOD(pts_t, get_my_public_value, bool,
 METHOD(pts_t, set_peer_public_value, bool,
 	private_pts_t *this, chunk_t value, chunk_t nonce)
 {
-	this->dh->set_other_public_value(this->dh, value);
+	if (!this->dh->set_other_public_value(this->dh, value))
+	{
+		return FALSE;
+	}
 
 	nonce = chunk_clone(nonce);
 	if (this->is_imc)

src/libstrongswan/crypto/diffie_hellman.h

@@ -100,8 +100,10 @@ struct diffie_hellman_t {
 	 * Chunk gets cloned and can be destroyed afterwards.
 	 *
 	 * @param value		public value of partner
+	 * @return			TRUE if other public value verified and set
 	 */
-	void (*set_other_public_value) (diffie_hellman_t *this, chunk_t value);
+	bool (*set_other_public_value)(diffie_hellman_t *this, chunk_t value)
+		__attribute__((warn_unused_result));
 
 	/**
 	 * Gets the own public value to transmit.

src/libstrongswan/plugins/gcrypt/gcrypt_dh.c

@@ -73,7 +73,7 @@ struct private_gcrypt_dh_t {
 	size_t p_len;
 };
 
-METHOD(diffie_hellman_t, set_other_public_value, void,
+METHOD(diffie_hellman_t, set_other_public_value, bool,
 	private_gcrypt_dh_t *this, chunk_t value)
 {
 	gcry_mpi_t p_min_1;
@@ -88,7 +88,7 @@ METHOD(diffie_hellman_t, set_other_public_value, void,
 	if (err)
 	{
 		DBG1(DBG_LIB, "importing mpi yb failed: %s", gpg_strerror(err));
-		return;
+		return FALSE;
 	}
 
 	p_min_1 = gcry_mpi_new(this->p_len * 8);
@@ -112,6 +112,7 @@ METHOD(diffie_hellman_t, set_other_public_value, void,
 			 " y < 2 || y > p - 1 ");
 	}
 	gcry_mpi_release(p_min_1);
+	return this->zz != NULL;
 }
 
 /**

src/libstrongswan/plugins/gmp/gmp_diffie_hellman.c

@@ -85,7 +85,7 @@ struct private_gmp_diffie_hellman_t {
 	bool computed;
 };
 
-METHOD(diffie_hellman_t, set_other_public_value, void,
+METHOD(diffie_hellman_t, set_other_public_value, bool,
 	private_gmp_diffie_hellman_t *this, chunk_t value)
 {
 	mpz_t p_min_1;
@@ -142,6 +142,7 @@ METHOD(diffie_hellman_t, set_other_public_value, void,
 			 " y < 2 || y > p - 1 ");
 	}
 	mpz_clear(p_min_1);
+	return this->computed;
 }
 
 METHOD(diffie_hellman_t, get_my_public_value, bool,

src/libstrongswan/plugins/ntru/ntru_ke.c

@@ -106,7 +106,7 @@ struct private_ntru_ke_t {
 	/**
 	 * Deterministic Random Bit Generator
 	 */
-    ntru_drbg_t *drbg;
+	ntru_drbg_t *drbg;
 };
 
 METHOD(diffie_hellman_t, get_my_public_value, bool,
@@ -153,8 +153,7 @@ METHOD(diffie_hellman_t, get_shared_secret, bool,
 	return TRUE;
 }
 
-
+METHOD(diffie_hellman_t, set_other_public_value, bool,
-METHOD(diffie_hellman_t, set_other_public_value, void,
 	private_ntru_ke_t *this, chunk_t value)
 {
 	if (this->privkey)
@@ -163,15 +162,15 @@ METHOD(diffie_hellman_t, set_other_public_value, void,
 		if (value.len == 0)
 		{
 			DBG1(DBG_LIB, "empty NTRU ciphertext");
-			return;
+			return FALSE;
 		}
 		DBG3(DBG_LIB, "NTRU ciphertext: %B", &value);
 
 		/* decrypt the shared secret */
- 		if (!this->privkey->decrypt(this->privkey, value, &this->shared_secret))
+		if (!this->privkey->decrypt(this->privkey, value, &this->shared_secret))
 		{
 			DBG1(DBG_LIB, "NTRU decryption of shared secret failed");
-			return;
+			return FALSE;
 		}
 		this->computed = TRUE;
 	}
@@ -186,13 +185,13 @@ METHOD(diffie_hellman_t, set_other_public_value, void,
 		pubkey = ntru_public_key_create_from_data(this->drbg, value);
 		if (!pubkey)
 		{
-			return;
+			return FALSE;
 		}
 		if (pubkey->get_id(pubkey) != this->param_set->id)
 		{
 			DBG1(DBG_LIB, "received NTRU public key with wrong OUI");
 			pubkey->destroy(pubkey);
-			return;
+			return FALSE;
 		}
 		this->pubkey = pubkey;
 
@@ -205,7 +204,7 @@ METHOD(diffie_hellman_t, set_other_public_value, void,
 		{
 			DBG1(DBG_LIB, "generation of shared secret failed");
 			chunk_free(&this->shared_secret);
-			return;
+			return FALSE;
 		}
 		this->computed = TRUE;
 
@@ -213,10 +212,11 @@ METHOD(diffie_hellman_t, set_other_public_value, void,
 		if (!pubkey->encrypt(pubkey, this->shared_secret, &this->ciphertext))
 		{
 			DBG1(DBG_LIB, "NTRU encryption of shared secret failed");
-			return;
+			return FALSE;
 		}
 		DBG3(DBG_LIB, "NTRU ciphertext: %B", &this->ciphertext);
 	}
+	return this->computed;
 }
 
 METHOD(diffie_hellman_t, get_dh_group, diffie_hellman_group_t,
@@ -302,10 +302,10 @@ ntru_ke_t *ntru_ke_create(diffie_hellman_group_t group, chunk_t g, chunk_t p)
 
 	drbg = ntru_drbg_create(strength, chunk_from_str("IKE NTRU-KE"), entropy);
 	if (!drbg)
- 	{
+	{
 		DBG1(DBG_LIB, "could not instantiate DRBG at %u bit security", strength);
 		entropy->destroy(entropy);
-        return NULL;
+		return NULL;
 	}
 
 	INIT(this,
@@ -327,4 +327,3 @@ ntru_ke_t *ntru_ke_create(diffie_hellman_group_t group, chunk_t g, chunk_t p)
 
 	return &this->public;
 }
-

src/libstrongswan/plugins/openssl/openssl_diffie_hellman.c

@@ -87,7 +87,7 @@ METHOD(diffie_hellman_t, get_shared_secret, bool,
 }
 
 
-METHOD(diffie_hellman_t, set_other_public_value, void,
+METHOD(diffie_hellman_t, set_other_public_value, bool,
 	private_openssl_diffie_hellman_t *this, chunk_t value)
 {
 	int len;
@@ -100,10 +100,11 @@ METHOD(diffie_hellman_t, set_other_public_value, void,
 	if (len < 0)
 	{
 		DBG1(DBG_LIB, "DH shared secret computation failed");
-		return;
+		return FALSE;
 	}
 	this->shared_secret.len = len;
 	this->computed = TRUE;
+	return TRUE;
 }
 
 METHOD(diffie_hellman_t, get_dh_group, diffie_hellman_group_t,

src/libstrongswan/plugins/openssl/openssl_ec_diffie_hellman.c

@@ -216,23 +216,24 @@ error:
 	return ret;
 }
 
-METHOD(diffie_hellman_t, set_other_public_value, void,
+METHOD(diffie_hellman_t, set_other_public_value, bool,
 	private_openssl_ec_diffie_hellman_t *this, chunk_t value)
 {
 	if (!chunk2ecp(this->ec_group, value, this->pub_key))
 	{
 		DBG1(DBG_LIB, "ECDH public value is malformed");
-		return;
+		return FALSE;
 	}
 
 	chunk_clear(&this->shared_secret);
 
 	if (!compute_shared_key(this, &this->shared_secret)) {
 		DBG1(DBG_LIB, "ECDH shared secret computation failed");
-		return;
+		return FALSE;
 	}
 
 	this->computed = TRUE;
+	return TRUE;
 }
 
 METHOD(diffie_hellman_t, get_my_public_value, bool,

src/libstrongswan/plugins/pkcs11/pkcs11_dh.c

@@ -81,7 +81,7 @@ struct private_pkcs11_dh_t {
  *
  * If this succeeds the shared secret is stored in this->secret.
  */
-static void derive_secret(private_pkcs11_dh_t *this, chunk_t other)
+static bool derive_secret(private_pkcs11_dh_t *this, chunk_t other)
 {
 	CK_OBJECT_CLASS klass = CKO_SECRET_KEY;
 	CK_KEY_TYPE type = CKK_GENERIC_SECRET;
@@ -102,17 +102,18 @@ static void derive_secret(private_pkcs11_dh_t *this, chunk_t other)
 	if (rv != CKR_OK)
 	{
 		DBG1(DBG_CFG, "C_DeriveKey() error: %N", ck_rv_names, rv);
-		return;
+		return FALSE;
 	}
 	if (!this->lib->get_ck_attribute(this->lib, this->session, secret,
 									 CKA_VALUE, &this->secret))
 	{
 		chunk_free(&this->secret);
-		return;
+		return FALSE;
 	}
+	return TRUE;
 }
 
-METHOD(diffie_hellman_t, set_other_public_value, void,
+METHOD(diffie_hellman_t, set_other_public_value, bool,
 	private_pkcs11_dh_t *this, chunk_t value)
 {
 	switch (this->group)
@@ -137,7 +138,7 @@ METHOD(diffie_hellman_t, set_other_public_value, void,
 			if (!lib->settings->get_bool(lib->settings,
 									"%s.ecp_x_coordinate_only", TRUE, lib->ns))
 			{	/* we only get the x coordinate back */
-				return;
+				return FALSE;
 			}
 			value = chunk_from_thing(params);
 			break;
@@ -145,7 +146,7 @@ METHOD(diffie_hellman_t, set_other_public_value, void,
 		default:
 			break;
 	}
-	derive_secret(this, value);
+	return derive_secret(this, value);
 }
 
 METHOD(diffie_hellman_t, get_my_public_value, bool,
@@ -444,4 +445,3 @@ pkcs11_dh_t *pkcs11_dh_create(diffie_hellman_group_t group,
 	}
 	return NULL;
 }
-

src/libstrongswan/tests/suites/test_ntru.c

@@ -1083,14 +1083,14 @@ START_TEST(test_ntru_ke)
 	r_ntru = lib->crypto->create_dh(lib->crypto, params[k].group);
 	ck_assert(r_ntru != NULL);
 
-	r_ntru->set_other_public_value(r_ntru, pub_key);
+	ck_assert(r_ntru->set_other_public_value(r_ntru, pub_key));
 	ck_assert(r_ntru->get_my_public_value(r_ntru, &cipher_text));
 	ck_assert(cipher_text.len > 0);
 
 	ck_assert(r_ntru->get_shared_secret(r_ntru, &r_shared_secret));
 	ck_assert(r_shared_secret.len > 0);
 
-	i_ntru->set_other_public_value(i_ntru, cipher_text);
+	ck_assert(i_ntru->set_other_public_value(i_ntru, cipher_text));
 	ck_assert(i_ntru->get_shared_secret(i_ntru, &i_shared_secret));
 	ck_assert(chunk_equals(i_shared_secret, r_shared_secret));
 
@@ -1136,7 +1136,7 @@ START_TEST(test_ntru_pubkey_oid)
 	chunk_t cipher_text;
 
 	r_ntru = lib->crypto->create_dh(lib->crypto, NTRU_128_BIT);
-	r_ntru->set_other_public_value(r_ntru, oid_tests[_i]);
+	ck_assert(!r_ntru->set_other_public_value(r_ntru, oid_tests[_i]));
 	ck_assert(r_ntru->get_my_public_value(r_ntru, &cipher_text));
 	ck_assert(cipher_text.len == 0);
 	r_ntru->destroy(r_ntru);
@@ -1158,7 +1158,7 @@ START_TEST(test_ntru_wrong_set)
 						  "libstrongswan.plugins.ntru.parameter_set",
 						  "optimum");
 	r_ntru = lib->crypto->create_dh(lib->crypto, NTRU_112_BIT);
-	r_ntru->set_other_public_value(r_ntru, pub_key);
+	ck_assert(!r_ntru->set_other_public_value(r_ntru, pub_key));
 	ck_assert(r_ntru->get_my_public_value(r_ntru, &cipher_text));
 	ck_assert(cipher_text.len == 0);
 
@@ -1191,7 +1191,7 @@ START_TEST(test_ntru_ciphertext)
 	{
 		i_ntru = lib->crypto->create_dh(lib->crypto, NTRU_128_BIT);
 		ck_assert(i_ntru->get_my_public_value(i_ntru, &pub_key));
-		i_ntru->set_other_public_value(i_ntru, test[i]);
+		ck_assert(!i_ntru->set_other_public_value(i_ntru, test[i]));
 		ck_assert(!i_ntru->get_shared_secret(i_ntru, &shared_secret));
 		ck_assert(shared_secret.len == 0);
 
@@ -1212,9 +1212,9 @@ START_TEST(test_ntru_wrong_ciphertext)
 
 	ck_assert(i_ntru->get_my_public_value(i_ntru, &pub_key_i));
 	ck_assert(m_ntru->get_my_public_value(m_ntru, &pub_key_m));
-	r_ntru->set_other_public_value(r_ntru, pub_key_m);
+	ck_assert(r_ntru->set_other_public_value(r_ntru, pub_key_m));
 	ck_assert(r_ntru->get_my_public_value(r_ntru, &cipher_text));
-	i_ntru->set_other_public_value(i_ntru, cipher_text);
+	ck_assert(!i_ntru->set_other_public_value(i_ntru, cipher_text));
 	ck_assert(!i_ntru->get_shared_secret(i_ntru, &shared_secret));
 	ck_assert(shared_secret.len == 0);
 

src/libtls/tls_peer.c

@@ -384,7 +384,12 @@ static status_t process_modp_key_exchange(private_tls_peer_t *this,
 		this->alert->add(this->alert, TLS_FATAL, TLS_INTERNAL_ERROR);
 		return NEED_MORE;
 	}
-	this->dh->set_other_public_value(this->dh, pub);
+	if (!this->dh->set_other_public_value(this->dh, pub))
+	{
+		DBG1(DBG_TLS, "applying DH public value failed");
+		this->alert->add(this->alert, TLS_FATAL, TLS_INTERNAL_ERROR);
+		return NEED_MORE;
+	}
 
 	this->state = STATE_KEY_EXCHANGE_RECEIVED;
 	return NEED_MORE;
@@ -494,7 +499,12 @@ static status_t process_ec_key_exchange(private_tls_peer_t *this,
 		this->alert->add(this->alert, TLS_FATAL, TLS_INTERNAL_ERROR);
 		return NEED_MORE;
 	}
-	this->dh->set_other_public_value(this->dh, chunk_skip(pub, 1));
+	if (!this->dh->set_other_public_value(this->dh, chunk_skip(pub, 1)))
+	{
+		DBG1(DBG_TLS, "applying DH public value failed");
+		this->alert->add(this->alert, TLS_FATAL, TLS_INTERNAL_ERROR);
+		return NEED_MORE;
+	}
 
 	this->state = STATE_KEY_EXCHANGE_RECEIVED;
 	return NEED_MORE;

src/libtls/tls_server.c

@@ -494,7 +494,12 @@ static status_t process_key_exchange_dhe(private_tls_server_t *this,
 		}
 		pub = chunk_skip(pub, 1);
 	}
-	this->dh->set_other_public_value(this->dh, pub);
+	if (!this->dh->set_other_public_value(this->dh, pub))
+	{
+		DBG1(DBG_TLS, "applying DH public value failed");
+		this->alert->add(this->alert, TLS_FATAL, TLS_INTERNAL_ERROR);
+		return NEED_MORE;
+	}
 	if (!this->dh->get_shared_secret(this->dh, &premaster))
 	{
 		DBG1(DBG_TLS, "calculating premaster from DH failed");