Re: Fwd: Bug#616482: strongswan-ikev1: virtual ips not released if xauth name does not match id
On Thursday 19 May 2011 22:34:27 Philipp Kern wrote:
> On Thu, May 19, 2011 at 01:34:10PM +0200, René Mayrhofer wrote:
> > Attached is only the patch to be applied to fix #616482.
>
> There was no such attachment, I'm afraid.
Mea culpa. The patch is trivial enough so as not to cause any trouble, IMHO.
> > Additionally, debian/rules needs this fix:
<snip>
> No, it doesn't. That code isn't in stable.
True, I based this on my stable branch in git. Please ignore this patch.
> > > ("Don't do it, it's insecure" vs. "hey, it's now common to do insecure stuff,
> > > so let's allow it" isn't exactly that convincing, yet.)
> > Well, unfortunately that's the practical use-case right now.
> > Without--enable-nat-transport, no smart phone will be able to connect
> > when it's behind a NAT, because most of them (including iPhone and
> > Android) use L2TP-over-IPsec by default and therefore rely on transport
> > connections. We know that there are some security weaknesses of NAT-T
> > for transport connections, which are (hopefully) alleviated by the L2TP
> > server (which should be the only allowed traffic through that IPsec
> > transport connection).
> >
> > I would therefore prefer to have it enabled at this time, because you
> > otherwise prevent a lot of potential clients from connection to a
> > strongswan IPsec gateway. And yes, most commercial IPsec gateways (e.g.
> > included in Linux-based firewalls) enable this right now.
>
> I guess I'd prefer to have the other stuff sorted out first and have a
> look at this later. It's strictly new functionality and albeit I
> sympathise with it, this might need more intense consideration.
I can't (and won't try to) force the stable team to make this change. However, as a maintainer I already enabled it for the packages in unstable. In terms of our Gibraltar firewall, we already use it in production - for the practical reasons mentioned above. I don't like this flaw in the IPsec specification respectively openswan/strongswan implementation, and as a security researcher I have not yet tried to verify whan the security implications are in detail. However, my current understanding is that, when only used in combination with L2TP within the transport connection (which is exactly the use case I enabled it for and can be enforced by limiting the connection to the L2TP ports only), it should not be exploitable.
best regards,
Rene
Index: strongswan/src/pluto/connections.c
===================================================================
--- strongswan.orig/src/pluto/connections.c 2011-04-06 21:30:25.069484558 +0200
+++ strongswan/src/pluto/connections.c 2011-04-06 21:30:43.749511954 +0200
@@ -297,6 +297,7 @@
{
modecfg_attribute_t *ca;
connection_t *old_cur_connection;
+ identification_t *client_id;
old_cur_connection = cur_connection == c? NULL : cur_connection;
#ifdef DEBUG
@@ -367,12 +368,14 @@
free(c->spd.that.virt);
}
+ client_id = (c->xauth_identity) ? c->xauth_identity : c->spd.that.id;
+
/* release virtual IP address lease if any */
if (c->spd.that.modecfg && c->spd.that.pool &&
!c->spd.that.host_srcip->is_anyaddr(c->spd.that.host_srcip))
{
hydra->attributes->release_address(hydra->attributes, c->spd.that.pool,
- c->spd.that.host_srcip, c->spd.that.id);
+ c->spd.that.host_srcip, client_id);
}
/* release requested attributes if any */
@@ -388,7 +391,7 @@
while (c->attributes->remove_last(c->attributes, (void **)&ca) == SUCCESS)
{
hydra->attributes->release(hydra->attributes, ca->handler,
- c->spd.that.id, ca->type, ca->value);
+ client_id, ca->type, ca->value);
modecfg_attribute_destroy(ca);
}
c->attributes->destroy(c->attributes);
Reply to: