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

[John Crispin <john@xxxxxxxxxxx>]

On 01/07/2019 18:56, Sven Eckelmann wrote:
> 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
I'll look into this in the morning and send a V3

    John


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