Hi,
with your comments, I have produceds a second version of the patch,
which includes the following changes:
On Sun, 11.04.2010 at 20:47:38 +0200, Toni Mueller <openbsd-t...@oeko.net>
wrote:
> * No IPv6 support (I have no clue).
* tried to add IPv6 support
* Logging is still not very useful, but at least it does now use
syslog's log() instead of stdio's printf().
The patch applies to /usr/src/sys/net. I have it running on one of my
machines, and intend to deploy it on others as well.
Comments and improvements are MUCH appreciated!
--
Kind regards,
--Toni++
Index: pfkeyv2.c
===================================================================
RCS file: /cvs/src/sys/net/pfkeyv2.c,v
retrieving revision 1.119
diff -u -r1.119 pfkeyv2.c
--- pfkeyv2.c 9 May 2008 15:48:15 -0000 1.119
+++ pfkeyv2.c 12 Apr 2010 11:40:59 -0000
@@ -78,6 +78,7 @@
#include <sys/kernel.h>
#include <sys/proc.h>
#include <sys/pool.h>
+#include <sys/syslog.h>
#include <net/route.h>
#include <netinet/ip_ipsp.h>
#include <net/pfkeyv2.h>
@@ -131,6 +132,148 @@
extern struct pool ipsec_policy_pool;
+
+/*
+ * DEBUGGING FUNCTIONS
+ */
+
+#define ON_PFKEY_DEBUG 1
+
+#ifdef ON_PFKEY_DEBUG
+
+/* Here are the prototypes for these purely internal functions: */
+
+int on_pfkey_check_tdb(struct tdb *, const char *);
+void on_pfkey_print_sadb_msg(struct sadb_msg *, const char *);
+
+int on_pfkey_check_flow(struct sockaddr_encap *, struct sockaddr_encap *);
+void on_pfkey_log_errors(struct sadb_msg *);
+
+
+/*
+ * on_pfkey_check_flow()
+ *
+ * Checks for the flows for occurrences of "0/0 -> 0/0".
+ *
+ * Return values:
+ * 0: ok
+ * 1: illegal flow detected
+ *
+ * Should be "bool" instead of "int" if we had it...
+ */
+
+int
+on_pfkey_check_flow(struct sockaddr_encap *flow, struct sockaddr_encap
*flowmask)
+{
+ int rval = 0;
+
+ switch (flow->sen_type) {
+ case SENT_IP6:
+ if (IN6_IS_ADDR_UNSPECIFIED(&(flow->Sen.Sip6.Src))
+ && IN6_IS_ADDR_UNSPECIFIED(&(flow->Sen.Sip6.Src))
+ && IN6_IS_ADDR_UNSPECIFIED(&(flowmask->Sen.Sip6.Src))
+ && IN6_IS_ADDR_UNSPECIFIED(&(flowmask->Sen.Sip6.Src)))
+ rval = 1;
+ break;
+ case SENT_IP4:
+ if ( flow->Sen.Sip4.Src.s_addr == INADDR_ANY
+ && flow->Sen.Sip4.Src.s_addr == INADDR_ANY
+ && flowmask->Sen.Sip4.Src.s_addr == INADDR_ANY
+ && flowmask->Sen.Sip4.Src.s_addr == INADDR_ANY)
+ rval = 1;
+
+ break;
+ }
+ return rval;
+}
+
+void on_pfkey_log_errors(struct sadb_msg *smsg)
+{
+ /* Improve on that ASAP: */
+ log(LOG_WARNING, "Illegal flow detected in message %016lx\n", smsg);
+}
+
+
+/*
+ * int on_pfkey_check_tdb(tdb *newsa, const char *f)
+ *
+ * This function is intended to check for source and destination
+ * addresses of flows being zero. This leaves to complete loss of
+ * connectivity.
+ *
+ * The function generates log entries for things it deems dubious.
+ *
+ * Input parameters:
+ * newsa - the to-be new SA
+ * f - a label, may be NULL, used for log entries
+ *
+ * Return value:
+ * 0: newsa is ok
+ * 1: otherwise
+ */
+
+
+int
+on_pfkey_check_tdb(struct tdb *newsa, const char *f) {
+ char *label = "on_pfkey_check_tdb";
+ union sockaddr_union *src, *dst, *proxy;
+
+ char s_addr[INET6_ADDRSTRLEN];
+ char d_addr[INET6_ADDRSTRLEN];
+ char p_addr[INET6_ADDRSTRLEN];
+
+ if (f && *f) /* label passed */
+ label = (char *)f;
+
+ /* So far, only print all stuff, do nothing: */
+ src = &newsa->tdb_src;
+ dst = &newsa->tdb_dst;
+ proxy = &newsa->tdb_proxy;
+
+ log(LOG_DEBUG, "on_pfkey_check_tdb(tdb at %0lx) families: src=%d,
dst=%d, proxy=%d\n",
+ newsa,
+ src->sa.sa_family,
+ dst->sa.sa_family,
+ proxy->sa.sa_family);
+
+ strncpy(s_addr, inet_ntoa(src->sin.sin_addr), INET6_ADDRSTRLEN);
+ strncpy(d_addr, inet_ntoa(dst->sin.sin_addr), INET6_ADDRSTRLEN);
+ strncpy(p_addr, inet_ntoa(proxy->sin.sin_addr), INET6_ADDRSTRLEN);
+
+ log(LOG_DEBUG,
+ "on_pfkey_check_tdb(tdb at %016lx) addresses: src=%s (%08lx),
dst=%s (%08lx), proxy=%s (%08lx)\n",
+ newsa,
+ s_addr, src->sin.sin_addr,
+ d_addr, dst->sin.sin_addr,
+ p_addr, proxy->sin.sin_addr);
+
+ return on_pfkey_check_flow(&newsa->tdb_filter, &newsa->tdb_filtermask);
+}
+
+
+
+void
+on_pfkey_print_sadb_msg(struct sadb_msg *m, const char *f) {
+ char *label = "on_pfkey_print_sadb_msg";
+
+ if (f && *f) /* label passed */
+ label = (char *)f;
+
+ log(LOG_DEBUG,
+ "%s(): received message: sadb_msg { version: %u, type: %u, errno:
%u, satype: %u, len: %u, reserved: %u, seq: %u, pid: %u }\n",
+ label,
+ m->sadb_msg_version,
+ m->sadb_msg_type,
+ m->sadb_msg_errno,
+ m->sadb_msg_satype,
+ m->sadb_msg_len,
+ m->sadb_msg_reserved,
+ m->sadb_msg_seq,
+ m->sadb_msg_pid);
+}
+
+#endif
+
/*
* Wrapper around m_devget(); copy data from contiguous buffer to mbuf
* chain.
@@ -855,9 +998,10 @@
bzero(headers, sizeof(headers));
for (pfkeyv2_socket = pfkeyv2_sockets; pfkeyv2_socket;
- pfkeyv2_socket = pfkeyv2_socket->next)
+ pfkeyv2_socket = pfkeyv2_socket->next) {
if (pfkeyv2_socket->socket == socket)
break;
+ }
if (!pfkeyv2_socket) {
rval = EINVAL;
@@ -910,6 +1054,17 @@
goto ret;
smsg = (struct sadb_msg *) headers[0];
+
+ /*
+ * At this point, we can check the addresses passed in this message, and
+ * bail out if they are both zero. We do it after parsing the message to
+ * be sure to not access invalid data.
+ */
+
+#ifdef ON_PFKEY_DEBUG
+ on_pfkey_print_sadb_msg(smsg, "pfkeyv2.c:pfkeyv2_send");
+#endif
+
switch (smsg->sadb_msg_type) {
case SADB_GETSPI: /* Reserve an SPI */
bzero(&sa, sizeof(struct tdb));
@@ -1055,6 +1210,16 @@
import_tag(newsa, headers[SADB_X_EXT_TAG]);
#endif
+#ifdef ON_PFKEY_DEBUG
+ /* Check for illegal parameters and conditionally
generate an error message: */
+ rval = on_pfkey_check_tdb(newsa,
"pfkeyv2.c:pfkeyv2_send SADB_UPDATE");
+ if (rval) {
+ rval = EINVAL;
+ freeme = NULL;
+ goto splxret;
+ }
+#endif
+
headers[SADB_EXT_KEY_AUTH] = NULL;
headers[SADB_EXT_KEY_ENCRYPT] = NULL;
headers[SADB_X_EXT_LOCAL_AUTH] = NULL;
@@ -1220,6 +1385,15 @@
#if NPF > 0
import_tag(newsa, headers[SADB_X_EXT_TAG]);
#endif
+#ifdef ON_PFKEY_DEBUG
+ /* Check for illegal parameters and conditionally
generate an error message: */
+ rval = on_pfkey_check_tdb(newsa,
"pfkeyv2.c:pfkeyv2_send SADB_UPDATE");
+ if (rval) {
+ rval = EINVAL;
+ freeme = NULL;
+ goto splxret;
+ }
+#endif
headers[SADB_EXT_KEY_AUTH] = NULL;
headers[SADB_EXT_KEY_ENCRYPT] = NULL;
@@ -1521,11 +1695,28 @@
else
ssrc = NULL;
+
+
import_flow(&encapdst, &encapnetmask,
headers[SADB_X_EXT_SRC_FLOW], headers[SADB_X_EXT_SRC_MASK],
headers[SADB_X_EXT_DST_FLOW], headers[SADB_X_EXT_DST_MASK],
headers[SADB_X_EXT_PROTOCOL],
headers[SADB_X_EXT_FLOW_TYPE]);
+#ifdef ON_PFKEY_DEBUG
+ /*
+ * Now check for illegal flows, containing zero source and
destination
+ * addresses.
+ *
+ * Where is the information to generate useful log messages,
ie, who
+ * actually tried to create the illegal flow, so we can go
after him?
+ */
+
+ if (on_pfkey_check_flow(&encapdst, &encapnetmask)) {
+ on_pfkey_log_errors(smsg);
+ rval = EINVAL;
+ break;
+ }
+#endif
/* Determine whether the exact same SPD entry already exists. */
bzero(&encapgw, sizeof(struct sockaddr_encap));
bzero(&re, sizeof(struct route_enc));
Index: pfkeyv2_convert.c
===================================================================
RCS file: /cvs/src/sys/net/pfkeyv2_convert.c,v
retrieving revision 1.31
diff -u -r1.31 pfkeyv2_convert.c
--- pfkeyv2_convert.c 22 Oct 2008 23:04:45 -0000 1.31
+++ pfkeyv2_convert.c 12 Apr 2010 11:40:59 -0000
@@ -99,6 +99,7 @@
#include <sys/mbuf.h>
#include <sys/kernel.h>
#include <sys/socket.h>
+#include <sys/syslog.h>
#include <net/route.h>
#include <net/if.h>
@@ -459,8 +460,16 @@
*/
if ((src->sa.sa_family != dst->sa.sa_family) ||
(src->sa.sa_family != srcmask->sa.sa_family) ||
- (src->sa.sa_family != dstmask->sa.sa_family))
+ (src->sa.sa_family != dstmask->sa.sa_family)) {
+ /*
+ * Be sure to *really* bail out and generate a better error
+ * message in pfkeyv2_send()!
+ */
+ log(LOG_ERR, "import_flow: mixed address families %d %d %d
%d!\n",
+ src->sa.sa_family, srcmask->sa.sa_family,
+ dst->sa.sa_family, dstmask->sa.sa_family);
return;
+ }
/*
* We set these as an indication that tdb_filter/tdb_filtermask are
