hostapd:Fix potential buffer overflow and null pointer dereference

Message ID 20180905060334.53417-1-sarada.prasanna.garnayak@intel.com
State Not Applicable
Headers show
Series
  • hostapd:Fix potential buffer overflow and null pointer dereference
Related show

Commit Message

Sarada Prasanna Garnayak Sept. 5, 2018, 6:03 a.m.
1.The event_dequeue() returns NULL as an error, so add
check against NULL pointer before dereference.

2.Use max attribute length macro as size for buffer
to store the radius attributes to avoid the potential
buffer overflow & underflow.

3.Clean the uninitialized memory before use.

4.Typecast the operand into compatible data type before
the bitwise operation.

Signed-off-by: Sarada Prasanna Garnayak <sarada.prasanna.garnayak@intel.com>
---
 src/ap/accounting.c      | 2 +-
 src/ap/ieee802_1x.c      | 2 +-
 src/ap/wpa_auth_ie.c     | 1 +
 src/radius/radius.c      | 2 +-
 src/wps/wps_registrar.c  | 3 ++-
 src/wps/wps_upnp_event.c | 2 ++
 6 files changed, 8 insertions(+), 4 deletions(-)

Comments

Jouni Malinen Oct. 16, 2018, 10:06 a.m. | #1
On Wed, Sep 05, 2018 at 11:33:34AM +0530, Sarada Prasanna Garnayak wrote:
> 1.The event_dequeue() returns NULL as an error, so add
> check against NULL pointer before dereference.
> 
> 2.Use max attribute length macro as size for buffer
> to store the radius attributes to avoid the potential
> buffer overflow & underflow.
> 
> 3.Clean the uninitialized memory before use.
> 
> 4.Typecast the operand into compatible data type before
> the bitwise operation.

Please split this into four separate patches, i.e., one per issue. That
said, none of these look like a real issue or well, the last one might
be reasonable cleanup with a bit more detailed commit message.

