| Message ID | 20180302201103.16264-7-msiedzik@extremenetworks.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | MKA bugfixes and enhancements | expand |
On Fri, Mar 02, 2018 at 03:10:54PM -0500, msiedzik@extremenetworks.com wrote: > Per IEEE802.1X-2010 Clause 11.11 EAPOL-MKA, each parameter set must be > padded to a multiple of 4 octets. In order for the length of variable > length parameters, such as CAK Name, to be correctly decoded the > parameter set body length must not include the length of null padding > octets. > > When allocating buffer space for the parameter set (e.g., wpabuf_put()) > the padded length must be used. When setting the 'Parameter set body > length' within the parameter set the unpadded length must be used. > > Consider the case were the length of a PSK's CKN is not a multiple of 4 > octets. Currently ieee802_1x_mka_encode_basic_body() will correctly > reserve the padded number buffer bytes. However it will incorrectly > set ieee8021_x_mka_hdr->length and ->length1 to the padded number of > bytes. The receiver will not be able to recover the original CKN > length. > > Note that the hostap will successfully interoperate with itself because > both sides incorrectly calculate CKN length. The problem is only seen > when interoperating with non-hostap KaY's. This conflicts with an earlier proposed patch that covers at least some of these cases: https://w1.fi/cgit/hostap/commit/?id=61127f162a3fdbe3b284d4c32a8352493e43036b Some of the changes are identical, but I'm not sure how exactly this patch 6/15 or the followup patches 1/2 (mka: ICV not encoded correctly, causing receive side to drop MKPDU) and 2/2 (mka: Optional ICV indicator encoded when not necessary) should be handled. I do now have automated test cases for testing MACsec with MKA PSK mode using the hwsim test framework (tests/hwsim/test_macsec.py). This allows testing between two wpa_supplicant instances (including option to test two different hostap.git snaphots for checking against some regressions). However, I do not have any other implementations available to test for interoperability, so I'm a bit hesitant on doing this type of merges of patches without someone else clearly indicating that the changes look fine and that they have been tested with other implementations. In other words, I'd prefer to get an updated version of this patch and the followup 1/2 that fixes an issue here and all the changes rebased on top of the current hostap.git master branch snapshot.
You are correct. My patch 6/15 and the commit you reference below resolve the same issue with CKN alignment in slightly different ways. There is no need to apply patch 6/15, so please discard it. My apologies for the conflict; I had resolved the issue on an older private branch and didn't notice the issue had been independently resolved before submitting 6/15. BTW, I just tested Michael Braun's fix (commit 61127f16) and it does handle unaligned CKNs correctly. Sincerely, - Mike Siedzik -----Original Message----- From: Jouni Malinen <j@w1.fi> Sent: Wednesday, December 26, 2018 6:27 PM To: Michael Siedzik <msiedzik@extremenetworks.com> Cc: hostap@lists.infradead.org Subject: Re: [PATCH 06/15] mka: KaY setting Parameter Set Body Length incorrectly On Fri, Mar 02, 2018 at 03:10:54PM -0500, msiedzik@extremenetworks.com wrote: > Per IEEE802.1X-2010 Clause 11.11 EAPOL-MKA, each parameter set must be > padded to a multiple of 4 octets. In order for the length of variable > length parameters, such as CAK Name, to be correctly decoded the > parameter set body length must not include the length of null padding > octets. > > When allocating buffer space for the parameter set (e.g., > wpabuf_put()) the padded length must be used. When setting the > 'Parameter set body length' within the parameter set the unpadded length must be used. > > Consider the case were the length of a PSK's CKN is not a multiple of > 4 octets. Currently ieee802_1x_mka_encode_basic_body() will correctly > reserve the padded number buffer bytes. However it will incorrectly > set ieee8021_x_mka_hdr->length and ->length1 to the padded number of > bytes. The receiver will not be able to recover the original CKN > length. > > Note that the hostap will successfully interoperate with itself > because both sides incorrectly calculate CKN length. The problem is > only seen when interoperating with non-hostap KaY's. This conflicts with an earlier proposed patch that covers at least some of these cases: https://w1.fi/cgit/hostap/commit/?id=61127f162a3fdbe3b284d4c32a8352493e43036b Some of the changes are identical, but I'm not sure how exactly this patch 6/15 or the followup patches 1/2 (mka: ICV not encoded correctly, causing receive side to drop MKPDU) and 2/2 (mka: Optional ICV indicator encoded when not necessary) should be handled. I do now have automated test cases for testing MACsec with MKA PSK mode using the hwsim test framework (tests/hwsim/test_macsec.py). This allows testing between two wpa_supplicant instances (including option to test two different hostap.git snaphots for checking against some regressions). However, I do not have any other implementations available to test for interoperability, so I'm a bit hesitant on doing this type of merges of patches without someone else clearly indicating that the changes look fine and that they have been tested with other implementations. In other words, I'd prefer to get an updated version of this patch and the followup 1/2 that fixes an issue here and all the changes rebased on top of the current hostap.git master branch snapshot.
diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c index 17519ae69..70fda1f2d 100644 --- a/src/pae/ieee802_1x_kay.c +++ b/src/pae/ieee802_1x_kay.c @@ -681,7 +681,7 @@ ieee802_1x_mka_basic_body_length(struct ieee802_1x_mka_participant *participant) length = sizeof(struct ieee802_1x_mka_basic_body); length += participant->ckn.len; - return MKA_ALIGN_LENGTH(length); + return length; } @@ -697,7 +697,7 @@ ieee802_1x_mka_encode_basic_body( struct ieee802_1x_kay *kay = participant->kay; unsigned int length = ieee802_1x_mka_basic_body_length(participant); - body = wpabuf_put(buf, length); + body = wpabuf_put(buf, MKA_ALIGN_LENGTH(length)); body->version = kay->mka_version; body->priority = kay->actor_priority; @@ -856,7 +856,7 @@ ieee802_1x_mka_get_live_peer_length( struct ieee802_1x_kay_peer, list) len += sizeof(struct ieee802_1x_mka_peer_id); - return MKA_ALIGN_LENGTH(len); + return len; } @@ -916,7 +916,7 @@ ieee802_1x_mka_get_potential_peer_length( struct ieee802_1x_kay_peer, list) len += sizeof(struct ieee802_1x_mka_peer_id); - return MKA_ALIGN_LENGTH(len); + return len; } @@ -1139,7 +1139,7 @@ ieee802_1x_mka_get_sak_use_length( if (participant->kay->macsec_desired && participant->advised_desired) length = sizeof(struct ieee802_1x_mka_sak_use_body); - return MKA_ALIGN_LENGTH(length); + return length; } @@ -1189,7 +1189,7 @@ ieee802_1x_mka_encode_sak_use_body( u32 pn = 1; length = ieee802_1x_mka_get_sak_use_length(participant); - body = wpabuf_put(buf, length); + body = wpabuf_put(buf, MKA_ALIGN_LENGTH(length)); body->type = MKA_SAK_USE; set_mka_param_body_len(body, length - MKA_HDR_LEN); @@ -1439,7 +1439,7 @@ ieee802_1x_mka_get_dist_sak_length( length += cipher_suite_tbl[cs_index].sak_len + 8; } - return MKA_ALIGN_LENGTH(length); + return length; } @@ -1458,7 +1458,7 @@ ieee802_1x_mka_encode_dist_sak_body( int sak_pos; length = ieee802_1x_mka_get_dist_sak_length(participant); - body = wpabuf_put(buf, length); + body = wpabuf_put(buf, MKA_ALIGN_LENGTH(length)); body->type = MKA_DISTRIBUTED_SAK; set_mka_param_body_len(body, length - MKA_HDR_LEN); if (length == MKA_HDR_LEN) { @@ -1683,7 +1683,7 @@ ieee802_1x_mka_get_icv_length(struct ieee802_1x_mka_participant *participant) length = sizeof(struct ieee802_1x_mka_icv_body); length += mka_alg_tbl[participant->kay->mka_algindex].icv_len; - return MKA_ALIGN_LENGTH(length); + return length; } @@ -1713,7 +1713,7 @@ ieee802_1x_mka_encode_icv_body(struct ieee802_1x_mka_participant *participant, if (length != DEFAULT_ICV_LEN) length -= MKA_HDR_LEN; - os_memcpy(wpabuf_put(buf, length), cmac, length); + os_memcpy(wpabuf_put(buf, MKA_ALIGN_LENGTH(length - MKA_HDR_LEN)), cmac, length - MKA_HDR_LEN); return 0; } @@ -2297,7 +2297,7 @@ ieee802_1x_participant_send_mkpdu( for (i = 0; i < ARRAY_SIZE(mka_body_handler); i++) { if (mka_body_handler[i].body_present && mka_body_handler[i].body_present(participant)) - length += mka_body_handler[i].body_length(participant); + length += MKA_ALIGN_LENGTH(mka_body_handler[i].body_length(participant)); } buf = wpabuf_alloc(length); @@ -2931,7 +2931,7 @@ static int ieee802_1x_kay_mkpdu_sanity_check(struct ieee802_1x_kay *kay, ieee802_1x_mka_dump_basic_body(body); body_len = get_mka_param_body_len(body); /* EAPOL-MKA body should comprise basic parameter set and ICV */ - if (mka_msg_len < MKA_HDR_LEN + body_len + DEFAULT_ICV_LEN) { + if (mka_msg_len < MKA_HDR_LEN + MKA_ALIGN_LENGTH(body_len) + DEFAULT_ICV_LEN) { wpa_printf(MSG_ERROR, "KaY: Received EAPOL-MKA Packet Body Length (%zu bytes) is less than the Basic Parameter Set Header Length (%zu bytes) + the Basic Parameter Set Body Length (%zu bytes) + %d bytes of ICV", mka_msg_len, MKA_HDR_LEN, @@ -3020,7 +3020,7 @@ static int ieee802_1x_kay_decode_mkpdu(struct ieee802_1x_kay *kay, /* to skip basic parameter set */ hdr = (struct ieee802_1x_mka_hdr *) pos; - body_len = get_mka_param_body_len(hdr); + body_len = MKA_ALIGN_LENGTH(get_mka_param_body_len(hdr)); pos += body_len + MKA_HDR_LEN; left_len -= body_len + MKA_HDR_LEN; @@ -3060,7 +3060,7 @@ static int ieee802_1x_kay_decode_mkpdu(struct ieee802_1x_kay *kay, pos += body_len + MKA_HDR_LEN, left_len -= body_len + MKA_HDR_LEN) { hdr = (struct ieee802_1x_mka_hdr *) pos; - body_len = get_mka_param_body_len(hdr); + body_len = MKA_ALIGN_LENGTH(get_mka_param_body_len(hdr)); body_type = get_mka_param_body_type(hdr); if (body_type == MKA_ICV_INDICATOR)