[PATCH 2/2] mka: Optional ICV indicator encoded when not necessary

[<msiedzik@xxxxxxxxxxxxxxxxxxx>]

From: Mike Siedzik <msiedzik@xxxxxxxxxxxxxxxxxxx>

IEEE802.1X-2010 Clause 11.11.3 "Encoding MKPDUs", item (e) states that
the ICV Indicator must be encoded if the ICV length is not 16 octets,
and Table 11-7 "MKPDU parameter sets" lists the "ICV Indicator" as
optional.

The function ieee802_1x_mka_encode_icv_body() intends to only encode the
ICV Indicator when the ICV length is not equal to DEFAULT_ICV_LEN(16).
However the comparison made is between the length of the entire ICV
parameter set, not just the ICV length.

This results in the KaY encoding and sending the optional ICV Indicator
when it was not necessary.  The resulting MKPDU is still valid, but is
4 octets longer than necessary.

Thanks to Jaap Keuter <jaap.keuter@xxxxxxxxx> for finding this bug.  He
recommends not applying this patch, as most real world implementations
of MKA send the ICV Indicator unconditionally.

Signed-off-by: Michael Siedzik <msiedzik@xxxxxxxxxxxxxxxxxxx>
---
 src/pae/ieee802_1x_kay.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c
index 9fa2c211f..e77edb724 100755
--- a/src/pae/ieee802_1x_kay.c
+++ b/src/pae/ieee802_1x_kay.c
@@ -1706,17 +1706,35 @@ ieee802_1x_mka_icv_body_present(struct ieee802_1x_mka_participant *participant)
        return TRUE;
 }

+/**
+ * ieee802_1x_mka_icv_indicator_required
+ *
+ * Per IEEE802.1X-2010, Clause 11.11.3 Encoding MKPDUs, item (e):
+ * "If Algorithm Agility parameter specifies the use of an ICV that
+ *  is not 16 octets in length, the ICV Indicator is encoded as
+ *  specified in Figure 11-6."
+ */
+static Boolean
+ieee802_1x_mka_icv_indicator_required(struct ieee802_1x_mka_participant *participant)
+{
+       return (mka_alg_tbl[participant->kay->mka_algindex].icv_len != DEFAULT_ICV_LEN);
+}

 /**
  * ieee802_1x_kay_get_icv_length
+ *
+ * Return the unaligned length of the ICV parameter set, which consists
+ * of an optional ICV Indicator plus a manditory ICV.
  */
 static int
 ieee802_1x_mka_get_icv_length(struct ieee802_1x_mka_participant *participant)
 {
        int length;

-       length = sizeof(struct ieee802_1x_mka_icv_body);
-       length += mka_alg_tbl[participant->kay->mka_algindex].icv_len;
+       length = mka_alg_tbl[participant->kay->mka_algindex].icv_len;
+       if (ieee802_1x_mka_icv_indicator_required(participant)) {
+               length += sizeof(struct ieee802_1x_mka_icv_body);
+       }

        return length;
 }
@@ -1730,14 +1748,18 @@ ieee802_1x_mka_encode_icv_body(struct ieee802_1x_mka_participant *participant,
                               struct wpabuf *buf)
 {
        struct ieee802_1x_mka_icv_body *body;
-       unsigned int length;
+       unsigned int icv_indicator_length;
+       unsigned int icv_length;
        u8 cmac[MAX_ICV_LEN];

-       length = ieee802_1x_mka_get_icv_length(participant);
-       if (length != DEFAULT_ICV_LEN)  {
-               body = wpabuf_put(buf, MKA_HDR_LEN);
+       icv_indicator_length = sizeof(struct ieee802_1x_mka_icv_body);
+       icv_length = mka_alg_tbl[participant->kay->mka_algindex].icv_len;
+
+       /* Only encode ICV Indicator if it is required */
+       if (ieee802_1x_mka_icv_indicator_required(participant)) {
+               body = wpabuf_put(buf, icv_indicator_length);
                body->type = MKA_ICV_INDICATOR;
-               set_mka_param_body_len(body, length - MKA_HDR_LEN);
+               set_mka_param_body_len(body, icv_length);
        }

        if (mka_alg_tbl[participant->kay->mka_algindex].icv_hash(
@@ -1746,7 +1768,8 @@ ieee802_1x_mka_encode_icv_body(struct ieee802_1x_mka_participant *participant,
                return -1;
        }

-       os_memcpy(wpabuf_put(buf, MKA_ALIGN_LENGTH(length - MKA_HDR_LEN)), cmac, length - MKA_HDR_LEN);
+       /* Always encode ICV */
+       os_memcpy(wpabuf_put(buf, MKA_ALIGN_LENGTH(icv_length)), cmac, icv_length);

        return 0;
 }
--
2.11.1


________________________________

DISCLAIMER:
This e-mail and any attachments to it may contain confidential and proprietary material and is solely for the use of the intended recipient. Any review, use, disclosure, distribution or copying of this transmittal is prohibited except by or on behalf of the intended recipient. If you have received this transmittal in error, please notify the sender and destroy this e-mail and any attachments and all copies, whether electronic or printed.


_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap