diff mbox

EAP-TTLS: Fix parsing auth= and autheap= Phase2 params

Message ID 1449399692-21998-1-git-send-email-pali.rohar@gmail.com
State Accepted
Headers show

Commit Message

Pali Rohár Dec. 6, 2015, 11:01 a.m. UTC
This patch fix security issue when Phase2 param auth=MSCHAPv2 was handled as
MSCHAP (v1) which degraded security. Now when invalid or unsupported auth=
Phase2 param combinations are specified then EAP-TTLS throw error instead
silently doing something.

More then one auth= Phase2 type cannot be specified and also both auth= and
autheap= options cannot be specified.

Parsing Phase2 type is case sensitive (as in other EAP parts), so Phase2
param auth=MSCHAPv2 is invalid. Only auth=MSCHAPV2 is correct.
---
 src/eap_peer/eap_ttls.c |   78 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 66 insertions(+), 12 deletions(-)

Comments

Jouni Malinen Dec. 6, 2015, 11:47 a.m. UTC | #1
On Sun, Dec 06, 2015 at 12:01:32PM +0100, Pali Rohár wrote:
> This patch fix security issue when Phase2 param auth=MSCHAPv2 was handled as
> MSCHAP (v1) which degraded security. Now when invalid or unsupported auth=
> Phase2 param combinations are specified then EAP-TTLS throw error instead
> silently doing something.
> 
> More then one auth= Phase2 type cannot be specified and also both auth= and
> autheap= options cannot be specified.
> 
> Parsing Phase2 type is case sensitive (as in other EAP parts), so Phase2
> param auth=MSCHAPv2 is invalid. Only auth=MSCHAPV2 is correct.
> ---

Could you please read the top level CONTRIBUTIONS file and resubmit this
with Signed-off-by: line added so that I can apply the changes?

As far as the changes are concerned, would it be more useful to make
phase2 parsing case insensitive to allow that previously invalid
auth=MSCHAPv2 case to be parsed in the same way as the valid
auth=MSCHAPV2 case?
Pali Rohár Dec. 7, 2015, 8:53 a.m. UTC | #2
On Sunday 06 December 2015 13:47:12 Jouni Malinen wrote:
> Could you please read the top level CONTRIBUTIONS file and resubmit this
> with Signed-off-by: line added so that I can apply the changes?

Ou, sorry, I forgot -s param in git commit... So add my:
Signed-off-by: Pali Rohár <pali.rohar@gmail.com>

> As far as the changes are concerned, would it be more useful to make
> phase2 parsing case insensitive to allow that previously invalid
> auth=MSCHAPv2 case to be parsed in the same way as the valid
> auth=MSCHAPV2 case?

I was thinking about it and it is not good idea. All auth (eap and non
eap) types are case sensitive and upper-case. And consistency here could
be good argument.

Second, this patch does not change documentation, it just fix parsing
code to work as expected. So config file from new version (after
applying this patch) should do same as if it is used by older version
(without this patch). But accepting lower-case MSCHAPv2 would mean that
new and old version would parse that argument differently.

More over, if we accept MSCHAPv2 and parse it as MSCHAPV2 it means that
people could start creating howto/manual on internet and use lowercase
MSCHAPv2. If somebody with older version of wpa supplicant will use that
howto/manual then it will use V1 and not V2!

So I rather do not allow MSCHAPv2 at all. That option was invalid and
due to parser error was mapped to V1. I really do not like idea when
software change meaning of some option when updating to new version.
Jouni Malinen Dec. 17, 2015, 11:07 p.m. UTC | #3
On Mon, Dec 07, 2015 at 09:53:12AM +0100, Pali Rohár wrote:
> Ou, sorry, I forgot -s param in git commit... So add my:
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>

Thanks, applied. I replaced the strdup() design in parsing with use of
cstr_token() to avoid unnecessary memory allocation and cleaned up the
changes a bit.
diff mbox

Patch

