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

[Jouni Malinen]

On Tue, Oct 25, 2016 at 10:10:30AM -0700, Derrick Pallas wrote:
> 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.

Interesting.. I can understand the issue with positive case, but I don't
see how this could have worked in the negative either before either of
these commit or after both the commit and this fix..

Would you be able to share hostapd debug logs showing the tests failing
without this new proposed commit and then passing with this commit
applied?

The main issue with this new patch is that it allows the STA to remain
associated while even if the RADIUS server were to reject the connection
afterwards. hostapd_acl_recv_radius() in src/ap/ieee802_11_auth.c would
need some additional code to handle the RADIUS_CODE_ACCESS_REJECT case
by disconnecting the relevant STA at the time Access-Reject is received.
I don't see such code there..

I don't think this RADIUS-based MAC ACL was ever designed or supposed to
work with WLAN drivers that implement AP SME, i.e., this has worked fine
only for the case where hostapd processes the Authentication frames. The
commit to add a check for HOSTAPD_ACL_ACCEPT in hostapd_notif_assoc()
makes this explicit.

I think I would be fine with extending this for AP SME in driver cases
as long as it does not introduce potential security issues. This would
likely need two additional changes to this proposed patch:
(1) hostapd_acl_recv_radius() to disconnect the STA on receiving
Access-Reject (if the STA is associated at that point in time)
(2) timeout to disconnect the STA if no RADIUS response is received
(this should be able to use ap_handle_timer() instead of having to come
up with something completely new; schedule that when
hostapd_notif_assoc() gets HOSTAPD_ACL_PENDING and allows the STA to
proceed)
(and see below for couple more possibly needed items..)

> 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?

One inconvenient part here is in mac80211_hwsim test cases not
supporting the AP SME in the driver case and because of this, there are
no automated test cases for this in hostap.git. If someone is interested
in working on those two items I listed above, it would be interested to
get them tested and assuming that works properly for the two cases where
the STA needs to get disconnected due to lack of Access-Accept, I'd like
to get these changes included. I don't think I'd apply this one-liner,
though, before those two other changes are available due to the
potential security issues this could open accidentally (sure, those were
there before v2.6, but now that the change in v2.6 blocks this, I don't
really want to revert it without more complete fix).

> On Tue, Oct 25, 2016 at 8:20 AM, M. Braun <mbrrc at fami-braun.de> wrote:
> > 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.

Hmm.. This may actually add item (3) to my list above, i.e., the
Access-Request message does indeed need to be sent out. Maybe the
hostapd_check_acl() call in hostapd_notif_assoc() would need to be
changed to use hostapd_allowed_address()..

The inconvenient part here is in having to accept the association before
knowing the RADIUS server response. I guess we could complete this using
the port authorization mechanism even in open network (which would be
the most likely used for MAC ACL) so that the STA can remain associated,
but it cannot really send any data frames (or receive unicast data
frames). So that maybe be item (4) in the list..

-- 
Jouni Malinen                                            PGP id EFC895FA