diff mbox

[1/7] wpa_supplicant: fix null dereference

Message ID 1421631879-8109-2-git-send-email-ilan.peer@intel.com
State Superseded
Headers show

Commit Message

Peer, Ilan Jan. 19, 2015, 1:44 a.m. UTC 
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.

Signed-off-by: Eytan Lifshitz <eytan.lifshitz@intel.com>
---
 src/ap/ieee802_1x.c                 | 14 ++++----------
 src/eap_server/eap_server_methods.c |  2 +-
 2 files changed, 5 insertions(+), 11 deletions(-)

Comments

Johannes Berg Jan. 19, 2015, 9:36 a.m. UTC | #1 
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
Peer, Ilan Jan. 19, 2015, 9:47 a.m. UTC | #2 
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
Johannes Berg Jan. 19, 2015, 9:49 a.m. UTC | #3 
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
Jouni Malinen Jan. 20, 2015, 12:06 a.m. UTC | #4 
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.
Peer, Ilan Jan. 20, 2015, 7:39 a.m. UTC | #5 
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 mbox

Patch

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 "??";
 }