[PATCH] Updates for stricter automatic memcpy bounds checking

[Arik Nemtsov]

On Sun, Apr 12, 2015 at 3:54 AM, Nick Kralevich <nnk at google.com> wrote:
> Both Android's libc and glibc support _FORTIFY_SOURCE, a compiler
> and libc feature which inserts automatic bounds checking into
> common C functions such as memcpy() and strcpy(). If a buffer
> overflow occurs when calling a hardened libc function, the
> automatic bounds checking will safely shutdown the program and
> prevent memory corruption.
>
> Android is experimenting with _FORTIFY_SOURCE=3, a new fortify
> level which enhances memcpy() to prevent overflowing an element
> of a struct. Under the enhancements, code such as
>
>   struct foo {
>     char empty[0];
>     char one[1];
>     char a[10];
>     char b[10];
>   };
>
>   int main() {
>     foo myfoo;
>     int n = atoi("11");
>     memcpy(myfoo.a, "01234567890123456789", n);
>     return 0;
>   }
>
> will cleanly crash when the memcpy() call is made.
>
> Fixup hostap code to support the new level. Specifically:
>
> * Modify the structures in ieee802_11_defs.h to use ISO C99 flexible
> array members (https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html)
> instead of a zero length array. Zero length arrays have zero length,
> and any attempt to call memcpy() on such elements will always overflow.
> Flexible array members have no such limitation.
>
> * Introduce an extra element into the structure probe_req. This is
> needed to keep the code compiling when an otherwise zero length
> structure would be present.
>
> * Fixup sha1_transform so it works with the enhanced bounds checking.
> The old memcpy() code was attempting to write to context.h0, but that
> structure element is too small and the write was extending (by design)
> into h1, h2, h3, and h4. Use explicit assignments instead of
> overflowing the struct element.
>
> Signed-off-by: Nick Kralevich <nnk at google.com>
> ---
>  src/common/ieee802_11_defs.h  | 41 +++++++++++++++++++++--------------------
>  src/crypto/fips_prf_openssl.c | 16 ++++++++++++----
>  2 files changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/src/common/ieee802_11_defs.h b/src/common/ieee802_11_defs.h
> index 2e51935..98b249a 100644
> --- a/src/common/ieee802_11_defs.h
> +++ b/src/common/ieee802_11_defs.h
> @@ -470,35 +470,35 @@ struct ieee80211_mgmt {
>                         le16 auth_transaction;
>                         le16 status_code;
>                         /* possibly followed by Challenge text */
> -                       u8 variable[0];
> +                       u8 variable[];
>                 } STRUCT_PACKED auth;
>                 struct {
>                         le16 reason_code;
> -                       u8 variable[0];
> +                       u8 variable[];
>                 } STRUCT_PACKED deauth;
>                 struct {
>                         le16 capab_info;
>                         le16 listen_interval;
>                         /* followed by SSID and Supported rates */
> -                       u8 variable[0];
> +                       u8 variable[];
>                 } STRUCT_PACKED assoc_req;
>                 struct {
>                         le16 capab_info;
>                         le16 status_code;
>                         le16 aid;
>                         /* followed by Supported rates */
> -                       u8 variable[0];
> +                       u8 variable[];
>                 } STRUCT_PACKED assoc_resp, reassoc_resp;
>                 struct {
>                         le16 capab_info;
>                         le16 listen_interval;
>                         u8 current_ap[6];
>                         /* followed by SSID and Supported rates */
> -                       u8 variable[0];
> +                       u8 variable[];
>                 } STRUCT_PACKED reassoc_req;
>                 struct {
>                         le16 reason_code;
> -                       u8 variable[0];
> +                       u8 variable[];
>                 } STRUCT_PACKED disassoc;
>                 struct {
>                         u8 timestamp[8];
> @@ -506,11 +506,12 @@ struct ieee80211_mgmt {
>                         le16 capab_info;
>                         /* followed by some of SSID, Supported rates,
>                          * FH Params, DS Params, CF Params, IBSS Params, TIM */
> -                       u8 variable[0];
> +                       u8 variable[];
>                 } STRUCT_PACKED beacon;
>                 struct {
> +                       u8 unused;
>                         /* only variable items: SSID, Supported rates */
> -                       u8 variable[0];
> +                       u8 variable[];
>                 } STRUCT_PACKED probe_req;

Isn't this introducing a bug? This piece of code will now point to the
wrong location I believe:

ie = mgmt->u.probe_req.variable;

Arik