[PATCH] Make ICV Indicator dependant on ICV length

[Jouni Malinen]

On Fri, Apr 07, 2017 at 11:39:23PM +0200, Jaap Keuter wrote:
> IEEE 802.1X-2010 Section 11.11 describes that the ICV is seperate from
> the parameter sets before it. Due to its convenient layout the ICV
> Indicator 'body part' is used to encode the ICV as well.
> 
> IEEE 802.1X-2010 Section 11.11.3 describes the encoding of MKPDUs.
> In bullet e) is desribed that the ICV Indicator itself is encoded when
> the ICV is not 16 octets in length. IEEE 802.1Xbx-2014 Table 11.7 note
> e) states that it will not be encoded unless the Algorithm Agility
> parameter specifies the use of an ICV that is not 16 octets in length.
> 
> Therefore the length calculation for the ICV indicator body part must
> take into account if the ICV Indicator is to be encoded or not. The
> actual encoder of the ICV body already takes care of the rest.

It would be good to clearly describe the reason and outcome of this type
of a change. Looking at the current mka_alg_tbl[], it looks like there
is only one algorithm and it has icv_len == DEFAULT_ICV_LEN. As such,
this patch would modify ieee802_1x_mka_get_icv_length() behavior for all
cases. What does that do in practice? Can this have interoperability
issues with other devices?

It looks like IEEE Std 802.1X-2010 did not specify clearly whether the
ICV Indicator is to be included or not (i.e., there is no statement
saying it must not be there if ICV is 16 octets). The rules in 11.11.4
for decoding MKPDUs seem to say that receivers should handle both cases.
In other words, it looks to me that the current implementation would be
compliant with 802.1X-2010, but 802.1Xbx-2014 changes this with the
"will not be encoded" language (which, by the way, is pretty bad
standards language.. it should be "shall not be" instead of "will not
be" if this is really making a mandatory requirement and I would not
really use "encoded" here, i.e., "included" etc. would be clearer).

> diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c
> @@ -1665,9 +1665,11 @@ ieee802_1x_mka_icv_body_present(struct ieee802_1x_mka_participant *participant)
>  static int
>  ieee802_1x_mka_get_icv_length(struct ieee802_1x_mka_participant *participant)
>  {
> -	int length;
> +	int length = 0;
>  
> -	length = sizeof(struct ieee802_1x_mka_icv_body);
> +	/* determine if we need space for the ICV Indicator */
> +	if (mka_alg_tbl[participant->kay->mka_algindex].icv_len != DEFAULT_ICV_LEN)
> +		length = sizeof(struct ieee802_1x_mka_icv_body);
>  	length += mka_alg_tbl[participant->kay->mka_algindex].icv_len;
>  
>  	return MKA_ALIGN_LENGTH(length);

So this used to return sizeof(struct ieee802_1x_mka_icv_body) +
mka_alg_tbl[participant->kay->mka_algindex].icv_len = 4 + 16 = 20 and
with this patch, this would return 16 ( == DEFAULT_ICV_LEN).

This would mean that ieee802_1x_mka_encode_icv_body() used to add
MKA_ICV_INDICATOR, but after this patch it wouldn't. Have you tested
potential interoperability impact with other implementations?

-- 
Jouni Malinen                                            PGP id EFC895FA