Merge branch 'proposal-checks'

Adds checks for proposals parsed from strings. For instance, the presence of DH, PRF and encryption algorithms for IKE are now enforced and AEAD and regular encryption algorithms are not allowed in the same proposal anymore. Also fixed is the mapping of the aes*gmac keywords to an integrity algorithm in AH proposals.
2016-10-05 14:59:31 +02:00
parent 597e057b9e 89054d9dcb
commit 26df5e9aa4
4 changed files with 206 additions and 48 deletions
@@ -419,25 +419,41 @@ static const struct {
 	{AUTH_AES_CMAC_96,					PRF_AES128_CMAC					},
 };

+/**
+ * Remove all entries of the given transform type
+ */
+static void remove_transform(private_proposal_t *this, transform_type_t type)
+{
+	enumerator_t *e;
+	entry_t *entry;
+
+	e = array_create_enumerator(this->transforms);
+	while (e->enumerate(e, &entry))
+	{
+		if (entry->type == type)
+		{
+			array_remove_at(this->transforms, e);
+		}
+	}
+	e->destroy(e);
+}
+
 /**
 * Checks the proposal read from a string.
 */
-static void check_proposal(private_proposal_t *this)
+static bool check_proposal(private_proposal_t *this)
 {
 	enumerator_t *e;
 	entry_t *entry;
 	uint16_t alg, ks;
-	bool all_aead = TRUE;
+	bool all_aead = TRUE, any_aead = FALSE, any_enc = FALSE;
 	int i;

 	if (this->protocol == PROTO_IKE)
 	{
-		e = create_enumerator(this, PSEUDO_RANDOM_FUNCTION);
-		if (!e->enumerate(e, &alg, &ks))
-		{
-			/* No explicit PRF found. We assume the same algorithm as used
-			 * for integrity checking */
-			e->destroy(e);
+		if (!get_algorithm(this, PSEUDO_RANDOM_FUNCTION, NULL, NULL))
+		{	/* No explicit PRF found. We assume the same algorithm as used
+			 * for integrity checking. */
 			e = create_enumerator(this, INTEGRITY_ALGORITHM);
 			while (e->enumerate(e, &alg, &ks))
 			{
@@ -451,8 +467,13 @@ static void check_proposal(private_proposal_t *this)
 					}
 				}
 			}
+			e->destroy(e);
+		}
+		if (!get_algorithm(this, PSEUDO_RANDOM_FUNCTION, NULL, NULL))
+		{
+			DBG1(DBG_CFG, "a PRF algorithm is mandatory in IKE proposals");
+			return FALSE;
 		}
-		e->destroy(e);
 		/* remove MODP_NONE from IKE proposal */
 		e = array_create_enumerator(this->transforms);
 		while (e->enumerate(e, &entry))
@@ -463,48 +484,103 @@ static void check_proposal(private_proposal_t *this)
 			}
 		}
 		e->destroy(e);
+		if (!get_algorithm(this, DIFFIE_HELLMAN_GROUP, NULL, NULL))
+		{
+			DBG1(DBG_CFG, "a DH group is mandatory in IKE proposals");
+			return FALSE;
+		}
+	}
+	else
+	{	/* remove PRFs from ESP/AH proposals */
+		remove_transform(this, PSEUDO_RANDOM_FUNCTION);
 	}

