From b715be5cf3978fbe589b03b09c9398d0d791f850 Mon Sep 17 00:00:00 2001 From: Erik Ekman Date: Mon, 16 Jun 2014 21:12:49 +0200 Subject: [PATCH] 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 | 1 + src/iodined.c | 44 +++++++++++++++++++++++++++++++++----------- src/user.c | 8 +++++++- src/user.h | 2 ++ tests/user.c | 9 +++++++++ 5 files changed, 52 insertions(+), 12 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 01173a1..9f3f967 100644 --- a/CHANGELOG +++ b/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. diff --git a/src/iodined.c b/src/iodined.c index c14f1a7..cdfc920 100644 --- a/src/iodined.c +++ b/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); diff --git a/src/user.c b/src/user.c index 67821a4..0dc95db 100644 --- a/src/user.c +++ b/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; diff --git a/src/user.h b/src/user.h index 00d883b..76fc54b 100644 --- a/src/user.h +++ b/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; diff --git a/tests/user.c b/tests/user.c index 6534317..2e4f36d 100644 --- a/tests/user.c +++ b/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++) {