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

[michael-dev <michael-dev@xxxxxxxxxxxxx>]

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@xxxxxxxxx> [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@xxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxx>
> > > > ---
> > > >  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);

_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap