From: Tobias Brunner Date: Fri, 16 Jul 2021 15:39:12 +0000 (+0200) Subject: ikev2: Delay IKE key derivation until next message X-Git-Tag: 5.9.7dr2~1^2~10 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=44629bbadbd4d7f0c3ca5f8f73237b1b49b1f6f3;p=thirdparty%2Fstrongswan.git ikev2: Delay IKE key derivation until next message In particular as responder, this delays costly cryptographic operations until the IKE_AUTH request is received, which is preferable to reduce the impact of DoS attacks. Another advantage is that the key material is not changed until all tasks built or processed a message. --- diff --git a/src/libcharon/sa/ikev2/task_manager_v2.c b/src/libcharon/sa/ikev2/task_manager_v2.c index 2e29f027d4..f1228ab072 100644 --- a/src/libcharon/sa/ikev2/task_manager_v2.c +++ b/src/libcharon/sa/ikev2/task_manager_v2.c @@ -475,6 +475,40 @@ METHOD(task_manager_t, retransmit, status_t, return INVALID_STATE; } +/** + * Derive IKE keys if necessary + */ +static bool derive_keys(private_task_manager_t *this, array_t *tasks) +{ + enumerator_t *enumerator; + task_t *task; + + enumerator = array_create_enumerator(tasks); + while (enumerator->enumerate(enumerator, (void*)&task)) + { + if (task->get_type(task) == TASK_IKE_INIT) + { + ike_init_t *ike_init = (ike_init_t*)task; + + switch (ike_init->derive_keys(ike_init)) + { + case SUCCESS: + array_remove_at(tasks, enumerator); + task->destroy(task); + break; + case NEED_MORE: + break; + default: + enumerator->destroy(enumerator); + return FALSE; + } + break; + } + } + enumerator->destroy(enumerator); + return TRUE; +} + METHOD(task_manager_t, initiate, status_t, private_task_manager_t *this) { @@ -609,6 +643,11 @@ METHOD(task_manager_t, initiate, status_t, } else { + if (!derive_keys(this, this->active_tasks)) + { + return DESTROY_ME; + } + DBG2(DBG_IKE, "reinitiating already active tasks"); enumerator = array_create_enumerator(this->active_tasks); while (enumerator->enumerate(enumerator, &task)) @@ -1531,6 +1570,36 @@ static status_t send_invalid_syntax(private_task_manager_t *this, return DESTROY_ME; } +/** + * Check for unsupported critical payloads + */ +static status_t has_unsupported_critical_payload(message_t *msg, uint8_t *type) +{ + enumerator_t *enumerator; + unknown_payload_t *unknown; + payload_t *payload; + status_t status = SUCCESS; + + enumerator = msg->create_payload_enumerator(msg); + while (enumerator->enumerate(enumerator, &payload)) + { + if (payload->get_type(payload) == PL_UNKNOWN) + { + unknown = (unknown_payload_t*)payload; + if (unknown->is_critical(unknown)) + { + *type = unknown->get_type(unknown); + DBG1(DBG_ENC, "payload type %N is not supported, " + "but payload is critical!", payload_type_names, *type); + status = NOT_SUPPORTED; + break; + } + } + } + enumerator->destroy(enumerator); + return status; +} + /** * Parse the given message and verify that it is valid. */ @@ -1539,34 +1608,22 @@ static status_t parse_message(private_task_manager_t *this, message_t *msg) status_t parse_status, status; uint8_t type = 0; - parse_status = msg->parse_body(msg, this->ike_sa->get_keymat(this->ike_sa)); - - if (parse_status == SUCCESS) - { /* check for unsupported critical payloads */ - enumerator_t *enumerator; - unknown_payload_t *unknown; - payload_t *payload; + if (derive_keys(this, this->passive_tasks)) + { + parse_status = msg->parse_body(msg, this->ike_sa->get_keymat(this->ike_sa)); - enumerator = msg->create_payload_enumerator(msg); - while (enumerator->enumerate(enumerator, &payload)) + if (parse_status == SUCCESS) { - if (payload->get_type(payload) == PL_UNKNOWN) - { - unknown = (unknown_payload_t*)payload; - if (unknown->is_critical(unknown)) - { - type = unknown->get_type(unknown); - DBG1(DBG_ENC, "payload type %N is not supported, " - "but payload is critical!", payload_type_names, type); - parse_status = NOT_SUPPORTED; - break; - } - } + parse_status = has_unsupported_critical_payload(msg, &type); } - enumerator->destroy(enumerator); - } - status = parse_status; + status = parse_status; + } + else + { /* there is no point in trying again */ + parse_status = INVALID_STATE; + status = DESTROY_ME; + } if (parse_status != SUCCESS) { diff --git a/src/libcharon/sa/ikev2/tasks/ike_init.c b/src/libcharon/sa/ikev2/tasks/ike_init.c index cae68559fc..a73c98ac9d 100644 --- a/src/libcharon/sa/ikev2/tasks/ike_init.c +++ b/src/libcharon/sa/ikev2/tasks/ike_init.c @@ -55,6 +55,16 @@ struct private_ike_init_t { */ bool initiator; + /** + * Whether the key exchange is done + */ + bool ke_done; + + /** + * Whether keys have already been derived + */ + bool ke_derived; + /** * diffie hellman group to use */ @@ -520,6 +530,7 @@ static void process_payloads(private_ike_init_t *this, message_t *message) { enumerator_t *enumerator; payload_t *payload; + ike_sa_id_t *id; ke_payload_t *ke_payload = NULL; enumerator = message->create_payload_enumerator(message); @@ -615,6 +626,21 @@ static void process_payloads(private_ike_init_t *this, message_t *message) if (this->proposal) { this->ike_sa->set_proposal(this->ike_sa, this->proposal); + + if (this->old_sa) + { /* retrieve SPI of new IKE_SA when rekeying */ + id = this->ike_sa->get_id(this->ike_sa); + if (this->initiator) + { + id->set_responder_spi(id, + this->proposal->get_spi(this->proposal)); + } + else + { + id->set_initiator_spi(id, + this->proposal->get_spi(this->proposal)); + } + } } if (ke_payload && this->proposal && @@ -769,8 +795,8 @@ METHOD(task_t, process_r, status_t, /** * Derive the keymat for the IKE_SA */ -static bool derive_keys(private_ike_init_t *this, - chunk_t nonce_i, chunk_t nonce_r) +static bool derive_keys_internal(private_ike_init_t *this, chunk_t nonce_i, + chunk_t nonce_r) { keymat_v2_t *old_keymat; pseudo_random_function_t prf_alg = PRF_UNDEFINED; @@ -783,14 +809,6 @@ static bool derive_keys(private_ike_init_t *this, /* rekeying: Include old SKd, use old PRF, apply SPI */ old_keymat = (keymat_v2_t*)this->old_sa->get_keymat(this->old_sa); prf_alg = old_keymat->get_skd(old_keymat, &skd); - if (this->initiator) - { - id->set_responder_spi(id, this->proposal->get_spi(this->proposal)); - } - else - { - id->set_initiator_spi(id, this->proposal->get_spi(this->proposal)); - } } if (!this->keymat->derive_ike_keys(this->keymat, this->proposal, this->dh, nonce_i, nonce_r, id, prf_alg, skd)) @@ -802,6 +820,35 @@ static bool derive_keys(private_ike_init_t *this, return TRUE; } +METHOD(ike_init_t, derive_keys, status_t, + private_ike_init_t *this) +{ + bool success; + + if (!this->ke_done || this->ke_derived) + { + return NEED_MORE; + } + + if (this->initiator) + { + success = derive_keys_internal(this, this->my_nonce, this->other_nonce); + } + else + { + success = derive_keys_internal(this, this->other_nonce, this->my_nonce); + } + + this->ke_derived = TRUE; + + if (!success) + { + DBG1(DBG_IKE, "key derivation failed"); + return FAILED; + } + return SUCCESS; +} + METHOD(task_t, build_r, status_t, private_ike_init_t *this, message_t *message) { @@ -864,18 +911,25 @@ METHOD(task_t, build_r, status_t, return FAILED; } - if (!derive_keys(this, this->other_nonce, this->my_nonce)) + if (!build_payloads(this, message)) { - DBG1(DBG_IKE, "key derivation failed"); message->add_notify(message, TRUE, NO_PROPOSAL_CHOSEN, chunk_empty); return FAILED; } - if (!build_payloads(this, message)) + this->ke_done = TRUE; + + if (this->old_sa) { - message->add_notify(message, TRUE, NO_PROPOSAL_CHOSEN, chunk_empty); - return FAILED; + /* during rekeying, we derive keys here directly */ + if (derive_keys(this) != SUCCESS) + { + message->add_notify(message, FALSE, NO_PROPOSAL_CHOSEN, chunk_empty); + return FAILED; + } + return SUCCESS; } - return SUCCESS; + /* key derivation is done before the next request is processed */ + return NEED_MORE; } /** @@ -1087,13 +1141,15 @@ METHOD(task_t, process_i, status_t, DBG1(DBG_IKE, "applying DH public value failed"); return FAILED; } + this->ke_done = TRUE; - if (!derive_keys(this, this->my_nonce, this->other_nonce)) + if (this->old_sa) { - DBG1(DBG_IKE, "key derivation failed"); - return FAILED; + /* during rekeying, we derive keys here directly */ + return derive_keys(this); } - return SUCCESS; + /* key derivation is done before we send the next message */ + return NEED_MORE; } METHOD(task_t, get_type, task_type_t, @@ -1107,6 +1163,8 @@ METHOD(task_t, migrate, void, { DESTROY_IF(this->proposal); chunk_free(&this->other_nonce); + this->ke_done = FALSE; + this->ke_derived = FALSE; this->ike_sa = ike_sa; this->keymat = (keymat_v2_t*)ike_sa->get_keymat(ike_sa); @@ -1154,6 +1212,7 @@ ike_init_t *ike_init_create(ike_sa_t *ike_sa, bool initiator, ike_sa_t *old_sa) .migrate = _migrate, .destroy = _destroy, }, + .derive_keys = _derive_keys, .get_lower_nonce = _get_lower_nonce, }, .ike_sa = ike_sa, diff --git a/src/libcharon/sa/ikev2/tasks/ike_init.h b/src/libcharon/sa/ikev2/tasks/ike_init.h index 6f2e47a6cd..eea4e58e0c 100644 --- a/src/libcharon/sa/ikev2/tasks/ike_init.h +++ b/src/libcharon/sa/ikev2/tasks/ike_init.h @@ -40,6 +40,18 @@ struct ike_init_t { */ task_t task; + /** + * Derive the keys for this IKE_SA when not used during a rekeying. + * + * @return + * - FAILED if key derivation failed + * - NEED_MORE if key drivation was successful but more + * key exchanges are necessary + * - SUCCESS if key derivation was successful and the task + * can be removed from the queue + */ + status_t (*derive_keys)(ike_init_t *this); + /** * Get the lower of the two nonces, used for rekey collisions. *