On Tue, 17 Jun 2014 06:57:18 +0200, Salvatore Bonaccorso wrote: (Cc'ing upstream)
> There was a new upstream version for iodine released Ha! The Debian security team is quicker than my daily uscan cronjob :) > fixing an > authentication bypass vulnerability. > > Upstream commit is at [1], but no CVE is yet assigned[2] so far. > > [1] > https://github.com/yarrick/iodine/commit/b715be5cf3978fbe589b03b09c9398d0d791f850 > [2] http://www.openwall.com/lists/oss-security/2014/06/16/5 Thanks! I suppose we also need the fix in (old?-)stable; and it might also make sense to upload the current 0.6 package with only this fix to unstable with urgency high before looking into 0.7.0. Unfortunately the patch doesn't apply cleanly (neither against 0.6.0~rc1-18 in Debian nor against the iodine-0.6 branch in upstream git). I've tried to resolve the merge conflicts and came up with the attached patch. Could the two of you please take a look at it to check if it's sane? -- Which it probably isn't since the tests fail now; or the test suite needs more adoption as well ... *sigh* #v+ dh_auto_test make[1]: Entering directory '/tmp/buildd/iodine-0.6.0~rc1' make[2]: Entering directory '/tmp/buildd/iodine-0.6.0~rc1/src' OS is LINUX, arch is x86_64 make[2]: Leaving directory '/tmp/buildd/iodine-0.6.0~rc1/src' !! The check library is required for compiling and running the tests !! Get it at http://check.sf.net make[2]: Entering directory '/tmp/buildd/iodine-0.6.0~rc1/tests' CC test.c gcc -g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D`uname | tr "a-z" "A-Z"` -I../src -I/usr/local/include -pedantic `../src/osflags cflags` -D_FORTIFY_SOURCE=2 -c test.c CC base32.c gcc -g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D`uname | tr "a-z" "A-Z"` -I../src -I/usr/local/include -pedantic `../src/osflags cflags` -D_FORTIFY_SOURCE=2 -c base32.c CC base64.c gcc -g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D`uname | tr "a-z" "A-Z"` -I../src -I/usr/local/include -pedantic `../src/osflags cflags` -D_FORTIFY_SOURCE=2 -c base64.c CC read.c gcc -g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D`uname | tr "a-z" "A-Z"` -I../src -I/usr/local/include -pedantic `../src/osflags cflags` -D_FORTIFY_SOURCE=2 -c read.c CC dns.c gcc -g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D`uname | tr "a-z" "A-Z"` -I../src -I/usr/local/include -pedantic `../src/osflags cflags` -D_FORTIFY_SOURCE=2 -c dns.c CC encoding.c gcc -g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D`uname | tr "a-z" "A-Z"` -I../src -I/usr/local/include -pedantic `../src/osflags cflags` -D_FORTIFY_SOURCE=2 -c encoding.c CC login.c gcc -g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D`uname | tr "a-z" "A-Z"` -I../src -I/usr/local/include -pedantic `../src/osflags cflags` -D_FORTIFY_SOURCE=2 -c login.c CC user.c gcc -g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D`uname | tr "a-z" "A-Z"` -I../src -I/usr/local/include -pedantic `../src/osflags cflags` -D_FORTIFY_SOURCE=2 -c user.c CC fw_query.c gcc -g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D`uname | tr "a-z" "A-Z"` -I../src -I/usr/local/include -pedantic `../src/osflags cflags` -D_FORTIFY_SOURCE=2 -c fw_query.c LD test gcc -o test ../src/base32.o ../src/base64.o ../src/read.o ../src/dns.o ../src/encoding.o ../src/login.o ../src/md5.o ../src/user.o ../src/fw_query.o test.o base32.o base64.o read.o dns.o encoding.o login.o user.o fw_query.o -L/usr/local/lib -lcheck `pkg-config --cflags --libs check` `../src/osflags link` Running suite(s): iodine 96%: Checks: 61, Failures: 2, Errors: 0 user.c:69:F:User:test_users_waiting:0: Assertion 'users_waiting_on_reply() == 1' failed user.c:96:F:User:test_find_user_by_ip:0: Assertion 'find_user_by_ip(testip) == -1' failed Makefile:13: recipe for target 'all' failed make[2]: *** [all] Error 1 make[2]: Leaving directory '/tmp/buildd/iodine-0.6.0~rc1/tests' Makefile:53: recipe for target 'test' failed make[1]: *** [test] Error 2 make[1]: Leaving directory '/tmp/buildd/iodine-0.6.0~rc1' dh_auto_test: make -j1 test returned exit code 2 #v- @Erik: Maybe you could also backport the fix to the iodine-0.6 branch? Cheers, gregor -- .''`. Homepage: http://info.comodo.priv.at/ - OpenPGP key 0xBB3A68018649AA06 : :' : Debian GNU/Linux user, admin, and developer - http://www.debian.org/ `. `' Member of VIBE!AT & SPI, fellow of the Free Software Foundation Europe `- NP: Sophie Hunger: House of Gods
From b715be5cf3978fbe589b03b09c9398d0d791f850 Mon Sep 17 00:00:00 2001
From: Erik Ekman <e...@kryo.se>
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(-)
--- a/src/iodined.c
+++ b/src/iodined.c
@@ -116,6 +116,7 @@
}
#endif
+/* This will not check that user has passed login challenge */
static int
check_user_and_ip(int userid, struct query *q)
{
@@ -142,6 +143,20 @@
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)
{
@@ -780,8 +795,10 @@
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;
@@ -810,7 +827,7 @@
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 */
}
@@ -847,7 +864,7 @@
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 */
}
@@ -888,7 +905,7 @@
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 */
}
@@ -1016,7 +1033,7 @@
/* 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 */
}
@@ -1051,7 +1068,7 @@
/* 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 */
}
@@ -1084,7 +1101,7 @@
/* 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 */
}
@@ -1214,7 +1231,7 @@
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 */
}
@@ -1785,10 +1802,11 @@
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) {
@@ -1813,15 +1831,18 @@
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);
@@ -1843,9 +1864,10 @@
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);
--- a/src/user.c
+++ b/src/user.c
@@ -78,6 +78,8 @@
users[i].disabled = 0;
created_users++;
}
+ users[i].authenticated = 0;
+ users[i].authenticated_raw = 0;
users[i].active = 0;
/* Rest is reset on login ('V' packet) */
}
@@ -101,7 +103,7 @@
ret = 0;
for (i = 0; i < USERS; 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) &&
users[i].q.id != 0 && users[i].conn == CONN_DNS_NULL) {
ret++;
@@ -171,6 +173,8 @@
/* 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;
--- a/src/user.h
+++ b/src/user.h
@@ -39,6 +39,8 @@
struct _user {
char id;
int active;
+ int authenticated;
+ int authenticated_raw;
int disabled;
time_t last_pkt;
int seed;
--- a/tests/user.c
+++ b/tests/user.c
@@ -93,6 +93,11 @@
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);
}
END_TEST
@@ -135,7 +140,11 @@
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++) {
signature.asc
Description: Digital Signature
