From 0658a22ef19793578c85ba26a8d2b0ea18cdfde8 Mon Sep 17 00:00:00 2001 From: Jouni Malinen Date: Mon, 7 Nov 2022 12:19:28 +0200 Subject: [PATCH] GAS: Try to make buffer length determination easier for static analyzers The received frame buffer was already verified to be long enough to include the Advertisement Protocol element and that element was verified to have a valid length value, but use of adv_proto[1] in another function may have been too difficult to figure out for analyzers. Signed-off-by: Jouni Malinen --- wpa_supplicant/gas_query.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/wpa_supplicant/gas_query.c b/wpa_supplicant/gas_query.c index 802f120ca..c301f7410 100644 --- a/wpa_supplicant/gas_query.c +++ b/wpa_supplicant/gas_query.c @@ -404,14 +404,14 @@ static void gas_query_tx_comeback_req_delay(struct gas_query *gas, static void gas_query_rx_initial(struct gas_query *gas, struct gas_query_pending *query, - const u8 *adv_proto, const u8 *resp, - size_t len, u16 comeback_delay) + const u8 *adv_proto, size_t adv_proto_len, + const u8 *resp, size_t len, u16 comeback_delay) { wpa_printf(MSG_DEBUG, "GAS: Received initial response from " MACSTR " (dialog_token=%u comeback_delay=%u)", MAC2STR(query->addr), query->dialog_token, comeback_delay); - query->adv_proto = wpabuf_alloc_copy(adv_proto, 2 + adv_proto[1]); + query->adv_proto = wpabuf_alloc_copy(adv_proto, adv_proto_len); if (query->adv_proto == NULL) { gas_query_done(gas, query, GAS_QUERY_INTERNAL_ERROR); return; @@ -436,9 +436,9 @@ static void gas_query_rx_initial(struct gas_query *gas, static void gas_query_rx_comeback(struct gas_query *gas, struct gas_query_pending *query, - const u8 *adv_proto, const u8 *resp, - size_t len, u8 frag_id, u8 more_frags, - u16 comeback_delay) + const u8 *adv_proto, size_t adv_proto_len, + const u8 *resp, size_t len, u8 frag_id, + u8 more_frags, u16 comeback_delay) { wpa_printf(MSG_DEBUG, "GAS: Received comeback response from " MACSTR " (dialog_token=%u frag_id=%u more_frags=%u " @@ -447,7 +447,7 @@ static void gas_query_rx_comeback(struct gas_query *gas, more_frags, comeback_delay); eloop_cancel_timeout(gas_query_rx_comeback_timeout, gas, query); - if ((size_t) 2 + adv_proto[1] != wpabuf_len(query->adv_proto) || + if (adv_proto_len != wpabuf_len(query->adv_proto) || os_memcmp(adv_proto, wpabuf_head(query->adv_proto), wpabuf_len(query->adv_proto)) != 0) { wpa_printf(MSG_DEBUG, "GAS: Advertisement Protocol changed " @@ -516,6 +516,7 @@ int gas_query_rx(struct gas_query *gas, const u8 *da, const u8 *sa, u8 action, dialog_token, frag_id = 0, more_frags = 0; u16 comeback_delay, resp_len; const u8 *pos, *adv_proto; + size_t adv_proto_len; int prot, pmf; unsigned int left; @@ -596,22 +597,26 @@ int gas_query_rx(struct gas_query *gas, const u8 *da, const u8 *sa, pos += 2; /* Advertisement Protocol element */ - if (pos + 2 > data + len || pos + 2 + pos[1] > data + len) { + adv_proto = pos; + left = data + len - adv_proto; + if (left < 2 || adv_proto[1] > left - 2) { wpa_printf(MSG_DEBUG, "GAS: No room for Advertisement " "Protocol element in the response from " MACSTR, MAC2STR(sa)); return 0; } - if (*pos != WLAN_EID_ADV_PROTO) { + if (*adv_proto != WLAN_EID_ADV_PROTO) { wpa_printf(MSG_DEBUG, "GAS: Unexpected Advertisement " "Protocol element ID %u in response from " MACSTR, - *pos, MAC2STR(sa)); + *adv_proto, MAC2STR(sa)); return 0; } + adv_proto_len = 2 + adv_proto[1]; + if (adv_proto_len > (size_t) (data + len - pos)) + return 0; /* unreachable due to an earlier check */ - adv_proto = pos; - pos += 2 + pos[1]; + pos += adv_proto_len; /* Query Response Length */ if (pos + 2 > data + len) { @@ -635,11 +640,12 @@ int gas_query_rx(struct gas_query *gas, const u8 *da, const u8 *sa, } if (action == WLAN_PA_GAS_COMEBACK_RESP) - gas_query_rx_comeback(gas, query, adv_proto, pos, resp_len, - frag_id, more_frags, comeback_delay); + gas_query_rx_comeback(gas, query, adv_proto, adv_proto_len, + pos, resp_len, frag_id, more_frags, + comeback_delay); else - gas_query_rx_initial(gas, query, adv_proto, pos, resp_len, - comeback_delay); + gas_query_rx_initial(gas, query, adv_proto, adv_proto_len, + pos, resp_len, comeback_delay); return 0; }