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. -- Jouni Malinen PGP id EFC895FA _______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap
