[Swan-dev] [Bug 299] crash after pluto: Fix addresspool reference count

Antony Antony antony at phenome.org
Mon Oct 9 10:53:03 UTC 2017


Hi Wolfgang,

your last patch is better, it seems ok to apply with minor fies and once a 
full test run confirm no other side effect. This mornign with the two 
patches some tests that passed y-day are failing now. Either something 
changed in master or I overlooked something. I need more time to figure this 
out.

the change in your last patch could be incomplete, it do not release lease 
before unrefrence.
the install_adresspool do not check the return values.
I fixed few such minor issues and also made to fail if the pool from file 
overlap.

Sorry the diff would be hard to read, because of re-factoring.

that also the reason I am testing more.

regards,
-antony


On Sat, Oct 07, 2017 at 01:57:54PM +0200, wolfgang at linogate.de wrote:
> On Sat, 7 Oct 2017 13:35:18 +0200, Antony Antony wrote
> > On Sat, Oct 07, 2017 at 12:02:59PM +0200, wolfgang at linogate.de wrote:
> > > I also couldn't stay away and found some time today to look into it. I 
> > > have added a solution and two test cases to lsw299, which I think worked 
> > >now properly.
> > 
> > Wow It is great to receive patches with tests, thanks.
> > 
> > Are you running the full KVM test suite? because you patched 
> > testing/baseconfigs/east/etc/ipsec.d/passwd
> 
> No I had problems to get the whole kvm suite running in the past, but the
> docker test runs fine for me at the moment, but I'm only running the single tests.
> 
> The additional users in testing/baseconfigs/east/etc/ipsec.d/passwd are needed
> for the two new tests and can be reused if another tests needed it.
> 
> > I had quick look. I will push the testcases. I will not apply the 
> > fix yet. There are some red flags here. May be some of the issues I 
> > am noticing now are not new.
> 
> I don't see an issue now and it only depends on the single side case when you
> use an ip in /etc/ipsec.d/passwd anyway.
> 
> > > We use this feature for years without problems. Sure it is not optimal, 
> > > but it
> > > works. The static address pool is only temporary installed to assign the user
> > > defined static ip to the client and deleted once the instance is gone.
> > 
> > why you specify range per user?
> > 
> > +use6:xOzlFlqtwJIu2:east-any:192.0.2.101-192.0.2.200
> > 
> > If you do that things will likely get messy.
> 
> I don't think so, because it is a possible configuration. The user can connect
> with the same username with multiple clients. If he is connected already with
> another client the addresspool is shared/re-used for the second connection, if
> it is the first connection a new addresspool with the range is installed.
> 
> > 
> > > Having multiple address pools on one connection would be a nice thing, but 
> > > I think it is not easy to implement.
> > 
> > yes.  multiple connections sharing exact pools is supported.
> > I don't see a need for multiple pools per connection yet.
> > 
> > If the address from the xauth file is made into an addresspool, used 
> > only by this specific instance. I would add a variable in "struct 
> > ip_pool" to indicate "do not share this pool".
> > 
> > > Overlapping ip addresses in global and static pools are configuration problems
> > > and the log clearly show the user that he need to configure to separate pools.
> > 
> > I don't think it will work as you imagine. Currently if an 
> > addrsspool is added in via xauth password file. That pool could be shared.
> > 
> 
> An that is necessary if you have a pool range(see above). The pool is not
> connected to the non-instance connection, but the last instance connection
> deletes the shared static pool.
> 
> Wolfgang
-------------- next part --------------
>From 72712497f975d8389817760d1627e0a08731853f Mon Sep 17 00:00:00 2001
From: Antony Antony <antony at phenome.org>
Date: Mon, 9 Oct 2017 07:35:21 +0200
Subject: [PATCH] ikev1: xauth addresspool from xauth passwd file fixes.

refactor xauth user addresspool

Add few more addresspool checks
 Before adding the new pool release the lease.
 No more overlapping addresspools, connection addresspools
 and xauth user pools can not partly overlap

 xauth fail when the address from passwd file is invalid.
 xauth fail when the addresspool fail to install (partial overlap)

Signed-off-by: Antony Antony <antony at phenome.org>
---
 programs/pluto/ikev1_xauth.c | 112 +++++++++++++++++++++++++++----------------
 1 file changed, 71 insertions(+), 41 deletions(-)

diff --git a/programs/pluto/ikev1_xauth.c b/programs/pluto/ikev1_xauth.c
index c5b0d753c..ecaed23c5 100644
--- a/programs/pluto/ikev1_xauth.c
+++ b/programs/pluto/ikev1_xauth.c
@@ -894,6 +894,61 @@ static stf_status xauth_send_status(struct state *st, int status)
 	return STF_OK;
 }
 
