Follow RFC 2408 more closely regarding how to better check the proposal

returned by the other peer (the responder). Some implementations (notably
the Cisco PIX) does not follow a SHOULD in section 4.2 of the RFC. With
certain proposal combinations this caused us to setup the wrong SA
resulting in us being unable to process incoming IPsec traffic (over this
tunnel).

Tested against a number of different IKE implementations.
hshoexer@ ok.

4 files changed, 215 insertions, 20 deletions

diff --git a/sbin/isakmpd/ike_phase_1.c b/sbin/isakmpd/ike_phase_1.c
index a897623fbe9..c144106f5d3 100644
--- a/sbin/isakmpd/ike_phase_1.c
+++ b/sbin/isakmpd/ike_phase_1.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ike_phase_1.c,v 1.42 2003/12/04 21:13:35 miod Exp $	*/
+/*	$OpenBSD: ike_phase_1.c,v 1.43 2004/02/27 09:01:18 ho Exp $	*/
 /*	$EOM: ike_phase_1.c,v 1.31 2000/12/11 23:47:56 niklas Exp $	*/
 
 /*
@@ -79,6 +79,7 @@ ike_phase_1_initiator_send_SA (struct message *msg)
   int i, value, update_nextp;
   struct payload *p;
   struct proto *proto;
+  struct proto_attr *pa;
   int group_desc = -1, new_group_desc;
 
   /* Get the list of transforms.  */
@@ -284,8 +285,24 @@ ike_phase_1_initiator_send_SA (struct message *msg)
   proto->no = 1;
   proto->proto = ISAKMP_PROTO_ISAKMP;
   proto->sa = TAILQ_FIRST (&exchange->sa_list);
-  TAILQ_INSERT_TAIL (&TAILQ_FIRST (&exchange->sa_list)->protos, proto,
-		     link);
+  proto->xf_cnt = conf->cnt;
+  TAILQ_INIT (&proto->xfs);
+  for (i = 0; i < proto->xf_cnt; i++)
+    {
+      pa = (struct proto_attr *)calloc (1, sizeof *pa);
+      if (!pa)
+	goto bail_out;
+      pa->len = transform_len[i];
+      pa->attrs = (u_int8_t *)malloc (pa->len);
+      if (!pa->attrs)
+	{
+	  free (pa);
+	  goto bail_out;
+	}
+      memcpy (pa->attrs, transform[i], pa->len);
+      TAILQ_INSERT_TAIL (&proto->xfs, pa, next);
+    }
+  TAILQ_INSERT_TAIL (&TAILQ_FIRST (&exchange->sa_list)->protos, proto, link);
 
   sa_len = ISAKMP_SA_SIT_OFF + IPSEC_SIT_SIT_LEN;
   sa_buf = malloc (sa_len);
