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

[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);
>>>