+static bool add_xauth_addresspool(struct connection *c, char *userid,
+		char *addresspool)
+{
+	/* set user defined ip address or pool */
+	char *temp;
+	char single_addresspool[128];
+	bool ret = FALSE;
+	err_t er = NULL;
+	ip_range *pool_range = alloc_thing(ip_range, "pool_range");
+
+	temp = strchr(addresspool, '-');
+
+	if (temp == NULL) {
+		/* convert single ip address to addresspool */
+		sprintf(single_addresspool, "%s-%s", addresspool, addresspool);
+		DBG(DBG_CONTROLMORE,
+				DBG_log("XAUTH: adding single ip addresspool entry "
+					"%s for the conn %s user=%s",
+					single_addresspool, c->name, userid));
+		er = ttorange(single_addresspool, 0, AF_INET, pool_range, TRUE);
+		if (er != NULL) {
+			libreswan_log("XAUTH IP address %s is not valid %s "
+					"user=%s", addresspool, er, userid);
+		}
+	} else {
+		DBG(DBG_CONTROLMORE,
+				DBG_log("XAUTH: adding addresspool entry %s for the conn %s user %s",
+					addresspool, c->name, userid));
+		er = ttorange(addresspool, 0, AF_INET, pool_range, TRUE);
+		if (er != NULL) {
+			libreswan_log("XAUTH IP address %s is not valid %s "
+					"user %s", addresspool, er, userid);
+		}
+	}
+
+	/* if valid install new addresspool */
+	if (pool_range->start.u.v4.sin_addr.s_addr > 0) {
+		/* delete existing pool if exits */
+		if (c->pool != NULL) {
+			rel_lease_addr(c);
+			unreference_addresspool(c);
+		}
+
+		c->pool = install_addresspool(pool_range);
+		if (c->pool != NULL) {
+			reference_addresspool(c);
+			ret = TRUE;
+		}
+	}
+
+	pfreeany(pool_range);
+
+	return ret;
+}
+
 /** Do authentication via /etc/ipsec.d/passwd file using MD5 passwords
  *
  * Structure is one entry per line.
@@ -952,7 +1007,6 @@ static bool do_file_authentication(struct state *st, const char *name,
 		char *connectionname = NULL;
 		char *addresspool = NULL;
 		struct connection *c = st->st_connection;
-		ip_range *pool_range;
 
 		lineno++;
 
@@ -1000,8 +1054,9 @@ static bool do_file_authentication(struct state *st, const char *name,
 		DBG(DBG_CONTROL,
 			DBG_log("XAUTH: found user(%s/%s) pass(%s) connid(%s/%s) addresspool(%s)",
 				userid, name, passwdhash,
-				connectionname == NULL? "" : connectionname, connname,
-				addresspool == NULL? "" : addresspool));
+				connectionname == NULL ? "" : connectionname,
+				connname,
+				addresspool == NULL ? "" : addresspool));
 
 		if (streq(userid, name) &&
 		    (connectionname == NULL || streq(connectionname, connname)))
@@ -1021,49 +1076,24 @@ static bool do_file_authentication(struct state *st, const char *name,
 			win = cp != NULL && streq(cp, passwdhash);
 			/* ??? DBG and real-world code mixed */
 			if (DBGP(DBG_CRYPT)) {
-				DBG_log("XAUTH: checking user(%s:%s) pass %s vs %s", userid, connectionname, cp,
-					passwdhash);
+				DBG_log("XAUTH: %s user(%s:%s) pass %s vs %s",
+						win ? "success" : "fail",
+					userid, connectionname, cp, passwdhash);
 			} else {
-				libreswan_log("XAUTH: checking user(%s:%s) ",
-					      userid, connectionname);
+				libreswan_log("XAUTH: %s user(%s:%s) ",
+					win ? "success" : "fail", userid,
+					connectionname);
 			}
 
-			if (win) {
-
-				if (addresspool != NULL && addresspool[0] != '\0') {
-					/* set user defined ip address or pool */
-					char *temp;
-					char single_addresspool[128];
-					pool_range = alloc_thing(ip_range, "pool_range");
-					if (pool_range != NULL) {
-						temp = strchr(addresspool, '-');
-						if (temp == NULL ) {
-							/* convert single ip address to addresspool */
-							sprintf(single_addresspool, "%s-%s", addresspool, addresspool);
-							DBG(DBG_CONTROLMORE,
-								DBG_log("XAUTH: adding single ip addresspool entry %s for the conn %s ",
-								single_addresspool, c->name));
-							ttorange(single_addresspool, 0, AF_INET, pool_range, TRUE);
-						} else {
-							DBG(DBG_CONTROLMORE,
-								DBG_log("XAUTH: adding addresspool entry %s for the conn %s ",
-								addresspool, c->name));
-							ttorange(addresspool, 0, AF_INET, pool_range, TRUE);
-						}
-						/* if valid install new addresspool */
-						if (pool_range->start.u.v4.sin_addr.s_addr) {
-						    /* delete existing pool if exits */
-							if (c->pool)
-								unreference_addresspool(c);
-							c->pool = install_addresspool(pool_range);
-							reference_addresspool(c);
-						}
-						pfree(pool_range);
-					}
+			if (win && addresspool != NULL &&
+					addresspool[0] != '\0') {
+				if (add_xauth_addresspool(c, userid,
+							addresspool)) {
+					break;
 				}
-				break;
+
+				win = FALSE;
 			}
-			libreswan_log("XAUTH: nope");
 		}
 	}
 
-- 
2.13.5



More information about the Swan-dev mailing list