[PATCH 14/44] 802.11 Factor out authentication code for reuse with FT-over-DS

[Jouni Malinen]

On Wed, Feb 24, 2016 at 12:53:20PM +0100, michael-dev at fami-braun.de wrote:

> diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c
> +int handle_auth_cfg_sta(struct hostapd_data *hapd, struct sta_info *sta,
> +			int res, struct hostapd_allowed_address_info *info,
> +			u16 *resp)

> +	hostapd_free_psk_list(sta->psk);
> +	if (hapd->conf->wpa_psk_radius != PSK_RADIUS_IGNORED) {
> +		sta->psk = info->psk;
> +		info->psk = NULL;
> +	} else {
> +		sta->psk = NULL;
> +	}

Wouldn't this be able to leave out info->psk allocated and then return
0?

> +	sta->identity = info->identity;
> +	info->identity = NULL;
> +	sta->radius_cui = info->radius_cui;
> +	info->radius_cui = NULL;

These free the other allocated items within struct
hostapd_allowed_address_info, but this is done without calling
hostapd_allowed_address_free() which can make it more likely for this
detail to be missed if new fields are added in the future.

> +	return 0;

So this is handle_auth_cfg_sta() returning 0 with info->psk potentially
left allocated.

>  static void handle_auth(struct hostapd_data *hapd,
>  			const struct ieee80211_mgmt *mgmt, size_t len)
>  {
> @@ -1068,9 +1129,8 @@ static void handle_auth(struct hostapd_data *hapd,
>  	res = hostapd_allowed_address(hapd, mgmt->sa, (u8 *) mgmt, len,
> -				      &session_timeout,
> -				      &acct_interim_interval, &vlan_id,
> -				      &psk, &identity, &radius_cui);
> +				      &handle_auth_restart_cb,
> +				      &details);

This allocates the details.* based on response.

> @@ -1085,6 +1145,7 @@ static void handle_auth(struct hostapd_data *hapd,
> +		hostapd_allowed_address_free(&details);
>  		return;

And many code paths within handle_auth() were modified to call
hostapd_allowed_address_free() before returning.

> @@ -1138,47 +1202,12 @@ static void handle_auth(struct hostapd_data *hapd,
> +	if (handle_auth_cfg_sta(hapd, sta, res, &details, &resp) < 0)
>  		goto fail;

But after this, only the "goto fail" cases end up calling
hostapd_allowed_address_free().

Is this correct?

> @@ -1288,9 +1317,7 @@ static void handle_auth(struct hostapd_data *hapd,
>   fail:
> -	os_free(identity);
> -	os_free(radius_cui);
> -	hostapd_free_psk_list(psk);
> +	hostapd_allowed_address_free(&details);

So there is the hostapd_allowed_address_free() for some of the error
cases and the success path. However, there are at least two return
statements above in WLAN_AUTH_FT and WLAN_AUTH_SAE cases of
switch (auth_alg). Do those free memory in all cases? Even if they do,
it looks a bit strange to see paths that do not have
hostapd_allowed_address_free() being called.

-- 
Jouni Malinen                                            PGP id EFC895FA