-	if (this->protocol == PROTO_ESP)
+	if (this->protocol == PROTO_IKE || this->protocol == PROTO_ESP)
 	{
 		e = create_enumerator(this, ENCRYPTION_ALGORITHM);
 		while (e->enumerate(e, &alg, &ks))
 		{
-			if (!encryption_algorithm_is_aead(alg))
+			any_enc = TRUE;
+			if (encryption_algorithm_is_aead(alg))
 			{
-				all_aead = FALSE;
-				break;
+				any_aead = TRUE;
+				continue;
+			}
+			all_aead = FALSE;
+		}
+		e->destroy(e);
+
+		if (!any_enc)
+		{
+			DBG1(DBG_CFG, "an encryption algorithm is mandatory in %N proposals",
+				 protocol_id_names, this->protocol);
+			return FALSE;
+		}
+		else if (any_aead && !all_aead)
+		{
+			DBG1(DBG_CFG, "classic and combined-mode (AEAD) encryption "
+				 "algorithms can't be contained in the same %N proposal",
+				 protocol_id_names, this->protocol);
+			return FALSE;
+		}
+		else if (all_aead)
+		{	/* if all encryption algorithms in the proposal are AEADs,
+			 * we MUST NOT propose any integrity algorithms */
+			remove_transform(this, INTEGRITY_ALGORITHM);
+		}
+	}
+	else
+	{	/* AES-GMAC is parsed as encryption algorithm, so we map that to the
+		 * proper integrity algorithm */
+		e = array_create_enumerator(this->transforms);
+		while (e->enumerate(e, &entry))
+		{
+			if (entry->type == ENCRYPTION_ALGORITHM)
+			{
+				if (entry->alg == ENCR_NULL_AUTH_AES_GMAC)
+				{
+					entry->type = INTEGRITY_ALGORITHM;
+					ks = entry->key_size;
+					entry->key_size = 0;
+					switch (ks)
+					{
+						case 128:
+							entry->alg = AUTH_AES_128_GMAC;
+							continue;
+						case 192:
+							entry->alg = AUTH_AES_192_GMAC;
+							continue;
+						case 256:
+							entry->alg = AUTH_AES_256_GMAC;
+							continue;
+						default:
+							break;
+					}
+				}
+				/* remove all other encryption algorithms */
+				array_remove_at(this->transforms, e);
 			}
 		}
 		e->destroy(e);

-		if (all_aead)
+		if (!get_algorithm(this, INTEGRITY_ALGORITHM, NULL, NULL))
 		{
-			/* if all encryption algorithms in the proposal are AEADs,
-			 * we MUST NOT propose any integrity algorithms */
-			e = array_create_enumerator(this->transforms);
-			while (e->enumerate(e, &entry))
-			{
-				if (entry->type == INTEGRITY_ALGORITHM)
-				{
-					array_remove_at(this->transforms, e);
-				}
-			}
-			e->destroy(e);
+			DBG1(DBG_CFG, "an integrity algorithm is mandatory in AH "
+				 "proposals");
+			return FALSE;
 		}
 	}

 	if (this->protocol == PROTO_AH || this->protocol == PROTO_ESP)
 	{
-		e = create_enumerator(this, EXTENDED_SEQUENCE_NUMBERS);
-		if (!e->enumerate(e, NULL, NULL))
+		if (!get_algorithm(this, EXTENDED_SEQUENCE_NUMBERS, NULL, NULL))
 		{	/* ESN not specified, assume not supported */
 			add_algorithm(this, EXTENDED_SEQUENCE_NUMBERS, NO_EXT_SEQ_NUMBERS, 0);
 		}
-		e->destroy(e);
 	}

 	array_compress(this->transforms);
+	return TRUE;
 }

 /**
@@ -1000,13 +1076,11 @@ proposal_t *proposal_create_from_string(protocol_id_t protocol, const char *algs
 	}
 	enumerator->destroy(enumerator);

-	if (failed)
+	if (failed || !check_proposal(this))
 	{
 		destroy(this);
 		return NULL;
 	}

-	check_proposal(this);
-
 	return &this->public;
 }
@@ -655,6 +655,7 @@ METHOD(task_t, build_r, status_t,
 		else
 		{
 			DBG1(DBG_IKE, "no acceptable proposal found");
+			message->add_notify(message, TRUE, NO_PROPOSAL_CHOSEN, chunk_empty);
 		}
 		return FAILED;
 	}
@@ -32,6 +32,22 @@ static test_configuration_t tests[] = {
 	{ .suite = NULL, }
 };

+static void initialize_logging()
+{
+	int level = LEVEL_SILENT;
+	char *verbosity;
+
+	verbosity = getenv("TESTS_VERBOSITY");
+	if (verbosity)
+	{
+		level = atoi(verbosity);
+	}
+	lib->settings->set_int(lib->settings, "%s.filelog.stderr.default",
+			lib->settings->get_int(lib->settings, "%s.filelog.stderr.default",
+								   level, lib->ns), lib->ns);
+	charon->load_loggers(charon, NULL, TRUE);
+}
+
 static bool test_runner_init(bool init)
 {
 	if (init)
@@ -39,6 +55,7 @@ static bool test_runner_init(bool init)
 		char *plugins, *plugindir;

 		libcharon_init();
+		initialize_logging();

 		plugins = getenv("TESTS_PLUGINS") ?:
 					lib->settings->get_str(lib->settings,
@@ -18,38 +18,100 @@
 #include <config/proposal.h>

 static struct {
+	protocol_id_t proto;
+	char *proposal;
+	char *expected;
+} create_data[] = {
+	{ PROTO_IKE, "", NULL },
+	{ PROTO_IKE, "sha256", NULL },
+	{ PROTO_IKE, "sha256-modp3072", NULL },
+	{ PROTO_IKE, "null-sha256-modp3072", "IKE:NULL/HMAC_SHA2_256_128/PRF_HMAC_SHA2_256/MODP_3072" },
+	{ PROTO_IKE, "aes128", NULL },
+	{ PROTO_IKE, "aes128-sha256", NULL },
+	{ PROTO_IKE, "aes128-sha256-modpnone", NULL },
+	{ PROTO_IKE, "aes128-sha256-modp3072", "IKE:AES_CBC_128/HMAC_SHA2_256_128/PRF_HMAC_SHA2_256/MODP_3072" },
+	{ PROTO_IKE, "aes128-sha256-prfsha384-modp3072", "IKE:AES_CBC_128/HMAC_SHA2_256_128/PRF_HMAC_SHA2_384/MODP_3072" },
+	{ PROTO_IKE, "aes128gcm16-modp3072", NULL },
+	{ PROTO_IKE, "aes128gcm16-prfsha256-modp3072", "IKE:AES_GCM_16_128/PRF_HMAC_SHA2_256/MODP_3072" },
+	{ PROTO_IKE, "aes128gcm16-sha256-modp3072", "IKE:AES_GCM_16_128/PRF_HMAC_SHA2_256/MODP_3072" },
+	{ PROTO_IKE, "aes128gcm16-aes128-modp3072", NULL },
+	{ PROTO_IKE, "aes128gcm16-aes128-sha256-modp3072", NULL },
+	{ PROTO_ESP, "", NULL },
+	{ PROTO_ESP, "sha256", NULL },
+	{ PROTO_ESP, "aes128-sha256", "ESP:AES_CBC_128/HMAC_SHA2_256_128/NO_EXT_SEQ" },
+	{ PROTO_ESP, "aes128-sha256-esn", "ESP:AES_CBC_128/HMAC_SHA2_256_128/EXT_SEQ" },
+	{ PROTO_ESP, "aes128-sha256-noesn", "ESP:AES_CBC_128/HMAC_SHA2_256_128/NO_EXT_SEQ" },
+	{ PROTO_ESP, "aes128-sha256-esn-noesn", "ESP:AES_CBC_128/HMAC_SHA2_256_128/EXT_SEQ/NO_EXT_SEQ" },
+	{ PROTO_ESP, "aes128-sha256-prfsha256-modp3072", "ESP:AES_CBC_128/HMAC_SHA2_256_128/MODP_3072/NO_EXT_SEQ" },
+	{ PROTO_ESP, "aes128gcm16-aes128-sha256-modp3072", NULL },
+	{ PROTO_ESP, "aes128gmac", "ESP:NULL_AES_GMAC_128/NO_EXT_SEQ" },
+	{ PROTO_AH,  "", NULL },
+	{ PROTO_AH,  "aes128", NULL },
+	{ PROTO_AH,  "aes128-sha256", "AH:HMAC_SHA2_256_128/NO_EXT_SEQ" },
+	{ PROTO_AH,  "sha256-sha1", "AH:HMAC_SHA2_256_128/HMAC_SHA1_96/NO_EXT_SEQ" },
+	{ PROTO_AH,  "aes128gmac-sha256", "AH:AES_128_GMAC/HMAC_SHA2_256_128/NO_EXT_SEQ" },
+	{ PROTO_AH,  "aes128gmac-sha256-prfsha256", "AH:AES_128_GMAC/HMAC_SHA2_256_128/NO_EXT_SEQ" },
+	{ PROTO_AH,  "aes128gmac-aes256gmac-aes128-sha256", "AH:AES_128_GMAC/AES_256_GMAC/HMAC_SHA2_256_128/NO_EXT_SEQ" },
+	{ PROTO_AH,  "sha256-esn", "AH:HMAC_SHA2_256_128/EXT_SEQ" },
+	{ PROTO_AH,  "sha256-noesn", "AH:HMAC_SHA2_256_128/NO_EXT_SEQ" },
+	{ PROTO_AH,  "sha256-esn-noesn", "AH:HMAC_SHA2_256_128/EXT_SEQ/NO_EXT_SEQ" },
+};
+
+START_TEST(test_create_from_string)
+{
+	proposal_t *proposal;
+	char str[BUF_LEN];
+
+	proposal = proposal_create_from_string(create_data[_i].proto,
+										   create_data[_i].proposal);
+	if (!create_data[_i].expected)
+	{
+		ck_assert(!proposal);
+		return;
+	}
+	snprintf(str, sizeof(str), "%P", proposal);
+	ck_assert_str_eq(create_data[_i].expected, str);
+	proposal->destroy(proposal);
+}
+END_TEST
+
+static struct {
+	protocol_id_t proto;
 	char *self;
 	char *other;
 	char *expected;
 } select_data[] = {
-	{ "aes128", "aes128", "aes128" },
-	{ "aes128", "aes256", NULL },
-	{ "aes128-aes256", "aes256-aes128", "aes128" },
-	{ "aes256-aes128", "aes128-aes256", "aes256" },
-	{ "aes128-aes256-sha1-sha256", "aes256-aes128-sha256-sha1", "aes128-sha1" },
-	{ "aes256-aes128-sha256-sha1", "aes128-aes256-sha1-sha256", "aes256-sha256" },
-	{ "aes128-sha256-modp3072", "aes128-sha256", NULL },
-	{ "aes128-sha256", "aes128-sha256-modp3072", NULL },
-	{ "aes128-sha256-modp3072", "aes128-sha256-modpnone", NULL },
-	{ "aes128-sha256-modpnone", "aes128-sha256-modp3072", NULL },
-	{ "aes128-sha256-modp3072-modpnone", "aes128-sha256", "aes128-sha256" },
-	{ "aes128-sha256", "aes128-sha256-modp3072-modpnone", "aes128-sha256" },
-	{ "aes128-sha256-modp3072-modpnone", "aes128-sha256-modpnone-modp3072", "aes128-sha256-modp3072" },
-	{ "aes128-sha256-modpnone-modp3072", "aes128-sha256-modp3072-modpnone", "aes128-sha256-modpnone" },
+	{ PROTO_ESP, "aes128", "aes128", "aes128" },
+	{ PROTO_ESP, "aes128", "aes256", NULL },
+	{ PROTO_ESP, "aes128-aes256", "aes256-aes128", "aes128" },
+	{ PROTO_ESP, "aes256-aes128", "aes128-aes256", "aes256" },
+	{ PROTO_ESP, "aes128-aes256-sha1-sha256", "aes256-aes128-sha256-sha1", "aes128-sha1" },
+	{ PROTO_ESP, "aes256-aes128-sha256-sha1", "aes128-aes256-sha1-sha256", "aes256-sha256" },
+	{ PROTO_ESP, "aes128-sha256-modp3072", "aes128-sha256", NULL },
+	{ PROTO_ESP, "aes128-sha256", "aes128-sha256-modp3072", NULL },
+	{ PROTO_ESP, "aes128-sha256-modp3072", "aes128-sha256-modpnone", NULL },
+	{ PROTO_ESP, "aes128-sha256-modpnone", "aes128-sha256-modp3072", NULL },
+	{ PROTO_ESP, "aes128-sha256-modp3072-modpnone", "aes128-sha256", "aes128-sha256" },
+	{ PROTO_ESP, "aes128-sha256", "aes128-sha256-modp3072-modpnone", "aes128-sha256" },
+	{ PROTO_ESP, "aes128-sha256-modp3072-modpnone", "aes128-sha256-modpnone-modp3072", "aes128-sha256-modp3072" },
+	{ PROTO_ESP, "aes128-sha256-modpnone-modp3072", "aes128-sha256-modp3072-modpnone", "aes128-sha256-modpnone" },
+	{ PROTO_IKE, "aes128-sha256-modp3072", "aes128-sha256-modp3072", "aes128-sha256-modp3072" },
+	{ PROTO_IKE, "aes128-sha256-modp3072", "aes128-sha256-modp3072-modpnone", "aes128-sha256-modp3072" },
+	{ PROTO_IKE, "aes128-sha256-modp3072-modpnone", "aes128-sha256-modp3072", "aes128-sha256-modp3072" },
 };

 START_TEST(test_select)
 {
 	proposal_t *self, *other, *selected, *expected;

-	self = proposal_create_from_string(PROTO_ESP,
+	self = proposal_create_from_string(select_data[_i].proto,
 									   select_data[_i].self);
-	other = proposal_create_from_string(PROTO_ESP,
+	other = proposal_create_from_string(select_data[_i].proto,
 										select_data[_i].other);
 	selected = self->select(self, other, FALSE);
 	if (select_data[_i].expected)
 	{
-		expected = proposal_create_from_string(PROTO_ESP,
+		expected = proposal_create_from_string(select_data[_i].proto,
 											   select_data[_i].expected);
 		ck_assert(selected);
 		ck_assert_msg(expected->equals(expected, selected), "proposal %P does "
@@ -73,6 +135,10 @@ Suite *proposal_suite_create()

 	s = suite_create("proposal");

+	tc = tcase_create("create_from_string");
+	tcase_add_loop_test(tc, test_create_from_string, 0, countof(create_data));
+	suite_add_tcase(s, tc);
+
 	tc = tcase_create("select");
 	tcase_add_loop_test(tc, test_select, 0, countof(select_data));
 	suite_add_tcase(s, tc);