| Message ID | 1428800084-10322-1-git-send-email-nnk@google.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, Apr 12, 2015 at 3:54 AM, Nick Kralevich <nnk@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@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
On Sun, Apr 12, 2015 at 12:53 AM, Arik Nemtsov <arik@wizery.com> wrote: > > 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; Yes, I believe this is a bug. I updated the patch to avoid changing this structure, since I couldn't figure out a clean way to fix it properly. gcc doesn't support the use of a flexible array element as the only element of a structure (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53548)
On Sun, Apr 12, 2015 at 11:26 PM, Nick Kralevich <nnk@google.com> wrote: > On Sun, Apr 12, 2015 at 12:53 AM, Arik Nemtsov <arik@wizery.com> wrote: >> > 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; > > Yes, I believe this is a bug. I updated the patch to avoid changing > this structure, since I couldn't figure out a clean way to fix it > properly. gcc doesn't support the use of a flexible array element as > the only element of a structure > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53548) Looks good. Yea I guess the simplest solution is to just avoid verification here for now. Arik
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; struct { u8 timestamp[8]; @@ -518,7 +519,7 @@ struct ieee80211_mgmt { le16 capab_info; /* followed by some of SSID, Supported rates, * FH Params, DS Params, CF Params, IBSS Params */ - u8 variable[0]; + u8 variable[]; } STRUCT_PACKED probe_resp; struct { u8 category; @@ -527,7 +528,7 @@ struct ieee80211_mgmt { u8 action_code; u8 dialog_token; u8 status_code; - u8 variable[0]; + u8 variable[]; } STRUCT_PACKED wmm_action; struct{ u8 action_code; @@ -541,14 +542,14 @@ struct ieee80211_mgmt { u8 action; u8 sta_addr[ETH_ALEN]; u8 target_ap_addr[ETH_ALEN]; - u8 variable[0]; /* FT Request */ + u8 variable[]; /* FT Request */ } STRUCT_PACKED ft_action_req; struct { u8 action; u8 sta_addr[ETH_ALEN]; u8 target_ap_addr[ETH_ALEN]; le16 status_code; - u8 variable[0]; /* FT Request */ + u8 variable[]; /* FT Request */ } STRUCT_PACKED ft_action_resp; struct { u8 action; @@ -561,23 +562,23 @@ struct ieee80211_mgmt { struct { u8 action; u8 dialogtoken; - u8 variable[0]; + u8 variable[]; } STRUCT_PACKED wnm_sleep_req; struct { u8 action; u8 dialogtoken; le16 keydata_len; - u8 variable[0]; + u8 variable[]; } STRUCT_PACKED wnm_sleep_resp; struct { u8 action; - u8 variable[0]; + u8 variable[]; } STRUCT_PACKED public_action; struct { u8 action; /* 9 */ u8 oui[3]; /* Vendor-specific content */ - u8 variable[0]; + u8 variable[]; } STRUCT_PACKED vs_public_action; struct { u8 action; /* 7 */ @@ -589,7 +590,7 @@ struct ieee80211_mgmt { * Session Information URL (optional), * BSS Transition Candidate List * Entries */ - u8 variable[0]; + u8 variable[]; } STRUCT_PACKED bss_tm_req; struct { u8 action; /* 8 */ @@ -599,7 +600,7 @@ struct ieee80211_mgmt { /* Target BSSID (optional), * BSS Transition Candidate List * Entries (optional) */ - u8 variable[0]; + u8 variable[]; } STRUCT_PACKED bss_tm_resp; struct { u8 action; /* 6 */ @@ -607,11 +608,11 @@ struct ieee80211_mgmt { u8 query_reason; /* BSS Transition Candidate List * Entries (optional) */ - u8 variable[0]; + u8 variable[]; } STRUCT_PACKED bss_tm_query; struct { u8 action; /* 15 */ - u8 variable[0]; + u8 variable[]; } STRUCT_PACKED slf_prot_action; } u; } STRUCT_PACKED action; diff --git a/src/crypto/fips_prf_openssl.c b/src/crypto/fips_prf_openssl.c index d69ecea..fb03efc 100644 --- a/src/crypto/fips_prf_openssl.c +++ b/src/crypto/fips_prf_openssl.c @@ -13,13 +13,21 @@ #include "crypto.h" -static void sha1_transform(u8 *state, const u8 data[64]) +static void sha1_transform(u32 *state, const u8 data[64]) { SHA_CTX context; os_memset(&context, 0, sizeof(context)); - os_memcpy(&context.h0, state, 5 * 4); + context.h0 = state[0]; + context.h1 = state[1]; + context.h2 = state[2]; + context.h3 = state[3]; + context.h4 = state[4]; SHA1_Transform(&context, data); - os_memcpy(state, &context.h0, 5 * 4); + state[0] = context.h0; + state[1] = context.h1; + state[2] = context.h2; + state[3] = context.h3; + state[4] = context.h4; } @@ -53,7 +61,7 @@ int fips186_2_prf(const u8 *seed, size_t seed_len, u8 *x, size_t xlen) /* w_i = G(t, XVAL) */ os_memcpy(_t, t, 20); - sha1_transform((u8 *) _t, xkey); + sha1_transform(_t, xkey); _t[0] = host_to_be32(_t[0]); _t[1] = host_to_be32(_t[1]); _t[2] = host_to_be32(_t[2]);
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@google.com> --- src/common/ieee802_11_defs.h | 41 +++++++++++++++++++++-------------------- src/crypto/fips_prf_openssl.c | 16 ++++++++++++---- 2 files changed, 33 insertions(+), 24 deletions(-)