diff --git a/sbin/isakmpd/ike_quick_mode.c b/sbin/isakmpd/ike_quick_mode.c
index 20ccd40bb65..3d266b57579 100644
--- a/sbin/isakmpd/ike_quick_mode.c
+++ b/sbin/isakmpd/ike_quick_mode.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ike_quick_mode.c,v 1.73 2004/02/20 11:31:10 hshoexer Exp $	*/
+/*	$OpenBSD: ike_quick_mode.c,v 1.74 2004/02/27 09:01:18 ho Exp $	*/
 /*	$EOM: ike_quick_mode.c,v 1.139 2001/01/26 10:43:17 niklas Exp $	*/
 
 /*
@@ -445,6 +445,7 @@ initiator_send_HASH_SA_NONCE (struct message *msg)
   struct ipsec_sa *isa = msg->isakmp_sa->data;
   struct hash *hash = hash_get (isa->hash);
   struct sockaddr *src;
+  struct proto_attr *pa;
 
   if (!ipsec_add_hash_payload (msg, hash->hashsize))
     return -1;
@@ -778,6 +779,23 @@ initiator_send_HASH_SA_NONCE (struct message *msg)
 	  proto->no = suite_no + 1;
 	  proto->proto = protocol_num;
 	  proto->sa = TAILQ_FIRST (&exchange->sa_list);
+	  proto->xf_cnt = transform_cnt[prop_no];
+	  TAILQ_INIT (&proto->xfs);
+	  for (xf_no = 0; xf_no < proto->xf_cnt; xf_no++)
+	    {
+	      pa = (struct proto_attr *)calloc (1, sizeof *pa);
+	      if (!pa)
+		goto bail_out;
+	      pa->len = transform_len[prop_no][xf_no];
+	      pa->attrs = (u_int8_t *)malloc (pa->len);
+	      if (!pa->attrs)
+		{
+		  free (pa);
+		  goto bail_out;
+		}
+	      memcpy (pa->attrs, transform[prop_no][xf_no], pa->len);
+	      TAILQ_INSERT_TAIL (&proto->xfs, pa, next);
+	    }
 	  TAILQ_INSERT_TAIL (&TAILQ_FIRST (&exchange->sa_list)->protos, proto,
 			     link);
 
diff --git a/sbin/isakmpd/sa.c b/sbin/isakmpd/sa.c
index a0817c7b5ea..802b2ae9229 100644
--- a/sbin/isakmpd/sa.c
+++ b/sbin/isakmpd/sa.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: sa.c,v 1.74 2004/01/06 00:22:48 hshoexer Exp $	*/
+/*	$OpenBSD: sa.c,v 1.75 2004/02/27 09:01:19 ho Exp $	*/
 /*	$EOM: sa.c,v 1.112 2000/12/12 00:22:52 niklas Exp $	*/
 
 /*
@@ -41,6 +41,7 @@
 
 #include "sysdep.h"
 
+#include "attribute.h"
 #include "conf.h"
 #include "connection.h"
 #include "cookie.h"
@@ -137,7 +138,7 @@ sa_find (int (*check) (struct sa *, void *), void *arg)
   return 0;
 }
 
-/* Check if SA is an ISAKMP SA with an initiar cookie equal to ICOOKIE.  */
+/* Check if SA is an ISAKMP SA with an initiator cookie equal to ICOOKIE.  */
 static int
 sa_check_icookie (struct sa *sa, void *icookie)
 {
@@ -711,6 +712,7 @@ proto_free (struct proto *proto)
 {
   int i;
   struct sa *sa = proto->sa;
+  struct proto_attr *pa;
 
   for (i = 0; i < 2; i++)
     if (proto->spi[i])
@@ -726,10 +728,16 @@ proto_free (struct proto *proto)
 	sa->doi->free_proto_data (proto->data);
       free (proto->data);
     }
+  if (proto->xf_cnt)
+    while ((pa = TAILQ_FIRST (&proto->xfs)) != NULL)
+      {
+	if (pa->attrs)
+	  free (pa->attrs);
+	TAILQ_REMOVE (&proto->xfs, pa, next);
+	free (pa);
+      }
 
-  /* XXX Use class LOG_SA instead?  */
-  LOG_DBG ((LOG_MISC, 90, "proto_free: freeing %p", proto));
-
+  LOG_DBG ((LOG_SA, 90, "proto_free: freeing %p", proto));
   free (proto);
 }
 
@@ -848,6 +856,127 @@ sa_isakmp_upgrade (struct message *msg)
   sa_enter (sa);
 }
 
