| Message ID | 20170407213923.30891-1-jaap.keuter@xs4all.nl |
|---|---|
| State | Accepted |
| Headers | show |
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?
> On 9 May 2017, at 11:48, Jouni Malinen <j@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
On Tue, May 09, 2017 at 01:42:32PM +0200, Jaap Keuter wrote: > 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. I ended up applying this now. That said, have no means of testing this myself with other implementations and I have not heard of any clear indication that this would work with devices that have been deployed so far. Anyway, I do agree that the IEEE 802.1X standard is pretty clear on the receiver requirements, so from that view point, everything out there is supposed to be able to process the ICV without the separate set parameters header. It would be nice to hear if anyone has a chance to test the current hostap.git snapshot of wpa_supplicant against some other implementations. There's been number of MKA/MACsec changes in the code recently since I finally implemented hwsim test cases to cover most of this functionality and felt more confident about applying the pending patches and other cleanup and fixes that came from my own review of the implementation.
diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c index 1d4ed89c0..4bfef256c 100644 --- 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); @@ -1882,7 +1884,7 @@ static struct mka_param_body_handler mka_body_handler[] = { .body_present = NULL }, - /* icv parameter set */ + /* icv indicator */ { .body_tx = ieee802_1x_mka_encode_icv_body, .body_rx = NULL,
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. Signed-off-by: Jaap Keuter <jaap.keuter@xs4all.nl> --- src/pae/ieee802_1x_kay.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)