From 76c7c951e11f7a4ed637c174c9f5b8dc22f2241a Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 23 Feb 2018 09:02:49 +0100 Subject: [PATCH] proposal: Make sure to consider all transform types when selecting proposals This way there will be a mismatch if one of the proposals contains transform types not contained in the other (the fix list of transform types used previously resulted in a match if unknown transform types were contained in one of the proposals). Merging the sets of types makes comparing proposals with optional transform types easier (e.g. DH for ESP with MODP_NONE). --- src/libstrongswan/crypto/proposal/proposal.c | 61 ++++++++++++++----- .../tests/suites/test_proposal.c | 59 +++++++++++++++++- 2 files changed, 103 insertions(+), 17 deletions(-) diff --git a/src/libstrongswan/crypto/proposal/proposal.c b/src/libstrongswan/crypto/proposal/proposal.c index 7e5486577..1fb6ee6d9 100644 --- a/src/libstrongswan/crypto/proposal/proposal.c +++ b/src/libstrongswan/crypto/proposal/proposal.c @@ -111,23 +111,49 @@ static int type_find(const void *a, const void *b) /** * Check if the given transform type is already in the set */ -static bool contains_type(private_proposal_t *this, transform_type_t type) +static bool contains_type(array_t *types, transform_type_t type) { - return array_bsearch(this->types, &type, type_find, NULL) != -1; + return array_bsearch(types, &type, type_find, NULL) != -1; } /** * Add the given transform type to the set */ -static void add_type(private_proposal_t *this, transform_type_t type) +static void add_type(array_t *types, transform_type_t type) { - if (!contains_type(this, type)) + if (!contains_type(types, type)) { - array_insert(this->types, ARRAY_TAIL, &type); - array_sort(this->types, type_sort, NULL); + array_insert(types, ARRAY_TAIL, &type); + array_sort(types, type_sort, NULL); } } +/** + * Merge two sets of transform types into a new array + */ +static array_t *merge_types(private_proposal_t *this, private_proposal_t *other) +{ + array_t *types; + transform_type_t type; + int i, count; + + count = max(array_count(this->types), array_count(other->types)); + types = array_create(sizeof(transform_type_t), count); + + for (i = 0; i < count; i++) + { + if (array_get(this->types, i, &type)) + { + add_type(types, type); + } + if (array_get(other->types, i, &type)) + { + add_type(types, type); + } + } + return types; +} + /** * Remove the given transform type from the set */ @@ -165,7 +191,7 @@ METHOD(proposal_t, add_algorithm, void, }; array_insert(this->transforms, ARRAY_TAIL, &entry); - add_type(this, type); + add_type(this->types, type); } CALLBACK(alg_filter, bool, @@ -397,6 +423,9 @@ METHOD(proposal_t, select_proposal, proposal_t*, bool private) { proposal_t *selected; + transform_type_t type; + array_t *types; + int i; DBG2(DBG_CFG, "selecting proposal:"); @@ -415,18 +444,20 @@ METHOD(proposal_t, select_proposal, proposal_t*, { selected = proposal_create(this->protocol, this->number); selected->set_spi(selected, this->spi); - } - if (!select_algo(this, other, selected, ENCRYPTION_ALGORITHM, private) || - !select_algo(this, other, selected, PSEUDO_RANDOM_FUNCTION, private) || - !select_algo(this, other, selected, INTEGRITY_ALGORITHM, private) || - !select_algo(this, other, selected, DIFFIE_HELLMAN_GROUP, private) || - !select_algo(this, other, selected, EXTENDED_SEQUENCE_NUMBERS, private)) + types = merge_types(this, (private_proposal_t*)other); + for (i = 0; i < array_count(types); i++) { - selected->destroy(selected); - return NULL; + array_get(types, i, &type); + if (!select_algo(this, other, selected, type, private)) + { + selected->destroy(selected); + array_destroy(types); + return NULL; + } } + array_destroy(types); DBG2(DBG_CFG, " proposal matches"); return selected; diff --git a/src/libstrongswan/tests/suites/test_proposal.c b/src/libstrongswan/tests/suites/test_proposal.c index 947c32159..9f8cc7e1f 100644 --- a/src/libstrongswan/tests/suites/test_proposal.c +++ b/src/libstrongswan/tests/suites/test_proposal.c @@ -204,14 +204,66 @@ START_TEST(test_unknown_transform_types_print) proposal->destroy(proposal); proposal = proposal_create_from_string(PROTO_IKE, - "aes128-sha256-modp3072-ecp256"); + "aes128-sha256-ecp256"); proposal->add_algorithm(proposal, 242, 42, 128); proposal->add_algorithm(proposal, 243, 1, 0); - assert_proposal_eq(proposal, "IKE:AES_CBC_128/HMAC_SHA2_256_128/PRF_HMAC_SHA2_256/MODP_3072/ECP_256/UNKNOWN_242_42_128/UNKNOWN_243_1"); + assert_proposal_eq(proposal, "IKE:AES_CBC_128/HMAC_SHA2_256_128/PRF_HMAC_SHA2_256/ECP_256/UNKNOWN_242_42_128/UNKNOWN_243_1"); proposal->destroy(proposal); } END_TEST +START_TEST(test_unknown_transform_types_select_fail) +{ + proposal_t *self, *other, *selected; + + self = proposal_create_from_string(PROTO_IKE, "aes128-sha256-ecp256"); + other = proposal_create_from_string(PROTO_IKE, "aes128-sha256-ecp256"); + other->add_algorithm(other, 242, 42, 0); + + selected = self->select(self, other, TRUE, FALSE); + ck_assert(!selected); + other->destroy(other); + self->destroy(self); +} +END_TEST + +START_TEST(test_unknown_transform_types_select_fail_subtype) +{ + proposal_t *self, *other, *selected; + + self = proposal_create_from_string(PROTO_IKE, "aes128-sha256-ecp256"); + self->add_algorithm(self, 242, 8, 0); + other = proposal_create_from_string(PROTO_IKE, "aes128-sha256-ecp256"); + other->add_algorithm(other, 242, 42, 0); + + selected = self->select(self, other, TRUE, FALSE); + ck_assert(!selected); + other->destroy(other); + self->destroy(self); +} +END_TEST + +START_TEST(test_unknown_transform_types_select_success) +{ + proposal_t *self, *other, *selected; + + self = proposal_create_from_string(PROTO_IKE, "aes128-sha256-ecp256"); + self->add_algorithm(self, 242, 42, 128); + other = proposal_create_from_string(PROTO_IKE, "aes128-sha256-ecp256"); + other->add_algorithm(other, 242, 42, 128); + other->add_algorithm(other, 242, 1, 0); + + selected = self->select(self, other, TRUE, FALSE); + ck_assert(selected); + assert_proposal_eq(selected, "IKE:AES_CBC_128/HMAC_SHA2_256_128/PRF_HMAC_SHA2_256/ECP_256/UNKNOWN_242_42_128"); + selected->destroy(selected); + other->destroy(other); + self->destroy(self); +} +END_TEST + + + Suite *proposal_suite_create() { Suite *s; @@ -236,6 +288,9 @@ Suite *proposal_suite_create() tc = tcase_create("unknown transform types"); tcase_add_test(tc, test_unknown_transform_types_print); + tcase_add_test(tc, test_unknown_transform_types_select_fail); + tcase_add_test(tc, test_unknown_transform_types_select_fail_subtype); + tcase_add_test(tc, test_unknown_transform_types_select_success); suite_add_tcase(s, tc); return s;