Re: [PATCH 1/2] HE: fix ieee80211_he_capabilities size

[Sven Eckelmann <sven@xxxxxxxxxxxxx>]

On Monday, 1 July 2019 15:13:48 CEST John Crispin wrote:
> 
> On 01/07/2019 15:00, Sven Eckelmann wrote:
> > On Monday, 1 July 2019 14:39:25 CEST John Crispin wrote:
> >> @@ -325,7 +353,7 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
> >>   {
> >>          if (!he_capab || !hapd->iconf->ieee80211ax ||
> >>              !check_valid_he_mcs(hapd, he_capab, opmode) ||
> >> -           he_capab_len > sizeof(struct ieee80211_he_capabilities)) {
> >> +           ieee80211_check_he_cap_size(he_capab, he_capab_len)) {
> >>                  sta->flags &= ~WLAN_STA_HE;
> >>                  os_free(sta->he_capab);
> >>                  sta->he_capab = NULL;
> >> @@ -334,13 +362,13 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
> >>   
> >>          if (!sta->he_capab) {
> >>                  sta->he_capab =
> >> -                       os_zalloc(sizeof(struct ieee80211_he_capabilities));
> >> +                       os_zalloc(he_capab_len);
> >>                  if (!sta->he_capab)
> >>                          return WLAN_STATUS_UNSPECIFIED_FAILURE;
> >>          }
> >>   
> >>          sta->flags |= WLAN_STA_HE;
> >> -       os_memset(sta->he_capab, 0, sizeof(struct ieee80211_he_capabilities));
> >> +       os_memset(sta->he_capab, 0, he_capab_len);
> >>          os_memcpy(sta->he_capab, he_capab, he_capab_len);
> >>          sta->he_capab_len = he_capab_len;
> > Isn't this creating the same sta->he_capab size uncertainty which Jouni
> > previously found in the old version of the patch [1].
> >
> > Kind regards,
> > 	Sven
> >
> no, the call to ieee80211_check_he_cap_size(he_capab, he_capab_len) will 
> make sure the length is valid

I think we are talking about two different things.  sta->he_capab is either 
NULL or has a size X. This size X is not checked anywhere. Please think about 
following scenario:

* first round of copy_sta_he_capab for sta
  - copy_sta_he_capab is called for size X, sta->he_capab is NULL
  - ieee80211_check_he_cap_size doesn't detect wrong he_capab_len
    (he_capab requires size X == he_capab_len)
  - sta->he_capab is allocated with size X
  - memcpy happens with size X
* second round of copy_sta_he_capab for sta
  - copy_sta_he_capab is called for size Y (Y > X), sta->he_capab is of size Y
  - ieee80211_check_he_cap_size doesn't detect wrong he_capab_len
    (he_capab requires size Y == he_capab_len)
  - sta->he_capab is not reallocated
  - memcpy happens with size Y -> *buffer overflow on heap*

Kind regards,
	Sven

Attachment: signature.asc
Description: This is a digitally signed message part.

_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap