Am 28.02.20 um 10:37 schrieb Jouni Malinen:
On Thu, Feb 27, 2020 at 10:52:46PM +0100, Alexander Wetzel wrote:
Stop using set_tx and cleanup/restructure the key install logic
depending on it.
The updated logic is also no longer incorrectly installing some pairwise
keys as default keys and has additional sanity checks refusing
unexpected keys.
Regarding the changes on setting the default pairwise key.. Isn't that
preventing some use cases? For example, in IEEE 802.1X with dynamic WEP
keys, the Authenticator could configure two different unicast keys with
different Key IDs and then swap which key is used for transmission. And
Isn't dynamic WEP using broadcast keys and only individual keys would be
affected? Whatever the right name is: Looks like it needs more attention
in the next patches.
For unicast WEP keys we decided to handle them identical to pairwise
keys and should now probably use KEY_FLAG_PAIRWISE_RX and later
KEY_FLAG_PAIRWISE_RX_TX_MODIFY to activate them, I guess.
Since we don't activate installed keys without reinstalling them at the
moment I guess we simply can use KEY_FLAG_PAIRWISE_RX and
KEY_FLAG_PAIRWISE_RX_TX and could keep the check.
Or we indeed allow KEY_FLAG_PAIRWISE to be paired with KEY_FLAG_DEFAULT.
I'm leaning to go for KEY_FLAG_PAIRWISE_RX/KEY_FLAG_PAIRWISE_RX_TX at
the moment. Do you have any preferences?
wouldn't this apply for the new PTK rekeying case with two Key ID values
as well?
With Extended Key ID we use KEY_FLAG_MODIFY to tell the driver that an
installed (pairwise) key only needs an update of its TX status.
KEY_FLAG_DEFAULT is only intended to be combined with group/broadcast
keys and tries to clean up the inconsistent use set_tx has for pairwise
keys. (Where if I remember it right it's generally set in wpa_supplicant
and never in hostapd.)
In general, it would probably make sense to split patch into quite a few
smaller patches that each address a clear individual change on their own
at least for the clear independent fixes. This patch was a bit difficult
to review and I could not come to conclusion that all of it is fine even
though most places were clear.
Ok, I'll split it up.
While working on the mail I found other oddities so it will probably
also do things differently.
diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
+ } else {
wpa_printf(MSG_DEBUG, " broadcast key");
-
- types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES);
- if (!types ||
- nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST))
- goto fail;
- nla_nest_end(key_msg, types);
}
So I take this removal here is because that attribute is not really used
in the NL80211_CMD_NEW_KEY case.
Correct. I remember checking the kernel nl80211 code for that and while
it pared the settings they are not passed down to the drivers and do
nothing. These attributes only have an effect with NL80211_CMD_SET_KEY.
@@ -3197,19 +3206,19 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss,
if (addr && is_broadcast_ether_addr(addr)) {
struct nlattr *types;
+ wpa_printf(MSG_DEBUG, " group key");
types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES);
if (!types ||
nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST))
goto fail;
nla_nest_end(key_msg, types);
And this is where we have that attribute for NL80211_CMD_SET_KEY when
selecting which one of the broadcast keys to use for transmission.
} else if (addr) {
- struct nlattr *types;
-
- types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES);
- if (!types ||
- nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_UNICAST))
- goto fail;
- nla_nest_end(key_msg, types);
+ wpa_printf(MSG_DEBUG,
+ "nl80211: Default group key can't use a unicast address");
+ ret = -EINVAL;
+ goto fail;
But this is the case that would be used for changing which of the
multiple unicast keys is used for transmission. Why would this not be an
acceptable operation?
I think you are right and I dropped too much here. It's a bit strange
this did not cause any tests to fail...
I'll add that again for WEP unicast and WPA-NONE Keys.
@@ -3226,8 +3235,6 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss,
}
ret = send_and_recv_msgs(drv, msg, NULL, NULL);
- if (ret == -ENOENT)
- ret = 0;
I'd assume this is because the error cannot happen anymore with the
error cases removed, but that was not obvious from the full context of
this patch. Splitting the NL80211_CMD_SET_KEY changes to a separate
patch with the commit message highlighting that detail would be helpful
for understanding this.
I could not find any user for that nor a reason we should ignore that.
When the driver is not able to set a key to default claiming success
only helps hiding what went really wrong.
This is comparable old, the initial git commit using version 0.6.3 as
seed already has it. (The corresponding function was named
i802_set_encryption() at that time.) The oldest version using it I could
find was hostapd-0.6.2 with what seems to be the time the netlink API
got introduced.
Is there any known reason to ignore the error? I commented it out and
did a test run without triggering a set_key error.
I guess we also should check if the exception for the first netlink call
should not be thrown out, too. When we can't install the "basic" key how
should it be possible to activate it? And what do we win by ignoring the
errors and than assuming the operation worked?
Looks like this risks sending out frames unencrypted...
Alexander
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
