WPA (IEEE802.11) decryption function cleanups

 - Updated AirPDcapPacketProcess function description
 - Try to return better error codes
 - Remove broken/useless return of keys from AirPDcapRsna4WHandshake

Change-Id: I1e4e0a76f6d1307e11c0466f17935dd7030561e1
Reviewed-on: https://code.wireshark.org/review/12033
Petri-Dish: Michael Mann <mmann78@netscape.net>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Michael Mann <mmann78@netscape.net>

2 files changed, 44 insertions, 56 deletions

diff --git a/epan/crypt/airpdcap.c b/epan/crypt/airpdcap.c
index 142061ff17..cc60176a27 100644
--- a/epan/crypt/airpdcap.c
+++ b/epan/crypt/airpdcap.c
@@ -193,7 +193,6 @@ static INT AirPDcapRsna4WHandshake(
     PAIRPDCAP_CONTEXT ctx,
     const UCHAR *data,
     AIRPDCAP_SEC_ASSOCIATION *sa,
-    PAIRPDCAP_KEY_ITEM key,
     INT offset)
     ;
 /**
@@ -314,7 +313,7 @@ typedef struct {
 #define GROUP_KEY_PAYLOAD_LEN_MIN RSN_KEY_WITHOUT_KEYBYTES_LEN+CCMP_GROUP_KEY_LEN
 
 /* XXX - what if this doesn't get the key? */
