wext: when setting keys, use default key if no key is specified

[Tormod Volden]

The below patch will set the default key (=current key, I suppose, but
this confuses me a little) if no other key is specified. Since I don't
know the wireless extensions so well, I am not sure if this is the
correct way, but the resulting behaviour seems consistent with what I
see on another card (intel 2915).

I just discovered that my patch makes "iwconfig wlan0 commit" do
strange things: the keys are then hidden from "iwlist wlan0 enc". They
reappear if I set current key number with "ifconfig wlan0 enc [1]". I
don't know if the "commit" code is broken, or if the patch does the
wrong thing.

Tormod

[Tormod Volden]

Post by Tormod Volden
I just discovered that my patch makes "iwconfig wlan0 commit" do
strange things: the keys are then hidden from "iwlist wlan0 enc". They
reappear if I set current key number with "ifconfig wlan0 enc [1]". I
don't know if the "commit" code is broken, or if the patch does the
wrong thing.

Sorry for the contradictory monologue, but now I saw the "hidden" keys
syndrome without my patch as well, so I guess the patch is not at
fault. Probably network-manager called "commit" and what not, anyway
iwlist showed no keys, but they got back after "iwconfig wlan0 enc
[1]" which is not logic.

I guess I should try to debug this without network-manager and only
plain iwconfig commands in the first place, since network manager is
not bug free to say the least. Is there a good reference (as in not
just read the code) somewhere about what each wext command should do?

Tormod

[Tormod Volden]

Hi,
If you run "iwconfig wlan0 key 1234-5678-90" nothing gets set. Compare
this to "iwconfig wlan0" which lists the current key.

The below patch will set the default key (=current key, I suppose, but
this confuses me a little) if no other key is specified. Since I don't
know the wireless extensions so well, I am not sure if this is the
correct way, but the resulting behaviour seems consistent with what I
see on another card (intel 2915).

Another thing is that "iwconfig wlan0 key 1234-5678-90 [2]" will not
only set key #2, but also set the current key to 2. This is not how I
understand the iwconfig man page, and it is not like on the intel
card.

Best regards,
Tormod

--- src/p80211/p80211wext.c.orig 2008-02-01 23:02:58.000000000 +0100
+++ src/p80211/p80211wext.c 2008-02-01 23:27:36.000000000 +0100
@@ -645,7 +645,9 @@ static int p80211wext_siwencode(netdevic

}
else {
- // Do not thing when no Key Index
+ // Use defaultkey if no Key Index
+ i = wlandev->hostwep & HOSTWEP_DEFAULTKEY_MASK;
+ enable=1;
}

/* Check if there is no key information in the iwconfig request */

[Tormod Volden]

Post by Tormod Volden
Another thing is that "iwconfig wlan0 key 1234-5678-90 [2]" will not
only set key #2, but also set the current key to 2. This is not how I
understand the iwconfig man page, and it is not like on the intel
card.

The attached patch changes this to conform with the iwconfig man page.
The patch includes the "Use defaultkey if no Key Index" patch in my
original post.

Summary:
1) if no key number is specified, but a key, set the current key
2) switch current key only if no keys are given (checks
IW_ENCODE_NOKEY for this).
3) both key number and a key: just set this key, do not switch current key
4) turn off PrivacyInvoked if IW_ENCODE_DISABLED, turn it on otherwise.

A possible variant would be to leave PrivacyInvoked unchanged in case
(3) bar (4), but I think it's safer to just turn it on here also. The
"enabled" flag could have been probed for this, since it is set in
case (1) and (2).

I have tested this with iwconfig and with network-manager 0.7 and it
seems to work fine.

Any comments are welcome. Thanks,
Tormod

[Tormod Volden]

Post by Tormod Volden
The attached patch changes this to conform with the iwconfig man page.
The patch includes the "Use defaultkey if no Key Index" patch in my
original post.

Duh, I posted the "variant" patch. Here is the one I meant to attach.
Sorry for the confusion.
Tormod

[Solomon Peachy]

