[PATCH 1/1] macsec: make pre-shared ckn variable length

[michael-dev]

Hi,

thanks for reviewing my patch.
I think I already addressed this in [PATCHv2 1/1] macsec: make 
pre-shared ckn variable length.

Regards,
M. Braun

Am 16.08.2017 08:39, schrieb Jaap Keuter:
> Hi,
> 
> I think you are correct in that the CKN length should be passed around 
> further.
> My patch can be discarded (as if it wasn't already).
> 
> A few notes on your patch.
> ssid->mka_ckn_len = len / 2;
> This assignment is done twice, before and after the value check.
> int mka_ckn_len;
> This should be of type size_t instead of int.
> 
> I hope this gets merged.
> 
> Thanks,
> Jaap
> 
> 
> On 16-08-17 06:30, michael-dev wrote:
>> Hi,
>> 
>> thanks for pointing that patch out.
>> 
>> The older patch looks flawed to me, as it does not use the actual 
>> length in
>> ieee802_1x_create_preshared_mka or when printing the CKN. While IEEE 
>> 802.1X-2010
>> adds suffix padding to ckn with zeros for some use cases, section 
>> 11.11.1 of
>> IEEE 802.1X-2010 requires a variable length encoding, that is no 
>> padding. So the
>> actual length needs to be passed around.
>> 
>> Regards,
>> M. Braun
>> 
>> 
>> Am 15.08.2017 18:41, schrieb Jaap Keuter:
>>> Hi,
>>> 
>>> How does this compare to the patch in
>>> <20170509190449.7947-1-jaap.keuter at xs4all.nl> [PATCH] Handle 
>>> preshared CKN sizes
>>> from 1 to 32 octets
>>> of April this year?
>>> 
>>> Thanks,
>>> Jaap
>>> 
>>> 
>>> On 15-08-17 17:16, Michael Braun wrote:
>>>> From: michael-dev <michael-dev at fami-braun.de>
>>>> 
>>>> IEEE 802.1X-2010 Section 9.3.1 restricts CKN
>>>>> MKA places no restriction on the format of the CKN, save that it 
>>>>> comprise
>>>>> an integral number of octets, between 1 and 32 (inclusive), and 
>>>>> that all
>>>>> potential members of the CA use the same CKN. No further 
>>>>> constraints are
>>>>> placed onthe CKNs used with PSKs, ... .
>>>> 
>>>> Hence do not require a 32 byte long CKN but instead allow a shorter 
>>>> ckn
>>>> to be configured.
>>>> 
>>>> This fixes interoperability with some Aruba Switches, that do not 
>>>> accept
>>>> 32 byte long ckn (only shorter ones).
>>>> 
>>>> Signed-off-by: Michael Braun <michae-dev at fami-braun.de>
>>>> ---
>>>>  wpa_supplicant/config.c      | 21 +++++++++++++++++----
>>>>  wpa_supplicant/config_ssid.h |  5 +++--
>>>>  wpa_supplicant/wpas_kay.c    |  2 +-
>>>>  3 files changed, 21 insertions(+), 7 deletions(-)
>>>> 
>>>> diff --git a/wpa_supplicant/config.c b/wpa_supplicant/config.c
>>>> index 37489f7..d03514c 100644
>>>> --- a/wpa_supplicant/config.c
>>>> +++ b/wpa_supplicant/config.c
>>>> @@ -1946,8 +1946,20 @@ static int wpa_config_parse_mka_ckn(const 
>>>> struct
>>>> parse_data *data,
>>>>                      struct wpa_ssid *ssid, int line,
>>>>                      const char *value)
>>>>  {
>>>> -    if (hexstr2bin(value, ssid->mka_ckn, MACSEC_CKN_LEN) ||
>>>> -        value[MACSEC_CKN_LEN * 2] != '\0') {
>>>> +    size_t len;
>>>> +
>>>> +    len = os_strlen(value);
>>>> +    ssid->mka_ckn_len = len / 2;
>>>> +    if (len > 2 * MACSEC_CKN_MAX_LEN || /* too long */
>>>> +        len < 2 || /* too short */
>>>> +        len % 2 != 0 /* not an integral number of bytes */
>>>> +       ) {
>>>> +        wpa_printf(MSG_ERROR, "Line %d: Invalid MKA-CKN '%s'.",
>>>> +               line, value);
>>>> +        return -1;
>>>> +    }
>>>> +    ssid->mka_ckn_len = len / 2;
>>>> +    if (hexstr2bin(value, ssid->mka_ckn, ssid->mka_ckn_len)) {
>>>>          wpa_printf(MSG_ERROR, "Line %d: Invalid MKA-CKN '%s'.",
>>>>                 line, value);
>>>>          return -1;
>>>> @@ -1955,7 +1967,8 @@ static int wpa_config_parse_mka_ckn(const 
>>>> struct
>>>> parse_data *data,
>>>> 
>>>>      ssid->mka_psk_set |= MKA_PSK_SET_CKN;
>>>> 
>>>> -    wpa_hexdump_key(MSG_MSGDUMP, "MKA-CKN", ssid->mka_ckn, 
>>>> MACSEC_CKN_LEN);
>>>> +    wpa_hexdump_key(MSG_MSGDUMP, "MKA-CKN", ssid->mka_ckn,
>>>> +            ssid->mka_ckn_len);
>>>>      return 0;
>>>>  }
>>>> 
>>>> @@ -1977,7 +1990,7 @@ static char * wpa_config_write_mka_ckn(const 
>>>> struct
>>>> parse_data *data,
>>>>  {
>>>>      if (!(ssid->mka_psk_set & MKA_PSK_SET_CKN))
>>>>          return NULL;
>>>> -    return wpa_config_write_string_hex(ssid->mka_ckn, 
>>>> MACSEC_CKN_LEN);
>>>> +    return wpa_config_write_string_hex(ssid->mka_ckn, 
>>>> ssid->mka_ckn_len);
>>>>  }
>>>> 
>>>>  #endif /* NO_CONFIG_WRITE */
>>>> diff --git a/wpa_supplicant/config_ssid.h 
>>>> b/wpa_supplicant/config_ssid.h
>>>> index 81f64a5..c8b9a4d 100644
>>>> --- a/wpa_supplicant/config_ssid.h
>>>> +++ b/wpa_supplicant/config_ssid.h
>>>> @@ -776,8 +776,9 @@ struct wpa_ssid {
>>>>      /**
>>>>       * mka_ckn - MKA pre-shared CKN
>>>>       */
>>>> -#define MACSEC_CKN_LEN 32
>>>> -    u8 mka_ckn[MACSEC_CKN_LEN];
>>>> +#define MACSEC_CKN_MAX_LEN 32
>>>> +    int mka_ckn_len;
>>>> +    u8 mka_ckn[MACSEC_CKN_MAX_LEN];
>>>> 
>>>>      /**
>>>>       * mka_cak - MKA pre-shared CAK
>>>> diff --git a/wpa_supplicant/wpas_kay.c b/wpa_supplicant/wpas_kay.c
>>>> index d087e00..6c381a4 100644
>>>> --- a/wpa_supplicant/wpas_kay.c
>>>> +++ b/wpa_supplicant/wpas_kay.c
>>>> @@ -415,7 +415,7 @@ void * ieee802_1x_create_preshared_mka(struct
>>>> wpa_supplicant *wpa_s,
>>>>      cak->len = MACSEC_CAK_LEN;
>>>>      os_memcpy(cak->key, ssid->mka_cak, cak->len);
>>>> 
>>>> -    ckn->len = MACSEC_CKN_LEN;
>>>> +    ckn->len = ssid->mka_ckn_len;
>>>>      os_memcpy(ckn->name, ssid->mka_ckn, ckn->len);
>>>> 
>>>>      res = ieee802_1x_kay_create_mka(wpa_s->kay, ckn, cak, 0, PSK, 
>>>> FALSE);
>>>>