[09/12] common: Allow WPA_CIPHER_GTK_NOT_USED in RSNE parsing
diff mbox series

Message ID 20200224091437.15212-10-ilan.peer@intel.com
State Changes Requested
Headers show
Series
  • Preparations for Pre association Security Negotiation(PASN) Support
Related show

Commit Message

Peer, Ilan Feb. 24, 2020, 9:14 a.m. UTC
PASN authentication requires that group management cipher suite
would be set to 00-0F-AC:7 in the RSN IE, so allow this value
when parsing and validating the RSN IE.

Signed-off-by: Ilan Peer <ilan.peer@intel.com>
---
 src/common/wpa_common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jouni Malinen Feb. 29, 2020, 10:06 p.m. UTC | #1
On Mon, Feb 24, 2020 at 11:14:34AM +0200, Ilan Peer wrote:
> PASN authentication requires that group management cipher suite
> would be set to 00-0F-AC:7 in the RSN IE, so allow this value
> when parsing and validating the RSN IE.

Can you please point me to the location in P802.11az/D2.0 that describes
this?

> diff --git a/src/common/wpa_common.c b/src/common/wpa_common.c
> @@ -1369,7 +1369,8 @@ int wpa_parse_wpa_ie_rsn(const u8 *rsn_ie, size_t rsn_ie_len,
>  
>  	if (left >= 4) {
>  		data->mgmt_group_cipher = rsn_selector_to_bitfield(pos);
> -		if (!wpa_cipher_valid_mgmt_group(data->mgmt_group_cipher)) {
> +		if (data->mgmt_group_cipher != WPA_CIPHER_GTK_NOT_USED &&
> +		    !wpa_cipher_valid_mgmt_group(data->mgmt_group_cipher)) {
>  			wpa_printf(MSG_DEBUG,
>  				   "%s: Unsupported management group cipher 0x%x (%08x)",
>  				   __func__, data->mgmt_group_cipher,

This looks problematic for PMF.. Are you sure this does not result in
unexpected behavior for BIP with Robust Management frames? This would
likely need some changes in other locations and clear understanding on
what to expect to happen with IGTK. The drivers would need to be able to
drop any unprotected group-addressed Robust Management frame in such
configuration. That would depend on there being an IGTK configured. That
would either need to be a random value from the AP or a random value
generated by wpa_supplicant internally if no IGTK is received from the
AP.

As far as consistent implementation is concerned, that check for
WPA_CIPHER_GTK_NOT_USED would belong in wpa_cipher_valid_mgmt_group()
similarly to the way this is handled with wpa_cipher_valid_group().
Peer, Ilan March 1, 2020, 8:30 a.m. UTC | #2
> -----Original Message-----
> From: Jouni Malinen <j@w1.fi>
> Sent: Sunday, March 01, 2020 00:06
> To: Peer, Ilan <ilan.peer@intel.com>
> Cc: hostap@lists.infradead.org
> Subject: Re: [PATCH 09/12] common: Allow WPA_CIPHER_GTK_NOT_USED in
> RSNE parsing
> 
> On Mon, Feb 24, 2020 at 11:14:34AM +0200, Ilan Peer wrote:
> > PASN authentication requires that group management cipher suite would
> > be set to 00-0F-AC:7 in the RSN IE, so allow this value when parsing
> > and validating the RSN IE.
> 
> Can you please point me to the location in P802.11az/D2.0 that describes
> this?
> 

See section 12.13.2.2 (PASN Frame Construction and Processing).

> > diff --git a/src/common/wpa_common.c b/src/common/wpa_common.c
> @@
> > -1369,7 +1369,8 @@ int wpa_parse_wpa_ie_rsn(const u8 *rsn_ie, size_t
> > rsn_ie_len,
> >
> >  	if (left >= 4) {
> >  		data->mgmt_group_cipher = rsn_selector_to_bitfield(pos);
> > -		if (!wpa_cipher_valid_mgmt_group(data-
> >mgmt_group_cipher)) {
> > +		if (data->mgmt_group_cipher !=
> WPA_CIPHER_GTK_NOT_USED &&
> > +		    !wpa_cipher_valid_mgmt_group(data-
> >mgmt_group_cipher)) {
> >  			wpa_printf(MSG_DEBUG,
> >  				   "%s: Unsupported management group
> cipher 0x%x (%08x)",
> >  				   __func__, data->mgmt_group_cipher,
> 
> This looks problematic for PMF.. Are you sure this does not result in
> unexpected behavior for BIP with Robust Management frames? This would
> likely need some changes in other locations and clear understanding on what
> to expect to happen with IGTK. The drivers would need to be able to drop
> any unprotected group-addressed Robust Management frame in such
> configuration. That would depend on there being an IGTK configured. That
> would either need to be a random value from the AP or a random value
> generated by wpa_supplicant internally if no IGTK is received from the AP.
> 

I'm not sure about this. From what I understand, during PASN not multicast
frames are allowed, so drivers are expected to drop any multicast frames.

> As far as consistent implementation is concerned, that check for
> WPA_CIPHER_GTK_NOT_USED would belong in
> wpa_cipher_valid_mgmt_group() similarly to the way this is handled with
> wpa_cipher_valid_group().

Ok. I'll move it there and see if other changes are required. 

Thanks,

Ilan.
Jouni Malinen March 1, 2020, 3:16 p.m. UTC | #3
On Sun, Mar 01, 2020 at 08:30:56AM +0000, Peer, Ilan wrote:
> > From: Jouni Malinen <j@w1.fi>
> > On Mon, Feb 24, 2020 at 11:14:34AM +0200, Ilan Peer wrote:
> > > PASN authentication requires that group management cipher suite would
> > > be set to 00-0F-AC:7 in the RSN IE, so allow this value when parsing
> > > and validating the RSN IE.
> > 
> > Can you please point me to the location in P802.11az/D2.0 that describes
> > this?
> 
> See section 12.13.2.2 (PASN Frame Construction and Processing).

Thanks. I'm not sure how I did not find that when searching through the
draft.. Anyway, that is quite clear on the design.

> > This looks problematic for PMF.. Are you sure this does not result in
> > unexpected behavior for BIP with Robust Management frames? This would
> > likely need some changes in other locations and clear understanding on what
> > to expect to happen with IGTK. The drivers would need to be able to drop
> > any unprotected group-addressed Robust Management frame in such
> > configuration. That would depend on there being an IGTK configured. That
> > would either need to be a random value from the AP or a random value
> > generated by wpa_supplicant internally if no IGTK is received from the AP.
> 
> I'm not sure about this. From what I understand, during PASN not multicast
> frames are allowed, so drivers are expected to drop any multicast frames.

I'm not that worried about the part of using this for PASN; I'm worried
about the implications of this particular change to non-PASN cases of
using PMF since 00-0F-AC:7 has not been used as a group management
cipher suite selector in the existing use cases. I'm not at all
convinced it would work securely and that's why it is important for the
parser to reject that group management cipher suite. If this patch alone
were applied that could result in the station accepting any unprotected
group-addressed Robust Management frame which is clearly not what should
happen.

For this change to be acceptable, the 00-0F-AC:7 case with group
management cipher suite needs to be first confirmed to work correctly in
today's (non-PASN) PMF cases without introducing security
vulnerabilities. That's what the steps noted in that paragraphs are
needed (make sure a random IGTK value gets configured into the driver
regardless of whether the AP sends an IGTK).
Peer, Ilan March 1, 2020, 8:33 p.m. UTC | #4
On Sun, 2020-03-01 at 17:16 +0200, Jouni Malinen wrote:
> On Sun, Mar 01, 2020 at 08:30:56AM +0000, Peer, Ilan wrote:
> > > From: Jouni Malinen <j@w1.fi>
> > > On Mon, Feb 24, 2020 at 11:14:34AM +0200, Ilan Peer wrote:
> > > > PASN authentication requires that group management cipher suite would
> > > > be set to 00-0F-AC:7 in the RSN IE, so allow this value when parsing
> > > > and validating the RSN IE.
> > > 
> > > Can you please point me to the location in P802.11az/D2.0 that describes
> > > this?
> > 
> > See section 12.13.2.2 (PASN Frame Construction and Processing).
> 
> Thanks. I'm not sure how I did not find that when searching through the
> draft.. Anyway, that is quite clear on the design.
> 
> > > This looks problematic for PMF.. Are you sure this does not result in
> > > unexpected behavior for BIP with Robust Management frames? This would
> > > likely need some changes in other locations and clear understanding on
> > > what
> > > to expect to happen with IGTK. The drivers would need to be able to drop
> > > any unprotected group-addressed Robust Management frame in such
> > > configuration. That would depend on there being an IGTK configured. That
> > > would either need to be a random value from the AP or a random value
> > > generated by wpa_supplicant internally if no IGTK is received from the
> > > AP.
> > 
> > I'm not sure about this. From what I understand, during PASN not multicast
> > frames are allowed, so drivers are expected to drop any multicast frames.
> 
> I'm not that worried about the part of using this for PASN; I'm worried
> about the implications of this particular change to non-PASN cases of
> using PMF since 00-0F-AC:7 has not been used as a group management
> cipher suite selector in the existing use cases. I'm not at all
> convinced it would work securely and that's why it is important for the
> parser to reject that group management cipher suite. If this patch alone
> were applied that could result in the station accepting any unprotected
> group-addressed Robust Management frame which is clearly not what should
> happen.
> 

I think I understand your concern now. At least from what I can tell about
mac80211 it does not have any handling for such a case, i.e., not allow any
group addressed frames. I do not know how other drivers would handle this.

> For this change to be acceptable, the 00-0F-AC:7 case with group
> management cipher suite needs to be first confirmed to work correctly in
> today's (non-PASN) PMF cases without introducing security
> vulnerabilities. That's what the steps noted in that paragraphs are
> needed (make sure a random IGTK value gets configured into the driver
> regardless of whether the AP sends an IGTK).
> 

I can change the implementation so this would be allowed only in the case of
PASN. This should be simple enough. If you want me to do it differently let me
know.

Regards,

Ilan.
Jouni Malinen March 1, 2020, 10:07 p.m. UTC | #5
On Sun, Mar 01, 2020 at 08:33:30PM +0000, Peer, Ilan wrote:
> I think I understand your concern now. At least from what I can tell about
> mac80211 it does not have any handling for such a case, i.e., not allow any
> group addressed frames. I do not know how other drivers would handle this.

I think the only safe way to do this is to configure a random IGTK so
that the drivers would not need to have any special handling for this.

> I can change the implementation so this would be allowed only in the case of
> PASN. This should be simple enough. If you want me to do it differently let me
> know.

It's a bit ugly in the generic parser function, but I guess that's fine
as an initial step. That said, it probably makes sense to extend
non-PASN PMF case to support no-BIP-used option as well even if that has
not really been used so far. Though, I'm not sure there is any easy way
of deploying this on the AP side if most already deployed STAs reject
such configuration in practice and won't connect.

I'll try to remember to bring this up in the REVmd (and/or P802.11az)
discussions as well to get the standard clear on what to expect as
allowed set of cipher suite selectors for the different cases using the
group management cipher.
Peer, Ilan March 3, 2020, 7:41 a.m. UTC | #6
> -----Original Message-----
> From: Jouni Malinen <j@w1.fi>
> Sent: Monday, March 02, 2020 00:07
> To: Peer, Ilan <ilan.peer@intel.com>
> Cc: hostap@lists.infradead.org
> Subject: Re: [PATCH 09/12] common: Allow WPA_CIPHER_GTK_NOT_USED in
> RSNE parsing
> 
> On Sun, Mar 01, 2020 at 08:33:30PM +0000, Peer, Ilan wrote:
> > I think I understand your concern now. At least from what I can tell
> > about
> > mac80211 it does not have any handling for such a case, i.e., not
> > allow any group addressed frames. I do not know how other drivers would
> handle this.
> 
> I think the only safe way to do this is to configure a random IGTK so that the
> drivers would not need to have any special handling for this.
> 

Agree. Should be simple enough. 

> > I can change the implementation so this would be allowed only in the
> > case of PASN. This should be simple enough. If you want me to do it
> > differently let me know.
> 
> It's a bit ugly in the generic parser function, but I guess that's fine as an initial
> step. That said, it probably makes sense to extend non-PASN PMF case to
> support no-BIP-used option as well even if that has not really been used so
> far. Though, I'm not sure there is any easy way of deploying this on the AP
> side if most already deployed STAs reject such configuration in practice and
> won't connect.
> 

As previously agreed, lets wait for the 802.11az standard to be more stable and clear about this,
and the specification allows to set 07 as group management cipher I'll align the code to match the expectation. 

Regards,

Ilan.

Patch
diff mbox series

diff --git a/src/common/wpa_common.c b/src/common/wpa_common.c
index 6cb9180ee8..bd79575d0f 100644
--- a/src/common/wpa_common.c
+++ b/src/common/wpa_common.c
@@ -1369,7 +1369,8 @@  int wpa_parse_wpa_ie_rsn(const u8 *rsn_ie, size_t rsn_ie_len,
 
 	if (left >= 4) {
 		data->mgmt_group_cipher = rsn_selector_to_bitfield(pos);
-		if (!wpa_cipher_valid_mgmt_group(data->mgmt_group_cipher)) {
+		if (data->mgmt_group_cipher != WPA_CIPHER_GTK_NOT_USED &&
+		    !wpa_cipher_valid_mgmt_group(data->mgmt_group_cipher)) {
 			wpa_printf(MSG_DEBUG,
 				   "%s: Unsupported management group cipher 0x%x (%08x)",
 				   __func__, data->mgmt_group_cipher,