[strongSwan-dev] [PATCH 2/2] Fix packet drop due to race condition on responder

[Christophe Gouault]

To check if a received IKE_SA_INIT request is a new request or a
retransmit, charon maintains hashes of the pending IKE_SA_INIT
exchanges.

However, the hash calculation is not reentrant because a single hasher
is used for the whole IKE SA manager. It leads to bogus calculations
under high load and hence dropped messages on responder
(IkeInInvalidSpi incremented).

Don't share a single hasher in the IKE SA manager, create a transient
one whenever a message must be hashed.

Signed-off-by: Christophe Gouault <christophe.gouault at 6wind.com>
---
 src/libcharon/sa/ike_sa_manager.c |   42 +++++++++++++++----------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/src/libcharon/sa/ike_sa_manager.c b/src/libcharon/sa/ike_sa_manager.c
index f38cc41..a6880c4 100644
--- a/src/libcharon/sa/ike_sa_manager.c
+++ b/src/libcharon/sa/ike_sa_manager.c
@@ -384,11 +384,6 @@ struct private_ike_sa_manager_t {
 	rng_t *rng;
 
 	/**
-	 * SHA1 hasher for IKE_SA_INIT retransmit detection
-	 */
-	hasher_t *hasher;
-
-	/**
 	 * reuse existing IKE_SAs in checkout_by_config
 	 */
 	bool reuse_ikesa;
@@ -966,9 +961,12 @@ static bool get_init_hash(private_ike_sa_manager_t *this, message_t *message,
 						  chunk_t *hash)
 {
 	host_t *src;
+	hasher_t *hasher;
+	bool result = FALSE;
 
-	if (!this->hasher)
-	{	/* this might be the case when flush() has been called */
+	hasher = lib->crypto->create_hasher(lib->crypto, HASH_SHA1);
+	if (hasher == NULL)
+	{
 		return FALSE;
 	}
 	if (message->get_first_payload_type(message) == PLV1_FRAGMENT)
@@ -977,34 +975,38 @@ static bool get_init_hash(private_ike_sa_manager_t *this, message_t *message,
 		u_int64_t spi;
 
 		src = message->get_source(message);
-		if (!this->hasher->allocate_hash(this->hasher,
+		if (!hasher->allocate_hash(hasher,
 										 src->get_address(src), NULL))
 		{
-			return FALSE;
+			goto end;
 		}
 		port = src->get_port(src);
-		if (!this->hasher->allocate_hash(this->hasher,
+		if (!hasher->allocate_hash(hasher,
 										 chunk_from_thing(port), NULL))
 		{
-			return FALSE;
+			goto end;
 		}
 		spi = message->get_initiator_spi(message);
-		return this->hasher->allocate_hash(this->hasher,
+		result = hasher->allocate_hash(hasher,
 										   chunk_from_thing(spi), hash);
+		goto end;
 	}
 	if (message->get_exchange_type(message) == ID_PROT)
 	{	/* include the source for Main Mode as the hash will be the same if
 		 * SPIs are reused by two initiators that use the same proposal */
 		src = message->get_source(message);
 
-		if (!this->hasher->allocate_hash(this->hasher,
+		if (!hasher->allocate_hash(hasher,
 										 src->get_address(src), NULL))
 		{
-			return FALSE;
+			goto end;
 		}
 	}
-	return this->hasher->allocate_hash(this->hasher,
+	result = hasher->allocate_hash(hasher,
 									   message->get_packet_data(message), hash);
+end:
+	hasher->destroy(hasher);
+	return result;
 }
 
 /**
@@ -2072,8 +2074,6 @@ METHOD(ike_sa_manager_t, flush, void,
 
 	this->rng->destroy(this->rng);
 	this->rng = NULL;
-	this->hasher->destroy(this->hasher);
-	this->hasher = NULL;
 }
 
 METHOD(ike_sa_manager_t, destroy, void,
@@ -2148,18 +2148,10 @@ ike_sa_manager_t *ike_sa_manager_create()
 		},
 	);
 
-	this->hasher = lib->crypto->create_hasher(lib->crypto, HASH_SHA1);
-	if (this->hasher == NULL)
-	{
-		DBG1(DBG_MGR, "manager initialization failed, no hasher supported");
-		free(this);
-		return NULL;
-	}
 	this->rng = lib->crypto->create_rng(lib->crypto, RNG_WEAK);
 	if (this->rng == NULL)
 	{
 		DBG1(DBG_MGR, "manager initialization failed, no RNG supported");
-		this->hasher->destroy(this->hasher);
 		free(this);
 		return NULL;
 	}
-- 
1.7.10.4