wpa.c GTK processing

Live forum: http://rt2x00.serialmonkey.com/viewtopic.php?t=4892

TomDeMan

16-07-2008 12:34:58

I saw an IE coming in at this place in wpa.c

if (pAd->PortCfg.WepStatus == Ndis802_11Encryption3Enabled) {
// Decrypt AES GTK
AES_GTK_KEY_UNWRAP(&pAd->PortCfg.PTK[16], KEYDATA, pMsg3->KeyDesc.KeyDataLen[1], pMsg3->KeyDesc.KeyData);



data =
30 14 1 0 0 f ac 4 1 0 0 f ac 4 1 0
0 f ac 2 0 0 dd 16 0 f ac 1 1 0 fd a
ce d5 96 3e c5 73 39 fb 29 5c 29 c3 ed a1 dd 0
cd cd cd cd cd cd cd cd

len = 56

And followed the code in wpa.c to see what it does starting from ParseKeyData()

so it starts with WPA2RSNIE (0x30) and 0x14!=0 (len) and 56 >= 2+0x14 (22)

pMyKeyData points to data +22 (where the first dd is)
KeyDataLength = 56 - 22 = 34

so look at dd, len = 0x16 (or including the header 0x18)


dd 16 0 f ac 1 1 0 fd a
ce d5 96 3e c5 73 39 fb 29 5c 29 c3 ed a1

all that's left now

dd 0
cd cd cd cd cd cd cd cd


pKDE->Type dd
pKDE->Len 0x16
pKDE->OUI 0 f ac
GTK Key[1] len=16 fd0aced5963ec57339fb295c29c3eda1

00-0F-AC-1 so it's a GTK KDE, len=16d

in ParseKeyData() there's

memset(pGroupKey, 0, sizeof(NDIS_802_11_KEY) + LEN_EAP_KEY);
pGroupKey->Length = sizeof(NDIS_802_11_KEY) + LEN_EAP_KEY;
pGroupKey->KeyIndex = 0x20000000 | pKDE->GTKEncap.Kid;
pGroupKey->KeyLength = GTKLEN;

memcpy(pGroupKey->BSSID, pAd->PortCfg.Bssid, ETH_ALEN);
memcpy(pGroupKey->KeyMaterial, pKDE->GTKEncap.GTK, 32);

Copying over too much (32) is no problem but I don't think the function following this will cope well

// Call Add peer key function
RTMPWPAAddKeyProc(pAd, pGroupKey);

// 4. Select RxMic / TxMic based on Supp / Authenticator
if (pAd->PortCfg.AuthMode == Ndis802_11AuthModeWPANone) {
// for WPA-None Tx, Rx MIC is the same
pTxMic = (PUCHAR) (&pKey->KeyMaterial) + 16;
pRxMic = pTxMic;
} else if (bAuthenticator == TRUE) {
pTxMic = (PUCHAR) (&pKey->KeyMaterial) + 16;
pRxMic = (PUCHAR) (&pKey->KeyMaterial) + 24;
} else {
pRxMic = (PUCHAR) (&pKey->KeyMaterial) + 16;
pTxMic = (PUCHAR) (&pKey->KeyMaterial) + 24;
}


Why does it refer to Keymaterial+16 (so the is the data where dd 0 cd cd... is), so this can't be right ?
Is my access point sending out badthings or should the code be changed here somehow ?

No need to explain me why I would be wrong here, if there's nothing wrong with the code you can tell me without further explaining (but I'm interested in learning of course)

TomDeMan

13-08-2008 09:43:57

I got key rotation fixed now, but need some help on how to make a patch file that I can post here.
From what I saw key rotation would never have worked for WPA2, it will always have reauthenticated completely (losing a lot of time and throughput). My changes fixes it here on my gentoo with the rt61 driver and a cisco AP.

I'm new to linux, so don't know if it's normal that "cvs diff -u" asks for a password for markwallis@rt2400.cvs.sourceforge.net ?
From which folder do I need to run this cvs command or do I really need some pwd to get it working ?

IvD

13-08-2008 16:25:20

You best copy the folder to a clean version and one with your modifications.
Then you can do

diff -rU3 rt61.clean rt61 > wpa2fix.diff

TomDeMan