diff --git a/src/eap_peer/eap_ttls.c b/src/eap_peer/eap_ttls.c
index b186c91..fa9d60d 100644
--- a/src/eap_peer/eap_ttls.c
+++ b/src/eap_peer/eap_ttls.c
@@ -70,6 +70,7 @@  static void * eap_ttls_init(struct eap_sm *sm)
 {
 	struct eap_ttls_data *data;
 	struct eap_peer_config *config = eap_get_config(sm);
+	int selected_non_eap;
 	char *selected;
 
 	data = os_zalloc(sizeof(*data));
@@ -77,26 +78,79 @@  static void * eap_ttls_init(struct eap_sm *sm)
 		return NULL;
 	data->ttls_version = EAP_TTLS_VERSION;
 	selected = "EAP";
+	selected_non_eap = 0;
 	data->phase2_type = EAP_TTLS_PHASE2_EAP;
 
+	/* Either one auth= type is specified or more autheap= methods are specified */
 	if (config && config->phase2) {
+		char *start, *pos, *buf;
+
+		start = buf = os_strdup(config->phase2);
+		if (buf == NULL) {
+			eap_ttls_deinit(sm, data);
+			return NULL;
+		}
+
+		while (start && *start != '\0') {
+			pos = os_strstr(start, "auth=");
+			if (pos == NULL)
+				break;
+			if (start != pos && *(pos - 1) != ' ') {
+				start = pos + 5; /* os_strlen("auth=") */
+				continue;
+			}
+
+			start = pos + 5; /* os_strlen("auth=") */
+			pos = os_strchr(start, ' ');
+			if (pos)
+				*pos++ = '\0';
+
+			if (os_strcmp(start, "MSCHAPV2") == 0) {
+				selected = "MSCHAPV2";
+				data->phase2_type = EAP_TTLS_PHASE2_MSCHAPV2;
+			} else if (os_strcmp(start, "MSCHAP") == 0) {
+				selected = "MSCHAP";
+				data->phase2_type = EAP_TTLS_PHASE2_MSCHAP;
+			} else if (os_strcmp(start, "PAP") == 0) {
+				selected = "PAP";
+				data->phase2_type = EAP_TTLS_PHASE2_PAP;
+			} else if (os_strcmp(start, "CHAP") == 0) {
+				selected = "CHAP";
+				data->phase2_type = EAP_TTLS_PHASE2_CHAP;
+			} else {
+				wpa_printf(MSG_ERROR, "EAP-TTLS: Unsupported Phase2 "
+					   "type '%s'", start);
+				os_free(buf);
+				eap_ttls_deinit(sm, data);
+				return NULL;
+			}
+
+			if (selected_non_eap) {
+				wpa_printf(MSG_ERROR, "EAP-TTLS: Only one Phase2 "
+					   "type can be specified");
+				os_free(buf);
+				eap_ttls_deinit(sm, data);
+				return NULL;
+			}
+
+			selected_non_eap = 1;
+			start = pos;
+		}
+
+		os_free(buf);
+
 		if (os_strstr(config->phase2, "autheap=")) {
+			if (selected_non_eap) {
+				wpa_printf(MSG_ERROR, "EAP-TTLS: Both auth= and "
+					   "autheap= params cannot be specified");
+				eap_ttls_deinit(sm, data);
+				return NULL;
+			}
 			selected = "EAP";
 			data->phase2_type = EAP_TTLS_PHASE2_EAP;
-		} else if (os_strstr(config->phase2, "auth=MSCHAPV2")) {
-			selected = "MSCHAPV2";
-			data->phase2_type = EAP_TTLS_PHASE2_MSCHAPV2;
-		} else if (os_strstr(config->phase2, "auth=MSCHAP")) {
-			selected = "MSCHAP";
-			data->phase2_type = EAP_TTLS_PHASE2_MSCHAP;
-		} else if (os_strstr(config->phase2, "auth=PAP")) {
-			selected = "PAP";
-			data->phase2_type = EAP_TTLS_PHASE2_PAP;
-		} else if (os_strstr(config->phase2, "auth=CHAP")) {
-			selected = "CHAP";
-			data->phase2_type = EAP_TTLS_PHASE2_CHAP;
 		}
 	}
+
 	wpa_printf(MSG_DEBUG, "EAP-TTLS: Phase2 type: %s", selected);
 
 	if (data->phase2_type == EAP_TTLS_PHASE2_EAP) {