From: Arran Cudbard-Bell Date: Tue, 8 Jan 2019 08:19:52 +0000 (+0800) Subject: Fix EAP-Session initialisation X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=54b28badbdcd8e68b073d2a19ec7915582195c0d;p=thirdparty%2Ffreeradius-server.git Fix EAP-Session initialisation The presence of the state attribute in the request is what dictates whether we're starting a new session or not. *NOT* the fact that the initial packet is an EAP-Identity response. --- diff --git a/src/modules/rlm_eap/eap.c b/src/modules/rlm_eap/eap.c index 2a3fb2c0544..753079d9c37 100644 --- a/src/modules/rlm_eap/eap.c +++ b/src/modules/rlm_eap/eap.c @@ -828,7 +828,7 @@ eap_session_t *eap_session_continue(eap_packet_raw_t **eap_packet_p, rlm_eap_t c { eap_session_t *eap_session = NULL; eap_packet_raw_t *eap_packet; - VALUE_PAIR *vp; + VALUE_PAIR *state, *user; /* * Ensure it's a valid EAP-Request, or EAP-Response. @@ -843,21 +843,71 @@ eap_session_t *eap_session_continue(eap_packet_raw_t **eap_packet_p, rlm_eap_t c eap_packet = *eap_packet_p; /* - * eap_session_t MUST be found in the list if it is not - * EAP-Identity response + * RFC 3579 - Once EAP has been negotiated, the NAS SHOULD + * send an initial EAP-Request message to the authenticating + * peer. This will typically be an EAP-Request/Identity, + * although it could be an EAP-Request for an authentication + * method (Types 4 and greater). + * + * This means that if there is no State attribute, we should + * consider this as the start of a new session. */ - if (eap_packet->data[0] != FR_EAP_IDENTITY) { - eap_session = eap_session_thaw(request); - if (!eap_session) { - vp = fr_pair_find_by_da(request->packet->vps, attr_state, TAG_ANY); - if (!vp) { - REDEBUG("EAP requires the State attribute to work, but no State exists in the Access-Request packet."); - REDEBUG("The RADIUS client is broken. No amount of changing FreeRADIUS will fix the RADIUS client."); + state = fr_pair_find_by_da(request->packet->vps, attr_state, TAG_ANY); + if (!state) { + eap_session = eap_session_alloc(inst, request); + if (!eap_session) goto error_round; + + RDEBUG4("New eap_session_t %p", eap_session); + + /* + * All fields in the eap_session are set to zero. + */ + switch (eap_packet->data[0]) { + case FR_EAP_IDENTITY: + eap_session->identity = eap_identity(request, eap_session, eap_packet); + if (!eap_session->identity) { + RDEBUG("Invalid identity response"); + goto error_session; } + break; - goto error_round; + case FR_EAP_INVALID: + case FR_EAP_NOTIFICATION: + case FR_EAP_NAK: + RDEBUG("Initial EAP method (%u) invalid", eap_packet->data[0]); + goto error_session; + + /* + * Initialise a zero length identity, as we've + * not been provided with one at the start of the + * EAP method. + */ + default: + eap_session->identity = talloc_bstrndup(eap_session, "", 0); + break; } + /* + * If the index is removed by something else + * like the state being cleaned up, then we + * still want the eap_session to be freed, which + * is why we set free_opaque to true. + * + * We must pass a NULL pointer to associate the + * the EAP_SESSION data with, else we'll break + * tunneled EAP, where the inner EAP module is + * a different instance to the outer one. + */ + request_data_talloc_add(request, NULL, REQUEST_DATA_EAP_SESSION, eap_session_t, + eap_session, true, true, true); + + /* + * Continue a previously started EAP-Session + */ + } else { + eap_session = eap_session_thaw(request); + if (!eap_session) goto error_round; + RDEBUG4("Got eap_session_t %p from request data", eap_session); (void) talloc_get_type_abort(eap_session, eap_session_t); eap_session->rounds++; @@ -885,41 +935,10 @@ eap_session_t *eap_session_continue(eap_packet_raw_t **eap_packet_p, rlm_eap_t c RERROR("Your Supplicant or NAS is probably broken"); goto error_round; } - /* - * Packet was EAP identity, allocate a new eap_session. - */ - } else { - eap_session = eap_session_alloc(inst, request); - if (!eap_session) goto error_round; - - RDEBUG4("New eap_session_t %p", eap_session); - - /* - * All fields in the eap_session are set to zero. - */ - eap_session->identity = eap_identity(request, eap_session, eap_packet); - if (!eap_session->identity) { - RDEBUG("Identity Unknown, authentication failed"); - goto error_session; - } - - /* - * If the index is removed by something else - * like the state being cleaned up, then we - * still want the eap_session to be freed, which - * is why we set free_opaque to true. - * - * We must pass a NULL pointer to associate the - * the EAP_SESSION data with, else we'll break - * tunneled EAP, where the inner EAP module is - * a different instance to the outer one. - */ - request_data_talloc_add(request, NULL, REQUEST_DATA_EAP_SESSION, eap_session_t, - eap_session, true, true, true); } - vp = fr_pair_find_by_da(request->packet->vps, attr_user_name, TAG_ANY); - if (!vp) { + user = fr_pair_find_by_da(request->packet->vps, attr_user_name, TAG_ANY); + if (!user) { /* * NAS did not set the User-Name * attribute, so we set it here and @@ -928,8 +947,8 @@ eap_session_t *eap_session_continue(eap_packet_raw_t **eap_packet_p, rlm_eap_t c * correctly */ RDEBUG2("Broken NAS did not set User-Name, setting from EAP Identity"); - MEM(pair_add_request(&vp, attr_user_name) >= 0); - fr_pair_value_bstrncpy(vp, eap_session->identity, talloc_array_length(eap_session->identity) - 1); + MEM(pair_add_request(&user, attr_user_name) >= 0); + fr_pair_value_bstrncpy(user, eap_session->identity, talloc_array_length(eap_session->identity) - 1); } else { /* * A little more paranoia. If the NAS @@ -940,9 +959,9 @@ eap_session_t *eap_session_continue(eap_packet_raw_t **eap_packet_p, rlm_eap_t c * request as the NAS is doing something * funny. */ - if (talloc_memcmp_bstr(eap_session->identity, vp->vp_strvalue) != 0) { + if (talloc_memcmp_bstr(eap_session->identity, user->vp_strvalue) != 0) { REDEBUG("Identity from EAP Identity-Response \"%s\" does not match User-Name attribute \"%s\"", - eap_session->identity, vp->vp_strvalue); + eap_session->identity, user->vp_strvalue); goto error_round; } }