Menu
▾
▴
[Ipsec-tools-devel] NAT-T port floating error w/ possible security implications
|
From: Rainer W. <rwe...@mo...> - 2011-06-12 19:38:48
|
An introductory remark: The reason why I noticed this was because one
of the customers of the company I presently work for operates a VPN
gateway which has two uplinks to the internet using two different ISPs,
with one of this uplinks being subject to NAT. For some reason, this
VPN gateway chose to use its NATted uplink for sending a DPD R-U-THERE
to 'our' (racoon-using) VPN endpoint and this caused the VPN to stop
working until the SA db in the kernel (Linux) was flushed manually
about three hours later.
Regarding the actual issue: The isakmp_main function in
src/racoon/isakmp.c contains a block of code (starting at line 467 in
0.8.0) which changes the source and destination addresses associated
with an existing ph1handle structure provided a message was received
whose source addresses differed from the adresses recorded in the
ph1handle structure, both endpoints are capable of NAT-T and no such
address change has already happened. This is wrong/ not really
sensible in a number of ways, specifically
- it will change the addresses of an existing ph1 SA which
isn't using NAT-T
- it will do so upon receipt of any kind of message despite
NAT-T 'port floating' is only supposed to happen during ph1
negotiation, in the initiator message containing the ID
payload.
- as opposed to procedure given in RFC3947, the change is not
done after the message itself was found to be valid but
before any payload processing, after a couple of generic
consistency checks have been done. Unless I'm very much
mistaken, this means that anyone who 'knows' the initiator
and responder cookies can fake a message which will cause
racoon to change the ph1 addresses to an abitrary value. At
least, this seems to be a DoS against a VPN using DPD.
Below is a patch which addresses this by tightening the address check
in isakmp_main to reject messages with unexpected addresses except if
the etype is an phase1 negotiation etype, NAT was detected and no port
floating has occured for this phase1 SA yet and by moving the address
changing code into ph1_main, in between the 'process received packet'
and 'send reply message' calls so that only valid messages can trigger
address changes. This is an adapted version of a patch against 0.7.3
and the 0.7.3 variant used in 'our product' is actually running in
production with this change (since last Friday).
----
diff -rNu ipsec-tools-0.8.0/src/racoon/isakmp.c ipsec-tools-0.8.0.patched//src/racoon/isakmp.c
--- ipsec-tools-0.8.0/src/racoon/isakmp.c 2011-03-15 13:20:14.000000000 +0000
+++ ipsec-tools-0.8.0.patched//src/racoon/isakmp.c 2011-06-12 19:45:02.000000000 +0100
@@ -177,8 +177,10 @@
static u_char r_ck0[] = { 0,0,0,0,0,0,0,0 }; /* used to verify the r_ck. */
+struct isakmp_addrs;
+
static int isakmp_main __P((vchar_t *, struct sockaddr *, struct sockaddr *));
-static int ph1_main __P((struct ph1handle *, vchar_t *));
+static int ph1_main __P((struct ph1handle *, vchar_t *, struct isakmp_addrs *));
static int quick_main __P((struct ph2handle *, vchar_t *));
static int isakmp_ph1begin_r __P((vchar_t *,
struct sockaddr *, struct sockaddr *, u_int8_t));
@@ -388,6 +390,70 @@
}
/*
+ * isakmp main address checking helpers
+ */
+struct isakmp_addrs {
+ struct sockaddr *local, *remote;
+};
+
+#define LOCAL_ADDR_MISMATCH 1
+#define REMOTE_ADDR_MISMATCH 2
+
+static inline u_int32_t
+port_floating_etype(etype)
+ u_int32_t etype;
+{
+#ifdef ENABLE_NATT
+#define FLOAT_MAP (1 << ISAKMP_ETYPE_IDENT | 1 << ISAKMP_ETYPE_AGG \
+ | 1 << ISAKMP_ETYPE_BASE)
+
+ return FLOAT_MAP & (1 << etype);
+
+#undef FLOAT_MAP
+#else
+ return 0;
+#endif
+}
+
+static inline int
+expect_natt_addr_change(iph1)
+ struct ph1handle *iph1;
+{
+#ifdef ENABLE_NATT
+
+ if (iph1->natt_flags & NAT_PORTS_CHANGED) return 0;
+ return iph1->natt_flags & NAT_DETECTED;
+
+#else
+ return 0;
+#endif
+}
+
+static void
+print_addr_mismatch_warning(which, sa0, sa1)
+ int which;
+ struct sockaddr *sa0;
+ struct sockaddr *sa1;
+{
+ char *sa0_s, *which_s;
+
+ which_s = which == REMOTE_ADDR_MISMATCH ?
+ "remote" : "local";
+
+ sa0_s = racoon_strdup(saddr2str(sa0));
+ if (!sa0_s) {
+ plog(LLV_WARNING, LOCATION, NULL,
+ "%s address mismatch\n", which_s);
+ return;
+ }
+
+ plog(LLV_WARNING, LOCATION, NULL,
+ "%s address mismatch (%s <-> %s)\n",
+ which_s, sa0_s, saddr2str(sa1));
+ racoon_free(sa0_s);
+}
+
+/*
* main processing to handle isakmp payload
*/
static int
@@ -399,6 +465,7 @@
isakmp_index *index = (isakmp_index *)isakmp;
u_int32_t msgid = isakmp->msgid;
struct ph1handle *iph1;
+ int addr_mismatch;
#ifdef HAVE_PRINT_ISAKMP_C
isakmp_printpacket(msg, remote, local, 0);
@@ -453,6 +520,7 @@
}
}
+ addr_mismatch = 0;
iph1 = getph1byindex(index);
if (iph1 != NULL) {
/* validity check */
@@ -464,74 +532,28 @@
return -1;
}
-#ifdef ENABLE_NATT
- /* Floating ports for NAT-T */
- if (NATT_AVAILABLE(iph1) &&
- ! (iph1->natt_flags & NAT_PORTS_CHANGED) &&
- ((cmpsaddr(iph1->remote, remote) != CMPSADDR_MATCH) ||
- (cmpsaddr(iph1->local, local) != CMPSADDR_MATCH)))
- {
- /* prevent memory leak */
- racoon_free(iph1->remote);
- racoon_free(iph1->local);
- iph1->remote = NULL;
- iph1->local = NULL;
-
- /* copy-in new addresses */
- iph1->remote = dupsaddr(remote);
- if (iph1->remote == NULL) {
- plog(LLV_ERROR, LOCATION, iph1->remote,
- "phase1 failed: dupsaddr failed.\n");
- remph1(iph1);
- delph1(iph1);
- return -1;
- }
- iph1->local = dupsaddr(local);
- if (iph1->local == NULL) {
- plog(LLV_ERROR, LOCATION, iph1->remote,
- "phase1 failed: dupsaddr failed.\n");
- remph1(iph1);
- delph1(iph1);
- return -1;
- }
-
- /* set the flag to prevent further port floating
- (FIXME: should we allow it? E.g. when the NAT gw
- is rebooted?) */
- iph1->natt_flags |= NAT_PORTS_CHANGED | NAT_ADD_NON_ESP_MARKER;
-
- /* print some neat info */
- plog (LLV_INFO, LOCATION, NULL,
- "NAT-T: ports changed to: %s\n",
- saddr2str_fromto ("%s<->%s", iph1->remote, iph1->local));
-
- natt_keepalive_add_ph1 (iph1);
- }
-#endif
-
- /* must be same addresses in one stream of a phase at least. */
- if (cmpsaddr(iph1->remote, remote) != CMPSADDR_MATCH) {
- char *saddr_db, *saddr_act;
-
- saddr_db = racoon_strdup(saddr2str(iph1->remote));
- saddr_act = racoon_strdup(saddr2str(remote));
- STRDUP_FATAL(saddr_db);
- STRDUP_FATAL(saddr_act);
-
- plog(LLV_WARNING, LOCATION, remote,
- "remote address mismatched. db=%s, act=%s\n",
- saddr_db, saddr_act);
-
- racoon_free(saddr_db);
- racoon_free(saddr_act);
- }
-
- /*
- * don't check of exchange type here because other type will be
- * with same index, for example, informational exchange.
- */
-
- /* XXX more acceptable check */
+ if (cmpsaddr(iph1->remote, remote) != CMPSADDR_MATCH)
+ addr_mismatch = REMOTE_ADDR_MISMATCH;
+ else if (cmpsaddr(iph1->local, local) != CMPSADDR_MATCH)
+ addr_mismatch = LOCAL_ADDR_MISMATCH;
+
+ if (addr_mismatch
+ &&
+ !((port_floating_etype(iph1->etype)
+ && expect_natt_addr_change(iph1)))) {
+ struct sockaddr *sa0, *sa1;
+
+ if (addr_mismatch == REMOTE_ADDR_MISMATCH) {
+ sa0 = iph1->remote;
+ sa1 = remote;
+ } else {
+ sa0 = iph1->local;
+ sa1 = local;
+ }
+
+ print_addr_mismatch_warning(addr_mismatch, sa0, sa1);
+ return -1;
+ }
}
switch (isakmp->etype) {
@@ -602,15 +624,27 @@
if (isakmp->np == ISAKMP_NPTYPE_FRAG)
return frag_handler(iph1, msg, remote, local);
#endif
-
- /* call main process of phase 1 */
- if (ph1_main(iph1, msg) < 0) {
- plog(LLV_ERROR, LOCATION, iph1->remote,
- "phase1 negotiation failed.\n");
- remph1(iph1);
- delph1(iph1);
- return -1;
+ {
+ struct isakmp_addrs addrs, *paddrs;
+
+ if (addr_mismatch) {
+ addrs.remote = remote;
+ addrs.local = local;
+
+ paddrs = &addrs;
+ } else
+ paddrs = NULL;
+
+ /* call main process of phase 1 */
+ if (ph1_main(iph1, msg, paddrs) < 0) {
+ plog(LLV_ERROR, LOCATION, iph1->remote,
+ "phase1 negotiation failed.\n");
+ remph1(iph1);
+ delph1(iph1);
+ return -1;
+ }
}
+
break;
case ISAKMP_ETYPE_AUTH:
@@ -770,9 +804,10 @@
* main function of phase 1.
*/
static int
-ph1_main(iph1, msg)
+ph1_main(iph1, msg, paddrs)
struct ph1handle *iph1;
vchar_t *msg;
+ struct isakmp_addrs *paddrs;
{
int error;
#ifdef ENABLE_STATS
@@ -833,6 +868,27 @@
/* turn off schedule */
sched_cancel(&iph1->scr);
+#ifdef ENABLE_NATT
+ /*
+ * Set if addresses changed and NAT-T port floating
+ * was expected to happen, see isakmp_main above.
+ *
+ */
+ if (paddrs) {
+ memcpy(iph1->remote, paddrs->remote, sizeof(*paddrs->remote));
+ memcpy(iph1->local, paddrs->local, sizeof(*paddrs->local));
+
+ iph1->natt_flags |= NAT_PORTS_CHANGED | NAT_ADD_NON_ESP_MARKER;
+
+ /* print some neat info */
+ plog (LLV_INFO, LOCATION, NULL,
+ "NAT-T: ports changed to: %s\n",
+ saddr2str_fromto ("%s<->%s", iph1->remote, iph1->local));
+
+ natt_keepalive_add_ph1 (iph1);
+ }
+#endif
+
/* send */
plog(LLV_DEBUG, LOCATION, NULL, "===\n");
if ((ph1exchange[etypesw1(iph1->etype)]
|
