| Message ID | 20131123004132.B2FE013FDEA@ushik.mtv.corp.google.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Nov 22, 2013 at 04:39:37PM -0800, Dmitry Shmidt wrote: > diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c > @@ -1442,16 +1442,27 @@ void wpas_dev_found(void *ctx, const u8 *addr, > #ifndef CONFIG_NO_STDOUT_DEBUG > struct wpa_supplicant *wpa_s = ctx; > char devtype[WPS_DEV_TYPE_BUFSIZE]; > - > +#define WFD_DEV_INFO_SIZE 9 > + char wfd_dev_info_hex[2 * WFD_DEV_INFO_SIZE + 1]; > + os_memset(wfd_dev_info_hex, 0, sizeof(wfd_dev_info_hex)); > +#ifdef CONFIG_WIFI_DISPLAY > + if (info->wfd_subelems) { > + wpa_snprintf_hex(wfd_dev_info_hex, sizeof(wfd_dev_info_hex), > + wpabuf_head(info->wfd_subelems), > + WFD_DEV_INFO_SIZE); Where does this assumption of there always being 9 octets of data in the full set of WFD elements come from? This also seem to assume that the device info would always be the first subelement. This does not look safe since the elements parser in p2p_parse_ies() does no such validation. This sounds like something that could result in buffer overflow based on data received over air and/or result in truncated or unexpected information being included in the event. > wpa_msg_global(wpa_s, MSG_INFO, P2P_EVENT_DEVICE_FOUND MACSTR > " p2p_dev_addr=" MACSTR > " pri_dev_type=%s name='%s' config_methods=0x%x " > - "dev_capab=0x%x group_capab=0x%x", > + "dev_capab=0x%x group_capab=0x%x%s%s", > MAC2STR(addr), MAC2STR(info->p2p_device_addr), > wps_dev_type_bin2str(info->pri_dev_type, devtype, > sizeof(devtype)), > - info->device_name, info->config_methods, > - info->dev_capab, info->group_capab); > + info->device_name, info->config_methods, > + info->dev_capab, info->group_capab, > + wfd_dev_info_hex[0] ? " wfd_dev_info=0x" : "", > + wfd_dev_info_hex); Why would this information be needed in the P2P-DEVICE-FOUND event? It is already available from the P2P_PEER command and that information is actually provided in full rather than this type of fixed length of 9 assumption. If there is real need to get the Wi-Fi Display information added to the P2P-DEVICE-FOUND event, this would need to be fixed to validate the data, parse the subelements, and only include the needed information. I guess that length 9 is referring to the current total size of the WFD Device Information subelement. If that is the information you want, more appropriate operation would be to parse that element and include just the six octets of payload in it. If you want to maintain this wasteful format with 0x prefix and id+len fields included for backwards compatibility with existing consumers of such data, the implementation here would need to verify that the first sublement is indeed WFD Device Info and only in that case, add this data. (Or for more completeness, find the WFD Device Info even if it is not the first subelement.)
On Sat, Nov 23, 2013 at 2:42 PM, Jouni Malinen <j@w1.fi> wrote: > On Fri, Nov 22, 2013 at 04:39:37PM -0800, Dmitry Shmidt wrote: >> diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c >> @@ -1442,16 +1442,27 @@ void wpas_dev_found(void *ctx, const u8 *addr, >> #ifndef CONFIG_NO_STDOUT_DEBUG >> struct wpa_supplicant *wpa_s = ctx; >> char devtype[WPS_DEV_TYPE_BUFSIZE]; >> - >> +#define WFD_DEV_INFO_SIZE 9 >> + char wfd_dev_info_hex[2 * WFD_DEV_INFO_SIZE + 1]; >> + os_memset(wfd_dev_info_hex, 0, sizeof(wfd_dev_info_hex)); >> +#ifdef CONFIG_WIFI_DISPLAY >> + if (info->wfd_subelems) { >> + wpa_snprintf_hex(wfd_dev_info_hex, sizeof(wfd_dev_info_hex), >> + wpabuf_head(info->wfd_subelems), >> + WFD_DEV_INFO_SIZE); > > Where does this assumption of there always being 9 octets of data in the > full set of WFD elements come from? This also seem to assume that the > device info would always be the first subelement. This does not look > safe since the elements parser in p2p_parse_ies() does no such > validation. This sounds like something that could result in buffer > overflow based on data received over air and/or result in truncated or > unexpected information being included in the event. You are right, issue should be addressed. > >> wpa_msg_global(wpa_s, MSG_INFO, P2P_EVENT_DEVICE_FOUND MACSTR >> " p2p_dev_addr=" MACSTR >> " pri_dev_type=%s name='%s' config_methods=0x%x " >> - "dev_capab=0x%x group_capab=0x%x", >> + "dev_capab=0x%x group_capab=0x%x%s%s", >> MAC2STR(addr), MAC2STR(info->p2p_device_addr), >> wps_dev_type_bin2str(info->pri_dev_type, devtype, >> sizeof(devtype)), >> - info->device_name, info->config_methods, >> - info->dev_capab, info->group_capab); >> + info->device_name, info->config_methods, >> + info->dev_capab, info->group_capab, >> + wfd_dev_info_hex[0] ? " wfd_dev_info=0x" : "", >> + wfd_dev_info_hex); > > Why would this information be needed in the P2P-DEVICE-FOUND event? It > is already available from the P2P_PEER command and that information is > actually provided in full rather than this type of fixed length of 9 > assumption. > > If there is real need to get the Wi-Fi Display information added to the > P2P-DEVICE-FOUND event, this would need to be fixed to validate the > data, parse the subelements, and only include the needed information. I > guess that length 9 is referring to the current total size of the WFD > Device Information subelement. If that is the information you want, more > appropriate operation would be to parse that element and include just > the six octets of payload in it. Done: http://patchwork.ozlabs.org/patch/294137/ > > If you want to maintain this wasteful format with 0x prefix and id+len > fields included for backwards compatibility with existing consumers of > such data, the implementation here would need to verify that the first > sublement is indeed WFD Device Info and only in that case, add this > data. (Or for more completeness, find the WFD Device Info even if it is > not the first subelement.) Adding this info allows to see if WFD exists without using P2P_PEER call per found P2P device. > > -- > Jouni Malinen PGP id EFC895FA > _______________________________________________ > HostAP mailing list > HostAP@lists.shmoo.com > http://lists.shmoo.com/mailman/listinfo/hostap
diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c index fecc259..1ef0391 100644 --- a/wpa_supplicant/p2p_supplicant.c +++ b/wpa_supplicant/p2p_supplicant.c @@ -1442,16 +1442,27 @@ void wpas_dev_found(void *ctx, const u8 *addr, #ifndef CONFIG_NO_STDOUT_DEBUG struct wpa_supplicant *wpa_s = ctx; char devtype[WPS_DEV_TYPE_BUFSIZE]; - +#define WFD_DEV_INFO_SIZE 9 + char wfd_dev_info_hex[2 * WFD_DEV_INFO_SIZE + 1]; + os_memset(wfd_dev_info_hex, 0, sizeof(wfd_dev_info_hex)); +#ifdef CONFIG_WIFI_DISPLAY + if (info->wfd_subelems) { + wpa_snprintf_hex(wfd_dev_info_hex, sizeof(wfd_dev_info_hex), + wpabuf_head(info->wfd_subelems), + WFD_DEV_INFO_SIZE); + } +#endif /* CONFIG_WIFI_DISPLAY */ wpa_msg_global(wpa_s, MSG_INFO, P2P_EVENT_DEVICE_FOUND MACSTR " p2p_dev_addr=" MACSTR " pri_dev_type=%s name='%s' config_methods=0x%x " - "dev_capab=0x%x group_capab=0x%x", + "dev_capab=0x%x group_capab=0x%x%s%s", MAC2STR(addr), MAC2STR(info->p2p_device_addr), wps_dev_type_bin2str(info->pri_dev_type, devtype, sizeof(devtype)), - info->device_name, info->config_methods, - info->dev_capab, info->group_capab); + info->device_name, info->config_methods, + info->dev_capab, info->group_capab, + wfd_dev_info_hex[0] ? " wfd_dev_info=0x" : "", + wfd_dev_info_hex); #endif /* CONFIG_NO_STDOUT_DEBUG */ wpas_notify_p2p_device_found(ctx, info->p2p_device_addr, new_device);
Signed-off-by: Dmitry Shmidt <dimitrysh@google.com> --- wpa_supplicant/p2p_supplicant.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)