Hi,
I've created a rough patch that should fix the immediate problem, but
is certainly far from perfect (yet). Things to note:
* No IPv6 support (I have no clue).
* No useful error messages - I want to log data about the offending
site, so admins can go after them.
* For some reason I don't yet understand, logging the IP numbers does
not work properly. While the correct IP numbers are logged in hex
code, the same string is emitted for all three IP numbers,
representing the first IP number passed.
* The natural place to emit an error message should be import_flows(),
but I would need to pass some other structure (which?) in for the
sake of generating an error message, AND redo the whole subsystem to
accommodate a non-void import_flow(). Smells like bad architecture...
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 11 Apr 2010 17:46:19 -0000
@@ -131,6 +131,138 @@
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)
+{
+ struct in_addr *src = &flow->Sen.Sip4.Src;
+ struct in_addr *dst = &flow->Sen.Sip4.Dst;
+ struct in_addr *srcmask = &flowmask->Sen.Sip4.Src;
+ struct in_addr *dstmask = &flowmask->Sen.Sip4.Dst;
+
+ if (flow->sen_type == SENT_IP6) {
+ printf( "on_pfkey_check_flow(): can't handle the IPv6 case yet,
pretending everything's ok\n");
+ return 0;
+ }
+
+ if ( src->s_addr == (in_addr_t) 0L
+ && dst->s_addr == (in_addr_t) 0L
+ && srcmask->s_addr == (in_addr_t) 0L
+ && dstmask->s_addr == (in_addr_t) 0L)
+ return 1;
+
+ return 0;
+}
+
+void on_pfkey_log_errors(struct sadb_msg *smsg)
+{
+ /* Improve on that ASAP: */
+ printf("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;
+
+ 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;
+
+ printf("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);
+
+ printf("on_pfkey_check_tdb(tdb at %016lx) addresses: src=%s (%08lx),
dst=%s (%08lx), proxy=%s (%08lx)\n",
+ newsa,
+ inet_ntoa(src->sin.sin_addr),
+ src->sin.sin_addr,
+ inet_ntoa(dst->sin.sin_addr),
+ dst->sin.sin_addr,
+ inet_ntoa(proxy->sin.sin_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;
+
+ printf("%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 +987,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 +1043,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));
@@ -1054,6 +1198,13 @@
#if NPF > 0
import_tag(newsa, headers[SADB_X_EXT_TAG]);
#endif
+ /* 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;
+ }
headers[SADB_EXT_KEY_AUTH] = NULL;
headers[SADB_EXT_KEY_ENCRYPT] = NULL;
@@ -1220,6 +1371,14 @@
#if NPF > 0
import_tag(newsa, headers[SADB_X_EXT_TAG]);
#endif
+ /* 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;
+ }
+
headers[SADB_EXT_KEY_AUTH] = NULL;
headers[SADB_EXT_KEY_ENCRYPT] = NULL;
@@ -1521,11 +1680,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 11 Apr 2010 17:46:19 -0000
@@ -459,8 +459,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()!
+ */
+ printf("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
