Re: [PATCH] ap/drv_callbacks: in hostapd_notif_assoc, !ACCEPT ≠ REJECT

[Derrick Pallas]

Howdy,

During our bump from hostap 2.5 to 2.6, our basic feature test flagged
this as a regression, specifically MAC-based auth using an external
RADIUS server. After a bisect, the referenced commit was identified as
the source. The code in ap/drv_callbacks was newly added, i.e. no
check was done previously, and hostapd_check_acl is definitely
returning pending here. This commit fixes the issue in our automated
test, for both the positive case and the negative case.

Unfortunately, I am not very familiar with hostap's internals, so I'm
not sure I can comment on the reasoning you lay out. Perhaps there is
a different bug that this change masks. Is there any testing or
debugging you'd like me to run on my side?

~Derrick

On Tue, Oct 25, 2016 at 8:20 AM, M. Braun <mbrrc at fami-braun.de> wrote:
> Hi,
>
> hostapd_check_acl can only return PENDING if macaddr_acl ==
> USE_EXTERNAL_RADIUS_AUTH.
>
> In that case, either
> a) hostapd_allowed_address is used before and so the RADIUS reply came in
> before
>    association is completed and hostapd_notif_assoc is called or
> b) hostapd_allowed_address is not called (e.g. SME in driver) and so
>    macaddr_acl == USE_EXTERNAL_RADIUS_AUTH is not implemented.
>
> In case b) this change would accept a station bypassing RADIUS, while
> currently
> admin would need to choose a different macaddr_acl value to disable RADIUS
> withmacaddr_acl
> when using SME in driver.
>
> Right?
>
> Regards,
> M. Braun
>
>
>
> Am 25.10.2016 02:05, schrieb Derrick Pallas:
>>
>> The commit
>>
>>         hostapd: Process MAC ACLs on a station association event (SME in
>> driver)
>>
>> added a MAC ACL check to hostapd_notif_assoc.  This check disconnects the
>> client if the response is not ACCEPT, but the function can actually return
>> PENDING too, as in the case of 802.1x MAC-based auth.  It feels like the
>> author probably meant to disconnect the client if the response is REJECT,
>> but not ACCEPT or PENDING instead.
>>
>> Signed-off-by: Derrick Pallas <pallas at meraki.com>
>> ---
>>  src/ap/drv_callbacks.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/ap/drv_callbacks.c b/src/ap/drv_callbacks.c
>> index 3552b3e..f065995 100644
>> --- a/src/ap/drv_callbacks.c
>> +++ b/src/ap/drv_callbacks.c
>> @@ -124,7 +124,7 @@ int hostapd_notif_assoc(struct hostapd_data *hapd,
>> const u8 *addr,
>>          * conflicting ACL rules.
>>          */
>>         if (hapd->iface->drv_max_acl_mac_addrs == 0 &&
>> -           hostapd_check_acl(hapd, addr, NULL) != HOSTAPD_ACL_ACCEPT) {
>> +           hostapd_check_acl(hapd, addr, NULL) == HOSTAPD_ACL_REJECT) {
>>                 wpa_printf(MSG_INFO, "STA " MACSTR " not allowed to
>> connect",
>>                            MAC2STR(addr));
>>                 reason = WLAN_REASON_UNSPECIFIED;