Index | Thread | Search

From:
YASUOKA Masahiko <yasuoka@openbsd.org>
Subject:
diff: isakmpd abort by freeing a wrong pointer
To:
tech@openbsd.org
Date:
Thu, 25 Dec 2025 18:19:57 +0900

Download raw body.

Thread
  • YASUOKA Masahiko:

    diff: isakmpd abort by freeing a wrong pointer

Hello,

As a summary, the diff fixes the problem isakmpd aborted by freeing a
wrong pointer.  The problem only happens on unusual setups.


The problem happens when isakmpd receives an acquire message from the
kernel, and it has a source/destination id formatted in prefix type,
but it is not formed as isakmpd expected.

   2777                         dstid = memchr(dstident + 1, '/', slen);
   2778                         if (!dstid) {
   2779                                 log_print("pf_key_v2_acquire: "
   2780                                     "badly formatted PREFIX identity");
   2781                                 goto fail;
   2782                         }

dstid is a middle of the buffer if it has '/'.

   2783                         masklen = atoi(dstid + 1);

   2784 
   2785                         /* XXX We only support host addresses. */
   2786                         if ((afamily == AF_INET6 && masklen != 128) ||
   2787                             (afamily == AF_INET && masklen != 32)) {
   2788                                 log_print("pf_key_v2_acquire: "
   2789                                     "non-host address specified in "
   2790                                     "destination identity (mask length %d), "
   2791                                     "ignoring request", masklen);
   2792                                 goto fail;

if it is not "/32" nor "/128", goto fail

   2793                         }
(snip)
   3169 fail:
   3170         if (ret)
   3171                 pf_key_v2_msg_free(ret);
   3172         if (askpolicy)
   3173                 pf_key_v2_msg_free(askpolicy);
   3174         free(srcid);
   3175         free(dstid);

dstid is a middle the buffer.  So the check in malloc.c fails.

Usually, the kernel never sends such a message.  isakmpd receives only 
acquire messages for the SAs created by isakmpd itself.  But if iked
could run beside isakmpd or an SA remains after iked terminated, this
could actually happen.  The SA registered by iked has a destination
identity like "IPV4/192.168.1.101".

The simplest fix for this, put

  dstid = NULL

before #2729.  But I'd like to propose the diff bellow which makes the checks simpler.

ok? comments?

Index: sbin/isakmpd/pf_key_v2.c
===================================================================
RCS file: /var/cvs/openbsd/src/sbin/isakmpd/pf_key_v2.c,v
diff -u -p -r1.205 pf_key_v2.c
--- sbin/isakmpd/pf_key_v2.c	7 Aug 2023 04:01:30 -0000	1.205
+++ sbin/isakmpd/pf_key_v2.c	24 Dec 2025 08:36:08 -0000
@@ -2341,7 +2341,7 @@ pf_key_v2_acquire(struct pf_key_v2_msg *
 	char		dstbuf[ADDRESS_MAX], srcbuf[ADDRESS_MAX], *peer = 0;
 	char		confname[120], *conn = 0;
 	char           *srcid = 0, *dstid = 0, *prefstring = 0;
-	int		slen, af, afamily, masklen;
+	int		slen, af, afamily;
 	struct sockaddr *smask, *sflow, *dmask, *dflow;
 	struct sadb_protocol *sproto;
 	char		ssflow[ADDRESS_MAX], sdflow[ADDRESS_MAX];
@@ -2623,35 +2623,24 @@ pf_key_v2_acquire(struct pf_key_v2_msg *
 		/* Check for valid type. */
 		switch (srcident->sadb_ident_type) {
 		case SADB_IDENTTYPE_PREFIX:
-			/* Determine what the address family is. */
-			srcid = memchr(srcident + 1, ':', slen);
-			if (srcid)
-				afamily = AF_INET6;
-			else
+			slen = strlen((char *)(srcident + 1));
+			if (slen > 3 && strcmp((char *)(srcident + 1) +
+			    slen - 3, "/32") == 0) {
+				*((char *)(srcident + 1) + slen - 3) = '\0';
 				afamily = AF_INET;
-
-			srcid = memchr(srcident + 1, '/', slen);
-			if (!srcid) {
+			} else if (slen > 4 && strcmp((char *)(srcident + 1)
+			    + slen - 4, "/128") == 0) {
+				*((char *)(srcident + 1) + slen - 4) = '\0';
+				afamily = AF_INET6;
+			} else {
 				log_print("pf_key_v2_acquire: "
 				    "badly formatted PREFIX identity");
 				goto fail;
 			}
-			masklen = atoi(srcid + 1);
-
-			/* XXX We only support host addresses. */
-			if ((afamily == AF_INET6 && masklen != 128) ||
-			    (afamily == AF_INET && masklen != 32)) {
-				log_print("pf_key_v2_acquire: "
-				    "non-host address specified in source "
-				    "identity (mask length %d), ignoring "
-				    "request", masklen);
-				goto fail;
-			}
 			/*
 			 * NUL-terminate the PREFIX string at the separator,
 			 * then dup.
 			 */
-			*srcid = '\0';
 			if (asprintf(&srcid, "id-%s",
 			    (char *) (srcident + 1)) == -1) {
 				log_error("pf_key_v2_acquire: asprintf() failed");
@@ -2763,39 +2752,32 @@ pf_key_v2_acquire(struct pf_key_v2_msg *
 	if (dstident) {
 		slen = (dstident->sadb_ident_len * sizeof(u_int64_t))
 			- sizeof(struct sadb_ident);
-
+		if (((unsigned char *) (dstident + 1))[slen - 1] != '\0') {
+			log_print("pf_key_v2_acquire: "
+			    "destination identity not NUL-terminated");
+			goto fail;
+		}
 		/* Check for valid type. */
 		switch (dstident->sadb_ident_type) {
 		case SADB_IDENTTYPE_PREFIX:
-			/* Determine what the address family is. */
-			dstid = memchr(dstident + 1, ':', slen);
-			if (dstid)
-				afamily = AF_INET6;
-			else
+			slen = strlen((char *)(dstident + 1));
+			if (slen > 3 && strcmp((char *)(dstident + 1) +
+			    slen - 3, "/32") == 0) {
+				*((char *)(dstident + 1) + slen - 3) = '\0';
 				afamily = AF_INET;
-
-			dstid = memchr(dstident + 1, '/', slen);
-			if (!dstid) {
+			} else if (slen > 4 && strcmp((char *)(dstident + 1)
+			    + slen - 4, "/128") == 0) {
+				*((char *)(dstident + 1) + slen - 4) = '\0';
+				afamily = AF_INET6;
+			} else {
 				log_print("pf_key_v2_acquire: "
 				    "badly formatted PREFIX identity");
 				goto fail;
 			}
-			masklen = atoi(dstid + 1);
-
-			/* XXX We only support host addresses. */
-			if ((afamily == AF_INET6 && masklen != 128) ||
-			    (afamily == AF_INET && masklen != 32)) {
-				log_print("pf_key_v2_acquire: "
-				    "non-host address specified in "
-				    "destination identity (mask length %d), "
-				    "ignoring request", masklen);
-				goto fail;
-			}
 			/*
 			 * NUL-terminate the PREFIX string at the separator,
 			 * then dup.
 			 */
-			*dstid = '\0';
 			if (asprintf(&dstid, "id-%s",
 			    (char *) (dstident + 1)) == -1) {
 				log_error("pf_key_v2_acquire: asprintf() failed");