MFP: Don't use MFP if it is optional and not supported by hardware

From: Markus Theil <theil.markus@gmail.com>

Currently, NetworkManager sends ieee80211w=1 for every connection,
if wpa_supplicant has pmf support enabled/compiled in. If the used
NIC does not support BIP ciphers, adding the IGTK fails.

This patch circumvents this, by ignoring ieee80211w=1 (optional MFP)
if hardware support is not given. Making NetworkManager aware of
per-interface MFP support would be the cleaner solution of course.

Signed-off-by: Markus Theil <theil.markus@gmail.com>
---
 wpa_supplicant/wpa_supplicant.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On Sat, Jan 05, 2019 at 12:39:46PM +0100, Markus Theil wrote:
> Currently, NetworkManager sends ieee80211w=1 for every connection,
> if wpa_supplicant has pmf support enabled/compiled in. If the used
> NIC does not support BIP ciphers, adding the IGTK fails.

That is a bit unfortunate in this context.. The better way of doing this
would have been setting the global pmf=1 parameter and not having
per-network profile parameters. That combination is already covering
this case of no driver support, i.e., pmf=1 was designed in a way that
it would fall back to no MFP if there is no driver support.

> This patch circumvents this, by ignoring ieee80211w=1 (optional MFP)
> if hardware support is not given. Making NetworkManager aware of
> per-interface MFP support would be the cleaner solution of course.

This patch is doing quite a bit more than that, though..

> diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
> @@ -6828,7 +6828,9 @@ int wpas_network_disabled(struct wpa_supplicant *wpa_s, struct wpa_ssid *ssid)
>  int wpas_get_ssid_pmf(struct wpa_supplicant *wpa_s, struct wpa_ssid *ssid)
>  {
>  #ifdef CONFIG_IEEE80211W
> -	if (ssid == NULL || ssid->ieee80211w == MGMT_FRAME_PROTECTION_DEFAULT) {
> +	if (ssid == NULL ||
> +		ssid->ieee80211w == MGMT_FRAME_PROTECTION_DEFAULT ||
> +		ssid->ieee80211w == MGMT_FRAME_PROTECTION_OPTIONAL) {
>  		if (wpa_s->conf->pmf == MGMT_FRAME_PROTECTION_OPTIONAL &&
>  		    !(wpa_s->drv_enc & WPA_DRIVER_CAPA_ENC_BIP)) {
>  			/*

This first if block ends in "return wpa_s->conf->pmf". In other words,
this patch would end up overriding network profile specific
ssid->ieee80211w with the global default wpa_s->conf->pmf if
ieee80211w=1 (optional) is used in the network profile. This applies in
case of all drivers where PMF is supported.

That is not a desired changed since it can result in quite incorrect
behavior. If the goal is to override ieee80211w=1 in the network
profile, that would need to be done at the end of this function just
before return ssid->ieee80211w instead of modifying this special case of
MGMT_FRAME_PROTECTION_DEFAULT (i.e., no explicit ieee80211w parameter in
the network profile).

On Sun, 2019-01-06 at 16:49 +0200, Jouni Malinen wrote:
> On Sat, Jan 05, 2019 at 12:39:46PM +0100, Markus Theil wrote:
> > Currently, NetworkManager sends ieee80211w=1 for every connection,
> > if wpa_supplicant has pmf support enabled/compiled in. If the used
> > NIC does not support BIP ciphers, adding the IGTK fails.
> 
> That is a bit unfortunate in this context.. The better way of doing this
> would have been setting the global pmf=1 parameter and not having
> per-network profile parameters. That combination is already covering
> this case of no driver support, i.e., pmf=1 was designed in a way that
> it would fall back to no MFP if there is no driver support.

[...]

> That is not a desired changed since it can result in quite incorrect
> behavior. If the goal is to override ieee80211w=1 in the network
> profile, that would need to be done at the end of this function just
> before return ssid->ieee80211w instead of modifying this special case of
> MGMT_FRAME_PROTECTION_DEFAULT (i.e., no explicit ieee80211w parameter in
> the network profile).

I'd argue that it's still an NM bug and can just be fixed there?

It probably has a shorter release cycle too ;-))

johannes

On Sun, 2019-01-06 at 16:49 +0200, Jouni Malinen wrote:
> On Sat, Jan 05, 2019 at 12:39:46PM +0100, Markus Theil wrote:
> > Currently, NetworkManager sends ieee80211w=1 for every connection,
> > if wpa_supplicant has pmf support enabled/compiled in. If the used
> > NIC does not support BIP ciphers, adding the IGTK fails.
> 
> That is a bit unfortunate in this context.. The better way of doing
> this
> would have been setting the global pmf=1 parameter and not having
> per-network profile parameters. That combination is already covering
> this case of no driver support, i.e., pmf=1 was designed in a way
> that
> it would fall back to no MFP if there is no driver support.

Perhaps I don't fully understand, but wouldn't pmf=1 try to enable PMF
for all SSIDs if supported by SSID/driver, even if the user does not
actually want to use PMF on that SSID?

Usually NetworkManager tries to have per-SSID switches for things,
because there are times when a network advertises a feature that you
don't actually want to use for whatever reason (it's broken on one side
but still advertised, or has some drawbacks that certain users don't
want to accept, etc).

Dan

> > This patch circumvents this, by ignoring ieee80211w=1 (optional
> > MFP)
> > if hardware support is not given. Making NetworkManager aware of
> > per-interface MFP support would be the cleaner solution of course.
> 
> This patch is doing quite a bit more than that, though..
> 
> > diff --git a/wpa_supplicant/wpa_supplicant.c
> > b/wpa_supplicant/wpa_supplicant.c
> > @@ -6828,7 +6828,9 @@ int wpas_network_disabled(struct
> > wpa_supplicant *wpa_s, struct wpa_ssid *ssid)
> >  int wpas_get_ssid_pmf(struct wpa_supplicant *wpa_s, struct
> > wpa_ssid *ssid)
> >  {
> >  #ifdef CONFIG_IEEE80211W
> > -	if (ssid == NULL || ssid->ieee80211w ==
> > MGMT_FRAME_PROTECTION_DEFAULT) {
> > +	if (ssid == NULL ||
> > +		ssid->ieee80211w == MGMT_FRAME_PROTECTION_DEFAULT ||
> > +		ssid->ieee80211w == MGMT_FRAME_PROTECTION_OPTIONAL) {
> >  		if (wpa_s->conf->pmf == MGMT_FRAME_PROTECTION_OPTIONAL
> > &&
> >  		    !(wpa_s->drv_enc & WPA_DRIVER_CAPA_ENC_BIP)) {
> >  			/*
> 
> This first if block ends in "return wpa_s->conf->pmf". In other
> words,
> this patch would end up overriding network profile specific
> ssid->ieee80211w with the global default wpa_s->conf->pmf if
> ieee80211w=1 (optional) is used in the network profile. This applies
> in
> case of all drivers where PMF is supported.
> 
> That is not a desired changed since it can result in quite incorrect
> behavior. If the goal is to override ieee80211w=1 in the network
> profile, that would need to be done at the end of this function just
> before return ssid->ieee80211w instead of modifying this special case
> of
> MGMT_FRAME_PROTECTION_DEFAULT (i.e., no explicit ieee80211w parameter
> in
> the network profile).
>

On Tue, Jan 08, 2019 at 10:18:28AM -0600, Dan Williams wrote:
> Perhaps I don't fully understand, but wouldn't pmf=1 try to enable PMF
> for all SSIDs if supported by SSID/driver, even if the user does not
> actually want to use PMF on that SSID?

Global pmf=1 would make wpa_supplicant try to use PMF for all RSN (WPA2)
connections if the AP advertises support for this. This should really be
the default behavior for everything now and I don't see much of a use
case for the user to try to not use PMF. (And if there is such a use
case, ieee80211w=0 in the network profile can be used to override the
global pmf=1).

> Usually NetworkManager tries to have per-SSID switches for things,
> because there are times when a network advertises a feature that you
> don't actually want to use for whatever reason (it's broken on one side
> but still advertised, or has some drawbacks that certain users don't
> want to accept, etc).

Other than some conformance/protocol testing purposes, I'm not sure why
PMF would not be used. I guess working around a broken AP could be one
reason, but I'm not aware of such cases and even if that were to be the
case, I'd claim that global pmf=1 and network profile specific
possibility to disable that would be better approach here.

Please also note that I'm planning on changing the default value for the
global pmf parameter to be 1 instead 0 in the next wpa_supplicant
release, i.e., enabling PMF automatically unless it has been explicitly
disabled with pmf=0 or network profile specific ieee80211w=0.

On Wed, 2019-01-09 at 01:39 +0200, Jouni Malinen wrote:
> On Tue, Jan 08, 2019 at 10:18:28AM -0600, Dan Williams wrote:
> > Perhaps I don't fully understand, but wouldn't pmf=1 try to enable
> > PMF
> > for all SSIDs if supported by SSID/driver, even if the user does
> > not
> > actually want to use PMF on that SSID?
> 
> Global pmf=1 would make wpa_supplicant try to use PMF for all RSN
> (WPA2)
> connections if the AP advertises support for this. This should really
> be
> the default behavior for everything now and I don't see much of a use
> case for the user to try to not use PMF. (And if there is such a use
> case, ieee80211w=0 in the network profile can be used to override the
> global pmf=1).

Ok, that seems like an acceptable override behavior to allow users to
turn it off per-SSID if for some reason they don't want it.  I'll pass
that along.

Thanks!
Dan

> > Usually NetworkManager tries to have per-SSID switches for things,
> > because there are times when a network advertises a feature that
> > you
> > don't actually want to use for whatever reason (it's broken on one
> > side
> > but still advertised, or has some drawbacks that certain users
> > don't
> > want to accept, etc).
> 
> Other than some conformance/protocol testing purposes, I'm not sure
> why
> PMF would not be used. I guess working around a broken AP could be
> one
> reason, but I'm not aware of such cases and even if that were to be
> the
> case, I'd claim that global pmf=1 and network profile specific
> possibility to disable that would be better approach here.
> 
> Please also note that I'm planning on changing the default value for
> the
> global pmf parameter to be 1 instead 0 in the next wpa_supplicant
> release, i.e., enabling PMF automatically unless it has been
> explicitly
> disabled with pmf=0 or network profile specific ieee80211w=0.
>

On Tue, 2019-01-08 at 19:31 -0600, Dan Williams wrote:
> On Wed, 2019-01-09 at 01:39 +0200, Jouni Malinen wrote:
> > On Tue, Jan 08, 2019 at 10:18:28AM -0600, Dan Williams wrote:
> > > Perhaps I don't fully understand, but wouldn't pmf=1 try to
> > > enable
> > > PMF
> > > for all SSIDs if supported by SSID/driver, even if the user does
> > > not
> > > actually want to use PMF on that SSID?
> > 
> > Global pmf=1 would make wpa_supplicant try to use PMF for all RSN
> > (WPA2)
> > connections if the AP advertises support for this. This should
> > really
> > be
> > the default behavior for everything now and I don't see much of a
> > use
> > case for the user to try to not use PMF. (And if there is such a
> > use
> > case, ieee80211w=0 in the network profile can be used to override
> > the
> > global pmf=1).
> 
> Ok, that seems like an acceptable override behavior to allow users to
> turn it off per-SSID if for some reason they don't want it.  I'll
> pass
> that along.

Follow-up... Beniamino is working on the NM changes to make this
happen.

Dan

> Thanks!
> Dan
> 
> > > Usually NetworkManager tries to have per-SSID switches for
> > > things,
> > > because there are times when a network advertises a feature that
> > > you
> > > don't actually want to use for whatever reason (it's broken on
> > > one
> > > side
> > > but still advertised, or has some drawbacks that certain users
> > > don't
> > > want to accept, etc).
> > 
> > Other than some conformance/protocol testing purposes, I'm not sure
> > why
> > PMF would not be used. I guess working around a broken AP could be
> > one
> > reason, but I'm not aware of such cases and even if that were to be
> > the
> > case, I'd claim that global pmf=1 and network profile specific
> > possibility to disable that would be better approach here.
> > 
> > Please also note that I'm planning on changing the default value
> > for
> > the
> > global pmf parameter to be 1 instead 0 in the next wpa_supplicant
> > release, i.e., enabling PMF automatically unless it has been
> > explicitly
> > disabled with pmf=0 or network profile specific ieee80211w=0.
> > 
> 
> _______________________________________________
> Hostap mailing list
> Hostap@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/hostap