+#define ATTRS_SIZE (IKE_ATTR_BLOCK_SIZE + 1)	 /* XXX Should be dynamic.  */
+
+struct attr_validation_state {
+  u_int8_t *attrp[ATTRS_SIZE];
+  u_int8_t checked[ATTRS_SIZE];
+  int phase;				/* IKE (1) or IPSEC (2) attrs? */
+  int mode;				/* 0 = 'load', 1 = check */
+};
+
+/* Validate an attribute. Return 0 on match.  */
+static int
+sa_validate_xf_attrs (u_int16_t type, u_int8_t *value, u_int16_t len,
+		      void *arg)
+{
+  struct attr_validation_state *avs = (struct attr_validation_state *)arg;
+
+  LOG_DBG ((LOG_SA, 95, "sa_validate_xf_attrs: phase %d mode %d type %d "
+	    "len %d", avs->phase, avs->mode, type, len));
+
+  /* Make sure the phase and type are valid.  */
+  if (avs->phase == 1)
+    {
+      if (type < IKE_ATTR_ENCRYPTION_ALGORITHM || type > IKE_ATTR_BLOCK_SIZE)
+	return 1;
+    }
+  else if (avs->phase == 2)
+    {
+      if (type < IPSEC_ATTR_SA_LIFE_TYPE || type > IPSEC_ATTR_ECN_TUNNEL)
+	return 1;
+    }
+  else
+    return 1;
+
+  if (avs->mode == 0) /* Load attrs.  */
+    {
+      avs->attrp[type] = value;
+      return 0;
+    }
+
+  /* Checking for a missing attribute is an immediate failure.  */
+  if (!avs->attrp[type])
+    return 1;
+
+  /* Match the loaded attribute against this one, mark it as checked.  */
+  avs->checked[type]++;
+  return memcmp (avs->attrp[type], value, len);
+}
+
+/*
+ * This function is used to validate the returned proposal (protection suite)
+ * we get from the responder against a proposal we sent. Only run as initiator.
+ * We return 0 if a match is found (in any transform of this proposal), 1 
+ * otherwise. Also see note in sa_add_transform() below.
+ */
+static int
+sa_validate_proto_xf (struct proto *match, struct payload *xf, int phase)
+{
+  struct proto_attr *pa;
+  struct attr_validation_state *avs;
+  int i, found = 0;
+  u_int8_t xf_id;
+
+  if (!match->xf_cnt)
+    return 0;
+
+  if (match->proto != GET_ISAKMP_PROP_PROTO (xf->context->p))
+    {
+      LOG_DBG ((LOG_SA, 70, "sa_validate_proto_xf: proto %p (#%d) "
+		"protocol mismatch", match, match->no));
+      return 1;
+    }
+
+  avs = (struct attr_validation_state *)calloc (1, sizeof *avs);
+  if (!avs)
+    {
+      log_error ("sa_validate_proto_xf: calloc (1, %lu)",
+		 (unsigned long)sizeof *avs);
+      return 1;
+    }
+  avs->phase = phase;
+
+  /* Load the "proposal candidate" attribute set.  */
+  (void)attribute_map (xf->p + ISAKMP_TRANSFORM_SA_ATTRS_OFF,
+		       GET_ISAKMP_GEN_LENGTH (xf->p) 
+		       - ISAKMP_TRANSFORM_SA_ATTRS_OFF,
+		       sa_validate_xf_attrs, avs);
+  xf_id = GET_ISAKMP_TRANSFORM_ID (xf->p);
+
+  /* Check against the transforms we suggested.  */
+  avs->mode++;
+  for (pa = TAILQ_FIRST (&match->xfs); pa && !found;
+       pa = TAILQ_NEXT (pa, next))
+    {
+      if (xf_id != GET_ISAKMP_TRANSFORM_ID (pa->attrs))
+	continue;
+
+      memset (avs->checked, 0, sizeof avs->checked);
+      if (attribute_map (pa->attrs + ISAKMP_TRANSFORM_SA_ATTRS_OFF,
+			 pa->len - ISAKMP_TRANSFORM_SA_ATTRS_OFF,
+			 sa_validate_xf_attrs, avs) == 0)
+	found++;
+
+      LOG_DBG ((LOG_SA, 80, "sa_validate_proto_xf: attr_map "
+		"xf %p proto %p pa %p found %d", xf, match, pa, found));
+
+      if (!found)
+	continue;
+
+      /* Require all attributes present and checked.  XXX perhaps not?  */
+      for (i = 0; i < sizeof avs->checked; i++)
+	if (avs->attrp[i] && !avs->checked[i])
+	  found = 0;
+
+      LOG_DBG ((LOG_SA, 80, "sa_validate_proto_xf: req_attr "
+		"xf %p proto %p pa %p found %d", xf, match, pa, found));
+    }
+  free (avs);
+
+  return found ? 0 : 1;
+}
+
 /*
  * Register the chosen transform XF into SA.  As a side effect set PROTOP
  * to point at the corresponding proto structure.  INITIATOR is true if we
@@ -869,11 +998,28 @@ sa_add_transform (struct sa *sa, struct payload *xf, int initiator,
 		   (unsigned long)sizeof *proto);
     }
   else
-    /* Find the protection suite that were chosen.  */
-    for (proto = TAILQ_FIRST (&sa->protos);
-	 proto && proto->no != GET_ISAKMP_PROP_NO (prop->p);
-	 proto = TAILQ_NEXT (proto, link))
-      ;
+    {
+      /*
+       * RFC 2408, section 4.2 states the responder SHOULD use the proposal 
+       * number from the initiator (i.e us), in it's selected proposal to make
+       * this lookup easier. Most vendors follow this. One noted exception is
+       * the CiscoPIX (and perhaps other Cisco products).
+       *
+       * We start by matching on the proposal number, as before.
+       */
+      for (proto = TAILQ_FIRST (&sa->protos);
+	   proto && proto->no != GET_ISAKMP_PROP_NO (prop->p);
+	   proto = TAILQ_NEXT (proto, link))
+	;
+      /*
+       * If we did not find a match, search through all proposals and xforms.
+       */
+      if (!proto || sa_validate_proto_xf (proto, xf, sa->phase) != 0)
+	for (proto = TAILQ_FIRST (&sa->protos);
+	     proto && sa_validate_proto_xf (proto, xf, sa->phase) != 0;
+	     proto = TAILQ_NEXT (proto, link))
+	  ;
+    }
   if (!proto)
     return -1;
   *protop = proto;
