[PATCH 1/3] AP: Add support for PASN comeback flow

[Peer, Ilan]

Hi,

> -----Original Message-----
> From: Jouni Malinen <j at w1.fi>
> Sent: Friday, March 19, 2021 18:28
> To: Peer, Ilan <ilan.peer at intel.com>
> Cc: hostap at lists.infradead.org
> Subject: Re: [PATCH 1/3] AP: Add support for PASN comeback flow
> 
> On Wed, Mar 17, 2021 at 06:13:27PM +0200, Ilan Peer wrote:
> > diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c
> > +static void handle_auth_pasn_comeback(struct hostapd_data *hapd,
> > +				      struct sta_info *sta)
> 
> > +	wpa_printf(MSG_DEBUG,
> > +		   "PASN: Building comeback frame 2. Comeback after=%u",
> > +		   hapd->conf->pasn_comeback_after);
> 
> > +	comeback = auth_build_token_req(hapd, sta->pasn->group, sta-
> >addr,
> > +0);
> 
> sta->pasn->group seems to be 0 here.. Shouldn't the cookie value use the
> group number that the station proposed?
> 

I'm removing the group from the cookie, since as you noted below it is not
used.

> > @@ -3133,6 +3182,27 @@ static void handle_auth_pasn_1(struct
> hostapd_data *hapd, struct sta_info *sta,
> >  		goto send_resp;
> > +	if (pasn_params.comeback) {
> > +		wpa_printf(MSG_DEBUG, "PASN: Checking peer comeback
> token");
> > +
> > +		/* The token includes 2 bytes for the group, so skip them */
> 
> Why would it be fine to skip them? Shouldn't we verify that the correct value
> is provided? If this is not verified, there is no point in including the group in
> the cookie value in the first place.
>

Agree. I'm removing the group from the token. 

> > +		ret = check_comeback_token(hapd, sta->addr,
> > +					   pasn_params.comeback + 2,
> > +					   pasn_params.comeback_len - 2);
> 
> That is really problematic.. pasn_params.comeback_len can be 1 here and
> that would result in (size_t)-1 going to the check_comeback_token()
> function and just by chance, this does not result in a security vulnerability
> due to the check against SHA256_MAC_LEN.. In other words, this really
> should very that pasn_params.comeback_len >= 2 before decrementing that
> by 2.
> 

With the removal of the group from the token, this would no longer be an issue.

> > +	} else if (use_anti_clogging(hapd)) {
> > +		wpa_printf(MSG_DEBUG, "PASN: Response with
> comeback");
> > +
> > +		handle_auth_pasn_comeback(hapd, sta);
> > +		ap_free_sta(hapd, sta);
> > +		return;
> > +	}
> > +
> >  	sta->pasn->ecdh = crypto_ecdh_init(pasn_params.group);
> ...
> 
> And this would be followed by setting sta->pasn->group which explains the 0
> value noted above. Maybe it would be better to add pasn_params.group as a
> new argument to handle_auth_pasn_comeback() so that the correct group
> gets used in the token without changes sta->pasn in this case?
> 

Thanks for the review and comments. Sending the fixed version shortly.

Ilan.