[05/44] FT: wpa_auth_ft rrb fix data length

From: Michael Braun <michael-dev@fami-braun.de>

The FT RRB hostapd packets have a length field.
For PULL frames, it counts the bytes starting with nonce and up to the last
before pad. For RESP frames, it counts the bytes starting with nonce and up
to the last before pad except for 2 bytes. For PUSH frames, it counts the
bytes starting with nonce and up to including pad.

AES encryption is not affected, as rounding hides the differences.
The packets data_length field is not used, so the differences have no
effect there.

As rounding is done with AES encryption, including pad does not make sense.
Not including the last field before pad does not make sense to me either.

So this patch changes the constants to match the bytes used, thus excluding
pad.
To validate the changes, look at remainder modulo 8 of the sum of the size
constants and the padding sizes.

Signed-off-by: Michael Braun <michael-dev@fami-braun.de>
---
 src/ap/wpa_auth.h | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

On Wed, Feb 24, 2016 at 12:53:11PM +0100, michael-dev@fami-braun.de wrote:
> The FT RRB hostapd packets have a length field.
> For PULL frames, it counts the bytes starting with nonce and up to the last
> before pad. For RESP frames, it counts the bytes starting with nonce and up
> to the last before pad except for 2 bytes. For PUSH frames, it counts the
> bytes starting with nonce and up to including pad.

Those missing 2 bytes and including pad are bugs from the change that
added the 2 octet pairwise field. data_length was supposed to be length
of the encrypted plaintext before padding.

> AES encryption is not affected, as rounding hides the differences.
> The packets data_length field is not used, so the differences have no
> effect there.

This change would still "break" the previously used message format by
fixing the data_length field. I was thinking of not applying this, but I
guess this could be final accepted change to the AP-to-AP protocol to
since it actually fixes the data_length values to be what they were
supposed to be after the pairwise 2 octet field was added. Since the
receiving processing does not use the data_length field in hostapd (and
I'm not aware of any other implementation using the message format),
this change could be accepted.

> As rounding is done with AES encryption, including pad does not make sense.
> Not including the last field before pad does not make sense to me either.

Agreed.

> So this patch changes the constants to match the bytes used, thus excluding
> pad.
> To validate the changes, look at remainder modulo 8 of the sum of the size
> constants and the padding sizes.

I think this message format has come to the end of its useful life. It
was a quick prototype implementation to allow some AP-to-AP
communication to be experimented with. It is clearly not extensible and
the last attempt to extend it (that pairwise field six years ago)
already failed.

I don't think there is much point in trying to extend this any further
without a complete redesign. As such, I think I'm likely to apply this
patch to document the earlier breakage, but not any of the following
patches that extend these structs. Instead, we should design a new
extensible format (e.g., TLVs that can be added to the message and
length fields that are actually used by the recipient to find the
validation data instead of using hardcoded offset in the build). In
addition, AES key wrap is not really ideal for this purpose either, so
that could be replaced as well.

Am 28.02.2016 um 10:57 schrieb Jouni Malinen:
> I don't think there is much point in trying to extend this any further
> without a complete redesign. As such, I think I'm likely to apply this
> patch to document the earlier breakage, but not any of the following
> patches that extend these structs. Instead, we should design a new
> extensible format (e.g., TLVs that can be added to the message and
> length fields that are actually used by the recipient to find the
> validation data instead of using hardcoded offset in the build). In
> addition, AES key wrap is not really ideal for this purpose either, so
> that could be replaced as well.
> 

I'm thinking about choosing one of a1 / b1 / a2 / b2.
I'd be fine implementing a1 as this provides all I need. But if any of
over variants is required, that might be feasable as well.

Structure:

a) use TLV. This completely breaks backward compatibility.

b) keep the existing format, add a magic cookie (like DHCP/BOOTP
   migration) after the existing message and then add TLV.
   This should keep compatibility for the attributes already in the
   message struct.

Encryption

1) As IPsec-over-UDP with static keys results in something similar to
   the current encryption, encryption might just be dropped in hostapd.
   Using IPsec or some other VPN solution is also more flexible with
   respect to the available key management schemes.

2) Hostapd is currently missing some MAC. So replace aeswrap with an
   AES-GCM implementation.
   Additionally, if the key provided is all-zero, encryption and
   authentication could be skipped (for example if used with a VPN).
   This way users that do not need in-hostapd encryption/authentication
   of the message can skip it and reduce cpu time needed.

Regards,
M. Braun

On Wed, Mar 02, 2016 at 07:10:52PM +0100, M. Braun wrote:
> I'm thinking about choosing one of a1 / b1 / a2 / b2.
> I'd be fine implementing a1 as this provides all I need. But if any of
> over variants is required, that might be feasable as well.
> 
> Structure:
> 
> a) use TLV. This completely breaks backward compatibility.
> 
> b) keep the existing format, add a magic cookie (like DHCP/BOOTP
>    migration) after the existing message and then add TLV.
>    This should keep compatibility for the attributes already in the
>    message struct.

For b, could define new frame_type values (though, this is not really
nice even with the current ones which are not really properly assigned)
or a completely new frame type. The latter could be considered
especially if we were to move from a MAC frame to something on top of IP
to allow routing of AP-to-AP communication.

As far as (a) vs. (b) is concerned, I think I'm willing to drop
backwards compatibility once for this taken into account the origin and
state of the current protocol. That said, if it is straightforward to
leave the current option in place as-is and define a new one
independently, there could be some value in doing so and remove the
current (i.e., the old at the time) version eventuality after couple of
released versions.

> Encryption
> 
> 1) As IPsec-over-UDP with static keys results in something similar to
>    the current encryption, encryption might just be dropped in hostapd.
>    Using IPsec or some other VPN solution is also more flexible with
>    respect to the available key management schemes.

In theory, yes, this would be quite nice. That said, I find it quite
unlikely that people would be setting up anything secure between APs..

> 2) Hostapd is currently missing some MAC. So replace aeswrap with an
>    AES-GCM implementation.

AES-GCM has the complexity of the IV value having to be unique for every
single use with a specific key. In other words, using AES-GCM would come
at the price of having to define a mechanism to derive and manage new
keys and/or unique IV space..

>    Additionally, if the key provided is all-zero, encryption and
>    authentication could be skipped (for example if used with a VPN).
>    This way users that do not need in-hostapd encryption/authentication
>    of the message can skip it and reduce cpu time needed.

I'm not sure I like this and certainly not with "all-zero key" as the
way of configuring this.

If the new mechanism is over an IP connection, it would be somewhat
easier to justify that external VPN configuration needs to be used.
Though, this would also come with the price of having to have IP
addresses for the APs to be configured statically are learned through
some mechanism (I guess this could use DNS as well, if desired).