Fix authentication bypass bug

The client could bypass the password check by continuing after getting error
from the server and guessing the network parameters. The server would still
accept the rest of the setup and also network traffic.

Add checks for normal and raw mode that user has authenticated before allowing
any other communication.

Problem found by Oscar Reparaz.

CHANGELOG

@@ -26,6 +26,7 @@ master:
 	- Do not let sockets be inherited by sub-processes, fixes #99.
 	- Add unspecified RR type (called PRIVATE; id 65399, in private use
 		range). For servers with RFC3597 support. Fixes #97.
+	- Fix authentication bypass vulnerability; found by Oscar Reparaz.
 
 2010-02-06: 0.6.0-rc1 "Hotspotify"
 	- Fixed tunnel not working on Windows.

src/iodined.c

@@ -174,6 +174,7 @@ syslog(int a, const char *str, ...)
 }
 #endif
 
+/* This will not check that user has passed login challenge */
 static int
 check_user_and_ip(int userid, struct query *q)
 {
@@ -200,6 +201,20 @@ check_user_and_ip(int userid, struct query *q)
 	return memcmp(&(users[userid].host), &(tempin->sin_addr), sizeof(struct in_addr));
 }
 
+/* This checks that user has passed normal (non-raw) login challenge */
+static int
+check_authenticated_user_and_ip(int userid, struct query *q)
+{
+	int res = check_user_and_ip(userid, q);
+	if (res)
+		return res;
+
+	if (!users[userid].authenticated)
+		return 1;
+
+	return 0;
+}
+
 static void
 send_raw(int fd, char *buf, int buflen, int user, int cmd, struct query *q)
 {
@@ -836,8 +851,10 @@ handle_null_request(int tun_fd, int dns_fd, struct query *q, int domain_len)
 			login_calculate(logindata, 16, password, users[userid].seed);
 
 			if (read >= 18 && (memcmp(logindata, unpacked+1, 16) == 0)) {
-				/* Login ok, send ip/mtu/netmask info */
+				/* Store login ok */
+				users[userid].authenticated = 1;
 
+				/* Send ip/mtu/netmask info */
 				tempip.s_addr = my_ip;
 				tmp[0] = strdup(inet_ntoa(tempip));
 				tempip.s_addr = users[userid].tun_ip;
@@ -866,7 +883,7 @@ handle_null_request(int tun_fd, int dns_fd, struct query *q, int domain_len)
 		char reply[5];
 
 		userid = b32_8to5(in[1]);
-		if (check_user_and_ip(userid, q) != 0) {
+		if (check_authenticated_user_and_ip(userid, q) != 0) {
 			write_dns(dns_fd, q, "BADIP", 5, 'T');
 			return; /* illegal id */
 		}
@@ -903,7 +920,7 @@ handle_null_request(int tun_fd, int dns_fd, struct query *q, int domain_len)
 
 		userid = b32_8to5(in[1]);
 
-		if (check_user_and_ip(userid, q) != 0) {
+		if (check_authenticated_user_and_ip(userid, q) != 0) {
 			write_dns(dns_fd, q, "BADIP", 5, 'T');
 			return; /* illegal id */
 		}
@@ -944,7 +961,7 @@ handle_null_request(int tun_fd, int dns_fd, struct query *q, int domain_len)
 
 		userid = b32_8to5(in[1]);
 
-		if (check_user_and_ip(userid, q) != 0) {
+		if (check_authenticated_user_and_ip(userid, q) != 0) {
 			write_dns(dns_fd, q, "BADIP", 5, 'T');
 			return; /* illegal id */
 		}
@@ -1072,7 +1089,7 @@ handle_null_request(int tun_fd, int dns_fd, struct query *q, int domain_len)
 
 		/* Downstream fragsize probe packet */
 		userid = (b32_8to5(in[1]) >> 1) & 15;
-		if (check_user_and_ip(userid, q) != 0) {
+		if (check_authenticated_user_and_ip(userid, q) != 0) {
 			write_dns(dns_fd, q, "BADIP", 5, 'T');
 			return; /* illegal id */
 		}
@@ -1107,7 +1124,7 @@ handle_null_request(int tun_fd, int dns_fd, struct query *q, int domain_len)
 
 		/* Downstream fragsize packet */
 		userid = unpacked[0];
-		if (check_user_and_ip(userid, q) != 0) {
+		if (check_authenticated_user_and_ip(userid, q) != 0) {
 			write_dns(dns_fd, q, "BADIP", 5, 'T');
 			return; /* illegal id */
 		}
@@ -1140,7 +1157,7 @@ handle_null_request(int tun_fd, int dns_fd, struct query *q, int domain_len)
 
 		/* Ping packet, store userid */
 		userid = unpacked[0];
-		if (check_user_and_ip(userid, q) != 0) {
+		if (check_authenticated_user_and_ip(userid, q) != 0) {
 			write_dns(dns_fd, q, "BADIP", 5, 'T');
 			return; /* illegal id */
 		}
@@ -1270,7 +1287,7 @@ handle_null_request(int tun_fd, int dns_fd, struct query *q, int domain_len)
 
 		userid = code;
 		/* Check user and sending ip number */
-		if (check_user_and_ip(userid, q) != 0) {
+		if (check_authenticated_user_and_ip(userid, q) != 0) {
 			write_dns(dns_fd, q, "BADIP", 5, 'T');
 			return; /* illegal id */
 		}
@@ -1847,10 +1864,11 @@ handle_raw_login(char *packet, int len, struct query *q, int fd, int userid)
 
 	if (len < 16) return;
 
-	/* can't use check_user_and_ip() since IP address will be different,
+	/* can't use check_authenticated_user_and_ip() since IP address will be different,
 	   so duplicate here except IP address */
 	if (userid < 0 || userid >= created_users) return;
 	if (!users[userid].active || users[userid].disabled) return;
+	if (!users[userid].authenticated) return;
 	if (users[userid].last_pkt + 60 < time(NULL)) return;
 
 	if (debug >= 1) {
@@ -1875,15 +1893,18 @@ handle_raw_login(char *packet, int len, struct query *q, int fd, int userid)
 		user_set_conn_type(userid, CONN_RAW_UDP);
 		login_calculate(myhash, 16, password, users[userid].seed - 1);
 		send_raw(fd, myhash, 16, userid, RAW_HDR_CMD_LOGIN, q);
+
+		users[userid].authenticated_raw = 1;
 	}
 }
 
 static void
 handle_raw_data(char *packet, int len, struct query *q, int dns_fd, int tun_fd, int userid)
 {
-	if (check_user_and_ip(userid, q) != 0) {
+	if (check_authenticated_user_and_ip(userid, q) != 0) {
 		return;
 	}
+	if (!users[userid].authenticated_raw) return;
 
 	/* Update query and time info for user */
 	users[userid].last_pkt = time(NULL);
@@ -1905,9 +1926,10 @@ handle_raw_data(char *packet, int len, struct query *q, int dns_fd, int tun_fd,
 static void
 handle_raw_ping(struct query *q, int dns_fd, int userid)
 {
-	if (check_user_and_ip(userid, q) != 0) {
+	if (check_authenticated_user_and_ip(userid, q) != 0) {
 		return;
 	}
+	if (!users[userid].authenticated_raw) return;
 
 	/* Update query and time info for user */
 	users[userid].last_pkt = time(NULL);

src/user.c

@@ -75,6 +75,8 @@ init_users(in_addr_t my_ip, int netbits)
 		users[i].tun_ip = ip;
 		net.s_addr = ip;
 		users[i].disabled = 0;
+		users[i].authenticated = 0;
+		users[i].authenticated_raw = 0;
 		users[i].active = 0;
  		/* Rest is reset on login ('V' packet) */
 	}
@@ -116,7 +118,9 @@ find_user_by_ip(uint32_t ip)
 
 	ret = -1;
 	for (i = 0; i < usercount; i++) {
-		if (users[i].active && !users[i].disabled &&
+		if (users[i].active &&
+			users[i].authenticated &&
+			!users[i].disabled &&
 			users[i].last_pkt + 60 > time(NULL) &&
 			ip == users[i].tun_ip) {
 			ret = i;
@@ -168,6 +172,8 @@ find_available_user()
 		/* Not used at all or not used in one minute */
 		if ((!users[i].active || users[i].last_pkt + 60 < time(NULL)) && !users[i].disabled) {
 			users[i].active = 1;
+			users[i].authenticated = 0;
+			users[i].authenticated_raw = 0;
 			users[i].last_pkt = time(NULL);
 			users[i].fragsize = 4096;
 			users[i].conn = CONN_DNS_NULL;

src/user.h

@@ -37,6 +37,8 @@
 struct tun_user {
 	char id;
 	int active;
+	int authenticated;
+	int authenticated_raw;
 	int disabled;
 	time_t last_pkt;
 	int seed;

tests/user.c

@@ -94,6 +94,11 @@ START_TEST(test_find_user_by_ip)
 
 	users[0].last_pkt = time(NULL);
 
+	testip = (unsigned int) inet_addr("127.0.0.2");
+	fail_unless(find_user_by_ip(testip) == -1);
+
+	users[0].authenticated = 1;
+
 	testip = (unsigned int) inet_addr("127.0.0.2");
 	fail_unless(find_user_by_ip(testip) == 0);
 }
@@ -137,7 +142,11 @@ START_TEST(test_find_available_user)
 	init_users(ip, 27);
 
 	for (i = 0; i < USERS; i++) {
+		users[i].authenticated = 1;
+		users[i].authenticated_raw = 1;
 		fail_unless(find_available_user() == i);
+		fail_if(users[i].authenticated);
+		fail_if(users[i].authenticated_raw);
 	}
 
 	for (i = 0; i < USERS; i++) {