[03/15] mka: Incorrect conf_offset sent in MKPDU when in policy mode "SHOULD_SECURE"

From: Mike Siedzik <msiedzik@extremenetworks.com>

Commit 7b4d546e introduced policy setting SHOULD_ENCRYPT (MACsec provides
integrity+confidentiality) in addition to SHOULD_SECURE (MACsec provides
integrity only).  In both cases the KaY is populating the
"Confidentiality Offset" parameter within the "Distributed SAK parameter
set" with CONFIDENTIALITY_OFFSET_0=1.  In the case of SHOULD_SECURE the
parameter should be populated with CONFIDENTIALITY_NONE=0.

IEEE802.1X-2010 Table 11-6 and Figure 11-11 define how the two
Confidentiality Offset bits in the "Distributed SAK parameter set" must
be set: "0 if confidentiality not used" and "1 if confidentiality with no
offset".  When policy is SHOULD_SECURE KaY should to send the former, and
when policy is SHOULD_ENCRYPT KaY should send the latter.

Signed-off-by: Michael Siedzik <msiedzik@extremenetworks.com>
---
 src/pae/ieee802_1x_kay.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

--
2.11.1

On Fri, Mar 02, 2018 at 03:10:51PM -0500, msiedzik@extremenetworks.com wrote:

> diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c
> @@ -3166,14 +3167,16 @@ ieee802_1x_kay_init(struct ieee802_1x_kay_ctx *ctx, enum macsec_policy policy,
>         } else {
>                 kay->macsec_desired = TRUE;
>                 kay->macsec_protect = TRUE;
> -               kay->macsec_encrypt = policy == SHOULD_ENCRYPT;
> +               if (policy == SHOULD_SECURE) {
> +                       kay->macsec_encrypt = FALSE;
> +                       kay->macsec_confidentiality = CONFIDENTIALITY_NONE;
> +               } else {  /* SHOULD_ENCRYPT */
> +                       kay->macsec_encrypt = TRUE;
> +                       kay->macsec_confidentiality = CONFIDENTIALITY_OFFSET_0;
> +               }
>                 kay->macsec_validate = Strict;
>                 kay->macsec_replay_protect = FALSE;
>                 kay->macsec_replay_window = 0;
> -               if (kay->macsec_capable >= MACSEC_CAP_INTEG_AND_CONF)
> -                       kay->macsec_confidentiality = CONFIDENTIALITY_OFFSET_0;
> -               else
> -                       kay->macsec_confidentiality = CONFIDENTIALITY_NONE;
>         }

Is this change dropping the kay->macsec_capable check on purpose for
SHOULD_ENCRYPT case? The new SHOULD_SECURE case looks fine, but should
the SHOULD_ENCRYPT case still use this kay->macsec_capable >=
MACSEC_CAP_INTEG_AND_CONF before setting CONFIDENTIALITY_OFFSET_0?

Hi Jouni,

You are correct.  In the case where policy is set to SHOULD_ENCRYPT but MACsec capability is only MACSEC_CAP_INTEGRITY (i.e., integrity without confidentiality), my patch would have attempted to encrypt with offset 0.  The patch should have retained the macsec_capable comparison.  Something like this:

                if ((kay->macsec_capable >= MACSEC_CAP_INTEG_AND_CONF) &&
                    (policy == SHOULD_ENCRYPT)) {
                        kay->macsec_encrypt = TRUE;
                        kay->macsec_confidentiality = CONFIDENTIALITY_OFFSET_0;
                } else {  /* SHOULD_SECURE */
                        kay->macsec_encrypt = FALSE;
                        kay->macsec_confidentiality = CONFIDENTIALITY_NONE;
                }

Thanks,
- Mike


-----Original Message-----
From: Jouni Malinen [mailto:j@w1.fi]
Sent: Sunday, March 11, 2018 10:37 AM
To: Michael Siedzik <msiedzik@extremenetworks.com>
Cc: hostap@lists.infradead.org
Subject: Re: [PATCH 03/15] mka: Incorrect conf_offset sent in MKPDU when in policy mode "SHOULD_SECURE"

On Fri, Mar 02, 2018 at 03:10:51PM -0500, msiedzik@extremenetworks.com wrote:

> diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c @@
> -3166,14 +3167,16 @@ ieee802_1x_kay_init(struct ieee802_1x_kay_ctx *ctx, enum macsec_policy policy,
>         } else {
>                 kay->macsec_desired = TRUE;
>                 kay->macsec_protect = TRUE;
> -               kay->macsec_encrypt = policy == SHOULD_ENCRYPT;
> +               if (policy == SHOULD_SECURE) {
> +                       kay->macsec_encrypt = FALSE;
> +                       kay->macsec_confidentiality = CONFIDENTIALITY_NONE;
> +               } else {  /* SHOULD_ENCRYPT */
> +                       kay->macsec_encrypt = TRUE;
> +                       kay->macsec_confidentiality = CONFIDENTIALITY_OFFSET_0;
> +               }
>                 kay->macsec_validate = Strict;
>                 kay->macsec_replay_protect = FALSE;
>                 kay->macsec_replay_window = 0;
> -               if (kay->macsec_capable >= MACSEC_CAP_INTEG_AND_CONF)
> -                       kay->macsec_confidentiality = CONFIDENTIALITY_OFFSET_0;
> -               else
> -                       kay->macsec_confidentiality = CONFIDENTIALITY_NONE;
>         }

Is this change dropping the kay->macsec_capable check on purpose for SHOULD_ENCRYPT case? The new SHOULD_SECURE case looks fine, but should the SHOULD_ENCRYPT case still use this kay->macsec_capable >= MACSEC_CAP_INTEG_AND_CONF before setting CONFIDENTIALITY_OFFSET_0?

--
Jouni Malinen                                            PGP id EFC895FA

On Mon, Mar 12, 2018 at 01:21:48PM +0000, Michael Siedzik wrote:
> You are correct.  In the case where policy is set to SHOULD_ENCRYPT but MACsec capability is only MACSEC_CAP_INTEGRITY (i.e., integrity without confidentiality), my patch would have attempted to encrypt with offset 0.  The patch should have retained the macsec_capable comparison.  Something like this:
> 
>                 if ((kay->macsec_capable >= MACSEC_CAP_INTEG_AND_CONF) &&
>                     (policy == SHOULD_ENCRYPT)) {
>                         kay->macsec_encrypt = TRUE;
>                         kay->macsec_confidentiality = CONFIDENTIALITY_OFFSET_0;
>                 } else {  /* SHOULD_SECURE */
>                         kay->macsec_encrypt = FALSE;
>                         kay->macsec_confidentiality = CONFIDENTIALITY_NONE;
>                 }

Thanks, applied.