Post by Tormod Volden
/* Check if there is no key information in the iwconfig request */
- if((erq->flags & IW_ENCODE_NOKEY) == 0 && enable == 1) {
+ if((erq->flags & IW_ENCODE_NOKEY) == 0 ) {

With this, 'enable' is never used -- in other words, privacy_invoked is
solely determined by the presence or absense of IW_ENCODE_DISABLED.

Also, did you test by switching back and forth between WEP'd APs and
non-WEP APs?

- Solomon

[Solomon Peachy]

Post by Solomon Peachy
Also, did you test by switching back and forth between WEP'd APs and
non-WEP APs?

Another thing I discovered while reviewing things:

The security mode may be open or restricted, and its meaning
depends on the card used. With most cards, in open mode no
authentication is used and the card may also accept non-
encrypted sessions, whereas in restricted mode only encrypted
sessions are accepted and the card will use authentication if
available.

According to the iwconfig man page. This implies that we should
probably set/clear our use of SHAREDKEY authentication based on the
ENCODE_RESTRICTED flag.

Without using the WEXT-17+ SIWAUTH call, there's no way for the user to
select sharedkey/opensystem authentication, and no way to determine
which one we need beyond try-one-if-fail-try-other.

- Solomon

[Tormod Volden]

Post by Solomon Peachy

Post by Tormod Volden
/* Check if there is no key information in the iwconfig request */
- if((erq->flags & IW_ENCODE_NOKEY) == 0 && enable == 1) {
+ if((erq->flags & IW_ENCODE_NOKEY) == 0 ) {

With this, 'enable' is never used -- in other words, privacy_invoked is
solely determined by the presence or absense of IW_ENCODE_DISABLED.

Did you mean to paste this instead?
}
- else if((erq->flags & IW_ENCODE_ENABLED) || enable == 1) {
+ else {
result = p80211wext_dorequest(wlandev,
DIDmib_dot11smt_dot11PrivacyTable_dot11PrivacyInvoked,
P80211ENUM_truth_true);

You're right, I forgot to clean out the unused variable. My reasoning
is that this function is called either to turn off encryption, or to
set a encryption parameter in which case you also want to turn on
encryption. The only usefulness of the "variant" patch (which uses the
enabled variable) would be if you want to set up the keys one by one
and then finally turn on encryption. Is that a valid use case?

Anyway, since there is no real "ENABLE" flag and only a "DISABLE"
flag, it seems natural to follow that flag blindly.

About the part that you pasted in above: I suppose IW_ENCODE_NOKEY
tells if a key is delivered or not, and if there's a key we should set
it, also when enable=0 (the case (3) where both key index and keys are
given). But wireless.h says it means "Key is write only, so not
present" so I am not totally sure about this. The old code seems to
use IW_ENCODE_NOKEY as presence of a key or not.

Post by Solomon Peachy
Also, did you test by switching back and forth between WEP'd APs and
non-WEP APs?

Nope, I didn't. But I monitor the IW_ENCODE_DISABLED flag, and I can
see NetworkManager turns it on when it disconnects from my WEP AP, so
I guess it works fine.

Thanks for your feedback,
Tormod

[Tormod Volden]

Post by Tormod Volden
present" so I am not totally sure about this. The old code seems to
use IW_ENCODE_NOKEY as presence of a key or not.

Sorry, the old code uses ~IW_ENCODE_NOKEY and (erq->length > 0) to
check for keys. Maybe I should have done that as well in this part:

+ /* Set current key number only if no keys are given */
+ if (erq->flags & IW_ENCODE_NOKEY) {
+ result = p80211wext_dorequest(wlandev,
DIDmib_dot11smt_dot11PrivacyTable_dot11WEPDefaultKeyID, i);

Tormod

[Tormod Volden]

Post by Tormod Volden
Sorry, the old code uses ~IW_ENCODE_NOKEY and (erq->length > 0) to
+ /* Set current key number only if no keys are given */
+ if (erq->flags & IW_ENCODE_NOKEY) {
+ result = p80211wext_dorequest(wlandev,
DIDmib_dot11smt_dot11PrivacyTable_dot11WEPDefaultKeyID, i);

Hi Solomon,
1) What's your opinion on the above, should it rather be like this?
if (erq->flags & IW_ENCODE_NOKEY || erq->length == 0) {

2) FWIW, this part could be tied together now:
/* Check if there is no key information in the iwconfig request */
if((erq->flags & IW_ENCODE_NOKEY) == 0 ) {

/*------------------------------------------------------------
* If there is WEP Key for setting, check the Key Information
* and then set it to the firmware.
-------------------------------------------------------------*/

if (erq->length > 0) {

into
if((erq->flags & IW_ENCODE_NOKEY) == 0 && erq->length > 0) {
just for clarity and to avoid some nesting and indenting.

3) I saw you committed all the new great stuff (svn 1850), but the
extra "disable" before "fwload" in shared.prism2 is still missing.

4) Will there be a 0.2.9 release soon? Just so distributions can know
if they should package a snapshot in the meantime.

Thanks and best regards,
Tormod

[Solomon Peachy]

Post by Tormod Volden
I have tested this with iwconfig and with network-manager 0.7 and it
seems to work fine.

I think the patch looks good. As you discovered WEXT is highly
under-specified. The API is well-documented, but behaivorally... things
are different. Each wireless driver historically implemented things
slightly differently, and there was no way of really saying which
behaivor was actually "correct".

Things are much better now, mostly thanks to NetworkManager (and its
developers, of course)

- Solomon