@@ -947,15 +1093,15 @@ sa_teardown_all (void)
   int i;
   struct sa *sa;
 
-  LOG_DBG((LOG_MISC, 70, "sa_teardown_all:"));
-  /* Get Phase 2 SAs. */
+  LOG_DBG ((LOG_SA, 70, "sa_teardown_all:"));
+  /* Get Phase 2 SAs.  */
   for (i = 0; i <= bucket_mask; i++)
     for (sa = LIST_FIRST (&sa_tab[i]); sa; sa = LIST_NEXT (sa, link))
       if (sa->phase == 2)
 	{
-	  /* Teardown the phase 2 SAs by name, similar to ui_teardown. */
-	  LOG_DBG((LOG_MISC, 70, "sa_teardown_all: tearing down SA %s",
-	      sa->name));
+	  /* Teardown the phase 2 SAs by name, similar to ui_teardown.  */
+	  LOG_DBG ((LOG_SA, 70, "sa_teardown_all: tearing down SA %s",
+		    sa->name));
 	  connection_teardown (sa->name);
 	  sa_delete (sa, 1);
 	}
diff --git a/sbin/isakmpd/sa.h b/sbin/isakmpd/sa.h
index ac9f6c747de..96ef6a38034 100644
--- a/sbin/isakmpd/sa.h
+++ b/sbin/isakmpd/sa.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: sa.h,v 1.32 2003/06/04 07:31:17 ho Exp $	*/
+/*	$OpenBSD: sa.h,v 1.33 2004/02/27 09:01:19 ho Exp $	*/
 /*	$EOM: sa.h,v 1.58 2000/10/10 12:39:01 provos Exp $	*/
 
 /*
@@ -50,6 +50,7 @@ struct exchange;
 struct keystate;
 struct message;
 struct payload;
+struct proto_attr;
 struct sa;
 struct transport;
 
@@ -82,6 +83,19 @@ struct proto {
 
   /* DOI-specific data.  */
   void *data;
+
+  /* Proposal transforms data, for validating the responders selection.  */
+  TAILQ_HEAD (proto_attr_head, proto_attr) xfs;
+  size_t xf_cnt;
+};
+
+struct proto_attr {
+  /* Link to next transform.  */
+  TAILQ_ENTRY (proto_attr) next;
+
+  /* Transform attribute data and size, suitable for attribute_map().  */
+  u_int8_t *attrs;
+  size_t len;
 };
 
 struct sa {