Message ID | 1421631879-8109-2-git-send-email-ilan.peer@intel.com |
---|---|
State | Superseded |
Headers | show |
On Sun, 2015-01-18 at 20:44 -0500, Ilan Peer wrote: > From: Eytan Lifshitz <eytan.lifshitz@intel.com> > > In ieee802_1x_decapsulate_radius(), eap_server_get_name() may return > null, and it will be dereferenced. Changed it to return "??" instead. Most libc implemenations will just print "(nil)" though. johannes
True, but as the behavior is not defined in the specification, it really depends on the C library implementation. Ilan. > -----Original Message----- > From: Johannes Berg [mailto:johannes@sipsolutions.net] > Sent: Monday, January 19, 2015 11:36 > To: Peer, Ilan > Cc: hostap@lists.shmoo.com; Eytan Lifshitz > Subject: Re: [PATCH 1/7] wpa_supplicant: fix null dereference > > On Sun, 2015-01-18 at 20:44 -0500, Ilan Peer wrote: > > From: Eytan Lifshitz <eytan.lifshitz@intel.com> > > > > In ieee802_1x_decapsulate_radius(), eap_server_get_name() may return > > null, and it will be dereferenced. Changed it to return "??" instead. > > Most libc implemenations will just print "(nil)" though. > > johannes
On Mon, 2015-01-19 at 09:47 +0000, Peer, Ilan wrote: > True, but as the behavior is not defined in the specification, it > really depends on the C library implementation. Yeah, I'm just not sure "??" is the best thing to print then? At least that constitutes a difference in what users will see. johannes
On Mon, Jan 19, 2015 at 10:49:14AM +0100, Johannes Berg wrote: > On Mon, 2015-01-19 at 09:47 +0000, Peer, Ilan wrote: > > True, but as the behavior is not defined in the specification, it > > really depends on the C library implementation. > > Yeah, I'm just not sure "??" is the best thing to print then? At least > that constitutes a difference in what users will see. It looks like eap_server_get_name() is used only in debug and log messages and it should be fine to change its behavior for the unrecognized EAP type case. I don't think "(nil)" is a very good string to use here since it results in number of debug messages with "((nil))". I'll replace this with "unknown" which is at least more readable if nothing else. In addition, eap_server_get_name() function documentation needs to be updated (did that as well) to avoid leaving it claiming to return NULL if EAP method was not found.
Thanks :). Ilan. > -----Original Message----- > From: Jouni Malinen [mailto:j@w1.fi] > Sent: Tuesday, January 20, 2015 02:06 > To: Johannes Berg > Cc: Peer, Ilan; Eytan Lifshitz; hostap@lists.shmoo.com > Subject: Re: [PATCH 1/7] wpa_supplicant: fix null dereference > > On Mon, Jan 19, 2015 at 10:49:14AM +0100, Johannes Berg wrote: > > On Mon, 2015-01-19 at 09:47 +0000, Peer, Ilan wrote: > > > True, but as the behavior is not defined in the specification, it > > > really depends on the C library implementation. > > > > Yeah, I'm just not sure "??" is the best thing to print then? At least > > that constitutes a difference in what users will see. > > It looks like eap_server_get_name() is used only in debug and log messages > and it should be fine to change its behavior for the unrecognized EAP type > case. I don't think "(nil)" is a very good string to use here since it results in > number of debug messages with "((nil))". > I'll replace this with "unknown" which is at least more readable if nothing > else. In addition, eap_server_get_name() function documentation needs to > be updated (did that as well) to avoid leaving it claiming to return NULL if EAP > method was not found. > > -- > Jouni Malinen PGP id EFC895FA
diff --git a/src/ap/ieee802_1x.c b/src/ap/ieee802_1x.c index f11a405..219e5c6 100644 --- a/src/ap/ieee802_1x.c +++ b/src/ap/ieee802_1x.c @@ -1211,15 +1211,11 @@ static void ieee802_1x_decapsulate_radius(struct hostapd_data *hapd, if (eap_type >= 0) sm->eap_type_authsrv = eap_type; os_snprintf(buf, sizeof(buf), "EAP-Request-%s (%d)", - eap_type >= 0 ? eap_server_get_name(0, eap_type) : - "??", - eap_type); + eap_server_get_name(0, eap_type), eap_type); break; case EAP_CODE_RESPONSE: os_snprintf(buf, sizeof(buf), "EAP Response-%s (%d)", - eap_type >= 0 ? eap_server_get_name(0, eap_type) : - "??", - eap_type); + eap_server_get_name(0, eap_type), eap_type); break; case EAP_CODE_SUCCESS: os_strlcpy(buf, "EAP Success", sizeof(buf)); @@ -2502,10 +2498,8 @@ int ieee802_1x_get_mib_sta(struct hostapd_data *hapd, struct sta_info *sta, ret = os_snprintf(buf + len, buflen - len, "last_eap_type_as=%d (%s)\n" "last_eap_type_sta=%d (%s)\n", - sm->eap_type_authsrv, - name1 ? name1 : "", - sm->eap_type_supp, - name2 ? name2 : ""); + sm->eap_type_authsrv, name1, sm->eap_type_supp, + name2); if (os_snprintf_error(buflen - len, ret)) return len; len += ret; diff --git a/src/eap_server/eap_server_methods.c b/src/eap_server/eap_server_methods.c index 0209fad..9639fab 100644 --- a/src/eap_server/eap_server_methods.c +++ b/src/eap_server/eap_server_methods.c @@ -167,5 +167,5 @@ const char * eap_server_get_name(int vendor, EapType type) if (m->vendor == vendor && m->method == type) return m->name; } - return NULL; + return "??"; }