[PATCH] Make ICV Indicator dependant on ICV length

[Jaap Keuter]

> On 9 May 2017, at 11:48, Jouni Malinen <j at w1.fi> wrote:
> 
> 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?
> 

In practice this means that the ICV Indicator parameter set is no longer encoded in the MKPDU, since the ICV has the default length, as per the only available algorithm in the standard.
This parameter set merely exists to cover future extensions, using different ICV lengths, while allowing pre-extension implementations to properly identify the PDU element boundaries.
With this patch, when in the future other ICV lengths were to be used, the ICV Indicator will be encoded again by wpa_supplicant, indicating the actual ICV length of the MKPDU.

As for interoperability, anything can have interop consequences with equipment that doesn’t implement the standard as defined. Whether this is the case here is hard to say. I have seen hardly any traces containing MKPDU’s. The only identifiable trace of a Cicso device I have seen had the ICV Indicator present (with default value 16), it is unknown if it would accept MKPDUs without the ICV Indicator. Currently I have no other equipment to test with.


> 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).

Completely agree, the language used is convoluted at best, IMHO. 
From the way how they changed the wording in 802.1Xbx-2014 I concluded that they try to say that the ICV indicator should only be encoded incase of a non-default ICV length.


> 
>> 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?
> 

This indeed returns 16 for the default ICV length, which causes ieee802_1x_mka_encode_icv_body() to skip encoding the ICV Indicator, which is correct, IMHO. So even if the function is called encode_icv_body, it actually encodes the ICV Indicator and the trailing ICV, in one go. This should have been done separately (the ICV Indicator and ICV have no defined spatial relation, other than that the ICV Indicator is to be the last Parameter Set and the ICV follows the last Parameter Set), but is very convenient from an implementation point of view.

As described above, interop tests were done with wpa_supplicant only (with and without this patch applied), because of lack of other MKA capable equipment.

So, if you feel this change is too risky, I can understand you dropping it, although it would implement the standard as intended, IMHO.
If anyone has an informed opinion on this I would be interested.

Thanks,
Jaap