> diff --git a/src/ap/accounting.c b/src/ap/accounting.c
> @@ -36,7 +36,7 @@ static struct radius_msg * accounting_msg(struct hostapd_data *hapd,
> -	char buf[128];
> +	char buf[RADIUS_MAX_ATTR_LEN];

> diff --git a/src/ap/ieee802_1x.c b/src/ap/ieee802_1x.c
> @@ -503,7 +503,7 @@ int add_common_radius_attr(struct hostapd_data *hapd,
> -	char buf[128];
> +	char buf[RADIUS_MAX_ATTR_LEN];

> diff --git a/src/radius/radius.c b/src/radius/radius.c
> @@ -1343,7 +1343,7 @@ radius_msg_add_attr_user_password(struct radius_msg *msg,
> -	u8 buf[128];
> +	u8 buf[RADIUS_MAX_ATTR_LEN];

What are the claimed "buffer overflow & underflow" here? These buffers
are used to construct a string with snprintf. I don't see how this could
overflow the buffer and how this proposed change would actually fix
anything.

> diff --git a/src/ap/wpa_auth_ie.c b/src/ap/wpa_auth_ie.c
> +++ b/src/ap/wpa_auth_ie.c
> @@ -428,6 +428,7 @@ int wpa_auth_gen_wpa_ie(struct wpa_authenticator *wpa_auth)
>  	u8 *pos, buf[128];

> +	memset(buf, 0, sizeof(buf));

Why? Aren't all the following steps setting all octets of the buffer
that are needed? If not, the correct fix would be there and not here in
this type of initialization that hides potential issues elsewhere.

> diff --git a/src/wps/wps_registrar.c b/src/wps/wps_registrar.c
> @@ -3424,7 +3424,8 @@ static void wps_registrar_sel_reg_add(struct wps_registrar *reg,
>  		reg->sel_reg_dev_password_id_override = s->dev_password_id;
>  	if (reg->sel_reg_config_methods_override == -1)
>  		reg->sel_reg_config_methods_override = 0;
> -	reg->sel_reg_config_methods_override |= s->config_methods;
> +	reg->sel_reg_config_methods_override |=
> +			(int)(unsigned)s->config_methods;

This type of double typecasting looks pretty ugly.. Does a compiler warn
about the code here or why would this be needed?

> diff --git a/src/wps/wps_upnp_event.c b/src/wps/wps_upnp_event.c
> @@ -282,6 +282,8 @@ static int event_send_start(struct subscription *s)
>  		return -1;
>  
>  	s->current_event = e = event_dequeue(s);
> +	if (!e)
> +		return -1;

That first return -1 is used if dl_list_empty(&s->event_queue). In other
words, event_dequeue() cannot return NULL here.

Patch

diff --git a/src/ap/accounting.c b/src/ap/accounting.c
index 0aacc3c95..69169375d 100644
--- a/src/ap/accounting.c
+++ b/src/ap/accounting.c
@@ -36,7 +36,7 @@  static struct radius_msg * accounting_msg(struct hostapd_data *hapd,
 					  int status_type)
 {
 	struct radius_msg *msg;
-	char buf[128];
+	char buf[RADIUS_MAX_ATTR_LEN];
 	u8 *val;
 	size_t len;
 	int i;
diff --git a/src/ap/ieee802_1x.c b/src/ap/ieee802_1x.c
index 985f8b787..52c211cc8 100644
--- a/src/ap/ieee802_1x.c
+++ b/src/ap/ieee802_1x.c
@@ -503,7 +503,7 @@  int add_common_radius_attr(struct hostapd_data *hapd,
 			   struct sta_info *sta,
 			   struct radius_msg *msg)
 {
-	char buf[128];
+	char buf[RADIUS_MAX_ATTR_LEN];
 	struct hostapd_radius_attr *attr;
 	int len;
 
diff --git a/src/ap/wpa_auth_ie.c b/src/ap/wpa_auth_ie.c
index 421dd5a6f..a753376c9 100644
--- a/src/ap/wpa_auth_ie.c
+++ b/src/ap/wpa_auth_ie.c
@@ -428,6 +428,7 @@  int wpa_auth_gen_wpa_ie(struct wpa_authenticator *wpa_auth)
 	u8 *pos, buf[128];
 	int res;
 
+	memset(buf, 0, sizeof(buf));
 #ifdef CONFIG_TESTING_OPTIONS
 	if (wpa_auth->conf.own_ie_override_len) {
 		wpa_hexdump(MSG_DEBUG, "WPA: Forced own IE(s) for testing",
diff --git a/src/radius/radius.c b/src/radius/radius.c
index 07240ea22..63be0d732 100644
--- a/src/radius/radius.c
+++ b/src/radius/radius.c
@@ -1343,7 +1343,7 @@  radius_msg_add_attr_user_password(struct radius_msg *msg,
 				  const u8 *data, size_t data_len,
 				  const u8 *secret, size_t secret_len)
 {
-	u8 buf[128];
+	u8 buf[RADIUS_MAX_ATTR_LEN];
 	int res;
 
 	res = radius_user_password_hide(msg, data, data_len,
diff --git a/src/wps/wps_registrar.c b/src/wps/wps_registrar.c
index 379925e3f..ed8c1d1e2 100644
--- a/src/wps/wps_registrar.c
+++ b/src/wps/wps_registrar.c
@@ -3424,7 +3424,8 @@  static void wps_registrar_sel_reg_add(struct wps_registrar *reg,
 		reg->sel_reg_dev_password_id_override = s->dev_password_id;
 	if (reg->sel_reg_config_methods_override == -1)
 		reg->sel_reg_config_methods_override = 0;
-	reg->sel_reg_config_methods_override |= s->config_methods;
+	reg->sel_reg_config_methods_override |=
+			(int)(unsigned)s->config_methods;
 	for (i = 0; i < WPS_MAX_AUTHORIZED_MACS; i++)
 		if (is_zero_ether_addr(reg->authorized_macs_union[i]))
 			break;
diff --git a/src/wps/wps_upnp_event.c b/src/wps/wps_upnp_event.c
index 94aae7542..f1f981138 100644
--- a/src/wps/wps_upnp_event.c
+++ b/src/wps/wps_upnp_event.c
@@ -282,6 +282,8 @@  static int event_send_start(struct subscription *s)
 		return -1;
 
 	s->current_event = e = event_dequeue(s);
+	if (!e)
+		return -1;
 
 	/* Use address according to number of retries */
 	itry = 0;