diff mbox series

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

Message ID 20180309205305.48783-2-msiedzik@extremenetworks.com
State Changes Requested
Headers show
Series [1/2] mka: ICV not encoded correctly, causing receive side to drop MKPDU | expand

Commit Message

Michael Siedzik March 9, 2018, 8:53 p.m. UTC 
From: Mike Siedzik <msiedzik@extremenetworks.com>

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@xs4all.nl> 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@extremenetworks.com>
---
 src/pae/ieee802_1x_kay.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

--
2.11.1
diff mbox series

Patch

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;
 }