| From 76968cdd6b79f6ae40d674554e902ced192fd33e Mon Sep 17 00:00:00 2001 |
| From: Tobias Brunner <tobias@strongswan.org> |
| 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, uint8_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, uint8_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, |
| uint32_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 |
| |