| Message ID | 1500953151-5022-1-git-send-email-tomoharu.hatano@sony.com |
|---|---|
| State | Accepted |
| Headers | show |
Hi Hostap, I'm waiting for your feedback. When you can start the review? Best Regards, Tomoharu Hatano -----Original Message----- From: Hatano, Tomoharu (Sony Mobile) Sent: Wednesday, August 16, 2017 9:52 PM To: 'Tomoharu Hatano' <tomoharu.hatano@sony.com>; hostap@lists.infradead.org Cc: Sogo, Shinji (Sony Mobile) <Shinji.Sogo@sony.com>; Nanbu, Tomonori (Sony Mobile) <Tomonori.Nanbu@sony.com>; Onodera, Akihiro X (Sony Mobile) <Akihiro.Onodera@sony.com>; Hatano, Tomoharu (Sony Mobile) <Tomoharu.Hatano@sony.com> Subject: RE: [PATCH] Send Client-Error when AT_KDF attributes from the server are incorrect Hi Hostap, Do you have any progress about review? Best Regards, Tomoharu Hatano -----Original Message----- From: Tomoharu Hatano [mailto:tomoharu.hatano@sony.com] Sent: Tuesday, July 25, 2017 12:26 PM To: hostap@lists.infradead.org Cc: Sogo, Shinji (Sony Mobile) <Shinji.Sogo@sony.com>; Nanbu, Tomonori (Sony Mobile) <Tomonori.Nanbu@sony.com>; Onodera, Akihiro X (Sony Mobile) <Akihiro.Onodera@sony.com>; Hatano, Tomoharu (Sony Mobile) <Tomoharu.Hatano@sony.com> Subject: [PATCH] Send Client-Error when AT_KDF attributes from the server are incorrect From: Akihiro Onodera <akihiro.onodera@sony.com> After KDF negotiation, must check only requested change occurred in the list of AT_KDF attributes. If there are any other changes, the peer must behave like the case that AT_MAC had been incorrect and authentication is failed. These are defined in EAP-AKA' specification RFC5448. Adds a complete check of AT_KDF attributes and sends Client-Error if a change which is not requested is included in it. Change-Id: Ic8ac504a7ff01992e2632d35c243f53bdd27df74 Signed-off-by: Tomoharu Hatano <tomoharu.hatano@sony.com> --- src/eap_peer/eap_aka.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/src/eap_peer/eap_aka.c b/src/eap_peer/eap_aka.c index 0bac62d..9a09184 100644 --- a/src/eap_peer/eap_aka.c +++ b/src/eap_peer/eap_aka.c @@ -53,6 +53,8 @@ struct eap_aka_data { size_t network_name_len; u16 kdf; int kdf_negotiation; + u16 last_kdf_attrs[EAP_AKA_PRIME_KDF_MAX]; + size_t last_kdf_count; }; @@ -817,9 +819,12 @@ static struct wpabuf * eap_aka_prime_kdf_neg(struct eap_aka_data *data, size_t i; for (i = 0; i < attr->kdf_count; i++) { - if (attr->kdf[i] == EAP_AKA_PRIME_KDF) + if (attr->kdf[i] == EAP_AKA_PRIME_KDF) { + os_memcpy(data->last_kdf_attrs, attr->kdf, sizeof(u16) * attr->kdf_count); + data->last_kdf_count = attr->kdf_count; return eap_aka_prime_kdf_select(data, id, EAP_AKA_PRIME_KDF); + } } /* No matching KDF found - fail authentication as if AUTN had been @@ -840,26 +845,30 @@ static int eap_aka_prime_kdf_valid(struct eap_aka_data *data, * of the selected KDF into the beginning of the list. */ if (data->kdf_negotiation) { + /* When the peer receives the new EAP-Request/AKA'-Challenge message, must check + * only requested change occurred in the list of AT_KDF attributes. If there are any + * other changes, the peer must behave like the case that AT_MAC had been incorrect + * and authentication is failed. These are defined in EAP-AKA' specification + * RFC5448. */ if (attr->kdf[0] != data->kdf) { wpa_printf(MSG_WARNING, "EAP-AKA': The server did not " "accept the selected KDF"); - return 0; + return -1; } - for (i = 1; i < attr->kdf_count; i++) { - if (attr->kdf[i] == data->kdf) - break; - } - if (i == attr->kdf_count && - attr->kdf_count < EAP_AKA_PRIME_KDF_MAX) { - wpa_printf(MSG_WARNING, "EAP-AKA': The server did not " - "duplicate the selected KDF"); - return 0; + if (attr->kdf_count > EAP_AKA_PRIME_KDF_MAX || + attr->kdf_count != (data->last_kdf_count + 1)) { + wpa_printf(MSG_WARNING, "EAP-AKA': The length of KDF attributes is wrong"); + return -1; } - /* TODO: should check that the list is identical to the one - * used in the previous Challenge message apart from the added - * entry in the beginning. */ + for (i = 1; i < attr->kdf_count; i++) { + if (attr->kdf[i] != data->last_kdf_attrs[i - 1]) { + wpa_printf(MSG_WARNING, "EAP-AKA': The KDF attributes except " + "selected KDF are not same as original one."); + return -1; + } + } } for (i = data->kdf ? 1 : 0; i < attr->kdf_count; i++) { @@ -922,8 +931,11 @@ static struct wpabuf * eap_aka_process_challenge(struct eap_sm *sm, data->network_name, data->network_name_len); /* TODO: check Network Name per 3GPP.33.402 */ - if (!eap_aka_prime_kdf_valid(data, attr)) + res = eap_aka_prime_kdf_valid(data, attr); + if (res == 0) return eap_aka_authentication_reject(data, id); + else if (res == -1) + return eap_aka_client_error(data, id, +EAP_AKA_UNABLE_TO_PROCESS_PACKET); if (attr->kdf[0] != EAP_AKA_PRIME_KDF) return eap_aka_prime_kdf_neg(data, id, attr); -- 2.7.4
On Tue, Jul 25, 2017 at 12:25:51PM +0900, Tomoharu Hatano wrote: > After KDF negotiation, must check only requested change occurred in the > list of AT_KDF attributes. If there are any other changes, the peer must > behave like the case that AT_MAC had been incorrect and authentication > is failed. These are defined in EAP-AKA' specification RFC5448. > > Adds a complete check of AT_KDF attributes and sends Client-Error if a > change which is not requested is included in it. Thanks, applied.
Hi Jouni, Thank you for your approval. Best Regards, Tomoharu Hatano -----Original Message----- From: Jouni Malinen [mailto:j@w1.fi] Sent: Monday, September 11, 2017 4:51 AM To: Hatano, Tomoharu (Sony Mobile) <Tomoharu.Hatano@sony.com> Cc: hostap@lists.infradead.org; Akihiro Onodera <akihiro.onodera@sony.com>; Nanbu, Tomonori (Sony Mobile) <Tomonori.Nanbu@sony.com>; Sogo, Shinji (Sony Mobile) <Shinji.Sogo@sony.com> Subject: Re: [PATCH] Send Client-Error when AT_KDF attributes from the server are incorrect On Tue, Jul 25, 2017 at 12:25:51PM +0900, Tomoharu Hatano wrote: > After KDF negotiation, must check only requested change occurred in > the list of AT_KDF attributes. If there are any other changes, the > peer must behave like the case that AT_MAC had been incorrect and > authentication is failed. These are defined in EAP-AKA' specification RFC5448. > > Adds a complete check of AT_KDF attributes and sends Client-Error if a > change which is not requested is included in it. Thanks, applied.
diff --git a/src/eap_peer/eap_aka.c b/src/eap_peer/eap_aka.c index 0bac62d..9a09184 100644 --- a/src/eap_peer/eap_aka.c +++ b/src/eap_peer/eap_aka.c @@ -53,6 +53,8 @@ struct eap_aka_data { size_t network_name_len; u16 kdf; int kdf_negotiation; + u16 last_kdf_attrs[EAP_AKA_PRIME_KDF_MAX]; + size_t last_kdf_count; }; @@ -817,9 +819,12 @@ static struct wpabuf * eap_aka_prime_kdf_neg(struct eap_aka_data *data, size_t i; for (i = 0; i < attr->kdf_count; i++) { - if (attr->kdf[i] == EAP_AKA_PRIME_KDF) + if (attr->kdf[i] == EAP_AKA_PRIME_KDF) { + os_memcpy(data->last_kdf_attrs, attr->kdf, sizeof(u16) * attr->kdf_count); + data->last_kdf_count = attr->kdf_count; return eap_aka_prime_kdf_select(data, id, EAP_AKA_PRIME_KDF); + } } /* No matching KDF found - fail authentication as if AUTN had been @@ -840,26 +845,30 @@ static int eap_aka_prime_kdf_valid(struct eap_aka_data *data, * of the selected KDF into the beginning of the list. */ if (data->kdf_negotiation) { + /* When the peer receives the new EAP-Request/AKA'-Challenge message, must check + * only requested change occurred in the list of AT_KDF attributes. If there are any + * other changes, the peer must behave like the case that AT_MAC had been incorrect + * and authentication is failed. These are defined in EAP-AKA' specification + * RFC5448. */ if (attr->kdf[0] != data->kdf) { wpa_printf(MSG_WARNING, "EAP-AKA': The server did not " "accept the selected KDF"); - return 0; + return -1; } - for (i = 1; i < attr->kdf_count; i++) { - if (attr->kdf[i] == data->kdf) - break; - } - if (i == attr->kdf_count && - attr->kdf_count < EAP_AKA_PRIME_KDF_MAX) { - wpa_printf(MSG_WARNING, "EAP-AKA': The server did not " - "duplicate the selected KDF"); - return 0; + if (attr->kdf_count > EAP_AKA_PRIME_KDF_MAX || + attr->kdf_count != (data->last_kdf_count + 1)) { + wpa_printf(MSG_WARNING, "EAP-AKA': The length of KDF attributes is wrong"); + return -1; } - /* TODO: should check that the list is identical to the one - * used in the previous Challenge message apart from the added - * entry in the beginning. */ + for (i = 1; i < attr->kdf_count; i++) { + if (attr->kdf[i] != data->last_kdf_attrs[i - 1]) { + wpa_printf(MSG_WARNING, "EAP-AKA': The KDF attributes except " + "selected KDF are not same as original one."); + return -1; + } + } } for (i = data->kdf ? 1 : 0; i < attr->kdf_count; i++) { @@ -922,8 +931,11 @@ static struct wpabuf * eap_aka_process_challenge(struct eap_sm *sm, data->network_name, data->network_name_len); /* TODO: check Network Name per 3GPP.33.402 */ - if (!eap_aka_prime_kdf_valid(data, attr)) + res = eap_aka_prime_kdf_valid(data, attr); + if (res == 0) return eap_aka_authentication_reject(data, id); + else if (res == -1) + return eap_aka_client_error(data, id, EAP_AKA_UNABLE_TO_PROCESS_PACKET); if (attr->kdf[0] != EAP_AKA_PRIME_KDF) return eap_aka_prime_kdf_neg(data, id, attr);