netdev - Re: question about potential integer truncation in mwifiex_set_wapi_ie and mwifiex_set_wps

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 30 Sep 2015 17:13:28 +0200
From:	"PaX Team" <pageexec@...email.hu>
To:	James Cameron <quozl@...top.org>
CC:	Amitkumar Karwar <akarwar@...vell.com>,
	Avinash Patil <patila@...vell.com>,
	Kalle Valo <kvalo@...eaurora.org>,
	linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
	re.emese@...il.com, spender@...ecurity.net
Subject: Re: question about potential integer truncation in mwifiex_set_wapi_ie and mwifiex_set_wps_ie

On 30 Sep 2015 at 9:10, James Cameron wrote:

> On Tue, Sep 29, 2015 at 05:21:28PM +0200, PaX Team wrote:
> > hi all,
> > 
> > in drivers/net/wireless/mwifiex/sta_ioctl.c the following functions
> > 
> > 	mwifiex_set_wpa_ie_helper
> > 	mwifiex_set_wapi_ie
> > 	mwifiex_set_wps_ie
> > 
> > can truncate the incoming ie_len argument from u16 to u8 when it gets
> > stored in mwifiex_private.wpa_ie_len, mwifiex_private.wapi_ie_len and
> > mwifiex_private.wps_ie_len, respectively. based on some light code
> > reading it seems a length value of 256 is valid (IEEE_MAX_IE_SIZE and
> > MWIFIEX_MAX_VSIE_LEN seem to limit it) and thus would get truncated
> > to 0 when stored in those u8 fields. the question is whether this is
> > intentional or a bug somewhere.
> 
> i agree, while there is a test to ensure ie_len is not greater than
> 256, there is a possibility that it will be exactly 256, which means
> 256 bytes will be given to memcpy but
> mwifiex_private.{wpa,wapi,wps}_ie_len will be zero.
> 
> i suggest changing the lengths to u16.  not tested.

while this is probably a necessary first step you'll also have to verify
all loads from these fields for integer truncation that can occur now
(the stores were checked by the plugin i mentioned above). based on a
naive grep the code seems to be fine but someone with actual knowledge
of the code had better verify it ;).

> diff --git a/drivers/net/wireless/mwifiex/main.h b/drivers/net/wireless/mwifiex/main.h
> index fe12560..b66e9a7 100644
> --- a/drivers/net/wireless/mwifiex/main.h
> +++ b/drivers/net/wireless/mwifiex/main.h
> @@ -512,14 +512,14 @@ struct mwifiex_private {
>   struct mwifiex_wep_key wep_key[NUM_WEP_KEYS];
>   u16 wep_key_curr_index;
>   u8 wpa_ie[256];
> -	u8 wpa_ie_len;
> +	u16 wpa_ie_len;
>   u8 wpa_is_gtk_set;
>   struct host_cmd_ds_802_11_key_material aes_key;
>   struct host_cmd_ds_802_11_key_material_v2 aes_key_v2;
>   u8 wapi_ie[256];
> -	u8 wapi_ie_len;
> +	u16 wapi_ie_len;
>   u8 *wps_ie;
> -	u8 wps_ie_len;
> +	u16 wps_ie_len;
>   u8 wmm_required;
>   u8 wmm_enabled;
>   u8 wmm_qosinfo;
> 
> -- 
> James Cameron
> http://quozl.linux.org.au/
> 



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html