From: Antonio Torres Date: Fri, 09 Dec 2022 Subject: Fix crash on unknown option in EAP-SIM When an EAP-SIM supplicant sends an unknown SIM option, the server will try to look that option up in the internal dictionaries. This lookup will fail, but the SIM code will not check for that failure. Instead, it will dereference a NULL pointer, and cause the server to crash. Backport of: https://github.com/FreeRADIUS/freeradius-server/commit/f1cdbb33ec61c4a64a https://github.com/FreeRADIUS/freeradius-server/commit/71128cac3ee236a88a05cc7bddd43e43a88a3089 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2151704 Signed-off-by: Antonio Torres --- diff --git a/src/modules/rlm_eap/libeap/eapsimlib.c b/src/modules/rlm_eap/libeap/eapsimlib.c index cf1e8a7dd92..e438a844eab 100644 --- a/src/modules/rlm_eap/libeap/eapsimlib.c +++ b/src/modules/rlm_eap/libeap/eapsimlib.c @@ -307,42 +307,77 @@ int unmap_eapsim_basictypes(RADIUS_PACKET *r, newvp->vp_length = 1; fr_pair_add(&(r->vps), newvp); + /* + * EAP-SIM has a 1 octet of subtype, and 2 octets + * reserved. + */ attr += 3; attrlen -= 3; - /* now, loop processing each attribute that we find */ - while(attrlen > 0) { + /* + * Loop over each attribute. The format is: + * + * 1 octet of type + * 1 octet of length (value 1..255) + * ((4 * length) - 2) octets of data. + */ + while (attrlen > 0) { uint8_t *p; - if(attrlen < 2) { + if (attrlen < 2) { fr_strerror_printf("EAP-Sim attribute %d too short: %d < 2", es_attribute_count, attrlen); return 0; } + if (!attr[1]) { + fr_strerror_printf("EAP-Sim attribute %d (no.%d) has no data", attr[0], + es_attribute_count); + return 0; + } + eapsim_attribute = attr[0]; eapsim_len = attr[1] * 4; + /* + * The length includes the 2-byte header. + */ if (eapsim_len > attrlen) { fr_strerror_printf("EAP-Sim attribute %d (no.%d) has length longer than data (%d > %d)", eapsim_attribute, es_attribute_count, eapsim_len, attrlen); return 0; } - if(eapsim_len > MAX_STRING_LEN) { - eapsim_len = MAX_STRING_LEN; - } - if (eapsim_len < 2) { - fr_strerror_printf("EAP-Sim attribute %d (no.%d) has length too small", eapsim_attribute, - es_attribute_count); - return 0; - } + newvp = fr_pair_afrom_num(r, eapsim_attribute + PW_EAP_SIM_BASE, 0); + if (!newvp) { + /* + * RFC 4186 Section 8.1 says 0..127 are + * "non-skippable". If one such + * attribute is found and we don't + * understand it, the server has to send: + * + * EAP-Request/SIM/Notification packet with an + * (AT_NOTIFICATION code, which implies general failure ("General + * failure after authentication" (0), or "General failure" (16384), + * depending on the phase of the exchange), which terminates the + * authentication exchange. + */ + if (eapsim_attribute <= 127) { + fr_strerror_printf("Unknown mandatory attribute %d, failing", + eapsim_attribute); + return 0; + } - newvp = fr_pair_afrom_num(r, eapsim_attribute+PW_EAP_SIM_BASE, 0); - newvp->vp_length = eapsim_len-2; - newvp->vp_octets = p = talloc_array(newvp, uint8_t, newvp->vp_length); - memcpy(p, &attr[2], eapsim_len-2); - fr_pair_add(&(r->vps), newvp); - newvp = NULL; + } else { + /* + * It's known, ccount for header, and + * copy the value over. + */ + newvp->vp_length = eapsim_len - 2; + + newvp->vp_octets = p = talloc_array(newvp, uint8_t, newvp->vp_length); + memcpy(p, &attr[2], newvp->vp_length); + fr_pair_add(&(r->vps), newvp); + } /* advance pointers, decrement length */ attr += eapsim_len;