-static void
+static INT
 AirPDcapDecryptWPABroadcastKey(const EAPOL_RSN_KEY *pEAPKey, guint8  *decryption_key, PAIRPDCAP_SEC_ASSOCIATION sa, gboolean group_hshake)
 {
     guint8 key_version;
@@ -338,12 +337,12 @@ AirPDcapDecryptWPABroadcastKey(const EAPOL_RSN_KEY *pEAPKey, guint8  *decryption
 
         /* AES keys must be at least 128 bits = 16 bytes. */
         if (key_bytes_len < 16) {
-            return;
+            return AIRPDCAP_RET_NO_VALID_HANDSHAKE;
         }
     }
 
     if (key_bytes_len > TKIP_GROUP_KEYBYTES_LEN_MAX || key_bytes_len == 0) { /* Don't read past the end of pEAPKey->ie */
-        return;
+        return AIRPDCAP_RET_NO_VALID_HANDSHAKE;
     }
 
     /* Encrypted key is in the information element field of the EAPOL key packet */
@@ -421,7 +420,7 @@ AirPDcapDecryptWPABroadcastKey(const EAPOL_RSN_KEY *pEAPKey, guint8  *decryption
 
             if (rsn_id != 0xdd){
                 if (key_index+1 >= key_bytes_len){
-                    return;
+                    return AIRPDCAP_RET_NO_VALID_HANDSHAKE;
                 }
                 key_index += decrypted_data[key_index+1]+2;
             }else{
@@ -431,7 +430,8 @@ AirPDcapDecryptWPABroadcastKey(const EAPOL_RSN_KEY *pEAPKey, guint8  *decryption
 
         if (key_found){
             if (key_index+8 >= key_bytes_len)
-                return;
+                return AIRPDCAP_RET_NO_VALID_HANDSHAKE;
+
             /* Skip over the GTK header info, and don't copy past the end of the encrypted data */
             memcpy(szEncryptedKey, decrypted_data+key_index+8, key_bytes_len-key_index-8);
         }
@@ -453,6 +453,7 @@ AirPDcapDecryptWPABroadcastKey(const EAPOL_RSN_KEY *pEAPKey, guint8  *decryption
     memset(sa->wpa.ptk, 0, sizeof(sa->wpa.ptk));
     memcpy(sa->wpa.ptk+32, szEncryptedKey, key_len);
     g_free(szEncryptedKey);
+    return AIRPDCAP_RET_SUCCESS_HANDSHAKE;
 }
 
 
@@ -480,8 +481,7 @@ static INT AirPDcapScanForKeys(
     const guint8 *data,
     const guint mac_header_len,
     const guint tot_len,
-    AIRPDCAP_SEC_ASSOCIATION_ID id,
-    PAIRPDCAP_KEY_ITEM key
+    AIRPDCAP_SEC_ASSOCIATION_ID id
 )
 {
     const UCHAR *addr;
@@ -556,11 +556,11 @@ static INT AirPDcapScanForKeys(
         sa = AirPDcapGetSaPtr(ctx, &id);
         if (sa == NULL){
             AIRPDCAP_DEBUG_PRINT_LINE("AirPDcapScanForKeys", "No SA for BSSID found", AIRPDCAP_DEBUG_LEVEL_3);
-            return AIRPDCAP_RET_UNSUCCESS;
+            return AIRPDCAP_RET_REQ_DATA;
         }
 
         /* It could be a Pairwise Key exchange, check */
-        if (AirPDcapRsna4WHandshake(ctx, data, sa, key, offset) == AIRPDCAP_RET_SUCCESS_HANDSHAKE)
+        if (AirPDcapRsna4WHandshake(ctx, data, sa, offset) == AIRPDCAP_RET_SUCCESS_HANDSHAKE)
             return AIRPDCAP_RET_SUCCESS_HANDSHAKE;
 
         if (mac_header_len + GROUP_KEY_PAYLOAD_LEN_MIN > tot_len) {
@@ -584,7 +584,7 @@ static INT AirPDcapScanForKeys(
         /* get the Security Association structure for the broadcast MAC and AP */
         sa = AirPDcapGetSaPtr(ctx, &id);
         if (sa == NULL){
-            return AIRPDCAP_RET_UNSUCCESS;
+            return AIRPDCAP_RET_REQ_DATA;
         }
 
         /* Get the SA for the STA, since we need its pairwise key to decrpyt the group key */
@@ -603,18 +603,18 @@ static INT AirPDcapScanForKeys(
 
         sta_sa = AirPDcapGetSaPtr(ctx, &id);
         if (sta_sa == NULL){
-            return AIRPDCAP_RET_UNSUCCESS;
+            return AIRPDCAP_RET_REQ_DATA;
         }
 
-        /* Extract the group key and install it in the SA */
-        AirPDcapDecryptWPABroadcastKey(pEAPKey, sta_sa->wpa.ptk+16, sa, TRUE);
+        /* Try to extract the group key and install it in the SA */
+        return (AirPDcapDecryptWPABroadcastKey(pEAPKey, sta_sa->wpa.ptk+16, sa, TRUE));
 
     }else{
         AIRPDCAP_DEBUG_PRINT_LINE("AirPDcapScanForKeys", "Skipping: not an EAPOL packet", AIRPDCAP_DEBUG_LEVEL_3);
     }
 
     AIRPDCAP_DEBUG_TRACE_END("AirPDcapScanForKeys");
-    return 0;
+    return AIRPDCAP_RET_NO_VALID_HANDSHAKE;;
 }
 
 
@@ -643,12 +643,12 @@ INT AirPDcapPacketProcess(
     if (ctx==NULL) {
         AIRPDCAP_DEBUG_PRINT_LINE("AirPDcapPacketProcess", "NULL context", AIRPDCAP_DEBUG_LEVEL_5);
         AIRPDCAP_DEBUG_TRACE_END("AirPDcapPacketProcess");
-        return AIRPDCAP_RET_UNSUCCESS;
+        return AIRPDCAP_RET_REQ_DATA;
     }
     if (data==NULL || tot_len==0) {
         AIRPDCAP_DEBUG_PRINT_LINE("AirPDcapPacketProcess", "NULL data or length=0", AIRPDCAP_DEBUG_LEVEL_5);
         AIRPDCAP_DEBUG_TRACE_END("AirPDcapPacketProcess");
-        return AIRPDCAP_RET_UNSUCCESS;
+        return AIRPDCAP_RET_REQ_DATA;
     }
 
     /* check if the packet is of data type */
@@ -686,8 +686,9 @@ INT AirPDcapPacketProcess(
         if (scanHandshake) {
             /* data is sent in cleartext, check if is an authentication message or end the process */
             AIRPDCAP_DEBUG_PRINT_LINE("AirPDcapPacketProcess", "Unencrypted data", AIRPDCAP_DEBUG_LEVEL_3);
-            return (AirPDcapScanForKeys(ctx, data, mac_header_len, tot_len, id, key));
+            return (AirPDcapScanForKeys(ctx, data, mac_header_len, tot_len, id));
         }
+        return AIRPDCAP_RET_NO_DATA_ENCRYPTED;
     } else {
         PAIRPDCAP_SEC_ASSOCIATION sa;
         int offset = 0;
@@ -695,7 +696,7 @@ INT AirPDcapPacketProcess(
         /* get the Security Association structure for the STA and AP */
         sa = AirPDcapGetSaPtr(ctx, &id);
         if (sa == NULL){
-            return AIRPDCAP_RET_UNSUCCESS;
+            return AIRPDCAP_RET_REQ_DATA;
         }
 
         /* cache offset in the packet data (to scan encryption data) */
@@ -730,7 +731,7 @@ INT AirPDcapPacketProcess(
                it needs to change the group key to be used. */
             if (AIRPDCAP_KEY_INDEX(data[offset+3])>=1){
 
-                AIRPDCAP_DEBUG_PRINT_LINE("AirPDcapPacketProcess", "The key index = 1. This is encrypted with a group key.", AIRPDCAP_DEBUG_LEVEL_3);
+                AIRPDCAP_DEBUG_PRINT_LINE("AirPDcapPacketProcess", "The key index >= 1. This is encrypted with a group key.", AIRPDCAP_DEBUG_LEVEL_3);
 
                 /* force STA address to broadcast MAC so we load the SA for the groupkey */
                 memcpy(id.sta, broadcast_mac, AIRPDCAP_MAC_LEN);
@@ -743,7 +744,7 @@ INT AirPDcapPacketProcess(
                 /* search for a cached Security Association for current BSSID and broadcast MAC */
                 sa = AirPDcapGetSaPtr(ctx, &id);
                 if (sa == NULL)
-                    return AIRPDCAP_RET_UNSUCCESS;
+                    return AIRPDCAP_RET_REQ_DATA;
             }
 
             /* Decrypt the packet using the appropriate SA */
@@ -752,7 +753,7 @@ INT AirPDcapPacketProcess(
                    The group key handshake could be sent at any time the AP wants to change the key (such as when
                    it is using key rotation) and it also could be a rekey for the Pairwise key. So we must scan every packet. */
                 if (scanHandshake) {
-                    return (AirPDcapScanForKeys(ctx, decrypt_data, mac_header_len, *decrypt_len, id, NULL));
+                    return (AirPDcapScanForKeys(ctx, decrypt_data, mac_header_len, *decrypt_len, id));
                 } else {
                     return AIRPDCAP_RET_SUCCESS;
                 }
@@ -1154,7 +1155,7 @@ AirPDcapWepMng(
 
     g_free(try_data);
     if (ret_value)
-        return ret_value;
+        return AIRPDCAP_RET_UNSUCCESS;
 
     AIRPDCAP_DEBUG_PRINT_LINE("AirPDcapWepMng", "WEP DECRYPTED!!!", AIRPDCAP_DEBUG_LEVEL_3);
 
@@ -1183,7 +1184,6 @@ AirPDcapRsna4WHandshake(
     PAIRPDCAP_CONTEXT ctx,
     const UCHAR *data,
     AIRPDCAP_SEC_ASSOCIATION *sa,
-    PAIRPDCAP_KEY_ITEM key,
     INT offset)
 {
     AIRPDCAP_KEY_ITEM *tmp_key, *tmp_pkt_key, pkt_key;
@@ -1329,15 +1329,6 @@ AirPDcapRsna4WHandshake(
                     /* the temporary key is the correct one, cached in the Security Association */
 
                     sa->key=tmp_key;
-
-                    if (key!=NULL) {
-                        memcpy(key, tmp_key, sizeof(AIRPDCAP_KEY_ITEM));
-                        if (AIRPDCAP_EAP_KEY_DESCR_VER(data[offset+1])==AIRPDCAP_WPA_KEY_VER_NOT_CCMP)
-                            key->KeyType=AIRPDCAP_KEY_TYPE_TKIP;
-                        else if (AIRPDCAP_EAP_KEY_DESCR_VER(data[offset+1])==AIRPDCAP_WPA_KEY_VER_AES_CCMP)
-                            key->KeyType=AIRPDCAP_KEY_TYPE_CCMP;
-                    }
-
                     break;
                 } else {
                     /* the cached key was not valid, try other keys */
@@ -1403,15 +1394,13 @@ AirPDcapRsna4WHandshake(
             broadcast_sa = AirPDcapGetSaPtr(ctx, &id);
 
             if (broadcast_sa == NULL){
-                return AIRPDCAP_RET_UNSUCCESS;
+                return AIRPDCAP_RET_REQ_DATA;
             }
-            AirPDcapDecryptWPABroadcastKey(pEAPKey, sa->wpa.ptk+16, broadcast_sa, FALSE);
+            return (AirPDcapDecryptWPABroadcastKey(pEAPKey, sa->wpa.ptk+16, broadcast_sa, FALSE));
         }
-
-        return AIRPDCAP_RET_SUCCESS_HANDSHAKE;
     }
 
-    return AIRPDCAP_RET_UNSUCCESS;
+    return AIRPDCAP_RET_NO_VALID_HANDSHAKE;
 }
 
 static INT
diff --git a/epan/crypt/airpdcap_system.h b/epan/crypt/airpdcap_system.h
index e947308ff5..1166e347c4 100644
--- a/epan/crypt/airpdcap_system.h
+++ b/epan/crypt/airpdcap_system.h
@@ -173,44 +173,43 @@ extern "C" {
 #endif
 
 /**
- * Given an 802.11 packet, either extract its key data (in the case of
- * WPA handshaking) or try to decrypt it.
+ * This will try to decrypt a 802.11 frame. If scanHandshake is
+ * true it will also check if it's a cleartext or encrypted eapol key
+ * frame which can be used to setup TK or GTK decryption keys.
  * @param ctx [IN] Pointer to the current context
  * @param data [IN] Pointer to a buffer with an 802.11 frame, including MAC
  *   header and payload
  * @param data_off [IN] Payload offset (aka the MAC header length)
  * @param data_len [IN] Total length of the MAC header and the payload
  * @param decrypt_data [OUT] Pointer to a buffer that will contain
- *   decrypted data
- * @param decrypt_len [OUT] Length of decrypted data
+ *   decrypted data. If this parameter is set to NULL, decrypted data will
+ *   be discarded.
+ * @param decrypt_len [OUT] Length of decrypted data if decrypt_data
+ *   is not NULL.
  * @param key [OUT] Pointer to a preallocated key structure containing
  *   the key used during the decryption process (if done). If this parameter
  *   is set to NULL, the key will be not returned.
  * @param scanHandshake [IN] If TRUE this function will additional check if
  *   the 802.11 frame data is pointing to has key information and if so use
- *   it to setup potential decryption keys.
+ *   it to setup potential decryption keys. Enables handshake return codes.
  * @return
  * - AIRPDCAP_RET_SUCCESS: Decryption has been done (decrypt_data and
  *   decrypt_length will contain the packet data decrypted and the length of
  *   the new packet)
- * - AIRPDCAP_RET_SUCCESS_HANDSHAKE: A step of the 4-way handshake for
- *   WPA key has been successfully done
  * - AIRPDCAP_RET_NO_DATA: The packet is not a data packet
  * - AIRPDCAP_RET_WRONG_DATA_SIZE: The size of the packet is below the
  *   accepted minimum
  * - AIRPDCAP_RET_REQ_DATA: Required data is not available and the
- *   processing must be interrupted
- * - AIRPDCAP_RET_NO_VALID_HANDSHAKE: The authentication is not for WPA or RSNA
- * - AIRPDCAP_RET_NO_DATA_ENCRYPTED: No encrypted data
- * - AIRPDCAP_RET_UNSUCCESS: No decryption has been done (decrypt_data
+ *   processing must be interrupted (can also occur after decryption when
+ *   scanHandshake is TRUE)
+ * - AIRPDCAP_RET_NO_DATA_ENCRYPTED: Not encrypted and no attempt to
+ *   extract key information
+ * - AIRPDCAP_RET_UNSUCCESS: Generic unspecified error (decrypt_data
  *   and decrypt_length will be not modified).
- * Some other errors could be:
- *   data not correct
- *   data not encrypted
- *   key handshake, not encryption
- *   decryption not successful
- *   key handshake not correct
- *   replay check not successful
+ * - AIRPDCAP_RET_SUCCESS_HANDSHAKE: An eapol handshake packet was successfuly parsed
+ *   and key information extracted
+ * - AIRPDCAP_RET_NO_VALID_HANDSHAKE: The handshake is invalid or was not used
+ *   for some reason. For encrypted packets decryption was still successful.
  * @note
  * The decrypted buffer should be allocated for a size equal or greater
  * than the packet data buffer size. Before decryption process original