14-08-2008 14:01:22

Thanks Ivo !
Attached is the diff file (I deleted the parts that said there were no differences, I hope that's OK)

About the changes the first is the IE mismatch warning which I think is because it must be 2 bytes further to compare, but I left in the original compare also (just in case someone knew what he was doing originally and maybe this +2 only needed to be added for other cases). If someone knows why +2 is always better we can remove the original without +2

The next change is the group key which needed cooking before adding to the crypt engine (which wasn't done so this would never have worked before).

Some source code for TKIP has moved around a bit to make less changes needed for WPA2 but hasn't changed.

Feedback and improvements are welcome !

Vern

14-08-2008 16:41:33

Hi TomDeMan,

Got it. I'll look at it. It'll be a bit, though.

Thanks,

Vern

19-08-2008 01:56:20

Hi TomDeMan,

I've started to go through your patch and try to understand what's happening.
From what I saw key rotation would never have worked for WPA2, it will always have reauthenticated completely[/quote3fyvtiuz]I'm a little unclear here. Are you saying it doesn't work - i.e. fails to re-key, or that it takes a long time?

Are you using WPA1 or WPA2? It sounds like the latter, but I'm not sure.
Thanks,

TomDeMan

19-08-2008 09:18:23

Hi Vern,

It failed to re-key using the group key broadcast because it got a cipher error not long after the group key came in.
It then recovered by re-authenticating (saw this in the AP logs). This is in WPA2 AES, not in WPA TKIP where I didn't see any of these problems.

I could reproduce this behaviour by setting the group rekeying interval on my cisco AP to 30 seconds. I noticed the pings having a longer timeout or even failure every 30 seconds then. After the patch pings went smoothly and no more cipher errors (at least not every 30 seconds).

I'm pretty sure WPA2 group rekeying didn't work before as the incoming data was used completely as a cipher for AES, without any cooking done. The ieee pdf I got earlier had a section about how the group key is made up, and it's with a headers so some cooking was needed.

Vern

20-08-2008 18:50:43

Hey TomDeMan,

Thanks for the patch. I do have a couple of observations

ParseKeyData()
Re. "RSN IE mismatched" msg Looks like you fixed a bug in the comparison.

For WPA2, pair handshake message 3 KDE Type field is set to 0xdd (i.e. == WPA1). So, as I read it, it should be that *pKeyData == 0xdd, and *(pKeyData+1) == length. So indeed the check starts in the wrong place for WPA2. Since the function is only called from WPA2, the original check can be deleted. Might check that indeed *pKeyData == 0xdd though. (What's life without paranoia?)

WpaGroupMsg1Action()
I recall getting into trouble with this myself with the rt2500 driver. For WPA1, the key data is just the key. So you need to get the index from where the current code gets it. For WPA2, I believe you have a fix.

The requirement here is to handle both WPA1 and WPA2, so could you change the patch to do that? Please do it better than the "cut-n-paste big gobs of code then change two lines" approach that you see here now.

Thanks,

TomDeMan

21-08-2008 10:34:28

Hi Vern,

The Eid is 0x30 = IE_RSN = WPA2RSNIE = IE_WPA2 here for WPA2 (checked it on an incoming frame), not 0xdd = WPARSNIE so the nice looking compare would be (for WPA2 only since it's only called for WPA2)

if (((PEID_STRUCT)pKeyData)->Eid == IE_RSN &&
((PEID_STRUCT)pKeyData)->Len == pEid->Len &&
memcmp(((PEID_STRUCT)pKeyData)->Octet, pEid->Octet, pEid->Len) == 0) {
DBGPRINT(RT_DEBUG_TRACE, " RSN IE matched !!!!!!!!!! \n");
} else {
DBGPRINT(RT_DEBUG_ERROR, " RSN IE mismatched !!!!!!!!!! \n");
}


but probably this is just the fastest way and the most similar to the existing one to check it, should work for WPA1 and WPA2 if pEid is packed

if (memcmp(pKeyData, pEid, pEid->Len+2) != 0) {
DBGPRINT(RT_DEBUG_ERROR, " RSN IE mismatched !!!!!!!!!! \n");
} else {
DBGPRINT(RT_DEBUG_TRACE, " RSN IE matched !!!!!!!!!! \n");
}

unless it's no mismatch if only the 0x30 and 0xdd Eid's mismatch in which case it should be

if (memcmp(pKeyData+1, &(pEid->Len), pEid->Len+1) != 0) {
DBGPRINT(RT_DEBUG_ERROR, " RSN IE mismatched !!!!!!!!!! \n");
} else {
DBGPRINT(RT_DEBUG_TRACE, " RSN IE matched !!!!!!!!!! \n");
}

but then why bother checking the Len also explicitly then, so this is probably the best thing since the packing doesn't need to be adapted

if (memcmp(pKeyData+2, pEid->Octet, pEid->Len) != 0) {
DBGPRINT(RT_DEBUG_ERROR, " RSN IE mismatched !!!!!!!!!! \n");
} else {
DBGPRINT(RT_DEBUG_TRACE, " RSN IE matched !!!!!!!!!! \n");
}

and by coincidence that's what I added to begin with in the patch, so maybe that's the best way after all ?





You say it's only called for WPA2 (since it's called from Wpa2PairMsg3Action), but where does it also say TKIP in Wpa2PairMsg3Action then ?


if (pAd->PortCfg.WepStatus == Ndis802_11Encryption3Enabled) {
// Decrypt AES GTK
AES_GTK_KEY_UNWRAP(&pAd->PortCfg.PTK[16], KEYDATA,
pMsg3->KeyDesc.KeyDataLen[1],
pMsg3->KeyDesc.KeyData);

ParseKeyData[/color2ig0js5c](pAd, KEYDATA, pMsg3->KeyDesc.KeyDataLen[1]);
} else // TKIP
{
INT i;
// Decrypt TKIP GTK
// Construct 32 bytes RC4 Key
memcpy(Key, pMsg3->KeyDesc.KeyIv, 16);
memcpy(&Key[16], &pAd->PortCfg.PTK[16], 16);
ARCFOUR_INIT(&pAd->PrivateInfo.WEPCONTEXT, Key, 32);
//discard first 256 bytes
for (i = 0; i < 256; i++)
ARCFOUR_BYTE(&pAd->PrivateInfo.WEPCONTEXT);
// Decrypt GTK. Becareful, there is no ICV to check the result is correct or not
ARCFOUR_DECRYPT(&pAd->PrivateInfo.WEPCONTEXT, KEYDATA,
pMsg3->KeyDesc.KeyData,
pMsg3->KeyDesc.KeyDataLen[1]);

DBGPRINT_RAW(RT_DEBUG_TRACE, "KEYDATA = \n");
for (i = 0; i < 100; i++) {
DBGPRINT_RAW(RT_DEBUG_TRACE, "%2x ", KEYDATA);
if (i % 16 == 15)
DBGPRINT_RAW(RT_DEBUG_TRACE, "\n ");
}
DBGPRINT_RAW(RT_DEBUG_TRACE, "\n \n");

ParseKeyData[/color2ig0js5c](pAd, KEYDATA, pMsg3->KeyDesc.KeyDataLen[1]);

}


ParseKeyData will probably only allow a WPA2 IE from pVIE = pAd->ScanTab.BssEntry[Idx].VarIEs

while (Len > 0) {
pEid = (PEID_STRUCT) pVIE;
if (pEid->Eid != IE_RSN) {
pVIE += (pEid->Len + 2);
Len -= (pEid->Len + 2);
continue;
}

so I don't really understand how it works for TKIP there anyway...

When adding a log to the TKIP section there in Wpa2PairMsg3Action I didn't manage to get there (tried AP in WPA1, WPA2, WPA1+WPA2) so maybe that TKIP part is just there to be safe but never used ?






Could you check my patch for WpaGroupMsg1Action() again, it should be fine as for WPA2 the key is coming from

pGroupKey->KeyIndex =
0x20000000 | pKDE->GTKEncap.Kid;

and for WPA1 it still comes from

pGroupKey->KeyIndex = 0x20000000 | pGroup->KeyDesc.KeyInfo.KeyIndex;


I just moved that last bit of code to WPA1 only (didn't remove it)



So do I need to change something to the patch after all ?

Vern

23-08-2008 01:25:49

Hi TomDeMan,

I did a little more digging.

Wrt. ParseKeyData()
Looking more into this, it turns out that Ralink's version of the rt61 legacy driver has the entire check commented out. In the rt73 and rt2570 legacy drivers, the result of the comparison is not used. And it makes sense The RSN IE that is being compared with is gotten from either a beacon or probe response frame, and never has a KDE in it; just cipher type specs. So the whole thing could just be deleted, as far as I can see.

Wrt. WpaGroupMsg1Action()
Basically, the decision as to where to get the index from needs to be based on whether we have WPA1 or WPA2; not on AES vs. TKIP. We can have AES or TKIP under either scheme.

Thanks,

TomDeMan

25-08-2008 12:49:18

Hi Vern,

You know this code, that's for sure !

So I'll make a new patch with the whole "RSN IE (mis)matched !!!!!!!!!!" check gone since it only got mismatch logs when nothing was wrong ?

And you mean for WpaGroupMsg1Action() there should be something like this in there ?

if (pAd->PortCfg.AuthMode == Ndis802_11AuthModeWPAPSK) {
pGroupKey->KeyIndex = 0x20000000 | pGroup->KeyDesc.KeyInfo.KeyIndex;
}
} else if (pAd->PortCfg.AuthMode == Ndis802_11AuthModeWPA2PSK) {
pGroupKey->KeyIndex =
0x20000000 | pKDE->GTKEncap.Kid;
}

Attached is a new go at fixing it for WPA2 (regardless of which encryption it's using). Let me know if the diff file is any good, I typed the text in windows (like I said, I'm very fresh into linux).
Somehow I think you'll have still some more suggestions for improvement. Be my guest !

Vern

25-08-2008 21:13:51

Hi TomDeMan,

Thanks for your work.

Overall, the patch looks like it achieves its intended result. Using my homemade AP (WPA1 only) it seems to work. However, I can only test it as far as getting through WPA group handshake message one OK (there are issues when AP and STA are on the same machine).

Here are two more comments

ParseKeyData()
The whole while loop to check for an RSN IE can be deleted, since the result of the check is not used.

WpaGroupMsg1Action()
Things are only good if AuthMode == Ndis802_11AuthModeWPAPSK or Ndis802_11AuthModeWPA2PSK, since the code only runs if one or the other of those is true. (cf. WpaEAPOLKeyAction().)

These are optimizations, really - reducing the amount of software entropy. If possible, could you modify the patch accordingly - if you wish - then verify WPA2 operation and - if possible - regress against WPA1?

If those end up OK, we can put it into CVS.

Thanks,

TomDeMan

26-08-2008 12:08:46

Hi Vern,

Maybe best you have it verified by someone else too, I'm not good enough in linux to recognise the symptoms of bad code (I just do some pings and view the logs to see if everything is OK).
But after all what you told me seems like this might be the right patch.

Vern

26-08-2008 17:42:22

Hi TomDeMan,

Well, it looks OK to me, as far as I can test WPA1 operation.

Would it be possible to build your patched driver without debug enabled, then interact with your re-keying AP to do whatever it was that caused the symptoms you originally observed, and verify that everything is OK now?

Thanks,

TomDeMan

28-08-2008 10:26:02

Hi Vern,

I've passed testing on to someone here who will test the patch independently from me, I should have the results early next week.

Meanwhile, there's still another smaller issue that I see which is similar but occurs less (because I've set the group rekeying to every 15 seconds to hit it hard) and recovers faster than the group key exchange when it went bad before

-----> WpaEAPOLKeyAction
KeyInfo Key Description Version 2
KeyInfo Key Type 1
KeyInfo Key Index 0
KeyInfo Install 0
KeyInfo Key Ack 1
KeyInfo Key MIC 0
KeyInfo Secure 0
KeyInfo Error 0
KeyInfo Request 0
KeyInfo EKD_DL 0
Receive EAPOL Key Pairwise Message 1
Wpa2PairMsg1Action ----->
Wpa2PairMsg1Action <-----
<----- WpaEAPOLKeyAction

With type=1 this isn't the group key update, it's an EAPOL Key Pairwise Message 1

so it hits this code

case SS_WAIT_GROUP // When doing group key exchange
case SS_FINISH // This happened when update group key
if (MsgType == EAPOL_PAIR_MSG_1) {
Wpa2PairMsg1Action(pAd, Elem);
pAd->PortCfg.WpaState = SS_WAIT_MSG_3;
// Reset port secured variable
pAd->PortCfg.PortSecured =
WPA_802_1X_PORT_NOT_SECURED;


then the next incoming hits this

case SS_WAIT_MSG_3
if (MsgType == EAPOL_PAIR_MSG_1) {
Wpa2PairMsg1Action(pAd, Elem);
pAd->PortCfg.WpaState = SS_WAIT_MSG_3;


but never seems to go into here

} else if (MsgType == EAPOL_PAIR_MSG_3) {
Wpa2PairMsg3Action(pAd, Elem);
pAd->PortCfg.WpaState = SS_WAIT_GROUP;


but it quickly recovers using

case SS_START
if (MsgType == EAPOL_PAIR_MSG_1) {
Wpa2PairMsg1Action(pAd, Elem);
pAd->PortCfg.WpaState = SS_WAIT_MSG_3;
}

and it logs a LINK DOWN and LINK UP. I recovers quick enough to not timeout a ping (but there will be some larger delay).


Could you point me into the right direction on fixing this one too possibly ? It probably means the packet we send out in Wpa2PairMsg1Action() isn't accepted by the access point, so it goes back into the reauthentication.
The access point shows something like

Interface Dot11Radio0, Deauthenticating Station 000d.f034.47a6 Reason Previous authentication no longer valid

but maybe that's just the reauthentication from SS_START.

It's not an urgent problem but I'd like to fix it too while I'm at it.

Vern

20-09-2008 01:07:46

Hi TomDeMan,

How're we doing with testing the AP rekey patch?

Thanks,

TomDeMan

26-09-2008 08:07:46

Just got word from the independant tester he'll do it next monday.

TomDeMan

01-10-2008 13:06:51

Hi Vern,
The rekeying on the AP was set to 15 seconds and with the old code he couldn't even make a connection (no time to get a DHCP address). With the newest patch applied everything worked fine. So the patch seems like a winner.

Vern

06-10-2008 16:08:37

Hi TomDeMan,

Sorry to keep you slowly turning in the breeze, here. You should see your patch start showing up in the CVS tarballs soon. Nice work, and thanks.

I've got some other irons in the fire right now, so I haven't looked at your other item yet.

Thanks again,

Vern

13-10-2008 20:25:33

Hi TomDeMan,

I've taken a brief look at your post about rekeying when WPA1 is used. The best I have right now is random musings.

From the information you provided, it looks like the 4-way handshake is used to re-key under WPA1. I guess this is legal, but I hadn't really thought about it. I suspect that it was also not envisaged when the driver was originally written; so that the working assumption for WPA1 was always a single sequence of authenticate -> associate -> pair key exchange -> group key exchange.

As a result, it's probably dumb luck that it works at all. What do you find if you walk thru the code assuming a sequence of
authenticate -> associate -> EAPOL pair -> EAPOL group -> EAPOL pair ->
EAPOL pair -> EAPOL pair -> ...

Maybe there'se something clever we could do with staying "Port Secured" when this happens? Let me know your thoughts. (Reassuring, huh?)

Thanks,

Vern

15-10-2008 16:44:42

Also, it looks like I gave you a bum steer about deleting the RSN check in the function ParseKeyData(). This function is only called from Wpa2PairMsg3Action() when we're processing WPA2 pair message 3. I guess I was confusing KDE and RSN. 802.11-2007 pp. 215 says this about the RSNIE[quote2g9xidho]"If it is not identical to that the STA received in the Beacon or Probe Response frame, the STA shall disssociate."[/quote2g9xidho]So I've restored that check back into CVS. There is still a problem because it looks like this routine doesn't do anything when an RSNIE mismatch is detected except issue messages about the result when debug is enabled - not sufficient.

We need to re-investigate this when the problem that caused us (OK, me) to think the check should be deleted pops up again. Possibilities are
[list2g9xidho]1. The driver didn't properly save the beacon/probe response RSN.
2. There is a bug in the check.
3. The AP is really screwing up.[/listu2g9xidho]
Regardless, to conform to the spec, we need to check, and disassociate if the check fails.

Thanks,