[RFC] use User-Name and Chargeable-User-Identity für Access-Accept for Accounting Messages even if 802.1X is not used

[Jouni Malinen]

On Mon, Jul 23, 2012 at 12:06:44AM +0200, michael-dev wrote:
> The attached patch therefore adds identity (aka user-name) and
> radius_cui (chargeable user identity) attributes to the struct
> sta_info, that are filled by the radius acl query and used when
> generating an accounting message.

This sounds fine. Though, some cleanup would be useful in how the
strings are handled. If you are using strlen(), the strings better be
"char *" rather than "u8 *".

> diff --git a/src/ap/accounting.c b/src/ap/accounting.c
> @@ -82,6 +82,10 @@ static struct radius_msg * accounting_msg(struct hostapd_data *hapd,
>  	if (sta) {
>  		val = ieee802_1x_get_identity(sta->eapol_sm, &len);
>  		if (!val) {
> +			val = sta->identity;
> +			len = os_strlen(sta->identity);

Couldn't sta->identity be NULL here? If so, that os_strlen() will result
in segfault due to dereferencing a NULL pointer..

> +		}
> +		if (!val) {
>  			os_snprintf(buf, sizeof(buf), RADIUS_ADDR_FORMAT,
>  				    MAC2STR(sta->addr));

This set of two "!val" cases looks a bit strange, i.e., the second one
could never be true taken into account the assumption on os_strlen() not
crashing above.. Or was the first one supposed to be "if (!val &&
sta->identity)"?

> @@ -196,6 +200,14 @@ static struct radius_msg * accounting_msg(struct hostapd_data *hapd,
>  			wpa_printf(MSG_ERROR, "Could not add CUI");
>  			goto fail;
>  		}
> +
> +		if (sta->radius_cui &&
> +		    !radius_msg_add_attr(msg,
> +					 RADIUS_ATTR_CHARGEABLE_USER_IDENTITY,
> +					 sta->radius_cui, os_strlen((char*) sta->radius_cui))) {

Couldn't this result in adding two CUI attributes? While that may be
theoretical with the code paths, it would be better to select only one
of the potentially conflicting values here.

> diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c
> @@ -311,6 +311,8 @@ static void handle_auth(struct hostapd_data *hapd,
>  	int has_psk = 0;
>  	u8 resp_ies[2 + WLAN_AUTH_CHALLENGE_LEN];
>  	size_t resp_ies_len = 0;
> +	u8* identity = 0;
> +	u8* radius_cui = 0;

Pointers should use NULL rather than 0 and "u8 *identity" instead of
"u8* identity" to match the style used elsewhere. In addition, these
should probably be char* instead of u8* taken into account how they are
used.

> @@ -421,6 +423,9 @@ static void handle_auth(struct hostapd_data *hapd,
>  		sta->psk = NULL;
>  	}
>  
> +	sta->identity = identity;
> +	sta->radius_cui = radius_cui;

Hmm.. There is a "goto fail" between the hostapd_allowed_address and
this location. Are identity/radius_cui allocated and something would
need to free them on the failure path? And if not, how would these
pointers remain valid throughout the lifetime of the STA entry (the
acl_cache entry could get removed earlier)?

> diff --git a/src/ap/ieee802_11_auth.c b/src/ap/ieee802_11_auth.c
> index 0c4c5f3..e7586fc 100644
> --- a/src/ap/ieee802_11_auth.c
> +++ b/src/ap/ieee802_11_auth.c
> @@ -91,6 +95,22 @@ static int hostapd_acl_cache_get(struct hostapd_data *hapd, const u8 *addr,
> +			if (identity) {
> +				if (entry->identity) {
> +					*identity = os_zalloc(os_strlen((char*) entry->identity)+1);
> +					os_memcpy(*identity, entry->identity, os_strlen((char*) entry->identity));

Need to verify that *identity != NULL before dereferencing it.

> +					*radius_cui = os_zalloc(os_strlen((char*) entry->radius_cui)+1);
> +					os_memcpy(*radius_cui, entry->radius_cui, os_strlen((char*) entry->radius_cui));

Same here - os_zalloc() can return NULL

> @@ -491,6 +519,16 @@ hostapd_acl_recv_radius(struct radius_msg *msg, struct radius_msg *req,
> +		if (radius_msg_get_attr_ptr(msg, RADIUS_ATTR_USER_NAME,
> +				    &buf, &len, NULL) == 0) {
> +			cache->identity = os_zalloc(len+1);
> +			os_memcpy(cache->identity, buf, len);

if (cache->identity == NULL) needs to be handled

> +			cache->radius_cui = os_zalloc(len+1);
> +			os_memcpy(cache->radius_cui, buf, len);

Same here

> diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h
> @@ -96,6 +96,9 @@ struct sta_info {
>  	int vlan_id;
>  	u8 *psk; /* PSK from RADIUS authentication server */
>  
> +	u8 *identity; /* User-Name from RADIUS */
> +	u8 *radius_cui; /* Chargeable-User-Identity from RADIUS */

Both of these should be char *. The alternative is to use u8 * and a
separate length field if these can contain arbitrary binary octets
(which would not be the case with these RADIUS attributes).

-- 
Jouni Malinen                                            PGP id EFC895FA