chunk: Improve constant time comparison for chunks with unequal length

While for most uses the length is fixed and public (e.g. PRF/MAC outputs),
there are a few (e.g. in xauth-generic) that compare variable length
data.

The previous code directly leaked a differing length by short-circuiting
before comparing anything.  While we could limit the comparison by the
minimum length (and call `memeq_const()`), that could still leak the
length because the time will plateau once the secret's length is reached.
Similarly, if the comparison was bound by the longer chunk (would prevent
the use of `memeq_const()`), the length could also be revealed once the
input gets longer than the secret and the time increases.

This changes the semantics of the function by declaring the first
argument the expected/reference secret and the second the variable input.
This strictly makes the function constant-time, bound by the secret's
length.  So the length can't be guessed by providing different input (but
if an attacker can trigger the comparison against different secrets, of
potentially known lengths, it might still be possible).  If the chunks
are known to have the same length, the order doesn't matter.

Callers of this function have been updated accordingly.

src/libcharon/plugins/eap_aka/eap_aka_server.c

@@ -426,7 +426,7 @@ static status_t process_challenge(private_eap_aka_server_t *this,
 	enumerator->destroy(enumerator);
 
 	/* compare received RES against stored XRES */
-	if (!chunk_equals_const(res, this->xres))
+	if (!chunk_equals_const(this->xres, res))
 	{
 		DBG1(DBG_IKE, "received RES does not match XRES");
 		return FAILED;
@@ -487,7 +487,7 @@ static status_t process_reauthentication(private_eap_aka_server_t *this,
 		this->crypto->clear_keys(this->crypto);
 		return challenge(this, out);
 	}
-	if (!chunk_equals_const(counter, this->counter))
+	if (!chunk_equals_const(this->counter, counter))
 	{
 		DBG1(DBG_IKE, "received counter does not match");
 		return FAILED;

src/libcharon/plugins/eap_sim/eap_sim_server.c

@@ -263,7 +263,7 @@ static status_t process_reauthentication(private_eap_sim_server_t *this,
 		this->crypto->clear_keys(this->crypto);
 		return initiate(this, out);
 	}
-	if (!chunk_equals_const(counter, this->counter))
+	if (!chunk_equals_const(this->counter, counter))
 	{
 		DBG1(DBG_IKE, "received counter does not match");
 		return FAILED;

src/libimcv/pts/components/ita/ita_comp_ima.c

@@ -549,7 +549,7 @@ static status_t verify_ima_measuremnt(pts_t *pts, pts_database_t *pts_db,
 			status = FAILED;
 			break;
 		}
-		if (chunk_equals_const(measurement, hash))
+		if (chunk_equals_const(hash, measurement))
 		{
 			status = SUCCESS;
 			break;
@@ -861,7 +861,7 @@ METHOD(pts_component_t, verify, status_t,
 	}
 
 	has_pcr_info = evidence->get_pcr_info(evidence, &pcr_before, &pcr_after);
-	if (has_pcr_info && !chunk_equals_const(pcr_before, pcrs->get(pcrs, pcr)))
+	if (has_pcr_info && !chunk_equals_const(pcrs->get(pcrs, pcr), pcr_before))
 	{
 		DBG1(DBG_PTS, "PCR %2u: pcr_before is not equal to register value", pcr);
 		return FAILED;
@@ -873,7 +873,7 @@ METHOD(pts_component_t, verify, status_t,
 		return FAILED;
 	}
 
-	if (has_pcr_info && !chunk_equals_const(pcr_after, pcr_value))
+	if (has_pcr_info && !chunk_equals_const(pcr_value, pcr_after))
 	{
 		DBG1(DBG_PTS, "PCR %2u: pcr_after is not equal to register value", pcr);
 		return FAILED;

src/libimcv/pts/components/ita/ita_comp_tboot.c

@@ -256,7 +256,7 @@ METHOD(pts_component_t, verify, status_t,
 	}
 
 	has_pcr_info = evidence->get_pcr_info(evidence, &pcr_before, &pcr_after);
-	if (has_pcr_info && !chunk_equals_const(pcr_before, pcrs->get(pcrs, pcr)))
+	if (has_pcr_info && !chunk_equals_const(pcrs->get(pcrs, pcr), pcr_before))
 	{
 		DBG1(DBG_PTS, "PCR %2u: pcr_before is not equal to register value", pcr);
 		return FAILED;
@@ -268,7 +268,7 @@ METHOD(pts_component_t, verify, status_t,
 		return FAILED;
 	}
 
-	if (has_pcr_info && !chunk_equals_const(pcr_after, pcr_value))
+	if (has_pcr_info && !chunk_equals_const(pcr_value, pcr_after))
 	{
 		DBG1(DBG_PTS, "PCR %2u: pcr_after is not equal to register value", pcr);
 		return FAILED;

src/libimcv/pts/components/ita/ita_comp_tgrub.c

@@ -143,7 +143,7 @@ METHOD(pts_component_t, verify, status_t,
 	/* TODO check measurement in database */
 
 	has_pcr_info = evidence->get_pcr_info(evidence, &pcr_before, &pcr_after);
-	if (has_pcr_info && !chunk_equals_const(pcr_before, pcrs->get(pcrs, pcr)))
+	if (has_pcr_info && !chunk_equals_const(pcrs->get(pcrs, pcr), pcr_before))
 	{
 		DBG1(DBG_PTS, "PCR %2u: pcr_before is not equal to pcr value", pcr);
 		return FAILED;
@@ -155,7 +155,7 @@ METHOD(pts_component_t, verify, status_t,
 		return FAILED;
 	}
 
-	if (has_pcr_info && !chunk_equals_const(pcr_after, pcr_value))
+	if (has_pcr_info && !chunk_equals_const(pcr_value, pcr_after))
 	{
 		DBG1(DBG_PTS, "PCR %2u: pcr_after is not equal to register value", pcr);
 		return FAILED;

src/libstrongswan/plugins/ml/ml_kem.c

@@ -845,7 +845,7 @@ static bool decaps_shared_secret(private_key_exchange_t *this, chunk_t ciphertex
 	/* replace the shared secret with K based on whether our own ciphertext
 	 * matches what we received (in constant time) */
 	memcpy_cond(this->shared_secret.ptr, Kr, this->shared_secret.len,
-				chunk_equals_const(ciphertext, c));
+				chunk_equals_const(c, ciphertext));
 
 	success = TRUE;
 

src/libstrongswan/plugins/openssl/openssl_pkcs7.c

@@ -318,7 +318,7 @@ static bool verify_digest(CMS_ContentInfo *cms, CMS_SignerInfo *si, int hash_oid
 	}
 	hasher->destroy(hasher);
 
-	if (!chunk_equals_const(digest, hash))
+	if (!chunk_equals_const(hash, digest))
 	{
 		free(hash.ptr);
 		DBG1(DBG_LIB, "invalid messageDigest");

src/libstrongswan/plugins/pkcs12/pkcs12_decode.c

@@ -337,7 +337,7 @@ static bool verify_mac_pw(signer_t *signer, hash_algorithm_t hash, chunk_t salt,
 	if (pkcs12_derive_key(hash, pw, salt, iterations, PKCS12_KEY_MAC, key) &&
 		signer->set_key(signer, key) &&
 		signer->get_signature(signer, data, calculated.ptr) &&
-		chunk_equals_const(mac, calculated))
+		chunk_equals_const(calculated, mac))
 	{
 		success = TRUE;
 	}

src/libstrongswan/plugins/pkcs7/pkcs7_signed_data.c

@@ -274,7 +274,7 @@ METHOD(enumerator_t, enumerate, bool,
 		hasher->destroy(hasher);
 		DBG3(DBG_LIB, "hash: %B", &hash);
 
-		valid = chunk_equals_const(chunk, hash);
+		valid = chunk_equals_const(hash, chunk);
 		free(hash.ptr);
 		if (!valid)
 		{

src/libstrongswan/tests/suites/test_chunk.c

@@ -76,14 +76,18 @@ START_TEST(test_chunk_equals_const)
 
 	chunk_a = chunk;
 	ck_assert(!chunk_equals_const(chunk_a, chunk_b));
+	ck_assert(!chunk_equals_const(chunk_b, chunk_a));
 	chunk_b = chunk;
 	ck_assert(chunk_equals_const(chunk_a, chunk_b));
+	ck_assert(chunk_equals_const(chunk_b, chunk_a));
 
 	chunk_b = chunk_from_str("asdf");
 	ck_assert(!chunk_equals_const(chunk_a, chunk_b));
+	ck_assert(!chunk_equals_const(chunk_b, chunk_a));
 
 	chunk_b = chunk_from_str("chunk");
 	ck_assert(chunk_equals_const(chunk_a, chunk_b));
+	ck_assert(chunk_equals_const(chunk_b, chunk_a));
 }
 END_TEST
 

src/libstrongswan/utils/chunk.h

@@ -362,16 +362,34 @@ static inline bool chunk_equals(chunk_t a, chunk_t b)
 }
 
 /**
- * Compare two chunks for equality, constant time for cryptographic purposes.
+ * Compare a secret chunk against some arbitrary input in constant time for
+ * cryptographic purposes.
  *
- * Note that this function is constant time only for chunks with the same
- * length, i.e. it does not protect against guessing the length of one of the
- * chunks.
+ * If neither chunk is NULL, this function is always constant time and bound by
+ * the secret's length.  If the length is known to be equal, the order of the
+ * arguments doesn't matter.
+ *
+ * @param secret        expected/reference value (loop bound)
+ * @param input         value to verify against secret
+ * @return              TRUE if the chunks are equal
  */
-static inline bool chunk_equals_const(chunk_t a, chunk_t b)
+static inline bool chunk_equals_const(chunk_t secret, chunk_t input)
 {
-	return a.ptr != NULL  && b.ptr != NULL &&
-			a.len == b.len && memeq_const(a.ptr, b.ptr, a.len);
+	u_int diff = 0;
+	size_t i;
+
+	if (!secret.ptr || !input.ptr)
+	{
+		return FALSE;
+	}
+	diff |= constant_time_neq64(secret.len, input.len);
+	for (i = 0; i < secret.len; i++)
+	{
+		/* the ternary here is OK as the input length is known to the attacker
+		 * and the overall time is always bound by the secret's length */
+		diff |= secret.ptr[i] ^ (i < input.len ? input.ptr[i]: 0);
+	}
+	return !diff;
 }
 
 /**

src/libtls/tls_peer.c

@@ -232,7 +232,7 @@ static status_t process_server_hello(private_tls_peer_t *this,
 		return NEED_MORE;
 	}
 
-	is_retry_request = chunk_equals_const(random, tls_hello_retry_request_magic);
+	is_retry_request = chunk_equals_const(tls_hello_retry_request_magic, random);
 
 	memcpy(this->server_random, random.ptr, sizeof(this->server_random));
 
@@ -1044,7 +1044,7 @@ static status_t process_finished(private_tls_peer_t *this, bio_reader_t *reader)
 		}
 	}
 
-	if (!chunk_equals_const(received, verify_data))
+	if (!chunk_equals_const(verify_data, received))
 	{
 		DBG1(DBG_TLS, "received server finished invalid");
 		this->alert->add(this->alert, TLS_FATAL, TLS_DECRYPT_ERROR);

src/libtls/tls_server.c

@@ -992,7 +992,7 @@ static status_t process_finished(private_tls_server_t *this,
 		this->crypto->change_cipher(this->crypto, TRUE);
 	}
 
-	if (!chunk_equals_const(received, verify_data))
+	if (!chunk_equals_const(verify_data, received))
 	{
 		DBG1(DBG_TLS, "received client finished invalid");
 		this->alert->add(this->alert, TLS_FATAL, TLS_DECRYPT_ERROR);

src/pki/commands/ocsp.c

@@ -106,7 +106,7 @@ static bool find_issuer_cacert(hash_algorithm_t hashAlgorithm,
 			break;
 		}
 
-		issuerKeyHash_ok = chunk_equals_const(issuerKeyHash, caKeyHash);
+		issuerKeyHash_ok = chunk_equals_const(caKeyHash, issuerKeyHash);
 		public->destroy(public);
 		if (!issuerKeyHash_ok)
 		{
@@ -132,7 +132,7 @@ static bool find_issuer_cacert(hash_algorithm_t hashAlgorithm,
 		}
 		hasher->destroy(hasher);
 
-		issuerNameHash_ok = chunk_equals_const(issuerNameHash, caNameHash);
+		issuerNameHash_ok = chunk_equals_const(caNameHash, issuerNameHash);
 		chunk_free(&caNameHash);
 		if (!issuerNameHash_ok)
 		{