Hi,
> -----Original Message-----
> From: Jouni Malinen <j@xxxxx>
> Sent: Friday, March 19, 2021 18:28
> To: Peer, Ilan <ilan.peer@xxxxxxxxx>
> Cc: hostap@xxxxxxxxxxxxxxxxxxx
> 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.
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
