From 76968cdd6b79f6ae40d674554e902ced192fd33e Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 14 Dec 2021 10:51:35 +0100 Subject: [PATCH] eap-authenticator: Enforce failure if MSK generation fails Without this, the authentication succeeded if the server sent an early EAP-Success message for mutual, key-generating EAP methods like EAP-TLS, which may be used in EAP-only scenarios but would complete without server or client authentication. For clients configured for such EAP-only scenarios, a rogue server could capture traffic after the tunnel is established or even access hosts behind the client. For non-mutual EAP methods, public key server authentication has been enforced for a while. A server previously could also crash a client by sending an EAP-Success immediately without initiating an actual EAP method. Fixes: 0706c39cda52 ("added support for EAP methods not establishing an MSK") Fixes: CVE-2021-45079 --- src/libcharon/plugins/eap_gtc/eap_gtc.c | 2 +- src/libcharon/plugins/eap_md5/eap_md5.c | 2 +- src/libcharon/plugins/eap_radius/eap_radius.c | 4 ++- src/libcharon/sa/eap/eap_method.h | 8 ++++- .../ikev2/authenticators/eap_authenticator.c | 32 ++++++++++++++++--- 5 files changed, 40 insertions(+), 8 deletions(-) diff --git a/src/libcharon/plugins/eap_gtc/eap_gtc.c b/src/libcharon/plugins/eap_gtc/eap_gtc.c index 95ba090b79ce..cffb6222c2f8 100644 --- a/src/libcharon/plugins/eap_gtc/eap_gtc.c +++ b/src/libcharon/plugins/eap_gtc/eap_gtc.c @@ -195,7 +195,7 @@ METHOD(eap_method_t, get_type, eap_type_t, METHOD(eap_method_t, get_msk, status_t, private_eap_gtc_t *this, chunk_t *msk) { - return FAILED; + return NOT_SUPPORTED; } METHOD(eap_method_t, get_identifier, u_int8_t, diff --git a/src/libcharon/plugins/eap_md5/eap_md5.c b/src/libcharon/plugins/eap_md5/eap_md5.c index ab5f7ff6a823..3a92ad7c0a04 100644 --- a/src/libcharon/plugins/eap_md5/eap_md5.c +++ b/src/libcharon/plugins/eap_md5/eap_md5.c @@ -213,7 +213,7 @@ METHOD(eap_method_t, get_type, eap_type_t, METHOD(eap_method_t, get_msk, status_t, private_eap_md5_t *this, chunk_t *msk) { - return FAILED; + return NOT_SUPPORTED; } METHOD(eap_method_t, is_mutual, bool, diff --git a/src/libcharon/plugins/eap_radius/eap_radius.c b/src/libcharon/plugins/eap_radius/eap_radius.c index 2dc7a423e702..5336dead13d9 100644 --- a/src/libcharon/plugins/eap_radius/eap_radius.c +++ b/src/libcharon/plugins/eap_radius/eap_radius.c @@ -733,7 +733,9 @@ METHOD(eap_method_t, get_msk, status_t, *out = msk; return SUCCESS; } - return FAILED; + /* we assume the selected method did not establish an MSK, if it failed + * to establish one, process() would have failed */ + return NOT_SUPPORTED; } METHOD(eap_method_t, get_identifier, u_int8_t, diff --git a/src/libcharon/sa/eap/eap_method.h b/src/libcharon/sa/eap/eap_method.h index 0b5218dfec15..33564831f86e 100644 --- a/src/libcharon/sa/eap/eap_method.h +++ b/src/libcharon/sa/eap/eap_method.h @@ -114,10 +114,16 @@ struct eap_method_t { * Not all EAP methods establish a shared secret. For implementations of * the EAP-Identity method, get_msk() returns the received identity. * + * @note Returning NOT_SUPPORTED is important for implementations of EAP + * methods that don't establish an MSK. In particular as client because + * key-generating EAP methods MUST fail to process EAP-Success messages if + * no MSK is established. + * * @param msk chunk receiving internal stored MSK * @return - * - SUCCESS, or + * - SUCCESS, if MSK is established * - FAILED, if MSK not established (yet) + * - NOT_SUPPORTED, for non-MSK-establishing methods */ status_t (*get_msk) (eap_method_t *this, chunk_t *msk); diff --git a/src/libcharon/sa/ikev2/authenticators/eap_authenticator.c b/src/libcharon/sa/ikev2/authenticators/eap_authenticator.c index e1e6cd7ee6f3..87548fc471a6 100644 --- a/src/libcharon/sa/ikev2/authenticators/eap_authenticator.c +++ b/src/libcharon/sa/ikev2/authenticators/eap_authenticator.c @@ -305,9 +305,17 @@ static eap_payload_t* server_process_eap(private_eap_authenticator_t *this, this->method->destroy(this->method); return server_initiate_eap(this, FALSE); } - if (this->method->get_msk(this->method, &this->msk) == SUCCESS) + switch (this->method->get_msk(this->method, &this->msk)) { - this->msk = chunk_clone(this->msk); + case SUCCESS: + this->msk = chunk_clone(this->msk); + break; + case NOT_SUPPORTED: + break; + case FAILED: + default: + DBG1(DBG_IKE, "failed to establish MSK"); + goto failure; } if (vendor) { @@ -326,6 +334,7 @@ static eap_payload_t* server_process_eap(private_eap_authenticator_t *this, return eap_payload_create_code(EAP_SUCCESS, in->get_identifier(in)); case FAILED: default: +failure: /* type might have changed for virtual methods */ type = this->method->get_type(this->method, &vendor); if (vendor) @@ -661,9 +670,24 @@ METHOD(authenticator_t, process_client, status_t, u_int32_t vendor; auth_cfg_t *cfg; - if (this->method->get_msk(this->method, &this->msk) == SUCCESS) + if (!this->method) { - this->msk = chunk_clone(this->msk); + DBG1(DBG_IKE, "received unexpected %N", + eap_code_names, eap_payload->get_code(eap_payload)); + return FAILED; + } + switch (this->method->get_msk(this->method, &this->msk)) + { + case SUCCESS: + this->msk = chunk_clone(this->msk); + break; + case NOT_SUPPORTED: + break; + case FAILED: + default: + DBG1(DBG_IKE, "received %N but failed to establish MSK", + eap_code_names, eap_payload->get_code(eap_payload)); + return FAILED; } type = this->method->get_type(this->method, &vendor); if (vendor) -- 2.25.1