6848192 get_ipsa_pair() does not always follow bucket lock entry rules, could potentially deadlock.

6846548 PF_KEY diagnostics need to be more specific
6853208 ipsecalgs(1m) does not cope when there are no algorithms registered.
6856693 sadb_update_sa() checks for duplicate SADB_UPDATE messages in the wrong place.
6846547 Faulty PF_KEY replies should not cause in.iked to halt

6 files changed, 140 insertions, 41 deletions

diff --git a/usr/src/lib/libipsecutil/common/ipsec_util.c b/usr/src/lib/libipsecutil/common/ipsec_util.c
index ade9d99e08..46700b4680 100644
--- a/usr/src/lib/libipsecutil/common/ipsec_util.c
+++ b/usr/src/lib/libipsecutil/common/ipsec_util.c
@@ -1052,7 +1052,6 @@ spdsock_get_ext(spd_ext_t *extv[], spd_msg_t *basehdr, uint_t msgsize,
 	while ((char *)extv[0] < ((char *)basehdr + msgsize)) {
 		/* Check for unknown headers. */
 		i++;
-
 		if (extv[0]->spd_ext_type == 0 ||
 		    extv[0]->spd_ext_type > SPD_EXT_MAX) {
 			if (diag_buf != NULL) {
@@ -1413,6 +1412,15 @@ keysock_diag(int diagnostic)
 	case SADB_X_DIAGNOSTIC_SA_EXPIRED:
 		return (dgettext(TEXT_DOMAIN,
 		    "Security association is not valid"));
+	case SADB_X_DIAGNOSTIC_BAD_CTX:
+		return (dgettext(TEXT_DOMAIN,
+		    "Algorithm invalid or not supported by Crypto Framework"));
+	case SADB_X_DIAGNOSTIC_INVALID_REPLAY:
+		return (dgettext(TEXT_DOMAIN,
+		    "Invalid Replay counter"));
+	case SADB_X_DIAGNOSTIC_MISSING_LIFETIME:
+		return (dgettext(TEXT_DOMAIN,
+		    "Inappropriate lifetimes"));
 	default:
 		return (dgettext(TEXT_DOMAIN, "Unknown diagnostic code"));
 	}
@@ -2986,7 +2994,21 @@ rparseidtype(uint16_t type)
  * error type. If the command calling this function was started by smf(5) the
  * error type could be used as a hint to the restarter. In the future this
  * function could be used to do something more intelligent with a process that
- * encounters an error.
+ * encounters an error. If exit() is called with an error code other than those
+ * defined by smf(5), the program will just get restarted. Unless restarting
+ * is likely to resolve the error condition, its probably sensible to just
+ * log the error and keep running.
+ *
+ * The SERVICE_* exit_types mean nothing if the command was run from the
+ * command line, just exit(). There are two special cases:
+ *
+ * SERVICE_DEGRADE - Not implemented in smf(5), one day it could hint that
+ *                   the service is not running as well is it could. For
+ *                   now, don't do anything, just record the error.
+ * DEBUG_FATAL - Something happened, if the command was being run in debug
+ *               mode, exit() as you really want to know something happened,
+ *               otherwise just keep running. This is ignored when running
+ *               under smf(5).
  *
  * The function will handle an optional variable args error message, this
  * will be written to the error stream, typically a log file or stderr.
@@ -3020,6 +3042,7 @@ ipsecutil_exit(exit_type_t type, char *fmri, FILE *fp, const char *fmt, ...)
 		case SERVICE_DISABLE:
 		case SERVICE_FATAL:
 		case SERVICE_RESTART:
+		case DEBUG_FATAL:
 			warnxfp(fp, "Fatal error - exiting.");
 			exit_status = 1;
 			break;
@@ -3030,7 +3053,9 @@ ipsecutil_exit(exit_type_t type, char *fmri, FILE *fp, const char *fmt, ...)
 		case SERVICE_EXIT_OK:
 			exit_status = SMF_EXIT_OK;
 			break;
-		case SERVICE_DEGRADE:
+		case SERVICE_DEGRADE: /* Not implemented yet. */
+		case DEBUG_FATAL:
+			/* Keep running, don't exit(). */
 			return;
 			break;
 		case SERVICE_BADPERM:
diff --git a/usr/src/lib/libipsecutil/common/ipsec_util.h b/usr/src/lib/libipsecutil/common/ipsec_util.h
index 350b423df9..ce1b552fe2 100644
--- a/usr/src/lib/libipsecutil/common/ipsec_util.h
+++ b/usr/src/lib/libipsecutil/common/ipsec_util.h
@@ -105,16 +105,26 @@ typedef struct keywdtab {
 	char	*kw_str;
 } keywdtab_t;
 
-/* Exit the programe and enter new state */
+/*
+ * These different exit states are designed to give consistant behaviour
+ * when a program needs to exit because of an error. These exit_types
+ * are used in macros, defined later in this file, which call ipsecutil_exit().
+ * What happens when ipsecutil_exit() may differ if the command was started
+ * on the command line or via smf(5), See ipsecutil_exit() source for details.
+ *
+ * Note: The calling function should decide what "debug mode" is before calling
+ * ipsecutil_exit() with DEBUG_FATAL.
+ */
 typedef enum exit_type {
-	SERVICE_EXIT_OK,
-	SERVICE_DEGRADE,
-	SERVICE_BADPERM,
-	SERVICE_BADCONF,
-	SERVICE_MAINTAIN,
-	SERVICE_DISABLE,
-	SERVICE_FATAL,
-	SERVICE_RESTART
+	SERVICE_EXIT_OK,	/* Exit without error. */
+	SERVICE_DEGRADE,	/* A hint that service should be degraded. */
+	SERVICE_BADPERM,	/* A Permission error occured. */
+	SERVICE_BADCONF,	/* Misconfiguration. */
+	SERVICE_MAINTAIN,	/* smf(5) to put service in maintenance mode. */
+	SERVICE_DISABLE,	/* Tell smf(5) to disable me. */
+	SERVICE_FATAL,		/* Whatever happened is not fixable. */
+	SERVICE_RESTART,	/* Tell smf(5) to restart the service. */
+	DEBUG_FATAL		/* Exit in debug mode. */
 } exit_type_t;
 
 /*
@@ -372,7 +382,8 @@ extern const char *do_inet_ntop(const void *, char *, size_t);
  * programs that use libipsecutil. These wll work in usr/src/cmd
  * and usr/src/lib, but because macros in usr/src/lib don't get
  * expanded when I18N message catalogs are built, avoid using
- * these with text inside libipsecutil.
+ * these with text inside libipsecutil. See source of ipsecutil_exit()
+ * for more details.
  */
 #define	EXIT_OK(x) \
 	ipsecutil_exit(SERVICE_EXIT_OK, my_fmri, debugfile, \
diff --git a/usr/src/uts/common/inet/ip/sadb.c b/usr/src/uts/common/inet/ip/sadb.c
index f069cd9770..32733026dc 100644
--- a/usr/src/uts/common/inet/ip/sadb.c
+++ b/usr/src/uts/common/inet/ip/sadb.c
@@ -2774,10 +2774,16 @@ sadb_delget_sa(mblk_t *mp, keysock_in_t *ksi, sadbp_t *spp,
 		/*
 		 * Bucket locks will be required if SA is actually unlinked.
 		 * get_ipsa_pair() returns valid hash bucket pointers even
-		 * if it can't find a pair SA pointer.
+		 * if it can't find a pair SA pointer. To prevent a potential
+		 * deadlock, always lock the outbound bucket before the inbound.
 		 */
-		mutex_enter(&ipsapp->ipsap_bucket->isaf_lock);
-		mutex_enter(&ipsapp->ipsap_pbucket->isaf_lock);
+		if (ipsapp->in_inbound_table) {
+			mutex_enter(&ipsapp->ipsap_pbucket->isaf_lock);
+			mutex_enter(&ipsapp->ipsap_bucket->isaf_lock);
+		} else {
+			mutex_enter(&ipsapp->ipsap_bucket->isaf_lock);
+			mutex_enter(&ipsapp->ipsap_pbucket->isaf_lock);
+		}
 
 		if (ipsapp->ipsap_sa_ptr != NULL) {
 			mutex_enter(&ipsapp->ipsap_sa_ptr->ipsa_lock);
@@ -2846,6 +2852,11 @@ sadb_delget_sa(mblk_t *mp, keysock_in_t *ksi, sadbp_t *spp,
  * association are also searched for. The "pair" of ipsa_t's and isaf_t's
  * are returned as a ipsap_t.
  *
+ * The hash buckets are returned for convenience, if the calling function
+ * needs to use the hash bucket locks, say to remove the SA's, it should
+ * take care to observe the convention of locking outbound bucket then
+ * inbound bucket. The flag in_inbound_table provides direction.
+ *
  * Note that a "pair" is defined as one (but not both) of the following:
  *
  * A security association which has a soft reference to another security
@@ -2869,7 +2880,6 @@ get_ipsa_pair(sadb_sa_t *assoc, sadb_address_t *srcext, sadb_address_t *dstext,
 	sadb_t *sp;
 	uint32_t *srcaddr, *dstaddr;
 	isaf_t *outbound_bucket, *inbound_bucket;
-	boolean_t in_inbound_table = B_FALSE;
 	ipsap_t *ipsapp;
 	sa_family_t af;
 
@@ -2881,6 +2891,8 @@ get_ipsa_pair(sadb_sa_t *assoc, sadb_address_t *srcext, sadb_address_t *dstext,
 	if (ipsapp == NULL)
 		return (NULL);
 
+	ipsapp->in_inbound_table = B_FALSE;
+
 	/*
 	 * Don't worry about IPv6 v4-mapped addresses, sadb_addrcheck()
 	 * takes care of them.
@@ -2929,7 +2941,7 @@ get_ipsa_pair(sadb_sa_t *assoc, sadb_address_t *srcext, sadb_address_t *dstext,
 		if (ipsapp->ipsap_sa_ptr != NULL) {
 			ipsapp->ipsap_bucket = inbound_bucket;
 			ipsapp->ipsap_pbucket = outbound_bucket;
-			in_inbound_table = B_TRUE;
+			ipsapp->in_inbound_table = B_TRUE;
 		} else {
 			ipsapp->ipsap_sa_ptr =
 			    ipsec_getassocbyspi(outbound_bucket,
@@ -2952,7 +2964,7 @@ get_ipsa_pair(sadb_sa_t *assoc, sadb_address_t *srcext, sadb_address_t *dstext,
 			ipsapp->ipsap_bucket = inbound_bucket;
 			ipsapp->ipsap_pbucket = outbound_bucket;
 			if (ipsapp->ipsap_sa_ptr != NULL)
-				in_inbound_table = B_TRUE;
+				ipsapp->in_inbound_table = B_TRUE;
 		}
 	}
 
@@ -2964,7 +2976,7 @@ get_ipsa_pair(sadb_sa_t *assoc, sadb_address_t *srcext, sadb_address_t *dstext,
 	}
 
 	if ((ipsapp->ipsap_sa_ptr->ipsa_state == IPSA_STATE_LARVAL) &&
-	    in_inbound_table) {
+	    ipsapp->in_inbound_table) {
 		mutex_exit(&outbound_bucket->isaf_lock);
 		mutex_exit(&inbound_bucket->isaf_lock);
 		return (ipsapp);
@@ -3001,7 +3013,7 @@ get_ipsa_pair(sadb_sa_t *assoc, sadb_address_t *srcext, sadb_address_t *dstext,
 
 	/* found sa in outbound sadb, peer should be inbound */
 
-	if (in_inbound_table) {
+	if (ipsapp->in_inbound_table) {
 		/* Found SA in inbound table, pair will be in outbound. */
 		if (af == AF_INET6) {
 			ipsapp->ipsap_pbucket = OUTBOUND_BUCKET_V6(sp,
@@ -3303,6 +3315,8 @@ sadb_common_add(queue_t *ip_q, queue_t *pfkey_q, mblk_t *mp, sadb_msg_t *samsg,
 					 */
 					mutex_exit(&newbie->ipsa_lock);
 					error = EPROTOTYPE;
+					*diagnostic =
+					    SADB_X_DIAGNOSTIC_INNER_AF_MISMATCH;
 					goto error;
 				} else {
 					/* Fill in with explicit protocol. */
@@ -3324,6 +3338,8 @@ sadb_common_add(queue_t *ip_q, queue_t *pfkey_q, mblk_t *mp, sadb_msg_t *samsg,
 					 */
 					mutex_exit(&newbie->ipsa_lock);
 					error = EPROTOTYPE;
+					*diagnostic =
+					    SADB_X_DIAGNOSTIC_INNER_AF_MISMATCH;
 					goto error;
 				} else {
 					/* Fill in with explicit protocol. */
@@ -3368,20 +3384,31 @@ sadb_common_add(queue_t *ip_q, queue_t *pfkey_q, mblk_t *mp, sadb_msg_t *samsg,
 	    src_addr_ptr, dst_addr_ptr);
 
 	newbie->ipsa_type = samsg->sadb_msg_satype;
+
 	ASSERT((assoc->sadb_sa_state == SADB_SASTATE_MATURE) ||
 	    (assoc->sadb_sa_state == SADB_X_SASTATE_ACTIVE_ELSEWHERE));
 	newbie->ipsa_auth_alg = assoc->sadb_sa_auth;
 	newbie->ipsa_encr_alg = assoc->sadb_sa_encrypt;
 
 	newbie->ipsa_flags |= assoc->sadb_sa_flags;
-	if ((newbie->ipsa_flags & SADB_X_SAFLAGS_NATT_LOC &&
-	    ksi->ks_in_extv[SADB_X_EXT_ADDRESS_NATT_LOC] == NULL) ||
-	    (newbie->ipsa_flags & SADB_X_SAFLAGS_NATT_REM &&
-	    ksi->ks_in_extv[SADB_X_EXT_ADDRESS_NATT_REM] == NULL) ||
-	    (newbie->ipsa_flags & SADB_X_SAFLAGS_TUNNEL &&
-	    ksi->ks_in_extv[SADB_X_EXT_ADDRESS_INNER_SRC] == NULL)) {
+	if (newbie->ipsa_flags & SADB_X_SAFLAGS_NATT_LOC &&
+	    ksi->ks_in_extv[SADB_X_EXT_ADDRESS_NATT_LOC] == NULL) {
 		mutex_exit(&newbie->ipsa_lock);
-		*diagnostic = SADB_X_DIAGNOSTIC_BAD_SAFLAGS;
+		*diagnostic = SADB_X_DIAGNOSTIC_MISSING_NATT_LOC;
+		error = EINVAL;
+		goto error;
+	}
+	if (newbie->ipsa_flags & SADB_X_SAFLAGS_NATT_REM &&
+	    ksi->ks_in_extv[SADB_X_EXT_ADDRESS_NATT_REM] == NULL) {
+		mutex_exit(&newbie->ipsa_lock);
+		*diagnostic = SADB_X_DIAGNOSTIC_MISSING_NATT_REM;
+		error = EINVAL;
+		goto error;
+	}
+	if (newbie->ipsa_flags & SADB_X_SAFLAGS_TUNNEL &&
+	    ksi->ks_in_extv[SADB_X_EXT_ADDRESS_INNER_SRC] == NULL) {
+		mutex_exit(&newbie->ipsa_lock);
+		*diagnostic = SADB_X_DIAGNOSTIC_MISSING_INNER_SRC;
 		error = EINVAL;
 		goto error;
 	}
@@ -3463,6 +3490,14 @@ sadb_common_add(queue_t *ip_q, queue_t *pfkey_q, mblk_t *mp, sadb_msg_t *samsg,
 		mutex_exit(&ipss->ipsec_alg_lock);
 		if (error != 0) {
 			mutex_exit(&newbie->ipsa_lock);
+			/*
+			 * An error here indicates that alg is the wrong type
+			 * (IE: not authentication) or its not in the alg tables
+			 * created by ipsecalgs(1m), or Kcf does not like the
+			 * parameters passed in with this algorithm, which is
+			 * probably a coding error!
+			 */
+			*diagnostic = SADB_X_DIAGNOSTIC_BAD_CTX;
 			goto error;
 		}
 	}
@@ -3497,6 +3532,8 @@ sadb_common_add(queue_t *ip_q, queue_t *pfkey_q, mblk_t *mp, sadb_msg_t *samsg,
 		mutex_exit(&ipss->ipsec_alg_lock);
 		if (error != 0) {
 			mutex_exit(&newbie->ipsa_lock);
+			/* See above for error explanation. */
+			*diagnostic = SADB_X_DIAGNOSTIC_BAD_CTX;
 			goto error;
 		}
 	}
@@ -3591,6 +3628,7 @@ sadb_common_add(queue_t *ip_q, queue_t *pfkey_q, mblk_t *mp, sadb_msg_t *samsg,
 		if ((replayext->sadb_x_rc_replay32 == 0) &&
 		    (replayext->sadb_x_rc_replay64 != 0)) {
 			error = EOPNOTSUPP;
+			*diagnostic = SADB_X_DIAGNOSTIC_INVALID_REPLAY;
 			mutex_exit(&newbie->ipsa_lock);
 			goto error;
 		}
@@ -4638,6 +4676,21 @@ sadb_update_sa(mblk_t *mp, keysock_in_t *ksi, mblk_t **ipkt_lst,
 		}
 	}
 
+	/*
+	 * At this point we have an UPDATE to a MATURE SA. There should
+	 * not be any keying material present.
+	 */
+	if (akey != NULL) {
+		*diagnostic = SADB_X_DIAGNOSTIC_AKEY_PRESENT;
+		error = EINVAL;
+		goto bail;
+	}
+	if (ekey != NULL) {
+		*diagnostic = SADB_X_DIAGNOSTIC_EKEY_PRESENT;
+		error = EINVAL;
+		goto bail;
+	}
+
 	if (assoc->sadb_sa_state == SADB_X_SASTATE_ACTIVE_ELSEWHERE) {
 		if (ipsapp->ipsap_sa_ptr != NULL &&
 		    ipsapp->ipsap_sa_ptr->ipsa_state == IPSA_STATE_IDLE) {
@@ -4703,6 +4756,7 @@ sadb_update_sa(mblk_t *mp, keysock_in_t *ksi, mblk_t **ipkt_lst,
 	}
 
 	if (ksi->ks_in_extv[SADB_EXT_LIFETIME_CURRENT] != NULL) {
+		*diagnostic = SADB_X_DIAGNOSTIC_MISSING_LIFETIME;
 		error = EOPNOTSUPP;
 		goto bail;
 	}
@@ -4711,16 +4765,6 @@ sadb_update_sa(mblk_t *mp, keysock_in_t *ksi, mblk_t **ipkt_lst,
 		error = EINVAL;
 		goto bail;
 	}
-	if (akey != NULL) {
-		*diagnostic = SADB_X_DIAGNOSTIC_AKEY_PRESENT;
-		error = EINVAL;
-		goto bail;
-	}
-	if (ekey != NULL) {
-		*diagnostic = SADB_X_DIAGNOSTIC_EKEY_PRESENT;
-		error = EINVAL;
-		goto bail;
-	}
 
 	if (ipsapp->ipsap_sa_ptr != NULL) {
 		if (ipsapp->ipsap_sa_ptr->ipsa_state == IPSA_STATE_DEAD) {
@@ -4793,6 +4837,8 @@ sadb_update_sa(mblk_t *mp, keysock_in_t *ksi, mblk_t **ipkt_lst,
 			if (ksi->ks_in_dsttype == KS_IN_ADDR_ME) {
 				if (!sadb_replay_check(ipsapp->ipsap_sa_ptr,
 				    replext->sadb_x_rc_replay32)) {
+					*diagnostic =
+					    SADB_X_DIAGNOSTIC_INVALID_REPLAY;
 					error = EINVAL;
 					goto bail;
 				}
@@ -4896,6 +4942,7 @@ update_pairing(ipsap_t *ipsapp, keysock_in_t *ksi, int *diagnostic,
 
 	if (oipsapp->ipsap_psa_ptr == NULL) {
 		*diagnostic = SADB_X_DIAGNOSTIC_PAIR_INAPPROPRIATE;
+		error = EINVAL;
 		undo_pair = B_TRUE;
 	} else {
 		ipsa_flags = oipsapp->ipsap_psa_ptr->ipsa_flags;
@@ -4922,7 +4969,6 @@ update_pairing(ipsap_t *ipsapp, keysock_in_t *ksi, int *diagnostic,
 		ipsapp->ipsap_sa_ptr->ipsa_flags &= ~IPSA_F_PAIRED;
 		ipsapp->ipsap_sa_ptr->ipsa_otherspi = 0;
 		mutex_exit(&ipsapp->ipsap_sa_ptr->ipsa_lock);
-		error = EINVAL;
 	} else {
 		mutex_enter(&oipsapp->ipsap_psa_ptr->ipsa_lock);
 		oipsapp->ipsap_psa_ptr->ipsa_otherspi = assoc->sadb_sa_spi;
diff --git a/usr/src/uts/common/inet/ip/spdsock.c b/usr/src/uts/common/inet/ip/spdsock.c
index a0e63ac0f0..a9f7ca511b 100644
--- a/usr/src/uts/common/inet/ip/spdsock.c
+++ b/usr/src/uts/common/inet/ip/spdsock.c
@@ -2419,6 +2419,8 @@ spdsock_dumpalgs(queue_t *q, mblk_t *mp)
 
 	msg->spd_msg_len = SPD_8TO64(size);
 	msg->spd_msg_errno = 0;
+	msg->spd_msg_type = SPD_ALGLIST;
+
 	msg->spd_msg_diagnostic = 0;
 
 	cur += sizeof (*msg);
@@ -2432,6 +2434,16 @@ spdsock_dumpalgs(queue_t *q, mblk_t *mp)
 	    ipss->ipsec_nalgs[IPSEC_ALG_ENCR];
 	act->spd_actions_reserved = 0;
 
+	/*
+	 * If there aren't any algorithms registered, return an empty message.
+	 * spdsock_get_ext() knows how to deal with this.
+	 */
+	if (act->spd_actions_count == 0) {
+		act->spd_actions_len = 0;
+		mutex_exit(&ipss->ipsec_alg_lock);
+		goto error;
+	}
+
 	attr = (struct spd_attribute *)cur;
 
 #define	EMIT(tag, value) {					\
@@ -2483,6 +2495,7 @@ spdsock_dumpalgs(queue_t *q, mblk_t *mp)
 	attr--;
 	attr->spd_attr_tag = SPD_ATTR_END;
 
+error:
 	freemsg(mp);
 	qreply(q, m);
 }
diff --git a/usr/src/uts/common/inet/sadb.h b/usr/src/uts/common/inet/sadb.h
index dd51ded73a..fe427e7275 100644
--- a/usr/src/uts/common/inet/sadb.h
+++ b/usr/src/uts/common/inet/sadb.h
@@ -510,6 +510,7 @@ typedef struct sadbp_s
  */
 typedef struct ipsap_s
 {
+	boolean_t	in_inbound_table;
 	isaf_t		*ipsap_bucket;
 	ipsa_t		*ipsap_sa_ptr;
 	isaf_t		*ipsap_pbucket;
diff --git a/usr/src/uts/common/net/pfkeyv2.h b/usr/src/uts/common/net/pfkeyv2.h
index 777e0dcaa0..81840ae3ef 100644
--- a/usr/src/uts/common/net/pfkeyv2.h
+++ b/usr/src/uts/common/net/pfkeyv2.h
@@ -19,7 +19,7 @@
  * CDDL HEADER END
  */
 /*
- * Copyright 2008 Sun Microsystems, Inc.  All rights reserved.
+ * Copyright 2009 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
 
@@ -805,7 +805,10 @@ typedef struct sadb_x_edump {
 
 #define	SADB_X_DIAGNOSTIC_SA_NOTFOUND		78
 #define	SADB_X_DIAGNOSTIC_SA_EXPIRED		79
-#define	SADB_X_DIAGNOSTIC_MAX			79
+#define	SADB_X_DIAGNOSTIC_BAD_CTX		80
+#define	SADB_X_DIAGNOSTIC_INVALID_REPLAY	81
+#define	SADB_X_DIAGNOSTIC_MISSING_LIFETIME	82
+#define	SADB_X_DIAGNOSTIC_MAX			82
 
 /* Algorithm type for sadb_x_algdesc above... */